davsclaus commented on code in PR #2013: URL: https://github.com/apache/camel-spring-boot/pull/2013#discussion_r4157562225
########## dsl-starter/camel-cli-connector-starter/src/test/java/org/apache/camel/springboot/cli/connector/CliConnectorWebSocketJdkClientTest.java: ########## @@ -0,0 +1,45 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.springboot.cli.connector; + +import org.apache.camel.spring.boot.CamelAutoConfiguration; +import org.apache.camel.test.spring.junit6.CamelSpringBootTest; +import org.apache.camel.test.spring.junit6.DisableJmx; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.test.annotation.DirtiesContext; + +/** + * The connector uses the JDK client when asked to, even with the Spring WebSocket client available. + */ +@DirtiesContext +@CamelSpringBootTest +// the status sent to the tool is built from the JMX management layer +@DisableJmx(false) +@SpringBootTest(classes = { + CamelAutoConfiguration.class, CliConnectorAutoConfiguration.class, + CliConnectorWebSocketTestSupport.Routes.class }, + properties = { + "camel.cli.transport=websocket", + "camel.cli.websocket.snapshot-interval=200", + "camel.cli.websocket.client=jdk" }) Review Comment: Nit: indentation is off compared to the two properties above (`mvn formatter:format` should fix it). ```suggestion "camel.cli.websocket.client=jdk" }) ``` ########## dsl-starter/camel-cli-connector-starter/src/main/java/org/apache/camel/springboot/cli/connector/SpringLocalCliConnector.java: ########## @@ -30,12 +32,27 @@ public SpringLocalCliConnector(CliConnectorFactory cliConnectorFactory, this.applicationContext = applicationContext; } + @Override + protected CliConnectorTransport createTransport(String name) { + // Camel refuses it with the Camel prod profile (camel.main.profile), which Spring profiles do not set + if ("websocket".equalsIgnoreCase(name) + && applicationContext.getEnvironment().acceptsProfiles(Profiles.of("prod"))) { + throw new IllegalStateException( + "The Camel CLI connector websocket transport gives the connected tool full control of this" + + " application and cannot be used with the Spring prod profile." + + " Remove camel.cli.transport=websocket, or use another profile."); + } + return super.createTransport(name); + } + @Override public void sigterm() { try { super.sigterm(); } finally { - applicationContext.stop(); + // close, not only stop: a stopped context keeps the JVM running (for example the embedded web server), + // while the Camel CLI (camel stop) and the websocket tools expect the application to exit + applicationContext.close(); Review Comment: Closing makes sense (the original `stop()` from CAMEL-18425 has no stated reason, and a stopped web app keeps the JVM alive). `super.sigterm()` has just started a "Terminate JVM task" that calls `camelContext.stop()`, so the two run at the same time. That's harmless as far as I can tell and was already the case with `stop()`, but a short comment would help the next reader. Could you also add a test for the file transport (lock file deleted → context closed)? Today only the websocket `stop` action is covered. ########## dsl-starter/camel-cli-connector-starter/src/main/java/org/apache/camel/springboot/cli/connector/CliConnectorAutoConfiguration.java: ########## @@ -69,4 +73,30 @@ public CliConnectorFactory cliConnectorFactory(AbstractApplicationContext applic return answer; } + /** + * The Spring WebSocket client for the websocket transport, when the application has spring-websocket and a Jakarta + * WebSocket client (such as Tomcat with spring-boot-starter-websocket). Otherwise Camel uses the JDK client. + */ + @Configuration(proxyBeanMethods = false) + @ConditionalOnClass(name = { + "org.springframework.web.socket.client.standard.StandardWebSocketClient", + "jakarta.websocket.ContainerProvider" }) Review Comment: `ContainerProvider` is in the Jakarta WebSocket API, so this checks for the API rather than an implementation. An app with `spring-websocket` plus the API jar but no implementation (for example, WebFlux/Netty with the API pulled in transitively) gets this bean. Camel's `auto` mode then picks it from the registry, every connect fails ("no Jakarta WebSocket implementation"), and the transport keeps reconnecting instead of falling back to the JDK client. Would it be worth also checking that `ServiceLoader.load(ContainerProvider.class)` finds a provider (a custom `Condition`), or having the client fall back to the JDK client in that case? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
