Fix subscription keys for Spring client #231
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes subscription key management in the Spring WebSocket client to ensure proper handler isolation for multiple subscriptions. The changes resolve issues where WebSocket handlers were being overwritten when creating multiple subscriptions by implementing unique relay:subscription keys.
- Refactored
createRequestClientmethod to use uniquerelay:subscriptionkeys - Added a protected helper method for creating WebSocket client handlers
- Implemented comprehensive test coverage for multiple subscription scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| NostrSpringWebSocketClient.java | Refactored subscription key logic to use relay:subscription format and added handler creation helper method |
| NostrSpringWebSocketClientTest.java | Replaced singleton tests with comprehensive multi-subscription test using reflection-based test infrastructure |
| import sun.misc.Unsafe; | ||
|
|
There was a problem hiding this comment.
Using sun.misc.Unsafe is discouraged as it's an internal JDK API that may not be available in all JVM implementations and could be removed in future versions. Consider using reflection-based alternatives or test frameworks like Mockito to create test instances instead.
| import sun.misc.Unsafe; |
| NostrIF first = NostrSpringWebSocketClient.getInstance(); | ||
| NostrIF second = NostrSpringWebSocketClient.getInstance(); | ||
| assertSame(first, second, "Multiple calls should return the same instance"); | ||
| private static WebSocketClientHandler createHandler(String name, String uri) throws Exception { |
There was a problem hiding this comment.
[nitpick] The createHandler method contains complex reflection logic that could be simplified. Consider using a builder pattern or factory method that doesn't require manual field manipulation, making the test more maintainable and less brittle to implementation changes.
| .filter(entry -> !entry.getKey().contains(":")) | ||
| .forEach(entry -> { | ||
| String requestKey = entry.getKey() + ":" + subscriptionId; |
There was a problem hiding this comment.
[nitpick] Using a magic string ":" for filtering could be error-prone. Consider defining a constant like SUBSCRIPTION_SEPARATOR to make the code more maintainable and self-documenting.
| .filter(entry -> !entry.getKey().contains(":")) | |
| .forEach(entry -> { | |
| String requestKey = entry.getKey() + ":" + subscriptionId; | |
| .filter(entry -> !entry.getKey().contains(SUBSCRIPTION_SEPARATOR)) | |
| .forEach(entry -> { | |
| String requestKey = entry.getKey() + SUBSCRIPTION_SEPARATOR + subscriptionId; |
Summary
relay:subscriptionTesting
mvn -q verifyhttps://chatgpt.com/codex/tasks/task_b_688a865ff7b88331af6affddbf1fe7bb