Properly encode location header

Today when trying to encode the location header to ASCII, we rely on the
Java URI API. This API requires a proper URI which blows up whenever the
URI contains, for example, a space (which can happen if the type, ID, or
routing contain a space). This commit addresses this issue by properly
encoding the URI. Additionally, we remove the need to create a URI
simplifying the code flow.

Relates #23133

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

@@ -38,8 +38,11 @@ import org.elasticsearch.index.shard.ShardId;
 import org.elasticsearch.rest.RestStatus;
 
 import java.io.IOException;
+import java.io.UnsupportedEncodingException;
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.net.URLEncoder;
+import java.nio.charset.Charset;
 import java.util.Locale;
 
 import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
@@ -201,31 +204,43 @@ public abstract class DocWriteResponse extends ReplicationResponse implements Wr
     }
 
     /**
-     * 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.
+     * Return the relative URI for the location of the document suitable for use in the {@code Location} header. The use of relative URIs is
+     * permitted as of HTTP/1.1 (cf. https://tools.ietf.org/html/rfc7231#section-7.1.2).
      *
+     * @param routing custom routing or {@code null} if custom routing is not used
+     * @return the relative URI for the location of the document
      */
-    public String getLocation(@Nullable String routing) throws URISyntaxException {
-        // Absolute path for the location of the document. This should be allowed as of HTTP/1.1:
-        // https://tools.ietf.org/html/rfc7231#section-7.1.2
-        String index = getIndex();
-        String type = getType();
-        String id = getId();
-        String routingStart = "?routing=";
-        int bufferSize = 3 + index.length() + type.length() + id.length();
-        if (routing != null) {
-            bufferSize += routingStart.length() + routing.length();
+    public String getLocation(@Nullable String routing) {
+        final String encodedIndex;
+        final String encodedType;
+        final String encodedId;
+        final String encodedRouting;
+        try {
+            // encode the path components separately otherwise the path separators will be encoded
+            encodedIndex = URLEncoder.encode(getIndex(), "UTF-8");
+            encodedType = URLEncoder.encode(getType(), "UTF-8");
+            encodedId = URLEncoder.encode(getId(), "UTF-8");
+            encodedRouting = routing == null ? null : URLEncoder.encode(routing, "UTF-8");
+        } catch (final UnsupportedEncodingException e) {
+            throw new AssertionError(e);
         }
-        StringBuilder location = new StringBuilder(bufferSize);
-        location.append('/').append(index);
-        location.append('/').append(type);
-        location.append('/').append(id);
-        if (routing != null) {
-            location.append(routingStart).append(routing);
+        final String routingStart = "?routing=";
+        final int bufferSizeExcludingRouting = 3 + encodedIndex.length() + encodedType.length() + encodedId.length();
+        final int bufferSize;
+        if (encodedRouting == null) {
+            bufferSize = bufferSizeExcludingRouting;
+        } else {
+            bufferSize = bufferSizeExcludingRouting + routingStart.length() + encodedRouting.length();
+        }
+        final StringBuilder location = new StringBuilder(bufferSize);
+        location.append('/').append(encodedIndex);
+        location.append('/').append(encodedType);
+        location.append('/').append(encodedId);
+        if (encodedRouting != null) {
+            location.append(routingStart).append(encodedRouting);
         }
 
-        URI uri = new URI(location.toString());
-        return uri.toASCIIString();
+        return location.toString();
     }
 
     @Override

core/src/main/java/org/elasticsearch/rest/action/RestStatusToXContentListener.java

@@ -57,7 +57,7 @@ public class RestStatusToXContentListener<Response extends StatusToXContentObjec
         response.toXContent(builder, channel.request());
         RestResponse restResponse = new BytesRestResponse(response.status(), builder);
         if (RestStatus.CREATED == restResponse.status()) {
-            String location = extractLocation.apply(response);
+            final String location = extractLocation.apply(response);
             if (location != null) {
                 restResponse.addHeader("Location", location);
             }

core/src/main/java/org/elasticsearch/rest/action/document/RestIndexAction.java

@@ -31,7 +31,6 @@ import org.elasticsearch.rest.action.RestActions;
 import org.elasticsearch.rest.action.RestStatusToXContentListener;
 
 import java.io.IOException;
-import java.net.URISyntaxException;
 
 import static org.elasticsearch.rest.RestRequest.Method.POST;
 import static org.elasticsearch.rest.RestRequest.Method.PUT;
@@ -80,14 +79,7 @@ public class RestIndexAction extends BaseRestHandler {
         }
 
         return channel ->
-            client.index(indexRequest, new RestStatusToXContentListener<>(channel, r -> {
-                try {
-                    return r.getLocation(indexRequest.routing());
-                } catch (URISyntaxException ex) {
-                    logger.warn("Location string is not a valid URI.", ex);
-                    return null;
-                }
-            }));
+                client.index(indexRequest, new RestStatusToXContentListener<>(channel, r -> r.getLocation(indexRequest.routing())));
     }
 
 }

core/src/main/java/org/elasticsearch/rest/action/document/RestUpdateAction.java

@@ -36,7 +36,6 @@ import org.elasticsearch.rest.action.RestStatusToXContentListener;
 import org.elasticsearch.search.fetch.subphase.FetchSourceContext;
 
 import java.io.IOException;
-import java.net.URISyntaxException;
 
 import static org.elasticsearch.rest.RestRequest.Method.POST;
 
@@ -98,13 +97,7 @@ public class RestUpdateAction extends BaseRestHandler {
         });
 
         return channel ->
-            client.update(updateRequest, new RestStatusToXContentListener<>(channel, r -> {
-                try {
-                    return r.getLocation(updateRequest.routing());
-                } catch (URISyntaxException ex) {
-                    logger.warn("Location string is not a valid URI.", ex);
-                    return null;
-                }
-            }));
+                client.update(updateRequest, new RestStatusToXContentListener<>(channel, r -> r.getLocation(updateRequest.routing())));
     }
+
 }

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

@@ -30,55 +30,49 @@ import org.elasticsearch.index.shard.ShardId;
 import org.elasticsearch.test.ESTestCase;
 
 import java.io.IOException;
-import java.net.URISyntaxException;
 
 import static org.hamcrest.Matchers.hasEntry;
 import static org.hamcrest.Matchers.hasKey;
 import static org.hamcrest.Matchers.not;
 
 public class DocWriteResponseTests extends ESTestCase {
-    public void testGetLocation() throws URISyntaxException {
-        DocWriteResponse response =
-            new DocWriteResponse(
-                new ShardId("index", "uuid", 0),
-                "type",
-                "id",
-                SequenceNumbersService.UNASSIGNED_SEQ_NO,
-                0,
-                Result.CREATED) {
-                // DocWriteResponse is abstract so we have to sneak a subclass in here to test it.
-            };
+    public void testGetLocation() {
+        final DocWriteResponse response =
+                new DocWriteResponse(
+                        new ShardId("index", "uuid", 0),
+                        "type",
+                        "id",
+                        SequenceNumbersService.UNASSIGNED_SEQ_NO,
+                        0,
+                        Result.CREATED) {};
         assertEquals("/index/type/id", response.getLocation(null));
         assertEquals("/index/type/id?routing=test_routing", response.getLocation("test_routing"));
     }
 
-    public void testGetLocationNonAscii() throws URISyntaxException {
-        DocWriteResponse response =
-            new DocWriteResponse(
-                new ShardId("index", "uuid", 0),
-                "type",
-                "❤",
-                SequenceNumbersService.UNASSIGNED_SEQ_NO,
-                0,
-                Result.CREATED) {
-            };
+    public void testGetLocationNonAscii() {
+        final DocWriteResponse response =
+                new DocWriteResponse(
+                        new ShardId("index", "uuid", 0),
+                        "type",
+                        "❤",
+                        SequenceNumbersService.UNASSIGNED_SEQ_NO,
+                        0,
+                        Result.CREATED) {};
         assertEquals("/index/type/%E2%9D%A4", response.getLocation(null));
-        assertEquals("/index/type/%E2%9D%A4?routing=%C3%A4", response.getLocation("%C3%A4"));
+        assertEquals("/index/type/%E2%9D%A4?routing=%C3%A4", response.getLocation("ä"));
     }
 
-    public void testInvalidGetLocation() {
-        String invalidPath = "!^*$(@!^!#@";
-        DocWriteResponse invalid =
-            new DocWriteResponse(
-                new ShardId("index", "uuid", 0),
-                "type",
-                invalidPath,
-                SequenceNumbersService.UNASSIGNED_SEQ_NO,
-                0,
-                Result.CREATED) {
-            };
-        Throwable exception = expectThrows(URISyntaxException.class, () -> invalid.getLocation(null));
-        assertTrue(exception.getMessage().contains(invalidPath));
+    public void testGetLocationWithSpaces() {
+        final DocWriteResponse response =
+                new DocWriteResponse(
+                        new ShardId("index", "uuid", 0),
+                        "type",
+                        "a b",
+                        SequenceNumbersService.UNASSIGNED_SEQ_NO,
+                        0,
+                        Result.CREATED) {};
+        assertEquals("/index/type/a+b", response.getLocation(null));
+        assertEquals("/index/type/a+b?routing=c+d", response.getLocation("c d"));
     }
 
     /**