MINOR: Remove Deadcode in NioTransport CORS #34324
Conversation
original-brownbear
commented
Oct 5, 2018
original-brownbear
commented
- Same as MINOR: Remove Dead Code from Netty4Transport #34134 but for nio transport
* Same as elastic#34134 but for nio transport
original-brownbear
added
>non-issue
:Distributed Coordination/Network
|
Pinging @elastic/es-distributed |
| @@ -167,11 +167,6 @@ private void setPreflightHeaders(final HttpResponse response) { | |||
| private boolean setOrigin(final HttpResponse response) { | |||
| final String origin = request.headers().get(HttpHeaderNames.ORIGIN); | |||
| if (!Strings.isNullOrEmpty(origin)) { | |||
| if ("null".equals(origin) && config.isNullOriginAllowed()) { | |||
There was a problem hiding this comment.
This seems needed? Or is this handled in a different way?
| @@ -201,10 +196,6 @@ private boolean validateOrigin() { | |||
| return true; | |||
| } | |||
|
|
|||
| if ("null".equals(origin) && config.isNullOriginAllowed()) { | |||
There was a problem hiding this comment.
This seems needed? Or is this handled in a different way?
There was a problem hiding this comment.
@danielmitterdorfer actually this is always false (there's no code assigning anything to that field in the builder) so this must be handled a different way or we have a bug on our hands ... (looking into that)
Looking at the Netty code it's unused there as well ... I guess I could drop this logic there also?
There was a problem hiding this comment.
It appears unused indeed and I think we don't want/need it but maybe @tbrooks8 or @jasontedor can chime in here?
There was a problem hiding this comment.
I can't find a single usage of allowNullOrigin() (which would set the return of isNullOriginAllowed to true) in git in any revision. I also don't see why we'd want to support "null" for the origin really? (plus it seems this was never set to true before anyway)
|
I’ll take a look at this in the next few days. For context, I’m pretty sure that this class is copied from netty. So that is why there is unused code. At some point we need to consolidate this into a single class that lives in server and operates on the elasticsearch request object. |
|
@tbrooks8 ping :) For what it's worth I'm pretty sure this is copied from Netty since this method was never used so we're probably good removing it here and then eventually just drying this class up as you suggest? |
|
I'll take a look at this tomorrow. |
|
@tbrooks8 thanks! |
* Same as elastic#34324 for the Netty transport, the `isNullOriginAllowed` setting is always false
* Same as #34134 but for nio transport
* Same as #34324 for the Netty transport, the `isNullOriginAllowed` setting is always false
