Skip to content
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

Support HTTP redirects with VertxHttpClientHTTPConduit #1609

Closed
dcheng1248 opened this issue Nov 19, 2024 · 7 comments · Fixed by #1617
Closed

Support HTTP redirects with VertxHttpClientHTTPConduit #1609

dcheng1248 opened this issue Nov 19, 2024 · 7 comments · Fixed by #1617

Comments

@dcheng1248
Copy link

When sending SOAP requests with the default VertxHttpClientHTTPConduitFactory I get a HTTP 302 Redirection error.

The request works outside the app and the problem is solved by setting
quarkus.cxf.http-conduit-factory=URLConnectionHTTPConduitFactory

Not sure what is causing this error in the new default factory, any advice would be appreciated.

@dcheng1248 dcheng1248 changed the title SOAP Request redirection error regression when using 3.16 with default http-conduit-factory VertxHttpClientHTTPConduitFactory SOAP Request HTTP 302 error regression when using 3.16 with default http-conduit-factory VertxHttpClientHTTPConduitFactory Nov 19, 2024
@ppalaga
Copy link
Contributor

ppalaga commented Nov 20, 2024

Do you have any stack trace by any chance? This would be to see where in the flow the exception happens. The main two suspects are accessing the remote service endpoint and accessing the remote WSDL. But could also be something else.
Can you perhaps give more hints, so that we can reproduce the behavior?

@dcheng1248
Copy link
Author

I researched this issue a bit more yesterday and the problem seems to lie in sending a POST SOAP request to the remote service endpoint. We don't do dynamic WSDL fetches. The Vertx HTTP Factory seems to disable redirection for POST requests (per documentation), and there is no simple option to enable it. One must define a custom redirection handler.

The URLConnectionHTTPConduit on the other hand seems to enable redirection for GET and POST requests by default, so we didn't have this problem before, and setting this as the Conduit factory fixed the problem.

This is quite a breaking change for us as a lot of our services send out POST SOAP requests, and all of them need to be updated with the fix after the dependency update. Perhaps it might be worth documenting this potential issue?

Please let me know if you need more information.

@ppalaga
Copy link
Contributor

ppalaga commented Nov 20, 2024

Thanks for the info, @dcheng1248. I really appreciate it as our aim is to make VertxHttpClientHTTPConduitFactory a drop in replacement for URLConnectionHTTPConduitFactory. I'll have a look how redirecting could be enabled for the VertxHttpClientHTTPConduitFactory.

@ppalaga ppalaga changed the title SOAP Request HTTP 302 error regression when using 3.16 with default http-conduit-factory VertxHttpClientHTTPConduitFactory Support HTTP redirects with VertxHttpClientHTTPConduit Nov 26, 2024
ppalaga added a commit to ppalaga/quarkus-cxf that referenced this issue Nov 26, 2024
ppalaga added a commit to ppalaga/quarkus-cxf that referenced this issue Nov 26, 2024
ppalaga added a commit to ppalaga/quarkus-cxf that referenced this issue Nov 26, 2024
@ppalaga
Copy link
Contributor

ppalaga commented Nov 28, 2024

@dcheng1248 the fix is available in Quarkus Platform 3.17.0. It would be great to hear whether it fulfills all your needs.
Note that offloading the request data to disk is not implemented yet #1628

@dcheng1248
Copy link
Author

@ppalaga many thanks for the quick update and notification. I will test it out soon and report back.

@dcheng1248
Copy link
Author

@ppalaga I just did a test and unfortunately there still seems to be an issue at the unit test level. I'm actually not sure if this is related to the request data caching vs offloading you mentioned, so any advice would be appreciated. Our request body is very small FWIW, < 10 lines of XML. If my issue is not related to the cache vs offloading, I think this reflects different behaviours of the two HTTP Conduits.

I will give the information I can but need to redact/change some things.

Unit test mocks:

private void mockRedirect(...) throws IOException {
    wireMockServer.stubFor(
            WireMock.post(urlPathEqualTo("/normal/test/path"))
                    .withBasicAuth(VALID_USERNAME, String.valueOf(VALID_PASSWORD))
                    .withHeader(...)
                    .withHeader(...)
                    .withRequestBody(matchingXPath(...))
                    .atPriority(1)
                    .willReturn(
                            aResponse()
                                    .withStatus(302)
                                    .withHeader("location", wireMockServer.baseUrl() + "/redirect-test")));
  }
private void mockRedirectFinal(...) throws IOException {
    wireMockServer.stubFor(
            WireMock.post(urlPathEqualTo("/redirect-test"))
                    .withBasicAuth(VALID_USERNAME, String.valueOf(VALID_PASSWORD))
                    .withHeader(...)
                    .withHeader(...)
                    .withRequestBody(matchingXPath(...))
                    .willReturn(
                            aResponse()
                                    .withStatus(200)
                                    .withHeader("content-type", "application/xml")
                                    .withBodyFile("sample-file.xml")));
  }

Tests done:
quarkus 3.15, 3.16 and 3.17 with URLConnectionHTTPConduit and VertxHTTPConduit

Results:
3.15:
I only tested the default URLConectionHTTPConduit here. Test passes as expected.

3.16:
The default VertxHTTP fails with a 302 Error as expected.
The URLConnectionHTTP passes as expected.

3.17:
The URLConnectionHTTP passes.
The VertxHTTP fails with a 404 Error. Stack trace:

Caused by: org.apache.cxf.transport.http.HTTPException: HTTP response '404: Not Found' when communicating with http://localhost:50575/redirect-test
	at io.quarkiverse.cxf.vertx.http.client.VertxHttpClientHTTPConduit$ResponseHandler.doProcessResponseCode(VertxHttpClientHTTPConduit.java:1272)
	at io.quarkiverse.cxf.vertx.http.client.VertxHttpClientHTTPConduit$ResponseHandler.handle(VertxHttpClientHTTPConduit.java:1176)
	at io.quarkiverse.cxf.vertx.http.client.VertxHttpClientHTTPConduit$ResponseHandler.handle(VertxHttpClientHTTPConduit.java:1154)
	at io.quarkiverse.cxf.vertx.http.client.VertxHttpClientHTTPConduit$RequestBodyHandler$Mode$Sync.awaitResponse(VertxHttpClientHTTPConduit.java:1044)
	at io.quarkiverse.cxf.vertx.http.client.VertxHttpClientHTTPConduit$RequestBodyHandler.handle(VertxHttpClientHTTPConduit.java:564)
	at io.quarkiverse.cxf.vertx.http.client.VertxHttpClientHTTPConduit$RequestBodyHandler.handle(VertxHttpClientHTTPConduit.java:425)
	at io.quarkiverse.cxf.vertx.http.client.VertxHttpClientHTTPConduit$RequestBodyOutputStream.close(VertxHttpClientHTTPConduit.java:420)
	at org.apache.cxf.ext.logging.LoggingOutputStream.postClose(LoggingOutputStream.java:53)
	at org.apache.cxf.io.CachedOutputStream.close(CachedOutputStream.java:228)
	at org.apache.cxf.transport.AbstractConduit.close(AbstractConduit.java:56)
	at org.apache.cxf.transport.http.HTTPConduit.close(HTTPConduit.java:717)
	at org.apache.cxf.interceptor.MessageSenderInterceptor$MessageSenderEndingInterceptor.handleMessage(MessageSenderInterceptor.java:63)
	... 103 more

Upon further investigation, removal of .withRequestBody(matchingXPath(...)) in the stub of RequestFinal() made the test pass. So it seems like it did redirect the request, but the body is not included in the redirected request? I tried to look through the source code but could not find the reason quickly.

PS: One tiny bug I did notice is that in VertxHttpClientHTTPConduit.redirectRetransmit, both the URLs in the debug log refer to the redirection URL (in the previous method the new URL is already added to the list, so list.size()-1 is the new URL, not the old one).

PPS: If it's not too difficult to implement, being able to switch on logging the redirection responses and requests might also help with debugging.

Thanks a lot for your help again.

@ppalaga
Copy link
Contributor

ppalaga commented Nov 29, 2024

@dcheng1248 thanks for the report, I have created a new issue #1630 - let's continue there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants