Add RequestContext method to get source address#2922
Conversation
Generate changelog in
|
pkoenig10
force-pushed
the
pkoenig/sourceAddress
branch
from
4851c1e to
0f9c281
Compare
| acceptedBreaks: | ||
| "8.78.0": | ||
| com.palantir.conjure.java:conjure-undertow-lib: | ||
| - code: "java.method.addedToInterface" | ||
| new: "method java.net.InetSocketAddress com.palantir.conjure.java.undertow.lib.RequestContext::sourceAddress()" | ||
| justification: "Added method to interface that should not have external implementations" |
There was a problem hiding this comment.
@pkoenig10 Just for SA, we have ~20 internal repos that implement this interface (mostly to create fake/test contexts) https://pl.ntr/2BN
This will unfortunately prevent them from upgrading without breaks (I don't disagree with your statement that these should not exist though)
There was a problem hiding this comment.
Hmm that is unfortunate. Do you think this prevents us from merging this?
There was a problem hiding this comment.
How about making sourceAddress() return an Optional and default it to empty instead?
Reading through the HttpServerExchange#getSourceAddress code (and things it calls), it appears this could return null (notably, if the underlying connection is a org.xnio.nio.NioPipeStreamConnection, though I don't think we'd hit that case internally).
We could also add a convenience method that returns the value or throws if absent (e.g. maybeSourceAddress() and sourceAddress() or something)
| */ | ||
| void requestArg(Arg<?> arg); | ||
|
|
||
| InetSocketAddress sourceAddress(); |
There was a problem hiding this comment.
Could you add some javadoc to this? (every other method has it, and it feels a bit weird not to have one - one thing I would potentially specify is that this is the IP address, as opposed to the hostname?)
There was a problem hiding this comment.
Ah yup, sorry, I intended to add Javadoc here.
2 participants
There are a number of existing use cases for wanting to know the source address of a request. For example, to include in audit logs.
Today we use
HttpServerExchangedirectly for these code paths, but it would be nice to be able to use the read-onlyReqeustContextinstead. This information feels aligned with the purpose ofRequestContext(to provide read-only information about a request) and seems reasonable to include here.