Enforce 512 byte document ID limit in bulk updates (#8039) (#8081)

* Enforce doc id limit on UpdateRequests as well



* PR comments



* Address comments 2



* Move changelog entry



---------




(cherry picked from commit 60afeb7)

Signed-off-by: Ankit Kala <[email protected]>
Signed-off-by: Andrew Ross <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Andrew Ross <[email protected]>

Author: 3 people

CHANGELOG.md

@@ -53,6 +53,7 @@ Remove `COMPRESSOR` variable from `CompressorFactory` and use `DEFLATE_COMPRESSO
 
 ### Fixed
 -  Fixing error: adding a new/forgotten parameter to the configuration for checking the config on startup in plugins/repository-s3 #7924
+- Enforce 512 byte document ID limit in bulk updates ([#8039](https://github.com/opensearch-project/OpenSearch/pull/8039))
 
 ### Security
 

server/src/internalClusterTest/java/org/opensearch/action/bulk/BulkIntegrationIT.java

@@ -43,6 +43,7 @@
 import org.opensearch.action.ingest.PutPipelineRequest;
 import org.opensearch.action.support.master.AcknowledgedResponse;
 import org.opensearch.action.support.replication.ReplicationRequest;
+import org.opensearch.action.update.UpdateRequest;
 import org.opensearch.cluster.metadata.IndexMetadata;
 import org.opensearch.common.bytes.BytesReference;
 import org.opensearch.core.xcontent.XContentBuilder;
@@ -218,4 +219,38 @@ public void testDeleteIndexWhileIndexing() throws Exception {
         assertFalse(thread.isAlive());
     }
 
+    public void testDocIdTooLong() {
+        String index = "testing";
+        createIndex(index);
+        String validId = String.join("", Collections.nCopies(512, "a"));
+        String invalidId = String.join("", Collections.nCopies(513, "a"));
+
+        // Index Request
+        IndexRequest indexRequest = new IndexRequest(index).source(Collections.singletonMap("foo", "baz"));
+        // Valid id shouldn't throw any exception
+        assertFalse(client().prepareBulk().add(indexRequest.id(validId)).get().hasFailures());
+        // Invalid id should throw the ActionRequestValidationException
+        validateDocIdLimit(() -> client().prepareBulk().add(indexRequest.id(invalidId)).get());
+
+        // Update Request
+        UpdateRequest updateRequest = new UpdateRequest(index, validId).doc("reason", "no source");
+        // Valid id shouldn't throw any exception
+        assertFalse(client().prepareBulk().add(updateRequest).get().hasFailures());
+        // Invalid id should throw the ActionRequestValidationException
+        validateDocIdLimit(() -> client().prepareBulk().add(updateRequest.id(invalidId)).get());
+    }
+
+    private void validateDocIdLimit(Runnable runner) {
+        try {
+            runner.run();
+            fail("Request validation for docId didn't fail");
+        } catch (ActionRequestValidationException e) {
+            assertEquals(
+                1,
+                e.validationErrors().stream().filter(msg -> msg.contains("is too long, must be no longer than 512 bytes but was")).count()
+            );
+        } catch (Exception e) {
+            fail("Request validation for docId failed with different exception: " + e);
+        }
+    }
 }

server/src/main/java/org/opensearch/action/DocWriteRequest.java

@@ -32,6 +32,7 @@
 package org.opensearch.action;
 
 import org.apache.lucene.util.Accountable;
+import org.apache.lucene.util.UnicodeUtil;
 import org.opensearch.action.delete.DeleteRequest;
 import org.opensearch.action.index.IndexRequest;
 import org.opensearch.action.support.IndicesOptions;
@@ -249,6 +250,25 @@ static DocWriteRequest<?> readDocumentRequest(@Nullable ShardId shardId, StreamI
         return docWriteRequest;
     }
 
+    /**
+     * Validates whether the doc id length is under the limit.
+     * @param id DocId to verify
+     * @param validationException containing all the validation errors.
+     * @return validationException
+     */
+    static ActionRequestValidationException validateDocIdLength(String id, ActionRequestValidationException validationException) {
+        if (id != null) {
+            int docIdLength = UnicodeUtil.calcUTF16toUTF8Length(id, 0, id.length());
+            if (docIdLength > 512) {
+                return addValidationError(
+                    "id [" + id + "] is too long, must be no longer than 512 bytes but was: " + docIdLength,
+                    validationException
+                );
+            }
+        }
+        return validationException;
+    }
+
     /** write a document write (index/delete/update) request*/
     static void writeDocumentRequest(StreamOutput out, DocWriteRequest<?> request) throws IOException {
         if (request instanceof IndexRequest) {

server/src/main/java/org/opensearch/action/index/IndexRequest.java

@@ -64,7 +64,6 @@
 import org.opensearch.index.shard.ShardId;
 
 import java.io.IOException;
-import java.nio.charset.StandardCharsets;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
@@ -238,12 +237,7 @@ public ActionRequestValidationException validate() {
 
         validationException = DocWriteRequest.validateSeqNoBasedCASParams(this, validationException);
 
-        if (id != null && id.getBytes(StandardCharsets.UTF_8).length > 512) {
-            validationException = addValidationError(
-                "id [" + id + "] is too long, must be no longer than 512 bytes but was: " + id.getBytes(StandardCharsets.UTF_8).length,
-                validationException
-            );
-        }
+        validationException = DocWriteRequest.validateDocIdLength(id, validationException);
 
         if (pipeline != null && pipeline.isEmpty()) {
             validationException = addValidationError("pipeline cannot be an empty string", validationException);

server/src/main/java/org/opensearch/action/update/UpdateRequest.java

@@ -254,6 +254,9 @@ public ActionRequestValidationException validate() {
         if (doc == null && docAsUpsert) {
             validationException = addValidationError("doc must be specified if doc_as_upsert is enabled", validationException);
         }
+
+        validationException = DocWriteRequest.validateDocIdLength(id, validationException);
+
         return validationException;
     }
 

server/src/test/java/org/opensearch/action/bulk/BulkRequestTests.java

@@ -53,6 +53,7 @@
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -272,6 +273,51 @@ public void testBulkRequestWithRefresh() throws Exception {
         );
     }
 
+    public void testBulkRequestInvalidDocIDDuringCreate() {
+        String validDocID = String.join("", Collections.nCopies(512, "a"));
+        String invalidDocID = String.join("", Collections.nCopies(513, "a"));
+
+        // doc id length under limit
+        IndexRequest indexRequest = new IndexRequest("index").id(validDocID).source(Requests.INDEX_CONTENT_TYPE, "field", "value");
+        BulkRequest bulkRequest = new BulkRequest();
+        bulkRequest.add(indexRequest);
+        assertNull(bulkRequest.validate());
+
+        // doc id length over limit
+        indexRequest.id(invalidDocID);
+        ActionRequestValidationException validate = bulkRequest.validate();
+        assertThat(validate, notNullValue());
+        assertEquals(
+            1,
+            validate.validationErrors()
+                .stream()
+                .filter(msg -> msg.contains("is too long, must be no longer than 512 bytes but was: "))
+                .count()
+        );
+    }
+
+    public void testBulkRequestInvalidDocIDDuringUpdate() {
+        String validDocID = String.join("", Collections.nCopies(512, "a"));
+        String invalidDocID = String.join("", Collections.nCopies(513, "a"));
+        // doc id length under limit
+        UpdateRequest updateRequest = new UpdateRequest("index", validDocID).doc("reason", "no source");
+        BulkRequest bulkRequest = new BulkRequest();
+        bulkRequest.add(updateRequest);
+        assertNull(bulkRequest.validate());
+
+        // doc id length over limit
+        updateRequest.id(invalidDocID);
+        ActionRequestValidationException validate = bulkRequest.validate();
+        assertThat(validate, notNullValue());
+        assertEquals(
+            1,
+            validate.validationErrors()
+                .stream()
+                .filter(msg -> msg.contains("is too long, must be no longer than 512 bytes but was: "))
+                .count()
+        );
+    }
+
     // issue 15120
     public void testBulkNoSource() throws Exception {
         BulkRequest bulkRequest = new BulkRequest();