davsclaus commented on code in PR #27248: URL: https://github.com/apache/camel/pull/27248#discussion_r4163317885
########## components/camel-coap/src/test/java/org/apache/camel/coap/CoAPProducerNoResponseTest.java: ########## @@ -0,0 +1,77 @@ +/* + * 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.coap; + +import org.apache.camel.BindToRegistry; +import org.apache.camel.CamelExchangeException; +import org.apache.camel.Exchange; +import org.apache.camel.builder.RouteBuilder; +import org.apache.camel.component.mock.MockEndpoint; +import org.apache.camel.test.AvailablePortFinder; +import org.eclipse.californium.core.CoapClient; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * The producer must fail the exchange when the CoAP server does not answer, instead of completing it with the request + * body. + */ +public class CoAPProducerNoResponseTest extends CoAPTestSupport { + + // nothing listens on this port + @RegisterExtension + static AvailablePortFinder.Port unusedPort = AvailablePortFinder.find(); + + @BindToRegistry("noAnswerClient") + private final CoapClient noAnswerClient = new CoapClient( + String.format("coap://localhost:%d/TestResource", unusedPort.getPort())).setTimeout(500L); + + @AfterEach + void shutdownClient() { + noAnswerClient.shutdown(); + } + + @Test + void testNoResponseFailsTheExchange() throws Exception { Review Comment: Thanks for the regression test for the missing response. The PR also changes two other behaviours: a lower-case `CamelCoapMethod` (e.g. `"post"`) is now sent, and an unknown method now fails with `IllegalArgumentException`. Could you add a small test for each, for example in `CoAPMethodTest` against the running test server, so those changes are covered too? ########## components/camel-coap/src/main/java/org/apache/camel/coap/CoAPProducer.java: ########## @@ -76,14 +80,17 @@ public void process(Exchange exchange) throws Exception { pingResponse = client.ping(); break; default: - break; + throw new IllegalArgumentException("Unsupported CoAP method: " + method); } if (response != null) { CoAPHelper.convertCoapResponseToMessage(response, exchange.getOut()); + } else if (!method.equals(CoAPConstants.METHOD_PING)) { + // the client returns null when no response was received (timeout, rejected or cancelled request) + throw new CamelExchangeException("No response received from CoAP server: " + client.getURI(), exchange); Review Comment: When the producer creates its own client, `client.getURI()` is the full Camel endpoint URI, including its query options (the client is built from `endpoint.getUri()`). Those are `#references` today rather than plain secrets, but since this message ends up in logs and DLC headers, could we pass it through `URISupport.sanitizeUri(...)`? Including the method would also help when troubleshooting: ```suggestion throw new CamelExchangeException( "No response received from CoAP server for " + method + ": " + URISupport.sanitizeUri(client.getURI()), exchange); ``` (This needs `import org.apache.camel.util.URISupport;`.) ########## docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_23.adoc: ########## @@ -737,6 +737,13 @@ in the charset that the `Content-Type` declares, so the bytes match the header. the declared charset and read such a response as UTF-8 must now use the declared charset, and characters that the declared charset cannot represent are written as `?`. +=== camel-coap - producer without a response Review Comment: The 4.22 → 4.23 section already has a `=== camel-coap` entry (around line 4052, about the consumer answering failed exchanges with `5.00`). Could this note go into that existing section, for example as a "Producer" paragraph, instead of adding a second camel-coap heading? Then readers find all the camel-coap upgrade notes in one place. ########## components/camel-coap/src/test/java/org/apache/camel/coap/CoAPProducerNoResponseTest.java: ########## @@ -0,0 +1,77 @@ +/* + * 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.coap; + +import org.apache.camel.BindToRegistry; +import org.apache.camel.CamelExchangeException; +import org.apache.camel.Exchange; +import org.apache.camel.builder.RouteBuilder; +import org.apache.camel.component.mock.MockEndpoint; +import org.apache.camel.test.AvailablePortFinder; +import org.eclipse.californium.core.CoapClient; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * The producer must fail the exchange when the CoAP server does not answer, instead of completing it with the request + * body. + */ +public class CoAPProducerNoResponseTest extends CoAPTestSupport { + + // nothing listens on this port + @RegisterExtension + static AvailablePortFinder.Port unusedPort = AvailablePortFinder.find(); + + @BindToRegistry("noAnswerClient") + private final CoapClient noAnswerClient = new CoapClient( + String.format("coap://localhost:%d/TestResource", unusedPort.getPort())).setTimeout(500L); + + @AfterEach + void shutdownClient() { + noAnswerClient.shutdown(); + } + + @Test + void testNoResponseFailsTheExchange() throws Exception { + MockEndpoint mock = getMockEndpoint("mock:result"); + mock.expectedMessageCount(0); + + Exchange out = template.request("direct:start", e -> e.getIn().setBody("Hello")); + + CamelExchangeException cause = assertInstanceOf(CamelExchangeException.class, out.getException()); + assertTrue(cause.getMessage().startsWith("No response received from CoAP server"), cause.getMessage()); + MockEndpoint.assertIsSatisfied(context); + } + + @Override + protected RouteBuilder createRouteBuilder() { + return new RouteBuilder() { + @Override + public void configure() { + fromF("coap://localhost:%d/TestResource", PORT.getPort()).to("log:exch"); Review Comment: Nit (optional, non-blocking): this consumer route is not used by the test, since the producer sends to `unusedPort`. It can be removed to keep the test focused. -- 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]
