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 support for remote ip/address without requiring HttpServletRequest #2176

Merged
merged 1 commit into from
Oct 9, 2019

Conversation

FroMage
Copy link
Contributor

@FroMage FroMage commented Oct 3, 2019

New interface ResteasyRequest extends JAX-RS Request with those methods
But it delegates to HttpRequest (perhaps we don't need both?)

This works, but we need to make some decisions:

  • Do we keep ResteasyRequest as a subtype of JAX-RS Request, or do we tell users to inject RESTEasy's HttpRequest which has to have those methods anyway?
  • Do we keep the String getRemoteAddress/getRemoteHost duo or go for something like InetSocketAddress getRemoteAddress like Netty has?

WDYT @patriot1burke @asoldano ?

@FroMage
Copy link
Contributor Author

FroMage commented Oct 3, 2019

Hah, saw an SSE test fail, but this is not due to my changes this time. That makes me think that perhaps it wasn't my changes which were guilty in the async io branch…

Copy link
Member

@asoldano asoldano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @FroMage . To answer your questions, I would actually prefer telling the users to inject HttpRequest and get rid of ResteasyRequest, if possible. Users already inject HttpRequest for few other scenarios.
I'm fine with getRemoteAddress() and getRemoteHost().

@asoldano asoldano added the main label Oct 8, 2019
@FroMage
Copy link
Contributor Author

FroMage commented Oct 8, 2019

OK, thanks for the feedback. I will udpate the PR.

@asoldano
Copy link
Member

asoldano commented Oct 8, 2019

Thanks!

@FroMage FroMage force-pushed the http-servlet-request-replacement branch from 837cad6 to b3f1122 Compare October 8, 2019 13:49
@FroMage
Copy link
Contributor Author

FroMage commented Oct 8, 2019

Done.

@FroMage
Copy link
Contributor Author

FroMage commented Oct 9, 2019

SseTest.testReconnect:309 expected:<0> but was:<1> failed again in one build, but this is not related to my PR. Restarted it.

@FroMage FroMage merged commit 7fb9869 into resteasy:master Oct 9, 2019
@FroMage
Copy link
Contributor Author

FroMage commented Oct 9, 2019

Huh, @asoldano I merged it because I thought you had accepted the PR, but now I checked and it's not clear. Is it alright or did I mess up?

@asoldano
Copy link
Member

@FroMage no problem for having merged it, I would have done that anyway (just haven't had time yesterday). The only caveat for next time would be that we require all commits to have a reference to the relevant jira, which in this case is missing (btw, if the original commit message does not have the reference, a trick is to add it merging the PR with "Squash and merge" as that allows editing the message)

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

Successfully merging this pull request may close these issues.

2 participants