Allow Dynamic PropertiesSupport

[aparajita31pandey]

Description

[Describe what this change achieves]

Related Issues

Resolves [#20397]

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit dec15f3)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Add DynamicProperty support to RootObjectMapper (parsing, merging, serialization, validation) Relevant files: server/src/main/java/org/opensearch/index/mapper/RootObjectMapper.java Sub-PR theme: Apply DynamicProperty during document parsing with tests Relevant files: server/src/main/java/org/opensearch/index/mapper/DocumentParser.java server/src/test/java/org/opensearch/index/mapper/DynamicMappingTests.java server/src/test/java/org/opensearch/index/mapper/RootObjectMapperTests.java

⚡ Recommended focus areas for review Mapper Reuse In parseDynamicValue, a new Mapper is built from scratch on every document parse for every field matching a dynamic property. This could be expensive at scale. Consider caching the built mapper per pattern/field name to avoid repeated parsing and building. if (dynamicProperty != null) { Map<String, Object> config = dynamicProperty.mappingForName(currentFieldName); Object typeNode = config.get("type"); if (typeNode != null) { String type = typeNode.toString(); Mapper.TypeParser typeParser = context.docMapperParser().parserContext(null).typeParser(type); if (typeParser != null) { Mapper.Builder<?> builder = typeParser.parse(currentFieldName, config, context.docMapperParser().parserContext(null)); Mapper.BuilderContext builderContext = new Mapper.BuilderContext( context.indexSettings().getSettings(), context.path() ); Mapper mapper = builder.build(builderContext); parseObjectOrField(context, mapper); return; } } Null ParserContext context.docMapperParser().parserContext(null) is called twice with null as the argument. It's unclear if null is a valid argument here and whether it could cause a NullPointerException or incorrect behavior depending on the implementation of parserContext. Mapper.TypeParser typeParser = context.docMapperParser().parserContext(null).typeParser(type); if (typeParser != null) { Mapper.Builder<?> builder = typeParser.parse(currentFieldName, config, context.docMapperParser().parserContext(null)); Ambiguity Detection The patternsAreAmbiguous method uses a heuristic approach (replacing * with single chars and building overlap candidates) that may produce false negatives for complex patterns with multiple wildcards or edge cases. Patterns with multiple * characters are not handled, and the method only checks the first * via indexOf. This could allow genuinely ambiguous patterns to pass validation. private static boolean patternsAreAmbiguous(String pattern1, String pattern2) { // Test 1: replace * with a single char in each; if the other pattern matches, ambiguous String s1 = pattern1.replace("*", "x"); String s2 = pattern2.replace("*", "y"); if (Regex.simpleMatch(pattern2, s1) || Regex.simpleMatch(pattern1, s2)) { return true; } // Test 2: build overlap candidate (e.g. *_i and i_* both match "i_i") int star1 = pattern1.indexOf('*'); int star2 = pattern2.indexOf('*'); if (star1 >= 0 && star2 >= 0) { String prefix1 = pattern1.substring(0, star1); String suffix1 = pattern1.substring(star1 + 1); String prefix2 = pattern2.substring(0, star2); String suffix2 = pattern2.substring(star2 + 1); String overlap1 = prefix1 + suffix2; String overlap2 = prefix2 + suffix1; if (overlap1.length() > 0 && Regex.simpleMatch(pattern1, overlap1) && Regex.simpleMatch(pattern2, overlap1)) { return true; } if (overlap2.length() > 0 && Regex.simpleMatch(pattern1, overlap2) && Regex.simpleMatch(pattern2, overlap2)) { return true; } } return false; } Merge Validation Gap During doMerge with INDEX_TEMPLATE reason, validateDynamicProperties is called on the merged list, but validateNoExplicitFieldMatchesDynamicProperty is not called. This means a template merge could result in a state where explicit fields overlap with dynamic property patterns without being caught. if (mergeWithObject.dynamicProperties.explicit()) { if (reason == MergeReason.INDEX_TEMPLATE) { Map<String, DynamicProperty> byPattern = new LinkedHashMap<>(); for (DynamicProperty dp : this.dynamicProperties.value()) { byPattern.put(dp.getPattern(), dp); } for (DynamicProperty dp : mergeWithObject.dynamicProperties.value()) { byPattern.put(dp.getPattern(), dp); } List<DynamicProperty> merged = new ArrayList<>(byPattern.values()); validateDynamicProperties(merged); this.dynamicProperties = new Explicit<>(merged.toArray(new DynamicProperty[0]), true); } else { this.dynamicProperties = mergeWithObject.dynamicProperties; } } Wildcard-only Requirement The validation requires that every dynamic_properties pattern must contain a * wildcard. However, the patternsAreAmbiguous check skips pairs where either pattern is exactly "*". This means a catch-all "*" pattern could be combined with other patterns that are effectively subsets, and the ambiguity check would silently skip them. The ordering/priority semantics should be clearly documented and enforced. if (pattern.contains("*") == false) { throw new MapperParsingException( "dynamic_properties pattern [" + pattern + "] must contain a wildcard [*]." ); } if ((entry.getValue() instanceof Map) == false) { throw new MapperParsingException( "dynamic_properties entry [" + pattern + "] must have a mapping object as value." ); } @SuppressWarnings("unchecked") Map<String, Object> mapping = new TreeMap<>((Map<String, Object>) entry.getValue()); Object typeNode = mapping.get("type"); if (typeNode == null) { throw new MapperParsingException("No type specified for dynamic_property pattern [" + pattern + "]"); } String type = typeNode.toString(); Mapper.TypeParser typeParser = parserContext.typeParser(type); if (typeParser == null) { throw new MapperParsingException( "No handler for type [" + type + "] declared for dynamic_property pattern [" + pattern + "]" ); } dynamicProperties.add(new DynamicProperty(pattern, mapping)); } // Order so that catch-all "*" is last (lowest priority) dynamicProperties.sort((a, b) -> { boolean aCatchAll = "*".equals(a.getPattern()); boolean bCatchAll = "*".equals(b.getPattern()); if (aCatchAll == bCatchAll) return 0; return aCatchAll ? 1 : -1; });

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to dec15f3
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent mutation of shared mapping config The config map returned by mappingForName is passed directly to typeParser.parse(), which may mutate it (many OpenSearch type parsers remove entries from the map as they consume them). Since config likely comes from the stored DynamicProperty mapping, mutating it would corrupt the property for subsequent document parses. A defensive copy should be made before passing it to the parser. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1331-1332] -Map<String, Object> config = dynamicProperty.mappingForName(currentFieldName); +Map<String, Object> config = new java.util.HashMap<>(dynamicProperty.mappingForName(currentFieldName)); Object typeNode = config.get("type"); Suggestion importance[1-10]: 8 __ Why: OpenSearch type parsers commonly mutate the config map by removing consumed entries. Since config comes from the stored DynamicProperty, mutating it would corrupt the mapping for all subsequent document parses, causing a real bug.	Medium
General	Avoid redundant parser context creation parserContext(null) is called twice, which may be expensive or produce inconsistent results if the method has side effects. Store the result in a local variable and reuse it for both the typeParser lookup and the parse call. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1335-1337] -Mapper.TypeParser typeParser = context.docMapperParser().parserContext(null).typeParser(type); +Mapper.TypeParser.ParserContext parserContext = context.docMapperParser().parserContext(null); +Mapper.TypeParser typeParser = parserContext.typeParser(type); if (typeParser != null) { - Mapper.Builder<?> builder = typeParser.parse(currentFieldName, config, context.docMapperParser().parserContext(null)); + Mapper.Builder<?> builder = typeParser.parse(currentFieldName, config, parserContext); Suggestion importance[1-10]: 6 __ Why: Calling parserContext(null) twice is redundant and could be expensive or produce inconsistent results. Storing it in a local variable improves both performance and correctness.	Low
	Improve ambiguity detection heuristic coverage The heuristic of replacing with a single character "x" or "y" can produce false negatives. For example, x and x both match "x_x", but s1 = "x_x" would be matched by pattern2 = "x_" correctly; however patterns like abc and abc would fail the single-char test since "xabc" doesn't match "abc*". The overlap candidate logic in Test 2 handles this, but Test 1 may give a false sense of completeness. More critically, if a pattern contains multiple wildcards (e.g., a), replace("*", "x") produces "xax" which may not correctly probe ambiguity. Consider documenting the known limitations or extending the check to handle multiple wildcards. server/src/main/java/org/opensearch/index/mapper/RootObjectMapper.java [629-635] private static boolean patternsAreAmbiguous(String pattern1, String pattern2) { - // Test 1: replace * with a single char in each; if the other pattern matches, ambiguous - String s1 = pattern1.replace("*", "x"); - String s2 = pattern2.replace("*", "y"); - if (Regex.simpleMatch(pattern2, s1) || Regex.simpleMatch(pattern1, s2)) { + // Test 1: replace * with empty string and a representative char to probe overlap + String s1a = pattern1.replace("*", ""); + String s1b = pattern1.replace("*", "x"); + String s2a = pattern2.replace("*", ""); + String s2b = pattern2.replace("*", "y"); + if (Regex.simpleMatch(pattern2, s1a) || Regex.simpleMatch(pattern2, s1b) + || Regex.simpleMatch(pattern1, s2a) || Regex.simpleMatch(pattern1, s2b)) { return true; } + // Test 2: build overlap candidate (e.g. *_i and i_* both match "i_i") + int star1 = pattern1.indexOf('*'); + int star2 = pattern2.indexOf('*'); + if (star1 >= 0 && star2 >= 0) { + String prefix1 = pattern1.substring(0, star1); + String suffix1 = pattern1.substring(star1 + 1); + String prefix2 = pattern2.substring(0, star2); + String suffix2 = pattern2.substring(star2 + 1); + String overlap1 = prefix1 + suffix2; + String overlap2 = prefix2 + suffix1; + if (overlap1.length() > 0 && Regex.simpleMatch(pattern1, overlap1) && Regex.simpleMatch(pattern2, overlap1)) { + return true; + } + if (overlap2.length() > 0 && Regex.simpleMatch(pattern1, overlap2) && Regex.simpleMatch(pattern2, overlap2)) { + return true; + } + } + return false; +} Suggestion importance[1-10]: 4 __ Why: The suggestion identifies a real limitation in the heuristic (single-char replacement may miss some ambiguous cases), but the improved code only marginally extends coverage and the existing Test 2 overlap candidate logic already handles the most common cases. The improvement is valid but has limited practical impact.	Low
	Ensure stable ordering of non-catch-all patterns The sort comparator is not consistent for non-catch-all patterns (it returns 0 for any two non-catch-all patterns), which means their relative order is undefined and may vary across JVM implementations. This could lead to non-deterministic behavior when multiple specific patterns are defined. The sort should only move the catch-all "*" to the end while preserving the original insertion order of all other patterns (use a stable sort or List.sort with a total order). server/src/main/java/org/opensearch/index/mapper/RootObjectMapper.java [326-332] +// Move catch-all "*" to the end while preserving insertion order of other patterns dynamicProperties.sort((a, b) -> { boolean aCatchAll = "*".equals(a.getPattern()); boolean bCatchAll = "*".equals(b.getPattern()); - if (aCatchAll == bCatchAll) return 0; + if (aCatchAll == bCatchAll) return 0; // stable sort preserves original order return aCatchAll ? 1 : -1; }); +// Note: List.sort() is guaranteed stable, so non-catch-all patterns retain their original order. validateDynamicProperties(dynamicProperties); Suggestion importance[1-10]: 2 __ Why: List.sort() in Java is already guaranteed to be stable, so non-catch-all patterns already retain their original insertion order. The suggestion's improved_code is functionally identical to the existing code, just adding a comment.	Low

---

Previous suggestions

Suggestions up to commit 10ea678

Category	Suggestion	Impact
Possible issue	Protect shared mapping config from mutation The config map returned by mappingForName is passed directly to typeParser.parse, which may mutate it (many type parsers remove consumed keys). Since this map is shared state from the DynamicProperty, subsequent uses of the same dynamic property could see a corrupted or empty config. Pass a defensive copy instead. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1330-1331] -Map<String, Object> config = dynamicProperty.mappingForName(currentFieldName); +Map<String, Object> config = new java.util.HashMap<>(dynamicProperty.mappingForName(currentFieldName)); Object typeNode = config.get("type"); Suggestion importance[1-10]: 8 __ Why: Many TypeParser.parse implementations remove consumed keys from the config map. Since DynamicProperty holds the original mapping, passing it directly could corrupt the shared state on subsequent uses of the same dynamic property pattern, causing silent failures.	Medium
	Maintain catch-all ordering after template merge This validation check is redundant because the sorting step earlier in processField already ensures is placed last. However, during doMerge for INDEX_TEMPLATE, the merged list is built via LinkedHashMap without re-sorting, so the catch-all may no longer be last after merging. The merged list should also be sorted (or the catch-all moved to the end) before calling validateDynamicProperties. server/src/main/java/org/opensearch/index/mapper/RootObjectMapper.java [509-511] -if (catchAllCount == 1 && dynamicProperties.size() > 1 - && "*".equals(dynamicProperties.get(dynamicProperties.size() - 1).getPattern()) == false) { - throw new MapperParsingException("dynamic_properties catch-all pattern [*] must be defined last."); -} +List<DynamicProperty> merged = new ArrayList<>(byPattern.values()); +// Ensure catch-all "*" remains last after merge +merged.sort((a, b) -> { + boolean aCatchAll = "*".equals(a.getPattern()); + boolean bCatchAll = "*".equals(b.getPattern()); + if (aCatchAll == bCatchAll) return 0; + return aCatchAll ? 1 : -1; +}); +validateDynamicProperties(merged); +this.dynamicProperties = new Explicit<>(merged.toArray(new DynamicProperty[0]), true); Suggestion importance[1-10]: 7 __ Why: During INDEX_TEMPLATE merges, the merged list is built via LinkedHashMap without re-sorting, so the catch-all * pattern may not remain last, causing validateDynamicProperties to throw a spurious error or allowing incorrect ordering. Adding a sort before validation is a real correctness fix.	Medium
General	Avoid duplicate parser context creation parserContext(null) is called twice, which may be expensive or produce inconsistent results if the context is stateful. Store the result in a local variable and reuse it for both the typeParser lookup and the parse call. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1334-1336] -Mapper.TypeParser typeParser = context.docMapperParser().parserContext(null).typeParser(type); +Mapper.TypeParser.ParserContext parserContext = context.docMapperParser().parserContext(null); +Mapper.TypeParser typeParser = parserContext.typeParser(type); if (typeParser != null) { - Mapper.Builder<?> builder = typeParser.parse(currentFieldName, config, context.docMapperParser().parserContext(null)); + Mapper.Builder<?> builder = typeParser.parse(currentFieldName, config, parserContext); Suggestion importance[1-10]: 6 __ Why: Calling parserContext(null) twice is wasteful and could produce inconsistent results if the context is stateful. Storing it in a local variable is a clean improvement with low risk.	Low
	Improve wildcard ambiguity detection reliability The heuristic of replacing with a single character "x" or "y" can produce false negatives. For example, i replaced becomes x_i, and Regex.simpleMatch("i", "x_i") is false, so the ambiguity of i and i* would not be caught by Test 1 alone. The logic relies entirely on Test 2 for that case, but Test 1 could also miss other multi-wildcard patterns. Consider using a more robust overlap-detection approach or at minimum document the known limitations clearly. server/src/main/java/org/opensearch/index/mapper/RootObjectMapper.java [629-635] private static boolean patternsAreAmbiguous(String pattern1, String pattern2) { - // Test 1: replace * with a single char in each; if the other pattern matches, ambiguous - String s1 = pattern1.replace("*", "x"); - String s2 = pattern2.replace("*", "y"); + // Test 1: replace * with a longer token to reduce false negatives + String s1 = pattern1.replace("*", "xyzxyz"); + String s2 = pattern2.replace("*", "xyzxyz"); if (Regex.simpleMatch(pattern2, s1) || Regex.simpleMatch(pattern1, s2)) { return true; } Suggestion importance[1-10]: 4 __ Why: The single-character replacement heuristic in Test 1 can produce false negatives for certain pattern combinations, though Test 2 compensates for the specific *_i/i_* case. Using a longer token reduces false negatives but doesn't fully solve the problem; the improvement is marginal given Test 2 already handles the main cases.	Low

[github-actions]

❌ Gradle check result for 10ea678: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit dec15f3

[github-actions]

❌ Gradle check result for dec15f3: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?