-
Notifications
You must be signed in to change notification settings - Fork 5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CAMEL-21884: camel-platform-http - Introduce useBodyHandler option #17528
base: main
Are you sure you want to change the base?
Conversation
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🤖 CI automation will test this PR automatically. 🐫 Apache Camel Committers, please review the following items:
|
/component-test platform-http platform-http-vertx Result ❌ The tests failed please check the logs |
🤖 The Apache Camel test robot will run the tests for you 👍 |
component-test is suck ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a comment as a potential improvement.
Also we need tests for this.
DefaultHttpRequestBodyHandler(Handler<RoutingContext> delegate) { | ||
final boolean useBodyHandler; | ||
|
||
DefaultHttpRequestBodyHandler(Handler<RoutingContext> delegate, boolean useBodyHandler) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick - I think it'd be better to have a dedicated class, like:
class NoOpHttpRequestBodyHandler extends HttpRequestBodyHandler {
NoOpHttpRequestBodyHandler(Handler<RoutingContext> delegate) {
super(delegate);
}
@Override
void configureRoute(Route route) {
}
@Override
Future<Void> handle(RoutingContext routingContext, Message message) {
return Future.succeededFuture();
}
}
That way you don't pollute DefaultHttpRequestBodyHandler
with useBodyHandler
checks. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it looks good to me. I will add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jamesnetherton ,
I create such a test
public class VertxPlatformHttpNoBodyHandlerTest {
private final int port = AvailablePortFinder.getNextAvailable();
private final WireMockServer wireMockServer = new WireMockServer(options().port(port));
@BeforeEach
void before() {
wireMockServer.stubFor(post(urlPathEqualTo("/test"))
.withRequestBody(containing("Hello World"))
.willReturn(aResponse()
.withBody("This is a test")));
wireMockServer.start();
}
@AfterEach
void after() {
if (wireMockServer != null) {
wireMockServer.stop();
}
}
@Test
void testNoBodyHandler() throws Exception {
final CamelContext context = VertxPlatformHttpEngineTest.createCamelContext();
final var mockUrl = "http://localhost:" + wireMockServer.port();
try {
context.addRoutes(new RouteBuilder() {
@Override
public void configure() {
from("platform-http:/camel?matchOnUriPrefix=true&useBodyHandler=false")
.process(exchange -> {
HttpMessage message = exchange.getMessage(HttpMessage.class);
message.setBody(message.getRequest());
})
.removeHeader("CamelHttpUri")
.setHeader("OrgCamelHttpUri", simple(mockUrl + "${header.CamelHttpPath}"))
.setHeader("CamelHttpPath", simple(""))
.toD("${bean:" + PathCreator.class.getName()
+ "?method=createNewUri(${header.OrgCamelHttpUri})}?bridgeEndpoint=true");
}
});
context.start();
given()
.body("Hello World")
.post("/camel/test")
.then()
.statusCode(200)
.body(is("This is a test"));
} finally {
context.stop();
}
}
}
But the test is hanging and can not get any response. When I remove the line message.setBody(message.getRequest());
, it starts to work. I'm struggling for a while. Can you give me some lights?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to take a look later today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jamesnetherton - I just push my commits with the above test on zhfeng@e9bf41a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused as to what's making the request hang. It's maybe related to the setup and configuration of VertxPlatformHttpServer
used in the tests.
If I run a similar test on Camel Quarkus, it seems to work. Likewise If I create just a plain Vert.x Server -> Client test (E.g no Camel), it also works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is WARN message in the log
[worker-thread-0] WARN VertxImpl - You're already on a Vert.x context, are you sure you want to create a new Vertx instance?
any hint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any hint?
It just means that the platform-http component and the vertx-http component created their own Vert.x instances. You can avoid the waning by binding a Vertx instance to the registry.
It seems it cannot find camel-buildtools 4.11.0-SNAPSHOT, but it's there https://repository.apache.org/content/repositories/public/org/apache/camel/camel-buildtools/4.11.0-SNAPSHOT/ |
Description
Target
main
branch)Tracking
Apache Camel coding standards and style
mvn clean install -DskipTests
locally from root folder and I have committed all auto-generated changes.