Fix a race condition in Derived Field parsing from search request

Signed-off-by: Rishabh Maurya <rishabhmaurya05@gmail.com>
opensearch-project · Jun 21, 2024 · cdbfb0e · cdbfb0e
1 parent f5dbbb0
commit cdbfb0e
Show file tree

Hide file tree

Showing 6 changed files with 66 additions and 42 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - [Remote Store] Rate limiter for remote store low priority uploads ([#14374](https://github.com/opensearch-project/OpenSearch/pull/14374/))
 - Apply the date histogram rewrite optimization to range aggregation ([#13865](https://github.com/opensearch-project/OpenSearch/pull/13865))
 - [Writable Warm] Add composite directory implementation and integrate it with FileCache ([12782](https://github.com/opensearch-project/OpenSearch/pull/12782))
+- Fix a race condition in derived field defined in search request causing flaky test ([14445](https://github.com/opensearch-project/OpenSearch/pull/14445))
 
 ### Dependencies
 - Bump `org.gradle.test-retry` from 1.5.8 to 1.5.9 ([#13442](https://github.com/opensearch-project/OpenSearch/pull/13442))

diff --git a/modules/ingest-common/src/main/java/org/opensearch/ingest/common/CopyProcessor.java b/modules/ingest-common/src/main/java/org/opensearch/ingest/common/CopyProcessor.java
@@ -8,6 +8,7 @@
 
 package org.opensearch.ingest.common;
 
+import org.opensearch.common.util.CopyUtil;
 import org.opensearch.core.common.Strings;
 import org.opensearch.ingest.AbstractProcessor;
 import org.opensearch.ingest.ConfigurationUtils;
@@ -95,7 +96,7 @@ public IngestDocument execute(IngestDocument document) {
 
         if (overrideTarget || document.hasField(target, true) == false || document.getFieldValue(target, Object.class) == null) {
             Object sourceValue = document.getFieldValue(source, Object.class);
-            document.setFieldValue(target, IngestDocument.deepCopy(sourceValue));
+            document.setFieldValue(target, CopyUtil.deepCopy(sourceValue));
         } else {
             throw new IllegalArgumentException("target field [" + target + "] already exists");
         }

diff --git a/server/src/main/java/org/opensearch/common/util/CopyUtil.java b/server/src/main/java/org/opensearch/common/util/CopyUtil.java
@@ -0,0 +1,57 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.common.util;
+
+import java.time.ZonedDateTime;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class CopyUtil {
+
+    public static Object deepCopy(Object value) {
+        if (value instanceof Map) {
+            Map<?, ?> mapValue = (Map<?, ?>) value;
+            Map<Object, Object> copy = new HashMap<>(mapValue.size());
+            for (Map.Entry<?, ?> entry : mapValue.entrySet()) {
+                copy.put(entry.getKey(), deepCopy(entry.getValue()));
+            }
+            return copy;
+        } else if (value instanceof List) {
+            List<?> listValue = (List<?>) value;
+            List<Object> copy = new ArrayList<>(listValue.size());
+            for (Object itemValue : listValue) {
+                copy.add(deepCopy(itemValue));
+            }
+            return copy;
+        } else if (value instanceof byte[]) {
+            byte[] bytes = (byte[]) value;
+            return Arrays.copyOf(bytes, bytes.length);
+        } else if (value == null
+            || value instanceof Byte
+            || value instanceof Character
+            || value instanceof Short
+            || value instanceof String
+            || value instanceof Integer
+            || value instanceof Long
+            || value instanceof Float
+            || value instanceof Double
+            || value instanceof Boolean
+            || value instanceof ZonedDateTime) {
+                return value;
+            } else if (value instanceof Date) {
+                return ((Date) value).clone();
+            } else {
+                throw new IllegalArgumentException("unexpected value type [" + value.getClass() + "]");
+            }
+    }
+}
diff --git a/server/src/main/java/org/opensearch/index/mapper/DefaultDerivedFieldResolver.java b/server/src/main/java/org/opensearch/index/mapper/DefaultDerivedFieldResolver.java
@@ -12,6 +12,7 @@
 import org.apache.logging.log4j.Logger;
 import org.opensearch.common.regex.Regex;
 import org.opensearch.index.query.QueryShardContext;
+import org.opensearch.ingest.IngestDocument;
 import org.opensearch.script.Script;
 
 import java.io.IOException;
@@ -189,9 +190,10 @@ private void initDerivedFieldTypes(Map<String, Object> derivedFieldsObject, List
 
     private Map<String, DerivedFieldType> getAllDerivedFieldTypeFromObject(Map<String, Object> derivedFieldObject) {
         Map<String, DerivedFieldType> derivedFieldTypes = new HashMap<>();
+        // deep copy of derivedFieldObject is required as DocumentMapperParser modifies the map
         DocumentMapper documentMapper = queryShardContext.getMapperService()
             .documentMapperParser()
-            .parse(DerivedFieldMapper.CONTENT_TYPE, derivedFieldObject);
+            .parse(DerivedFieldMapper.CONTENT_TYPE, IngestDocument.deepCopyMap(derivedFieldObject));
         if (documentMapper != null && documentMapper.mappers() != null) {
             for (Mapper mapper : documentMapper.mappers()) {
                 if (mapper instanceof DerivedFieldMapper) {

diff --git a/server/src/main/java/org/opensearch/index/store/FsDirectoryFactory.java b/server/src/main/java/org/opensearch/index/store/FsDirectoryFactory.java
@@ -115,6 +115,7 @@ protected Directory newFSDirectory(Path location, LockFactory lockFactory, Index
                     // Otherwise, get the list of nio extensions from the nio setting
                     nioExtensions = Set.copyOf(indexSettings.getValue(IndexModule.INDEX_STORE_HYBRID_NIO_EXTENSIONS));
                 }
+                System.out.println(nioExtensions);
                 if (primaryDirectory instanceof MMapDirectory) {
                     MMapDirectory mMapDirectory = (MMapDirectory) primaryDirectory;
                     return new HybridDirectory(lockFactory, setPreload(mMapDirectory, lockFactory, preLoadExtensions), nioExtensions);

diff --git a/server/src/main/java/org/opensearch/ingest/IngestDocument.java b/server/src/main/java/org/opensearch/ingest/IngestDocument.java
@@ -32,6 +32,7 @@
 
 package org.opensearch.ingest;
 
+import org.opensearch.common.util.CopyUtil;
 import org.opensearch.core.common.Strings;
 import org.opensearch.core.common.util.CollectionUtils;
 import org.opensearch.index.VersionType;
@@ -45,10 +46,8 @@
 import java.time.ZoneOffset;
 import java.time.ZonedDateTime;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Base64;
 import java.util.Collections;
-import java.util.Date;
 import java.util.EnumMap;
 import java.util.HashMap;
 import java.util.LinkedHashSet;
@@ -754,44 +753,7 @@ public Map<String, Object> getSourceAndMetadata() {
     @SuppressWarnings("unchecked")
     public static <K, V> Map<K, V> deepCopyMap(Map<K, V> source) {
         CollectionUtils.ensureNoSelfReferences(source, "IngestDocument: Self reference present in object.");
-        return (Map<K, V>) deepCopy(source);
-    }
-
-    public static Object deepCopy(Object value) {
-        if (value instanceof Map) {
-            Map<?, ?> mapValue = (Map<?, ?>) value;
-            Map<Object, Object> copy = new HashMap<>(mapValue.size());
-            for (Map.Entry<?, ?> entry : mapValue.entrySet()) {
-                copy.put(entry.getKey(), deepCopy(entry.getValue()));
-            }
-            return copy;
-        } else if (value instanceof List) {
-            List<?> listValue = (List<?>) value;
-            List<Object> copy = new ArrayList<>(listValue.size());
-            for (Object itemValue : listValue) {
-                copy.add(deepCopy(itemValue));
-            }
-            return copy;
-        } else if (value instanceof byte[]) {
-            byte[] bytes = (byte[]) value;
-            return Arrays.copyOf(bytes, bytes.length);
-        } else if (value == null
-            || value instanceof Byte
-            || value instanceof Character
-            || value instanceof Short
-            || value instanceof String
-            || value instanceof Integer
-            || value instanceof Long
-            || value instanceof Float
-            || value instanceof Double
-            || value instanceof Boolean
-            || value instanceof ZonedDateTime) {
-                return value;
-            } else if (value instanceof Date) {
-                return ((Date) value).clone();
-            } else {
-                throw new IllegalArgumentException("unexpected value type [" + value.getClass() + "]");
-            }
+        return (Map<K, V>) CopyUtil.deepCopy(source);
     }
 
     /**