Adds percent-encoding for Location headers #21057

a2lin · 2016-10-21T08:29:40Z

This pr should cause unicode elements in the location header to be percent-encoded, instead of being left alone.

For the cases mentioned by @weltenwort in #21016, they now return:

curl -XPUT -v 'http://localhost:9200/someindex/sometype/%C3%A4' -d {}
< HTTP/1.1 201 Created
< Location: /someindex/sometype/%C3%A4
< content-type: application/json; charset=UTF-8
< content-length: 148
<

curl -XPUT -v 'http://localhost:9200/someindex/sometype/%E2%9D%A4' -d '{}'
< HTTP/1.1 201 Created
< Location: /someindex/sometype/%E2%9D%A4
< content-type: application/json; charset=UTF-8
< content-length: 149
<

The above responses compare favorably with the responses from a checkout of current master:

curl -XPUT -v 'http://localhost:9200/someindex/sometype/%C3%A4' -d {}
< HTTP/1.1 201 Created
< Location: /someindex/sometype/‰
< content-type: application/json; charset=UTF-8
< content-length: 147
<

curl -XPUT -v 'http://localhost:9200/someindex/sometype/%E2%9D%A4' -d '{}'
< HTTP/1.1 201 Created
< Location: /someindex/sometype/?
< content-type: application/json; charset=UTF-8
< content-length: 149
<

Closes #21016

jasontedor

I left a comment.

jasontedor · 2016-10-21T09:19:13Z

core/src/main/java/org/elasticsearch/action/DocWriteResponse.java

+        try {
+            URI uri = new URI(location.toString());
+            return uri.toASCIIString();
+        } catch (URISyntaxException ex) {


I think using an exception for flow control should be avoided here. Is there another way?

Additionally, when using an exception for flow control like this, it at a minimum needs to be commented to explain what is going on.

I'm fairly sure Location is supposed to be a valid URI. I'd be ok with catching the exception and logging a warning for it unless we expect this to fail frequently. If we do then we shouldn't be using URI.

nik9000 · 2016-10-21T12:18:07Z

core/src/main/java/org/elasticsearch/action/DocWriteResponse.java

+        try {
+            URI uri = new URI(location.toString());
+            return uri.toASCIIString();
+        } catch (URISyntaxException ex) {


I'm fairly sure Location is supposed to be a valid URI. I'd be ok with catching the exception and logging a warning for it unless we expect this to fail frequently. If we do then we shouldn't be using URI.

a2lin · 2016-10-22T06:27:51Z

@jasontedor, @nik9000 I've collapsed the try/catch return statements into a single return statement, commented it and added a logging statement. I don't expect the exception case to be hit at all, as the Location header should contain a valid URI.

Thanks for the comments and reviews!

nik9000 · 2016-10-31T17:43:54Z

core/src/main/java/org/elasticsearch/action/DocWriteResponse.java

+            locationString = uri.toASCIIString();
+        } catch (URISyntaxException ex) {
+            // could not encode URI properly, using toString instead.
+            logger.debug("Location string is not a valid URI.");


Could you add the exception to the log statement? If we really don't expect this to happen I think it should be a warn level. If someone is able to reproduce it then they can silence it with the update settings API before we work around it.

Now that I think about it, it'd be nicer to throw the URISyntaxException out of the method and then catch it when we go to write the location, warning there instead of here. That way we don't add the Location header with an empty string.

a2lin · 2016-11-10T05:57:10Z

@nik9000 @jasontedor I've rebased onto latest master. Returning null from this method will cause the downstream consumer to not add the 'Location' header to the response. We expect to almost never return null.

nik9000 · 2016-11-10T16:12:57Z

core/src/main/java/org/elasticsearch/action/DocWriteResponse.java

+        try {
+            URI uri = new URI(location.toString());
+            locationString = uri.toASCIIString();
+        } catch (URISyntaxException ex) {


I think I was thinking that it'd be better to just throw the exception and catch it in the caller. That way the caller can log the warning and not add the location header.

a2lin · 2016-11-11T16:55:54Z

@nik9000 I've made the change and added a test for the exception behavior.

jasontedor

Thank you @a2lin, I'm much happier with this now. I left a request for one more change.

jasontedor · 2016-11-11T17:02:51Z

core/src/main/java/org/elasticsearch/action/DocWriteResponse.java


    public DocWriteResponse(ShardId shardId, String type, String id, long version, Result result) {
        this.shardId = shardId;
        this.type = type;
        this.id = id;
        this.version = version;
        this.result = result;
+        this.logger = LogManager.getLogger(getClass());


This shouldn't be here. You should use ESLoggerFactory instead.

Actually, I think this logger can be removed now (thankfully).

jasontedor · 2016-11-11T17:03:03Z

core/src/main/java/org/elasticsearch/action/DocWriteResponse.java

@@ -176,7 +182,7 @@ public RestStatus status() {
     * Gets the location of the written document as a string suitable for a {@code Location} header.
     * @param routing any routing used in the request. If null the location doesn't include routing information.
     */
-    public String getLocation(@Nullable String routing) {
+    public String getLocation(@Nullable String routing) throws URISyntaxException {


This is much better.

jasontedor

I looked more closely, I think the logger can just go now.

jasontedor · 2016-11-11T17:05:11Z

core/src/main/java/org/elasticsearch/action/DocWriteResponse.java


    public DocWriteResponse(ShardId shardId, String type, String id, long version, Result result) {
        this.shardId = shardId;
        this.type = type;
        this.id = id;
        this.version = version;
        this.result = result;
+        this.logger = LogManager.getLogger(getClass());


Actually, I think this logger can be removed now (thankfully).

a2lin · 2016-11-14T10:52:46Z

@jasontedor Oops, sorry 'bout that. I've killed the logger.
Thanks for the catch!

a2lin · 2016-12-14T06:20:59Z

@jasontedor @nik9000 rebased to master. Was there something else I should have fixed?

nik9000

I left a small thing but it looks good to me.

nik9000 · 2016-12-14T16:42:29Z

core/src/test/java/org/elasticsearch/action/DocWriteResponseTests.java

+            };
+        try {
+            invalid.getLocation(null);
+            fail("Did not correctly throw exception on invalid Location parameters");


I'd use expectThrows here. I also like assertion something about the exception's massage just to make sure I caught the one I expected.

Closes elastic#21016

a2lin · 2016-12-15T06:55:34Z

@nik9000 Wow, did not know that expectThrows was a thing. Nifty.

nik9000 · 2016-12-19T19:01:15Z

This looks good to me now.

elasticmachine, please test this

nik9000 · 2016-12-19T20:56:53Z

Thanks @a2lin! Sorry it took me so long to review this. I've merged to master and I'll backport to 5.x so it should go out with 5.2.

master: 0ab3cbe
5.x: 8e3d09d

This should cause unicode elements in the location header to be percent-encoded, instead of being left alone. Closes #21016

a2lin · 2016-12-20T04:57:04Z

Thanks for reviewing!

jasontedor requested changes Oct 21, 2016

View reviewed changes

nik9000 requested changes Oct 21, 2016

View reviewed changes

nik9000 added >enhancement :Core/Infra/REST API REST infrastructure and utilities v6.0.0-alpha1 v5.1.1 labels Oct 21, 2016

nik9000 requested changes Oct 31, 2016

View reviewed changes

clintongormley added >bug and removed >enhancement labels Nov 5, 2016

a2lin force-pushed the encode_location_header branch 2 times, most recently from dd74d08 to 5dfa3a9 Compare November 10, 2016 05:54

nik9000 reviewed Nov 10, 2016

View reviewed changes

jasontedor requested changes Nov 11, 2016

View reviewed changes

clintongormley added v5.2.0 and removed >enhancement v5.1.1 labels Dec 7, 2016

a2lin force-pushed the encode_location_header branch from c63f339 to d9b8b42 Compare December 14, 2016 06:12

nik9000 approved these changes Dec 14, 2016

View reviewed changes

Adds percent-encoding for Location headers

4e64ba3

Closes elastic#21016

a2lin force-pushed the encode_location_header branch from d9b8b42 to 4e64ba3 Compare December 15, 2016 05:43

nik9000 merged commit 0ab3cbe into elastic:master Dec 19, 2016

nik9000 pushed a commit that referenced this pull request Dec 19, 2016

Adds percent-encoding for Location headers (#21057)

8e3d09d

This should cause unicode elements in the location header to be percent-encoded, instead of being left alone. Closes #21016

weltenwort mentioned this pull request Dec 20, 2016

Remove Location header encoding workaround from ES proxy handler elastic/kibana#9567

Closed

jasontedor mentioned this pull request Feb 12, 2017

Properly encode location header #23133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds percent-encoding for Location headers #21057

Adds percent-encoding for Location headers #21057

a2lin commented Oct 21, 2016

jasontedor left a comment

jasontedor Oct 21, 2016

jasontedor Oct 21, 2016

nik9000 Oct 21, 2016

nik9000 Oct 21, 2016

a2lin commented Oct 22, 2016 •

edited

Loading

nik9000 Oct 31, 2016

a2lin commented Nov 10, 2016

nik9000 Nov 10, 2016

a2lin commented Nov 11, 2016

jasontedor left a comment

jasontedor Nov 11, 2016

jasontedor Nov 11, 2016

jasontedor Nov 11, 2016

jasontedor left a comment

jasontedor Nov 11, 2016

a2lin commented Nov 14, 2016

a2lin commented Dec 14, 2016

nik9000 left a comment

nik9000 Dec 14, 2016

a2lin commented Dec 15, 2016

nik9000 commented Dec 19, 2016

nik9000 commented Dec 19, 2016 •

edited

Loading

a2lin commented Dec 20, 2016

Adds percent-encoding for Location headers #21057

Adds percent-encoding for Location headers #21057

Conversation

a2lin commented Oct 21, 2016

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a2lin commented Oct 22, 2016 • edited Loading

Choose a reason for hiding this comment

a2lin commented Nov 10, 2016

Choose a reason for hiding this comment

a2lin commented Nov 11, 2016

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a2lin commented Nov 14, 2016

a2lin commented Dec 14, 2016

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a2lin commented Dec 15, 2016

nik9000 commented Dec 19, 2016

nik9000 commented Dec 19, 2016 • edited Loading

a2lin commented Dec 20, 2016

a2lin commented Oct 22, 2016 •

edited

Loading

nik9000 commented Dec 19, 2016 •

edited

Loading