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

Add quarkus.cxf.client."client-name".max-same-uri configuration option #1639

Closed
dcheng1248 opened this issue Dec 3, 2024 · 14 comments · Fixed by #1669
Closed

Add quarkus.cxf.client."client-name".max-same-uri configuration option #1639

dcheng1248 opened this issue Dec 3, 2024 · 14 comments · Fixed by #1669
Assignees

Comments

@dcheng1248
Copy link
Contributor

The default implementation of the HTTP Conduit now uses the VertxHttpClientHTTPConduit, which throws an Redirection Loop error when redirected to the same URL if the http.redirect.max.same.uri.count property is not set. There is no quarkus property corresponding to this property and it needs to be set as a system property when starting the app.

This is in contrast to the original default (in quarkus 3.15 and before) URLConnectionHTTPConduit where redirection (also to the same URL) are automatically enabled so no properties need to be set.

Would it be possible to add a quarkus property corresponding to this for easier usage? Thank you!

@dcheng1248
Copy link
Contributor Author

@ppalaga I'm not sure how contribution to quarkus works, but any chance I could perhaps try my hands on this in my free time and submit a PR?

@ppalaga
Copy link
Contributor

ppalaga commented Dec 4, 2024

That would be highly welcome! You may want to have a look at recent commits to see where to add the feature and a test. Let me assign this issue to you.

@ppalaga
Copy link
Contributor

ppalaga commented Dec 4, 2024

We have a basic Contributor guide, FWIW.

@dcheng1248
Copy link
Contributor Author

Thank you! I will try my best, but it might take a bit of time to learn the workflow. Please let me know if it becomes an urgent issue and I'm delaying progress!

@dcheng1248
Copy link
Contributor Author

@ppalaga so I've been trying to get my hands on it a little. One problem that I'm running into (unrelated to this issue) is when running mvn verify, the AntoraTest in the doc module hangs. I've poked around a bit but made no progress. I use Podman instead of Docker. Any advice there - am I missing something? Thanks!

@ppalaga
Copy link
Contributor

ppalaga commented Dec 13, 2024

Does your machine have an ARM CPU? If so,

adding

quarkus.antora.image = docker.io/antora/antora:3.1.10

to docs/src/main/resources/application.properties might help.

3.1.10 is the first build of Antora container for linux/arm64

I merged prepared that upgrade to quarkus-antora just yesterday and it would actually be good to release quarkus-antora so that folks on Apple silicon do not need to apply the workaround anymore.

@dcheng1248
Copy link
Contributor Author

dcheng1248 commented Dec 13, 2024

I'm on apple M1 so that is probably it. I'll try the new version - thanks!

Edit: the tests now work with the new image version.

@ppalaga
Copy link
Contributor

ppalaga commented Dec 13, 2024

quarkus-antora is now at 1.0.1, where quarkus.antora.image defaults to docker.io/antora/antora:3.1.10. So you just need to rebase.

@dcheng1248
Copy link
Contributor Author

@ppalaga dumb question: is there a way to enable cookie session maintenance or is that part of #1616 ?

I'm testing the new property and trying to get the test service to return a cookie that stores the number of self redirections for counting. The response comes back with the correct set-cookie header, but the cookie does not get added to the new headers because the maintain session flag of the cookies in the conduit is set to false. I've dug around a bit but failed to find a way to enable this.

I can work around this but the test service won't be as pretty... and I guess cookie session maintenance is also good to know about in general!

@dcheng1248
Copy link
Contributor Author

@ppalaga I created a PR towards my own forked main, can you take a look? I tried my best to match the formatting of the code but there may still be things off. For the test service I used the workaround for now. If things look OK later I can create a PR towards the cxf main.

Side note: when I do mvn install -Pnative I get an error:
WsReliableMessagingIT>WsReliableMessagingTest.testTwowayMessageLoss:38 » Runtime Timed out waiting for Installed features: [ in target/ws-rm-server-native.log
However I get this also on the main so I don't think it's my changes. However, I cannot figure out what the problem is - I am admittedly super unfamiliar with the native build as I've never worked with it before.
All tests pass on JVM.

@ppalaga
Copy link
Contributor

ppalaga commented Dec 16, 2024

I created a PR

Thanks a lot! I commented there.

@dcheng1248
Copy link
Contributor Author

@ppalaga thank you! I've got my hands a bit full these few days but will get back onto it once I can. Note: having this quarkus property does not solve my full issue with the server, now it complains that we try to retransmit to a closed connection...

I will try to go through the URLConnectionHTTPConduit and the VertxHTTPConduit in dev mode to see if I can pinpoint the exact cause for the difference in behaviour. Would you prefer if I find it out and fix that before submitting a PR to main, or is creating this quarkus property anyway helpful?

@ppalaga ppalaga changed the title Adding a quarkus property for setting the http.redirect.max.same.uri.count property Add quarkus.cxf.client."client-name".max-same-uri configuration option Dec 20, 2024
@dcheng1248
Copy link
Contributor Author

@ppalaga I just wanted to say thank you again for helping me make a tiny contribution! You did most of the work but it's still my first ever OS contribution and it makes me proud. As mentioned in the previous comment, there still seems to be some difference in behaviour (our actual issue on prod is not solved), but I will investigate more and open another issue for this.

@ppalaga
Copy link
Contributor

ppalaga commented Dec 23, 2024

Thank you very much @dcheng1248 for your part! I am happy I could help.

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