Remove client address #280
Conversation
dvlato
changed the title
| @Override | ||
| public Optional<InetSocketAddress> clientAddress() { | ||
| return Optional.empty(); | ||
| } |
There was a problem hiding this comment.
Hi @kvosper I notice InetSocketAddress is an Optional. Under what circumstances it is absent? Can you remind me? Apart from unit tests I cannot think of any situation where client address wouldn't be available in the Context.
There was a problem hiding this comment.
When we use the Styx client, there is a line host.hostClient().handle(request, new HttpInterceptorContext()). This requires a context because hostClient returns an HttpHandler, however clientAddress is not available in StyxHttpClient (nor it is relevant).
| * @param secure true if the request was received via SSL | ||
| * @param clientAddress address that request came from, or null if not-applicable | ||
| */ | ||
| public HttpInterceptorContext(boolean secure, InetSocketAddress clientAddress) { |
There was a problem hiding this comment.
Please back off this change to the constructor and the static factory method. This PR is now changing 53 files. Majority of them simply because of this unrelated constructor change. Let's keep this PR strictly to removing the client address. This way it is easier to justify and manage the changes, and to review the PRs.
Whether or not it was good idea back in the days to hide the constructor in favour of static factory method is another matter. If you feel strongly about it then please propose that in a separate pull request.
This change will also invalidate any plugins that use HttpInterceptorContext.create in unit tests.
There was a problem hiding this comment.
I have added back the method HttpInterceptorContext.create() and reverted tests that use the new constructor.
| } | ||
|
|
||
| // TODO deprecate | ||
| public static HttpInterceptor.Context create() { |
There was a problem hiding this comment.
BTW why are you so keen to deprecate this?
There was a problem hiding this comment.
The static-factory-method isn't really adding anything. It seems to just be a totally redundant wrapper for the constructor.
Sometimes when we add static-factory-methods to our classes it's to make it read more fluently when statically-imported, but this is just called create so it doesn't have that benefit.
It just changes new HttpInterceptorContext() to HttpInterceptorContext.create() which is actually longer and less convenient.
| @@ -159,7 +159,8 @@ class ConditionRouterConfigSpec extends FunSpec with ShouldMatchers with Mockito | |||
| |""".stripMargin) | |||
|
|
|||
| val router = new ConditionRouter.ConfigFactory().build(List(), routeHandlerFactory, config) | |||
| val resp = StyxFutures.await(router.handle(request, HttpInterceptorContext.create(false)).asCompletableFuture()) | |||
|
|
|||
| val resp = StyxFutures.await(router.handle(request, new HttpInterceptorContext()).asCompletableFuture()) | |||
|
|
|||
| resp.status() should be(BAD_GATEWAY) | |||
| } | |||
There was a problem hiding this comment.
Are the changes in this file still necessary?
There was a problem hiding this comment.
I put back the old HttpInterceptorContext.create() but not HttpInterceptorContext.create(boolean) as I assume we do not want to set a precedent of following that convention (see my reply to your other comment of why I think the create methods are bad).
| public HttpInterceptorContext(boolean secure) { | ||
| this.secure = secure; | ||
| private static InetSocketAddress remoteAddress(ChannelHandlerContext ctx) { | ||
| if (ctx.channel() instanceof EmbeddedChannel) { |
There was a problem hiding this comment.
Might be worth adding a comment that this is just for unit testing?
There was a problem hiding this comment.
I only moved this method, it's not new.
|
|
||
| @Test | ||
| public void matchesHttpProtocol() throws Exception { | ||
| public void matchesHttpProtocol() { | ||
| AntlrMatcher matcher = AntlrMatcher.antlrMatcher("protocol() == 'https'"); |
There was a problem hiding this comment.
These changes can also be backed off?
|
|
||
| </dependencies> |
There was a problem hiding this comment.
Is this change relevant? Looks like it could be backed off?
Let's try to minimise unnecessary changes. Later on if or when we need to consult the Git change log for the relevant changes, perhaps to find out what merge caused a bug, perhaps for another reason, irrelevant or stylistic changes are just a distraction. Therefore they should go into separate pull requests if possible.
There was a problem hiding this comment.
I moved it because I got briefly confused about the module relationships due to the styx-common dependency not being situated near styx-api, and wanting to prevent such confusion in the future. I don't think it merits an entirely separate PR though.
| @@ -37,28 +44,42 @@ public RequestEnrichingInterceptor(StyxHeaderConfig styxHeaderConfig) { | |||
|
|
|||
| @Override | |||
| public StyxObservable<HttpResponse> intercept(HttpRequest request, Chain chain) { | |||
| return chain.proceed(enrich(request, chain.context().isSecure())); | |||
| return chain.proceed(enrich(request, chain)); | |||
There was a problem hiding this comment.
I would pass in the Context only, as enrich doesn't need the full chain.
Implements #275