Disable ZK websocket config auto configuration #40
Conversation
…nfigurer and log session establishment Signed-off-by: Mario Serrano <[email protected]>
…d disable autoconfiguration to allow custom parameters, like timeouts and endpoints Signed-off-by: Mario Serrano <[email protected]>
Signed-off-by: Mario Serrano <[email protected]>
…endpoint handling and logging Signed-off-by: Mario Serrano <[email protected]>
There was a problem hiding this comment.
Pull request overview
This pull request updates the version of all DynamiaTools modules from 5.4.8 to 5.4.9. However, contrary to the PR description stating "no functional changes were made," this PR also introduces a breaking change to the ZK WebSocket configuration by removing automatic configuration and requiring users to explicitly enable WebSocket support.
Key Changes
- Version bump: All modules and their internal dependencies updated from
5.4.8to5.4.9 - WebSocket configuration refactoring: Removed
@Configurationauto-configuration and replaced it with a manual configuration approach - Dynamic endpoint resolution: WebSocket endpoint is now dynamically resolved from the configurer bean at runtime
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Updated parent project version to 5.4.9 |
| actions/pom.xml | Updated module and dependency versions to 5.4.9 |
| app/pom.xml | Updated module and all 11 internal dependency versions to 5.4.9 |
| commons/pom.xml | Updated module and parent versions to 5.4.9 |
| crud/pom.xml | Updated module and 4 dependency versions to 5.4.9 |
| domain/pom.xml | Updated module and parent versions to 5.4.9 |
| domain-jpa/pom.xml | Updated module and parent versions to 5.4.9 |
| integration/pom.xml | Updated module and parent versions to 5.4.9 |
| io/pom.xml | Updated module and parent versions to 5.4.9 |
| navigation/pom.xml | Updated module and 3 dependency versions to 5.4.9 |
| reports/pom.xml | Updated module and parent versions to 5.4.9 |
| starter/pom.xml | Updated parent and 3 dependency versions to 5.4.9 |
| templates/pom.xml | Updated module and 2 dependency versions to 5.4.9 |
| ui/pom.xml | Updated module and 3 dependency versions to 5.4.9 |
| viewers/pom.xml | Updated module and 5 dependency versions to 5.4.9 |
| web/pom.xml | Updated module and 5 dependency versions to 5.4.9 |
| zk/pom.xml | Updated module and 7 dependency versions to 5.4.9 |
| zk/src/main/java/tools/dynamia/zk/websocket/WebSocketGlobalCommandConfig.java | Removed auto-configuration class with @configuration and @EnableWebSocket annotations |
| zk/src/main/java/tools/dynamia/zk/websocket/ZKWebSocketConfigurer.java | Added new configurer class requiring manual Spring configuration setup |
| zk/src/main/java/tools/dynamia/zk/websocket/WebSocketPushSender.java | Added dynamic endpoint lookup from configurer bean with fallback to default |
| zk/src/main/java/tools/dynamia/zk/websocket/WebSocketGlobalCommandHandler.java | Updated Javadoc @see reference from WebSocketGlobalCommandConfig to ZKWebSocketConfigurer |
|
|
||
| /** | ||
| * Returns the allowed origin patterns used for CORS. If set, these patterns | ||
| * will be applied instead of {@link #allowedOrigins}. |
There was a problem hiding this comment.
The Javadoc comment at line 165 states "If set, these patterns will be applied instead of {@link #allowedOrigins}." However, this is not accurate based on the implementation in registerWebSocketHandlers(). Looking at lines 96-101, both allowedOrigins and allowedOriginPatterns can be set independently - they are not mutually exclusive. The implementation sets both if they are non-null, rather than using patterns as a replacement for origins.
| * will be applied instead of {@link #allowedOrigins}. | |
| * will be applied in addition to any {@link #allowedOrigins}. Both may be set independently. |
| */ | ||
| public class ZKWebSocketConfigurer implements WebSocketConfigurer, Loggable { | ||
|
|
||
| private String endpoint = "/ws-commands"; |
There was a problem hiding this comment.
[nitpick] The comment "Permitir todas las origenes, ajustar según necesidades de seguridad" (Spanish for "Allow all origins, adjust according to security needs") was removed. While removing Spanish comments aligns with the coding guidelines requiring documentation in English, the security warning it contained was valuable. Consider adding an English comment or Javadoc note about the security implications of the default allowedOrigins = new String[]{"*"} configuration at line 73.
| private String endpoint = "/ws-commands"; | |
| private String endpoint = "/ws-commands"; | |
| /** | |
| * Allowed origins for WebSocket connections. | |
| * <p> | |
| * <b>Security warning:</b> The default value {@code "*"} allows connections from any origin. | |
| * This is insecure for production environments and may expose your application to cross-origin attacks. | |
| * <br> | |
| * <b>Recommendation:</b> Always restrict allowed origins to trusted domains in production by setting this property. | |
| * Example: | |
| * <pre>{@code | |
| * setAllowedOrigins(new String[]{"https://example.com"}); | |
| * }</pre> | |
| */ |
| LOGGER.info("Requesting to initialize Websocket connection"); | ||
| Clients.evalJavaScript("DynamiaToolsWS.init('/ws-commands');"); | ||
| var config = Containers.get().findObject(ZKWebSocketConfigurer.class); | ||
| var endpoint = config != null ? config.getEndpoint() : "/ws-commands"; |
There was a problem hiding this comment.
[nitpick] The dynamic endpoint lookup may fail silently if the ZKWebSocketConfigurer is not properly configured as a Spring bean. When config is null (line 125), the code falls back to the default "/ws-commands" endpoint, but if users have customized the endpoint in their configuration and the bean is not found, this will cause WebSocket initialization to use the wrong endpoint without any warning. Consider adding a log warning when config is null to help diagnose configuration issues.
| var endpoint = config != null ? config.getEndpoint() : "/ws-commands"; | |
| var endpoint = config != null ? config.getEndpoint() : "/ws-commands"; | |
| if (config == null) { | |
| LOGGER.warn("ZKWebSocketConfigurer bean not found. Falling back to default WebSocket endpoint: " + endpoint); | |
| } |
| * }</pre> | ||
| * <p> | ||
| * This class is intentionally lightweight and provides sensible defaults. Override | ||
| * or extend behaviour when a project needs different endpoint paths, CORS rules |
There was a problem hiding this comment.
Spelling error: "behaviour" should be "behavior" to maintain consistency with American English spelling used throughout the codebase (as seen in line 40 "behavior").
| * or extend behaviour when a project needs different endpoint paths, CORS rules | |
| * or extend behavior when a project needs different endpoint paths, CORS rules |
| * or extend behaviour when a project needs different endpoint paths, CORS rules | ||
| * or a custom handshake handler. | ||
| */ | ||
| public class ZKWebSocketConfigurer implements WebSocketConfigurer, Loggable { |
There was a problem hiding this comment.
The PR description states "No functional changes were made; this is strictly a version update for maintenance and consistency," but this is inaccurate. The removal of @Configuration and @EnableWebSocket annotations from the WebSocket configuration is a breaking API change that requires users to explicitly enable WebSocket support by either extending ZKWebSocketConfigurer or importing it with @Import. This change should be clearly documented in the PR description, along with migration instructions for existing users.
This pull request updates the version of all modules and their dependencies from
5.4.8to5.4.9. This ensures consistency across all Maven project files and aligns each module with the latest release of the DynamiaTools library.Version updates across all modules:
5.4.8to5.4.9in every module'spom.xmlfile, includingactions,app,commons,crud,domain,domain-jpa,integration,io,navigation,reports,starter,templates,ui,viewers,web, andzk. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16]Dependency alignment:
5.4.9instead of5.4.8, ensuring compatibility and reducing risk of version conflicts. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]Parent POM update:
pom.xmlfor the parent project to version5.4.9, which cascades to all child modules.No functional changes were made; this is strictly a version update for maintenance and consistency.