Skip to content

Commit

Permalink
Fix a race condition in Derived Field parsing from search request
Browse files Browse the repository at this point in the history
Signed-off-by: Rishabh Maurya <rishabhmaurya05@gmail.com>
  • Loading branch information
rishabhmaurya committed Jun 21, 2024
1 parent f5dbbb0 commit 9d517d7
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 42 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 ([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))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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");
}
Expand Down
64 changes: 64 additions & 0 deletions server/src/main/java/org/opensearch/common/util/CopyUtil.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* 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;

/**
* Utility class for deep copying Object of different types.
*/
public class CopyUtil {

/**
* Deep copies the provided object
* @return cloned object after deep copy. Throws {@link IllegalArgumentException} for unsupported object type.
*/
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() + "]");
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.opensearch.common.regex.Regex;
import org.opensearch.common.util.CopyUtil;
import org.opensearch.index.query.QueryShardContext;
import org.opensearch.script.Script;

Expand Down Expand Up @@ -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, (Map) CopyUtil.deepCopy(derivedFieldObject));
if (documentMapper != null && documentMapper.mappers() != null) {
for (Mapper mapper : documentMapper.mappers()) {
if (mapper instanceof DerivedFieldMapper) {
Expand Down
42 changes: 2 additions & 40 deletions server/src/main/java/org/opensearch/ingest/IngestDocument.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}

/**
Expand Down

0 comments on commit 9d517d7

Please sign in to comment.