feat: Reimplemented octopus sdk - #6
Conversation
WalkthroughThis change set refactors the Octopus plugin and API to shift from a client-based architecture to direct gRPC stub interaction for event publishing, entry retrieval, and subscription management. Several classes related to the old client model were removed and replaced with new interfaces and implementations. Public APIs were updated, and related test and configuration files were adjusted to reflect the new structure and usage patterns. Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin as Bukkit Plugin
participant OctopusImpl
participant OctopusSubscriptions
participant gRPCServer as Octopus gRPC Server
Plugin->>OctopusImpl: publishEvent(Event)
OctopusImpl->>gRPCServer: EventPublication via gRPC stub
gRPCServer-->>OctopusImpl: Success/Failure
Plugin->>OctopusImpl: getEntry(key)
OctopusImpl->>gRPCServer: EntryRequest via gRPC stub
gRPCServer-->>OctopusImpl: List<Entry>
Plugin->>OctopusImpl: setSubscriptions(subscriptions)
OctopusImpl->>OctopusSubscriptions: updateSubscriptions(subscriptions)
OctopusSubscriptions->>gRPCServer: EventSubscriptionUpdate via gRPC stream
gRPCServer-->>OctopusSubscriptions: EventPublication (stream)
OctopusSubscriptions->>Plugin: Dispatch Bukkit events (PlayerNotification, EntryMutation, Action)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
api/src/main/java/studio/o7/octopus/plugin/api/Octopus.java (1)
3-4: Remove redundant null-safety annotationsBoth
lombok.NonNullandorg.jetbrains.annotations.NotNullare imported, but the interface is already marked with@NullMarked(line 13). Consider using only one annotation strategy for consistency.Since the interface is
@NullMarked, you can remove the explicit@NonNullannotations:-import lombok.NonNull; -import org.jetbrains.annotations.NotNull;And update method signatures to rely on
@NullMarked:- boolean publishEvent(@NonNull Event event); + boolean publishEvent(Event event); - @NotNull List<Entry> getEntry(@NonNull String key); + List<Entry> getEntry(String key);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.devcontainer/devcontainer.json(1 hunks)README.md(1 hunks)api/build.gradle.kts(1 hunks)api/src/main/java/studio/o7/octopus/plugin/api/Octopus.java(2 hunks)api/src/main/java/studio/o7/octopus/plugin/api/client/OctopusClient.java(1 hunks)api/src/main/java/studio/o7/octopus/plugin/api/events/OctopusActionEvent.java(1 hunks)api/src/main/java/studio/o7/octopus/plugin/api/events/OctopusEntryMutationEvent.java(1 hunks)api/src/main/java/studio/o7/octopus/plugin/api/events/OctopusPlayerNotificationEvent.java(1 hunks)plugin/src/main/java/studio/o7/octopus/plugin/OctopusImpl.java(1 hunks)plugin/src/main/java/studio/o7/octopus/plugin/OctopusPlugin.java(0 hunks)plugin/src/main/java/studio/o7/octopus/plugin/OctopusSubscriptions.java(1 hunks)plugin/src/main/java/studio/o7/octopus/plugin/client/OctopusClientImpl.java(0 hunks)plugin/src/main/java/studio/o7/octopus/plugin/client/ServerClient.java(0 hunks)plugin/src/main/java/studio/o7/octopus/plugin/client/SubscriptionClient.java(0 hunks)plugin/src/main/java/studio/o7/octopus/plugin/client/SubscriptionObserver.java(0 hunks)plugin/src/main/resources/plugin.yml(1 hunks)test-plugin/src/main/java/studio/o7/octopus/test/OctopusTestPlugin.java(1 hunks)test-plugin/src/main/java/studio/o7/octopus/test/command/MessageCommand.java(2 hunks)test-plugin/src/main/java/studio/o7/octopus/test/listeners/OctopusListener.java(3 hunks)test-plugin/src/main/resources/plugin.yml(1 hunks)
💤 Files with no reviewable changes (5)
- plugin/src/main/java/studio/o7/octopus/plugin/OctopusPlugin.java
- plugin/src/main/java/studio/o7/octopus/plugin/client/SubscriptionObserver.java
- plugin/src/main/java/studio/o7/octopus/plugin/client/ServerClient.java
- plugin/src/main/java/studio/o7/octopus/plugin/client/SubscriptionClient.java
- plugin/src/main/java/studio/o7/octopus/plugin/client/OctopusClientImpl.java
🧰 Additional context used
🧬 Code Graph Analysis (3)
api/src/main/java/studio/o7/octopus/plugin/api/events/OctopusActionEvent.java (2)
api/src/main/java/studio/o7/octopus/plugin/api/events/OctopusEntryMutationEvent.java (1)
Getter(11-32)api/src/main/java/studio/o7/octopus/plugin/api/events/OctopusPlayerNotificationEvent.java (1)
Getter(10-31)
api/src/main/java/studio/o7/octopus/plugin/api/events/OctopusPlayerNotificationEvent.java (2)
api/src/main/java/studio/o7/octopus/plugin/api/events/OctopusActionEvent.java (1)
Getter(10-31)api/src/main/java/studio/o7/octopus/plugin/api/events/OctopusEntryMutationEvent.java (1)
Getter(11-32)
plugin/src/main/java/studio/o7/octopus/plugin/OctopusImpl.java (1)
plugin/src/main/java/studio/o7/octopus/plugin/OctopusSubscriptions.java (2)
RequiredArgsConstructor(20-79)RequiredArgsConstructor(49-78)
🔇 Additional comments (24)
plugin/src/main/resources/plugin.yml (1)
6-6: Confirmapi-versionaccuracyPaper/Spigot typically expose
api-versionvalues that mirror released Minecraft majors (e.g."1.20","1.21"). The appended patch component.8might be ignored or rejected by older server loaders.Please verify that
• the build pipeline actually compiles against a matchingpaper-api/spigot-apiartifact tagged1.21.8, and
• target servers accept the value unchanged.If not, fallback to the plain major + minor (
"1.21") or omit the field until the final number is released.test-plugin/src/main/resources/plugin.yml (1)
5-5: Mirror the main plugin decision on API versionEnsure the same validation outlined for the core plugin also applies here—if servers reject
"1.21.8", align with whatever value you settle on in the primaryplugin.ymlto avoid load-order mismatches..devcontainer/devcontainer.json (1)
15-15: Java 23 is available but Gradle support remains unverifiedOur check of https://jdk.java.net/23/ shows JDK 23 is published (it “remains available in the OpenJDK Archive”), so you can update your devcontainer base image to pull in Java 23. However, the Gradle compatibility guide (https://docs.gradle.org/current/userguide/compatibility.html) does not yet list JDK 23, so you’ll need to:
- Confirm your
.devcontainer/devcontainer.jsonuses a base image that includes JDK 23 (for example,mcr.microsoft.com/vscode/devcontainers/java:23or a custom build).- Update your Gradle wrapper (in
gradle/wrapper/gradle-wrapper.properties) to a release known to support Java 23 (e.g., Gradle 8.4+ or a nightly build).- Run a full project build under JDK 23 locally (and in CI) to surface any plugin or dependency issues caused by the jump from Java 21 to 23.
README.md (1)
41-41: Repository URL VerifiedThe updated GitHub repository URL is accessible and the default branch is correctly set to
main. No further action is required.
- URL https://github.com/o7studios/octopus-plugin returns HTTP 200
- Default branch:
maintest-plugin/src/main/java/studio/o7/octopus/test/OctopusTestPlugin.java (2)
33-33: LGTM! Constructor simplification aligns with API refactoring.The removal of client parameter from the
OctopusListenerconstructor correctly reflects the architectural shift away from client-based dependency injection.
36-36: LGTM! Constructor simplification aligns with API refactoring.The removal of client parameter from the
MessageCommandconstructor correctly reflects the architectural shift away from client-based dependency injection.test-plugin/src/main/java/studio/o7/octopus/test/listeners/OctopusListener.java (3)
15-17: LGTM! Constructor simplification removes client dependency.The removal of the
OctopusClientparameter correctly reflects the architectural shift to direct Octopus interface usage.
64-64: Consistent API migration pattern.The migration from
client.updateSubscriptions(keys)toOctopus.get().setSubscriptions(keys)is consistent with the pattern used in the join event handler.
52-52: Singleton and Subscription API Verified
Octopus.get()(api/src/main/java/studio/o7/octopus/plugin/api/Octopus.java:16) delegates toUnsafe(api/src/main/java/studio/o7/octopus/plugin/Unsafe.java), which uses Lombok’s@UtilityClassto expose a singlePluginInstance. Initialization is performed on the main thread, avoiding concurrent calls tosetInstance.setSubscriptions(Collection<String>)inOctopusImpl(plugin/src/main/java/studio/o7/octopus/plugin/OctopusImpl.java:61) is public andsynchronized, and now serves as the client-facing setter that replaces the formerclient.updateSubscriptions(keys)call.updateSubscriptions(Collection<String>)inOctopusSubscriptions(plugin/src/main/java/studio/o7/octopus/plugin/OctopusSubscriptions.java:33) remains the internal streaming API for incremental event updates.The change from
updateSubscriptionstosetSubscriptionsis intentional: one is the public “reset” API, the other drives lower-level, in-stream updates.api/src/main/java/studio/o7/octopus/plugin/api/events/OctopusActionEvent.java (2)
4-4: LGTM: Consistent import addition for null safety.The addition of
@NonNullimport aligns with the explicit constructor pattern used across event classes.
17-21: LGTM: Well-implemented explicit constructor.The replacement of Lombok's
@RequiredArgsConstructorwith an explicit constructor provides better control and consistency. The constructor properly:
- Enforces non-null parameters with
@NonNullannotations- Marks the event as asynchronous with
super(true)- Explicitly assigns fields for clarity
This change aligns perfectly with similar modifications in
OctopusEntryMutationEventandOctopusPlayerNotificationEvent.test-plugin/src/main/java/studio/o7/octopus/test/command/MessageCommand.java (2)
10-10: LGTM: Correct import for new singleton pattern.The import change to
studio.o7.octopus.plugin.api.Octopusaligns with the architectural shift away from client injection.
38-38: LGTM: Clean migration to singleton pattern.The change from client injection to
Octopus.get().publishEvent(event)correctly implements the new architectural pattern. This eliminates the need for dependency injection and simplifies event publishing across the codebase.api/src/main/java/studio/o7/octopus/plugin/api/events/OctopusPlayerNotificationEvent.java (2)
4-4: LGTM: Consistent null safety import.The
@NonNullimport addition maintains consistency with the explicit constructor pattern across all event classes.
17-21: LGTM: Consistent explicit constructor implementation.The explicit constructor follows the exact same pattern as
OctopusActionEventandOctopusEntryMutationEvent, ensuring consistency across the event class hierarchy. The implementation correctly:
- Validates non-null parameters
- Configures asynchronous event handling
- Maintains field immutability
api/src/main/java/studio/o7/octopus/plugin/api/events/OctopusEntryMutationEvent.java (4)
4-4: LGTM: Updated imports align with SDK changes.The imports correctly reflect the shift to the new SDK structure with
@NonNullfor null safety andEntryMutationfor the updated event payload.Also applies to: 9-9
12-12: LGTM: Final class modifier enhances immutability.Making the class
finalis a good practice for event classes, preventing unwanted inheritance and ensuring the event structure remains stable.
15-16: LGTM: Field restructuring aligns with new SDK.The separation of concerns into
keyandentryMutationfields provides clearer semantics compared to a single entry field and aligns with the updated SDK structure.
18-22: LGTM: Consistent constructor pattern.The explicit constructor maintains the same pattern as other event classes in this refactoring, ensuring consistency across the event system while supporting the new field structure.
api/src/main/java/studio/o7/octopus/plugin/api/client/OctopusClient.java (4)
3-4: LGTM: Updated imports support interface changes.The imports correctly reflect the new null safety annotations and SDK types required for the updated interface methods.
Also applies to: 7-7
12-12: LGTM: Method rename improves clarity.The rename from
emitEventtopublishEventprovides clearer semantics, and the@NonNullannotation improves null safety. This aligns with the architectural shift described in the PR objectives.
16-16: LGTM: Parameter type generalization improves API design.The change from
List<String>toCollection<String>is good API design as it accepts a broader range of collection types. The@NonNullannotation ensures null safety.
14-14: All getEntry return types and usages updatedVerified that the interface (
Octopus), client (OctopusClient), and implementation (OctopusImpl) signatures have been changed to returnList<Entry>, and the only call site inOctopusImplnow invokesblockingStub.getEntry(...).getEntriesList(). No remaining callers expect the oldEntryResponsetype.plugin/src/main/java/studio/o7/octopus/plugin/OctopusImpl.java (1)
68-76: Well-implemented identifier resolutionThe lazy initialization with multiple fallback sources (system property → environment variable → default) is a good pattern for configuration management.
| @@ -1,5 +1,5 @@ | |||
| dependencies { | |||
| api("studio.o7:octopus-sdk:0.0.2") | |||
| api("studio.o7:octopus-sdk:0.1.2") | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify SDK version availability and breaking changes.
The SDK version jump from 0.0.2 to 0.1.2 corresponds to significant architectural changes. Please verify:
- The new SDK version is published and available
- Breaking changes are documented
- Migration from client-based to gRPC stub architecture is properly supported
🏁 Script executed:
#!/bin/bash
# Check if the new SDK version is available on Maven Central or the appropriate repository
echo "Checking SDK version availability..."
curl -s "https://repo1.maven.org/maven2/studio/o7/octopus-sdk/" | grep -o "0\.1\.2" || echo "SDK version 0.1.2 not found on Maven Central"
# Check for any release notes or changelog
curl -s "https://api.github.com/repos/o7studios/octopus-sdk/releases" | jq -r '.[] | select(.tag_name | contains("0.1.2")) | .body' 2>/dev/null || echo "No release information found"Length of output: 317
SDK 0.1.2 Verified on Maven Central; Release Notes Missing
- Version 0.1.2 is published and available on Maven Central.
- No release notes or changelog entry for 0.1.2 were found in the GitHub releases.
Action items:
- Please add or link to official release notes (e.g. CHANGELOG.md or GitHub release body) for 0.1.2.
- Confirm that all breaking changes (client → gRPC stub refactoring, subscription management updates) are documented.
- Include a migration guide in the project docs to assist users upgrading from 0.0.2 to 0.1.2.
🤖 Prompt for AI Agents
In api/build.gradle.kts at line 2, the SDK version 0.1.2 is added but there are
no release notes or changelog entries for this version. Add or link to the
official release notes for version 0.1.2 in the project documentation, ensure
all breaking changes like client to gRPC stub refactoring and subscription
management updates are documented, and include a migration guide to help users
upgrade from version 0.0.2 to 0.1.2.
| /** | ||
| * Get entries by key, optionally filtered. | ||
| */ | ||
| @NotNull List<Entry> getEntry(@NonNull String key); |
There was a problem hiding this comment.
Documentation inconsistency with method signature
The documentation mentions "optionally filtered" but the method signature doesn't include any filter parameters.
Update the documentation to match the actual functionality:
/**
- * Get entries by key, optionally filtered.
+ * Get entries by key.
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Get entries by key, optionally filtered. | |
| */ | |
| @NotNull List<Entry> getEntry(@NonNull String key); | |
| /** | |
| * Get entries by key. | |
| */ | |
| @NotNull List<Entry> getEntry(@NonNull String key); |
🤖 Prompt for AI Agents
In api/src/main/java/studio/o7/octopus/plugin/api/Octopus.java around lines 25
to 28, the method documentation mentions "optionally filtered" but the method
signature does not accept any filter parameters. Update the Javadoc comment to
remove the reference to optional filtering so it accurately reflects that the
method only retrieves entries by key without any filters.
| public OctopusImpl(Plugin plugin) { | ||
| this.identifier = "test-minecraft-plugin"; | ||
| this.host = "127.0.0.1"; | ||
| this.port = 50051; | ||
| this.client = new OctopusClientImpl(host, port, plugin, identifier); | ||
| this.plugin = plugin; | ||
| subscriptions = new OctopusSubscriptions(plugin, stub); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pass identifier to OctopusSubscriptions to avoid circular dependency
Following up on the circular dependency issue in OctopusSubscriptions, the identifier should be initialized and passed during construction.
public OctopusImpl(Plugin plugin) {
this.plugin = plugin;
- subscriptions = new OctopusSubscriptions(plugin, stub);
+ subscriptions = new OctopusSubscriptions(plugin, stub, getIdentifier());
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In plugin/src/main/java/studio/o7/octopus/plugin/OctopusImpl.java around lines
31 to 34, the constructor initializes OctopusSubscriptions without passing the
identifier, which causes a circular dependency issue. To fix this, initialize
the identifier before creating OctopusSubscriptions and pass it as an argument
to the OctopusSubscriptions constructor to break the circular dependency.
| switch (event.getBodyCase()) { | ||
| case PLAYER_NOTIFICATION -> pm.callEvent(new OctopusPlayerNotificationEvent(key, event.getPlayerNotification())); | ||
| case ENTRY_MUTATION -> pm.callEvent(new OctopusEntryMutationEvent(key, event.getEntryMutation())); | ||
| case ACTION -> pm.callEvent(new OctopusActionEvent(key, event.getAction())); | ||
| case BODY_NOT_SET -> logger.error("Event subscription on key {} without body", key); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add default case to handle unknown event types
The switch statement should include a default case to handle potential future event types gracefully.
switch (event.getBodyCase()) {
case PLAYER_NOTIFICATION -> pm.callEvent(new OctopusPlayerNotificationEvent(key, event.getPlayerNotification()));
case ENTRY_MUTATION -> pm.callEvent(new OctopusEntryMutationEvent(key, event.getEntryMutation()));
case ACTION -> pm.callEvent(new OctopusActionEvent(key, event.getAction()));
case BODY_NOT_SET -> logger.error("Event subscription on key {} without body", key);
+ default -> logger.warn("Unknown event body type {} on key {}", event.getBodyCase(), key);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch (event.getBodyCase()) { | |
| case PLAYER_NOTIFICATION -> pm.callEvent(new OctopusPlayerNotificationEvent(key, event.getPlayerNotification())); | |
| case ENTRY_MUTATION -> pm.callEvent(new OctopusEntryMutationEvent(key, event.getEntryMutation())); | |
| case ACTION -> pm.callEvent(new OctopusActionEvent(key, event.getAction())); | |
| case BODY_NOT_SET -> logger.error("Event subscription on key {} without body", key); | |
| } | |
| switch (event.getBodyCase()) { | |
| case PLAYER_NOTIFICATION -> pm.callEvent(new OctopusPlayerNotificationEvent(key, event.getPlayerNotification())); | |
| case ENTRY_MUTATION -> pm.callEvent(new OctopusEntryMutationEvent(key, event.getEntryMutation())); | |
| case ACTION -> pm.callEvent(new OctopusActionEvent(key, event.getAction())); | |
| case BODY_NOT_SET -> logger.error("Event subscription on key {} without body", key); | |
| default -> logger.warn("Unknown event body type {} on key {}", event.getBodyCase(), key); | |
| } |
🤖 Prompt for AI Agents
In plugin/src/main/java/studio/o7/octopus/plugin/OctopusSubscriptions.java
around lines 60 to 65, the switch statement on event.getBodyCase() lacks a
default case to handle unknown or future event types. Add a default case that
logs an error or warning indicating an unrecognized event type was received,
ensuring the code handles unexpected cases gracefully.
|
|
||
| // When using subscriptions. You need to initialize it first | ||
| client.initializeSubscription(List.of("test")); | ||
| octopus.setSubscriptions(List.of("test")); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the new subscription API method exists.
The migration from client.initializeSubscription(List.of("test")) to octopus.setSubscriptions(List.of("test")) represents the new direct API approach. Please ensure this method is properly implemented in the Octopus interface.
🏁 Script executed:
#!/bin/bash
# Verify the setSubscriptions method exists in the Octopus interface
echo "Checking for setSubscriptions method in Octopus interface..."
ast-grep --pattern 'interface Octopus {
$$$
setSubscriptions($$$) {
$$$
}
$$$
}' || ast-grep --pattern 'setSubscriptions($$$)' Length of output: 225
Ensure setSubscriptions Is Declared in the Octopus Interface
The call to octopus.setSubscriptions(List.of("test")) will not compile because no such method exists. Please update your API by adding the following declaration to the Octopus interface (and implement it in all concrete classes):
void setSubscriptions(List<String> subscriptions);• Locate your Octopus interface (e.g. src/main/java/studio/o7/octopus/Octopus.java)
• Add the above method signature
• Update any existing implementations to handle the new API
🤖 Prompt for AI Agents
In test-plugin/src/main/java/studio/o7/octopus/test/OctopusTestPlugin.java at
line 39, the call to octopus.setSubscriptions(List.of("test")) fails because the
setSubscriptions method is missing from the Octopus interface. To fix this,
locate the Octopus interface file (likely
src/main/java/studio/o7/octopus/Octopus.java), add the method declaration void
setSubscriptions(List<String> subscriptions); to the interface, and then update
all classes implementing Octopus to provide a concrete implementation of this
method.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
plugin/src/main/java/studio/o7/octopus/plugin/OctopusImpl.java (2)
32-35: Potential circular dependency issue persists.The constructor still doesn't pass the identifier to
OctopusSubscriptions, which was flagged in previous reviews. IfOctopusSubscriptionsneeds the identifier during construction, this could still cause circular dependency issues.Verify if
OctopusSubscriptionsrequires the identifier during construction:#!/bin/bash # Check if OctopusSubscriptions constructor requires identifier parameter ast-grep --pattern 'class OctopusSubscriptions { $$$ OctopusSubscriptions($$$) { $$$ } $$$ }'
49-66: Synchronize all subscription methods for atomicity.While the
subscriptionListis now thread-safe, the compound operations (collection update +updateSubscriptionscall) are not atomic. This could lead to inconsistencies between the internal list and actual subscriptions.@Override -public void addSubscriptions(@NonNull Collection<String> subscriptions) { +public synchronized void addSubscriptions(@NonNull Collection<String> subscriptions) { this.subscriptionList.addAll(subscriptions); this.subscriptions.updateSubscriptions(subscriptionList); } @Override -public void removeSubscriptions(@NonNull Collection<String> subscriptions) { +public synchronized void removeSubscriptions(@NonNull Collection<String> subscriptions) { this.subscriptionList.removeAll(subscriptions); this.subscriptions.updateSubscriptions(subscriptionList); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugin/src/main/java/studio/o7/octopus/plugin/OctopusImpl.java(1 hunks)
🔇 Additional comments (2)
plugin/src/main/java/studio/o7/octopus/plugin/OctopusImpl.java (2)
3-18: LGTM! Import changes support the new architecture.The new imports appropriately support the shift from client-based to direct gRPC stub interaction, including thread-safe collections and required SDK components.
21-30: Good! Thread safety concern from previous review has been addressed.The
subscriptionListnow usesCollections.synchronizedSet()which resolves the previous thread safety issue. The field restructuring appropriately supports the new gRPC stub-based architecture.
| @Override | ||
| public @NotNull String getIdentifier() { | ||
| if (this.identifier != null) return identifier; | ||
| identifier = System.getProperty("octopus.identifier"); | ||
| if (identifier == null || identifier.isEmpty()) | ||
| identifier = System.getenv("OCTOPUS_IDENTIFIER"); | ||
| if (identifier == null || identifier.isEmpty()) | ||
| identifier = "default"; | ||
| return identifier; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add thread safety to lazy identifier initialization.
The lazy initialization pattern is not thread-safe. Multiple threads could initialize the identifier field simultaneously.
@Override
-public @NotNull String getIdentifier() {
+public synchronized @NotNull String getIdentifier() {
if (this.identifier != null) return identifier;
identifier = System.getProperty("octopus.identifier");
if (identifier == null || identifier.isEmpty())
identifier = System.getenv("OCTOPUS_IDENTIFIER");
if (identifier == null || identifier.isEmpty())
identifier = "default";
return identifier;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public @NotNull String getIdentifier() { | |
| if (this.identifier != null) return identifier; | |
| identifier = System.getProperty("octopus.identifier"); | |
| if (identifier == null || identifier.isEmpty()) | |
| identifier = System.getenv("OCTOPUS_IDENTIFIER"); | |
| if (identifier == null || identifier.isEmpty()) | |
| identifier = "default"; | |
| return identifier; | |
| } | |
| @Override | |
| public synchronized @NotNull String getIdentifier() { | |
| if (this.identifier != null) return identifier; | |
| identifier = System.getProperty("octopus.identifier"); | |
| if (identifier == null || identifier.isEmpty()) | |
| identifier = System.getenv("OCTOPUS_IDENTIFIER"); | |
| if (identifier == null || identifier.isEmpty()) | |
| identifier = "default"; | |
| return identifier; | |
| } |
🤖 Prompt for AI Agents
In plugin/src/main/java/studio/o7/octopus/plugin/OctopusImpl.java around lines
68 to 77, the lazy initialization of the identifier field is not thread-safe,
which can cause multiple threads to initialize it concurrently. To fix this,
synchronize the block of code that checks and sets the identifier or use a
thread-safe lazy initialization pattern such as double-checked locking to ensure
only one thread initializes the identifier field.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
plugin/src/main/java/studio/o7/octopus/plugin/OctopusSubscriptions.java (1)
64-69: Add default case to handle unknown event typesThe switch statement should include a default case to handle potential future event types gracefully.
switch (event.getBodyCase()) { case PLAYER_NOTIFICATION -> pm.callEvent(new OctopusPlayerNotificationEvent(key, event.getPlayerNotification())); case ENTRY_MUTATION -> pm.callEvent(new OctopusEntryMutationEvent(key, event.getEntryMutation())); case ACTION -> pm.callEvent(new OctopusActionEvent(key, event.getAction())); case BODY_NOT_SET -> logger.error("Event subscription on key {} without body", key); + default -> logger.warn("Unknown event body type {} on key {}", event.getBodyCase(), key); }plugin/src/main/java/studio/o7/octopus/plugin/OctopusImpl.java (2)
33-37: Pass identifier to OctopusSubscriptions to avoid circular dependencyThe identifier should be initialized and passed during construction to avoid the circular dependency issue in
OctopusSubscriptions.public OctopusImpl(Plugin plugin) { this.plugin = plugin; logger = plugin.getSLF4JLogger(); - subscriptions = new OctopusSubscriptions(plugin, logger, stub); + subscriptions = new OctopusSubscriptions(plugin, logger, stub, getIdentifier()); }
80-89: Add thread safety to lazy identifier initializationThe lazy initialization pattern is not thread-safe. Multiple threads could initialize the
identifierfield simultaneously.@Override -public @NotNull String getIdentifier() { +public synchronized @NotNull String getIdentifier() { if (this.identifier != null) return identifier; identifier = System.getProperty("octopus.identifier"); if (identifier == null || identifier.isEmpty()) identifier = System.getenv("OCTOPUS_IDENTIFIER"); if (identifier == null || identifier.isEmpty()) identifier = "default"; return identifier; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/src/main/java/studio/o7/octopus/plugin/api/Octopus.java(2 hunks)plugin/src/main/java/studio/o7/octopus/plugin/OctopusImpl.java(1 hunks)plugin/src/main/java/studio/o7/octopus/plugin/OctopusSubscriptions.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/main/java/studio/o7/octopus/plugin/api/Octopus.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugin/src/main/java/studio/o7/octopus/plugin/OctopusImpl.java (1)
plugin/src/main/java/studio/o7/octopus/plugin/OctopusSubscriptions.java (2)
RequiredArgsConstructor(20-83)RequiredArgsConstructor(53-82)
🔇 Additional comments (2)
plugin/src/main/java/studio/o7/octopus/plugin/OctopusImpl.java (2)
39-49: Excellent error handling implementation!The error handling for gRPC exceptions has been properly implemented, preventing unhandled exceptions from propagating to callers.
51-59: Well-implemented error handling!The method properly handles gRPC exceptions and returns a safe default value, addressing the previous review comments.
| public void updateSubscriptions(@NonNull Collection<String> subscriptions) { | ||
| try { | ||
| if (subscriptionObserver == null) { | ||
| init(); | ||
| updateSubscriptions(subscriptions); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Potential stack overflow risk in recursive call
The recursive call to updateSubscriptions on line 37 could lead to a stack overflow if init() repeatedly fails. Consider using a loop with retry limit or propagating the initialization failure.
public void updateSubscriptions(@NonNull Collection<String> subscriptions) {
try {
if (subscriptionObserver == null) {
init();
- updateSubscriptions(subscriptions);
- return;
+ if (subscriptionObserver == null) {
+ throw new IllegalStateException("Failed to initialize subscription observer");
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void updateSubscriptions(@NonNull Collection<String> subscriptions) { | |
| try { | |
| if (subscriptionObserver == null) { | |
| init(); | |
| updateSubscriptions(subscriptions); | |
| return; | |
| } | |
| public void updateSubscriptions(@NonNull Collection<String> subscriptions) { | |
| try { | |
| if (subscriptionObserver == null) { | |
| init(); | |
| if (subscriptionObserver == null) { | |
| throw new IllegalStateException("Failed to initialize subscription observer"); | |
| } | |
| } | |
| // ... existing subscription update logic ... | |
| } catch (Exception e) { | |
| logger.error("Failed to update subscriptions", e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In plugin/src/main/java/studio/o7/octopus/plugin/OctopusSubscriptions.java
around lines 33 to 39, the method updateSubscriptions calls itself recursively
after init(), which risks a stack overflow if init() repeatedly fails to
initialize subscriptionObserver. Replace the recursive call with a loop that
attempts initialization a limited number of times or modify the code to
propagate an initialization failure instead of retrying indefinitely, ensuring
the method does not recurse infinitely.
| var update = EventSubscriptionUpdate.newBuilder() | ||
| .setIdentifier(Octopus.get().getIdentifier()) | ||
| .addAllKeys(subscriptions) | ||
| .build(); |
There was a problem hiding this comment.
Circular dependency issue with Octopus.get().getIdentifier()
Using Octopus.get().getIdentifier() creates a circular dependency since OctopusImpl creates OctopusSubscriptions. Consider passing the identifier as a constructor parameter or method argument.
+private final String identifier;
+
-public OctopusSubscriptions(Plugin plugin, Logger logger, OctopusServiceGrpc.OctopusServiceStub stub) {
+public OctopusSubscriptions(Plugin plugin, Logger logger, OctopusServiceGrpc.OctopusServiceStub stub, String identifier) {
this.plugin = plugin;
this.logger = logger;
this.stub = stub;
+ this.identifier = identifier;
}
var update = EventSubscriptionUpdate.newBuilder()
- .setIdentifier(Octopus.get().getIdentifier())
+ .setIdentifier(identifier)
.addAllKeys(subscriptions)
.build();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var update = EventSubscriptionUpdate.newBuilder() | |
| .setIdentifier(Octopus.get().getIdentifier()) | |
| .addAllKeys(subscriptions) | |
| .build(); | |
| --- a/plugin/src/main/java/studio/o7/octopus/plugin/OctopusSubscriptions.java | |
| +++ b/plugin/src/main/java/studio/o7/octopus/plugin/OctopusSubscriptions.java | |
| @@ | |
| public class OctopusSubscriptions { | |
| private final Plugin plugin; | |
| private final Logger logger; | |
| private final OctopusServiceGrpc.OctopusServiceStub stub; | |
| + private final String identifier; | |
| - public OctopusSubscriptions(Plugin plugin, Logger logger, OctopusServiceGrpc.OctopusServiceStub stub) { | |
| + public OctopusSubscriptions(Plugin plugin, Logger logger, OctopusServiceGrpc.OctopusServiceStub stub, String identifier) { | |
| this.plugin = plugin; | |
| this.logger = logger; | |
| this.stub = stub; | |
| + this.identifier = identifier; | |
| } | |
| // … | |
| @@ | |
| var update = EventSubscriptionUpdate.newBuilder() | |
| - .setIdentifier(Octopus.get().getIdentifier()) | |
| + .setIdentifier(identifier) | |
| .addAllKeys(subscriptions) | |
| .build(); | |
| // … | |
| } |
🤖 Prompt for AI Agents
In plugin/src/main/java/studio/o7/octopus/plugin/OctopusSubscriptions.java
around lines 41 to 44, avoid calling Octopus.get().getIdentifier() directly to
prevent circular dependency with OctopusImpl. Instead, modify the
OctopusSubscriptions class to accept the identifier as a constructor parameter
or method argument and use that passed identifier in the EventSubscriptionUpdate
builder.
1 participant
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores
Style