Add plugin SPI for dynamic field-type inference and dynamic-template types

[naykudev]

Description

This PR introduces a generic core interface and plugin SPI that lets any mapper plugin participate in dynamic mapping for its own field types. Core defines the extension points (DynamicFieldTypeInferencer, DynamicTemplateTypeHandler, FieldValueParserSupplier) and the registration SPI on MapperPlugin; core itself only detects the JSON value and delegates the "is this mine, and what type is it" decision to whichever plugin registered. No plugin-specific type knowledge lives in core.

The k-NN plugin is simply the first consumer of this SPI (auto-mapping vector fields and match_mapping_type: "knn_vector" dynamic templates, in a companion PR), but the interface is deliberately generic — e.g. a geospatial plugin could register an inferencer that claims {"lat": .., "lon": ..} objects as geo_point through the exact same SPI, with zero further core changes.

Motivation. Today dynamic mapping can only produce core's built-in field types. When an unmapped numeric array arrives, core parses it element-by-element and maps it as float — it never looks at the array as a whole, and a plugin has no way to claim it. Likewise, match_mapping_type only accepts the built-in XContentFieldType values, so a plugin type can't be targeted by a dynamic template. This change gives plugins a single, generic seam for both.

What it adds (all @ExperimentalApi):

• DynamicFieldTypeInferencer — inferFieldType(FieldValueParserSupplier) → Map<String,Object> | null. Called for an unmapped field with no matching template; the plugin inspects the value and returns a mapping config to claim it, or null to pass.
• DynamicTemplateTypeHandler — backs a plugin-registered match_mapping_type (validated against the plugin registry when it isn't a built-in XContentFieldType). adjustMappingConfig(...) completes the template config before the TypeParser builds the mapper; isConfigComplete(...) gates eager index-creation validation.
• FieldValueParserSupplier — hands the plugin a fresh XContentParser over the buffered field bytes on each get(). Lazy (no parser allocated unless the plugin reads it), preserves JSON fidelity, no boxing. Core stays free of any deserialized-representation contract.
• MapperPlugin SPI methods getDynamicFieldTypeInferencers() and getDynamicTemplateTypes(), collected into MapperRegistry by IndicesModule at startup (same pattern as getMappers()).

How it hooks in. A single hook, tryPluginInference(), fires in DocumentParser.innerParseObject() before the token-type switch — one placement covering arrays, objects, and scalars, so the SPI is genuinely generic. Resolution order: explicit mapping → plugin template → standard template → plugin inferencer → existing per-element fallback. If no plugin claims the field, the buffered bytes are replayed through the original path, so all existing behavior is preserved. When no plugin registers an inferencer or template type, the hook fast-exits before any buffering — zero overhead for clusters without such a plugin.

Ambiguity is a hard error, not a silent pick. Rather than taking the first claim by registration/load order, core consults all registered plugin template types and all inferencers for an unmapped field and throws a clear MapperParsingException (naming the conflicting claimants) if more than one claims it. Zero claims fall through as before; exactly one is used. Duplicate match_mapping_type registration across plugins is rejected at node startup.

Backwards compatibility. Strictly additive. The hook only fires for unmapped fields, only when a plugin is registered; existing dynamic templates, explicit mappings, and per-element inference are unchanged. All new types are @ExperimentalApi.

Scope note. This is the core SPI only. The k-NN implementation (inferencer + knn_vector template handler) is a companion PR in the k-NN repo and depends on this change; it cannot build against core until this merges and a snapshot publishes.

Testing

• PluginDynamicTemplateTests (28) — parsing, index-creation validation (registered/unregistered/typo/complete/incomplete/{name} placeholder), inference claims/declines for scalars/strings/booleans, precedence (explicit beats inference, builtin fallback), and fast-path exit.
• PluginInferenceConflictTests (3) — two inferencers claiming the same field throw; two plugin templates matching the same field throw; a single match resolves without throwing.
• IndicesModuleTests (+3) — plugins register inferencers/template types correctly; duplicate template-type registration throws at startup; empty by default.
• DynamicTemplateTests — plugin-type parse/serialization.
• Full DocumentParserTests (130) pass — regression guard for the changed hot path.

All server mapper/indices suites green; spotlessJavaCheck passes.

Related Issues

• RFC: [RFC] Plugin SPI for dynamic field-type inference and plugin-typed dynamic templates (dynamic knn_vector mapping) #22576
• Related to [BUG] Dynamic template for vectorized output fields k-NN#2247 (the k-NN use case this SPI enables)

Check List

• Functionality includes testing.
• New functionality has javadoc added.
• 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.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 840945c.

⛔ Hard block: Issues at Medium severity or above will block this PR from merging.

Path	Line	Severity	Description
server/src/main/java/org/opensearch/index/mapper/DocumentParser.java	1186	medium	The new tryPluginInference SPI calls plugin-supplied inferencer code (inferencer.inferFieldType) during every document parse for unmapped fields, passing a FieldValueParserSupplier that gives each plugin direct streaming access to the raw buffered field bytes. A malicious or compromised plugin installed on the cluster could use this hook to exfiltrate document field values. This is architecturally consistent with OpenSearch's trusted-plugin model, but the hook is broader than prior plugin extension points — it fires on every unmapped field rather than only at mapping-registration time.
server/src/test/java/org/opensearch/index/mapper/DynamicTemplateTests.java	58	low	The test previously verified that an unknown match_mapping_type value throws an IllegalArgumentException immediately at parse time. The change removes that assertion and instead accepts any unknown string as a pluginMatchType. Validation is now deferred to index-creation time via the registry check. This is intentional for the plugin SPI feature, but it relaxes the parse-time rejection that previously caught typos and invalid type strings before they could be stored.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 1 | Low: 1

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 000d534)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

📝 TODO sections server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1370]: put these into one if

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue In tryPluginInference, when getMapper() returns non-null (field already mapped), the method returns false without releasing the path slots that were added by getDynamicParentMapper — wait, actually the early-return happens before getDynamicParentMapper is called, so that path is fine. However, the STRICT/FALSE branch calls getDynamicParentMapper and then releases slots and returns false, meaning innerParseObject will proceed to the normal switch which will call getDynamicParentMapper again through the standard path. This double-invocation may have side effects (path mutation, dynamic parent creation) that were not intended to occur twice. Verify getDynamicParentMapper is safe to invoke twice for the same field. final String[] resolvedPaths = paths != null ? paths : splitAndValidatePath(fieldName); Tuple<Integer, ObjectMapper> parentMapperTuple = getDynamicParentMapper(context, resolvedPaths, parentMapper); ObjectMapper resolvedParent = parentMapperTuple.v2(); ObjectMapper.Dynamic dynamic = dynamicOrDefault(resolvedParent, context); if (dynamic == ObjectMapper.Dynamic.STRICT || dynamic == ObjectMapper.Dynamic.FALSE) { // Release path-slots added by getDynamicParentMapper before returning for (int i = 0; i < parentMapperTuple.v1(); i++) { context.path().remove(); } return false; } Possible Issue replayThroughExistingPath returns true from tryPluginInference, and the caller in innerParseObject then executes token = parser.nextToken(); continue;. But the replay was performed on a temporary replay parser via context.switchParser(replayParser); the original outer parser's position after copyCurrentStructure should already be at the end of the value. Confirm that parser.nextToken() in the caller correctly advances the original parser past the consumed field structure — otherwise fields following an unclaimed buffered field could be skipped or double-parsed. Also, context.switchParser inside replayThroughExistingPath mutates context state; ensure the original parser is restored after the try-with-resources exits (no visible restoration logic). private static void replayThroughExistingPath( ParseContext context, ObjectMapper parentMapper, String fieldName, byte[] rawContent, int pathsAddedByGetDynamicParentMapper ) throws IOException { XContentParser originalParser = context.parser(); try ( XContentParser replayParser = originalParser.contentType() .xContent() .createParser(originalParser.getXContentRegistry(), originalParser.getDeprecationHandler(), rawContent) ) { replayParser.nextToken(); // position at value start ParseContext replayContext = context.switchParser(replayParser); XContentParser.Token replayToken = replayParser.currentToken(); String[] replayPaths = splitAndValidatePath(fieldName); switch (replayToken) { case START_OBJECT: parseObject(replayContext, parentMapper, fieldName, replayPaths); break; case START_ARRAY: parseArray(replayContext, parentMapper, fieldName, replayPaths); break; case VALUE_NULL: parseNullValue(replayContext, parentMapper, fieldName, replayPaths); break; default: if (replayToken != null && replayToken.isValue()) { parseValue(replayContext, parentMapper, fieldName, replayToken, replayPaths); } } } finally { for (int i = 0; i < pathsAddedByGetDynamicParentMapper; i++) { context.path().remove(); } } } Memory / Performance tryPluginInference unconditionally buffers the entire field value into a byte array (via copyCurrentStructure) whenever any plugin inferencer OR template type is registered, even for tiny scalar fields on every unmapped field in every document. For workloads with many unmapped fields per document, this adds a full serialize+parse round-trip per field. Consider skipping the buffer for scalar tokens when no inferencer/template can possibly claim them, or short-circuiting when template types exist but no template patterns could match the field name. // Buffer the complete field value — needed for replay regardless of whether // a plugin claims the field or not (streaming parser can only be read once) byte[] rawContent; try (XContentBuilder bufferBuilder = XContentBuilder.builder(contentType.xContent())) { bufferBuilder.copyCurrentStructure(parser); rawContent = BytesReference.toBytes(BytesReference.bytes(bufferBuilder)); } // Hand plugins a factory that produces a fresh parser over the buffered bytes rather than a // pre-deserialized object. Core stays free of any representation contract: each plugin streams // the tokens it needs. Plugins whose config is already complete never call get(), so no parsing // happens for them. final FieldValueParserSupplier fieldValueParser = new FieldValueParserSupplier( contentType, parser.getXContentRegistry(), parser.getDeprecationHandler(), rawContent ); Behavior Change Previously, calling DynamicTemplate.parse(name, conf) with an unknown match_mapping_type (e.g. "text") threw IllegalArgumentException. Now the single-arg overload silently accepts any unknown string as a pluginMatchType. Callers that relied on the old validation to reject typos at parse time (e.g. anywhere not going through RootObjectMapper.processField with the plugin registry) will no longer see errors. The existing test testParseUnknownMatchType was changed to accommodate this, but any external code or path that called the two-arg form with null inherits this weakened validation. public static DynamicTemplate parse(String name, Map<String, Object> conf) throws MapperParsingException { return parse(name, conf, null); } /** * Parses a dynamic template. If {@code knownPluginTypes} is non-null, a {@code match_mapping_type} * that is neither an {@link XContentFieldType} nor a registered plugin type fails here, before any * {@link DynamicTemplate} is constructed. */ static DynamicTemplate parse(String name, Map<String, Object> conf, Map<String, DynamicTemplateTypeHandler> knownPluginTypes) throws MapperParsingException { Path Corruption Risk In the template-match branch, context.path().add(resolvedFieldName) is called after context.addDynamicMapper(templateMapper) and inside a nested try/finally. However, the outer finally releasing parentMapperTuple.v1() slots runs after the inner finally removes the field-name slot — correct order. But if templateBuilder.build() or addDynamicMapper throws before entering the try-with-resources, the parent path slots added by getDynamicParentMapper are never released, leaving ContentPath corrupt for subsequent fields in the same document. if (templateBuilder != null) { Mapper.BuilderContext templateBuilderContext = new Mapper.BuilderContext(context.indexSettings().getSettings(), context.path()); Mapper templateMapper = templateBuilder.build(templateBuilderContext); context.addDynamicMapper(templateMapper); try ( XContentParser replayParser = contentType.xContent() .createParser(context.parser().getXContentRegistry(), context.parser().getDeprecationHandler(), rawContent) ) { replayParser.nextToken(); ParseContext replayContext = context.switchParser(replayParser); context.path().add(resolvedFieldName); try { parseObjectOrField(replayContext, templateMapper); } finally { // Release the field-name slot even if replay throws, so ContentPath is not left // corrupt for subsequent fields in the same document. context.path().remove(); } } finally { for (int i = 0; i < parentMapperTuple.v1(); i++) { context.path().remove(); } } return true; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 000d534

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Preserve strict validation without plugin registry When knownPluginTypes is null (the 2-arg parse overload), any unknown match_mapping_type is silently accepted as a plugin type — this breaks the previous strict behavior for callers using the old API and can hide typos in mappings created via that path. Consider preserving strict validation in the 2-arg path (throw as before) and only relaxing it in the 3-arg path when a registry is supplied. server/src/main/java/org/opensearch/index/mapper/DynamicTemplate.java [257-270] if (xcontentFieldType == null) { pluginMatchType = matchMappingType; - // Validate plugin type before constructing the template - if (knownPluginTypes != null && !knownPluginTypes.containsKey(pluginMatchType)) { + if (knownPluginTypes == null) { + // Preserve legacy strict behavior when no plugin registry supplied + throw new IllegalArgumentException( + "No field type matched on [" + matchMappingType + "], possible values are " + + Arrays.toString(XContentFieldType.values()) + ); + } + if (!knownPluginTypes.containsKey(pluginMatchType)) { List<String> allTypes = new ArrayList<>(); for (XContentFieldType t : XContentFieldType.values()) { allTypes.add(t.toString()); } allTypes.addAll(knownPluginTypes.keySet()); throw new IllegalArgumentException( "No field type matched on [" + pluginMatchType + "], possible values are " + allTypes ); } } Suggestion importance[1-10]: 7 __ Why: Valid concern about backward compatibility — the 2-arg parse overload now silently accepts unknown types where it previously threw, which could hide mapping typos in legacy callers and contradicts the existing test expectations.	Medium
	Ensure paths matches current field name currentFieldName is captured before the else branch and is not reset per iteration; if tryPluginInference is invoked with a stale paths value from a prior FIELD_NAME token, path resolution may be incorrect. Ensure paths corresponds to the current currentFieldName by resolving it here (or asserting non-null) before delegating, to avoid mis-routing fields when the parser state and captured paths become out of sync. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [661-664] +if (paths == null) { + paths = resolvePathForParsing(mapper, currentFieldName); +} if (tryPluginInference(context, mapper, currentFieldName, paths)) { token = parser.nextToken(); continue; } Suggestion importance[1-10]: 3 __ Why: The concern is largely unfounded: paths is set in the FIELD_NAME branch right before the else branch executes on the next token for the same field. The suggestion adds redundant defensive code.	Low
	Ensure parser state consistency on exceptions After the plugin inference/template path completes, the original parser (context.parser()) is still positioned at the end of the buffered structure, but the caller in innerParseObject executes token = parser.nextToken(); — this is correct for scalar values but for START_OBJECT/START_ARRAY the buffering via copyCurrentStructure already advanced the parser past END_OBJECT/END_ARRAY, so nextToken() will move to the token after the field value as expected. However, verify switchParser restores state properly on exception; if parseObjectOrField throws, the original parser may be left in an inconsistent state relative to the outer loop. Consider restoring the parser explicitly in a finally block to guarantee cleanup. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1305-1323] +ParseContext replayContext = null; try (XContentParser replayParser = contentType.xContent() .createParser(context.parser().getXContentRegistry(), context.parser().getDeprecationHandler(), rawContent) ) { replayParser.nextToken(); - ParseContext replayContext = context.switchParser(replayParser); + replayContext = context.switchParser(replayParser); context.path().add(resolvedFieldName); try { parseObjectOrField(replayContext, templateMapper); } finally { - // Release the field-name slot even if replay throws, so ContentPath is not left - // corrupt for subsequent fields in the same document. context.path().remove(); } } finally { for (int i = 0; i < parentMapperTuple.v1(); i++) { context.path().remove(); } } Suggestion importance[1-10]: 2 __ Why: The suggestion is vague ("verify switchParser restores state") and the improved code is essentially identical to the existing code, just moving a variable declaration. No concrete issue is fixed.	Low
Possible issue	Guard against non-String type values from plugins The cast (String) inferredFieldMapping.get("type") will throw ClassCastException if a plugin returns a non-String value for the "type" key (e.g. an integer or map), bypassing the intended graceful fallback. Retrieve the value as Object first and validate it is a String before casting, so a misbehaving inferencer causes a fallthrough to the existing path rather than crashing document parsing. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1376-1380] -if (inferredFieldMapping == null) { // put these into one if +if (inferredFieldMapping == null) { // No template and no inferencer claimed this field — fall through to existing path replayThroughExistingPath(context, resolvedParent, resolvedFieldName, rawContent, parentMapperTuple.v1()); return true; } -String inferredType = (String) inferredFieldMapping.get("type"); -if (inferredType == null) { +Object inferredTypeObj = inferredFieldMapping.get("type"); +if (inferredTypeObj instanceof String == false) { replayThroughExistingPath(context, resolvedParent, resolvedFieldName, rawContent, parentMapperTuple.v1()); return true; } +String inferredType = (String) inferredTypeObj; Suggestion importance[1-10]: 6 __ Why: Valid defensive check — a plugin returning a non-String type would cause ClassCastException and break document parsing. The fix aligns with existing graceful fallback behavior for unknown types.	Low

---

Previous suggestions

Suggestions up to commit 2ae39c1

Category	Suggestion	Impact
Possible issue	Release path slots on builder failure If typeParser.parse(...) or builder.build(...) throws (e.g. a malformed config from a buggy inferencer), the path slots added by getDynamicParentMapper are never released, corrupting ContentPath for subsequent fields in the same document. Wrap this block so the slots are always released on failure, mirroring the try/finally already used for successful replay. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1391-1394] -Mapper.Builder<?> builder = typeParser.parse(resolvedFieldName, inferredFieldMapping, parserContext); -Mapper.BuilderContext builderContext = new Mapper.BuilderContext(context.indexSettings().getSettings(), context.path()); -Mapper inferredMapper = builder.build(builderContext); +Mapper inferredMapper; +try { + Mapper.Builder<?> builder = typeParser.parse(resolvedFieldName, inferredFieldMapping, parserContext); + Mapper.BuilderContext builderContext = new Mapper.BuilderContext(context.indexSettings().getSettings(), context.path()); + inferredMapper = builder.build(builderContext); +} catch (RuntimeException e) { + for (int i = 0; i < parentMapperTuple.v1(); i++) { + context.path().remove(); + } + throw e; +} context.addDynamicMapper(inferredMapper); Suggestion importance[1-10]: 7 __ Why: Correctly identifies that a throw from typeParser.parse or builder.build would leak path slots added by getDynamicParentMapper, corrupting ContentPath for subsequent fields in the document.	Medium
	Guard cast against non-String type values The cast (String) inferredFieldMapping.get("type") will throw ClassCastException if a plugin returns a non-String value for the "type" key, defeating the "fall through" behavior documented for buggy plugins. Guard the cast by checking the value's type before casting, and fall through if it is not a String. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1370-1380] -if (inferredFieldMapping == null) { // put these into one if +if (inferredFieldMapping == null) { // No template and no inferencer claimed this field — fall through to existing path replayThroughExistingPath(context, resolvedParent, resolvedFieldName, rawContent, parentMapperTuple.v1()); return true; } -String inferredType = (String) inferredFieldMapping.get("type"); -if (inferredType == null) { +Object typeValue = inferredFieldMapping.get("type"); +if (typeValue instanceof String == false) { replayThroughExistingPath(context, resolvedParent, resolvedFieldName, rawContent, parentMapperTuple.v1()); return true; } +String inferredType = (String) typeValue; Suggestion importance[1-10]: 6 __ Why: Valid defensive concern: a plugin returning a non-String type would cause ClassCastException rather than the documented graceful fall-through. Moderate impact since well-behaved plugins wouldn't hit this.	Low
General	Legacy parse overload weakens type validation The public single-argument parse(name, conf) overload now accepts any unknown match_mapping_type string as a plugin type, silently changing pre-existing behavior: tests and callers that relied on IllegalArgumentException for typos (e.g. "text") will no longer see errors when using the legacy overload. Consider deprecating or removing the null-tolerant path, or making the legacy overload delegate through the strict validation used elsewhere so behavior stays consistent. server/src/main/java/org/opensearch/index/mapper/DynamicTemplate.java [248-270] XContentFieldType xcontentFieldType = null; String pluginMatchType = null; if (matchMappingType != null && !matchMappingType.equals("*")) { for (XContentFieldType t : XContentFieldType.values()) { if (t.toString().equals(matchMappingType)) { xcontentFieldType = t; break; } } if (xcontentFieldType == null) { pluginMatchType = matchMappingType; - // Validate plugin type before constructing the template + // Validate plugin type before constructing the template (strict when registry is provided) if (knownPluginTypes != null && !knownPluginTypes.containsKey(pluginMatchType)) { Suggestion importance[1-10]: 5 __ Why: Valid observation about a behavior change in the legacy parse(name, conf) overload (test was updated to reflect this), but the improved_code only changes a comment and doesn't fix the concern.	Low
	Ensure symmetric path add/remove on failure context.path().add(resolvedFieldName) is executed after the parser is opened; if switchParser or add throws before the inner try, the path slot is never released and the parent slots from the outer finally are still removed, leaving ContentPath corrupt. Move context.path().add(...) to be the first statement inside the inner try, or place it before try and adjust the finally accordingly, to ensure symmetric add/remove. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1305-1323] try ( XContentParser replayParser = contentType.xContent() .createParser(context.parser().getXContentRegistry(), context.parser().getDeprecationHandler(), rawContent) ) { replayParser.nextToken(); ParseContext replayContext = context.switchParser(replayParser); context.path().add(resolvedFieldName); try { parseObjectOrField(replayContext, templateMapper); } finally { - // Release the field-name slot even if replay throws, so ContentPath is not left - // corrupt for subsequent fields in the same document. context.path().remove(); } } finally { for (int i = 0; i < parentMapperTuple.v1(); i++) { context.path().remove(); } } Suggestion importance[1-10]: 2 __ Why: The improved_code is essentially identical to the existing_code; the suggestion doesn't actually change the ordering it flags as problematic, so it provides no real improvement.	Low

Suggestions up to commit 6dae7af

Category	Suggestion	Impact
Possible issue	Avoid double-adding leaf field to ContentPath The template-match branch calls context.path().add(resolvedFieldName) before invoking parseObjectOrField, but standard mapper paths (e.g. in parseObject) only add the leaf field name inside parseObjectOrField if the mapper is not a FieldMapper. This double-add will corrupt the ContentPath for leaf field mappers built from templates. Match the existing convention by letting parseObjectOrField (or the mapper itself) manage the path. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1305-1323] try ( XContentParser replayParser = contentType.xContent() .createParser(context.parser().getXContentRegistry(), context.parser().getDeprecationHandler(), rawContent) ) { replayParser.nextToken(); ParseContext replayContext = context.switchParser(replayParser); - context.path().add(resolvedFieldName); - try { - parseObjectOrField(replayContext, templateMapper); - } finally { - // Release the field-name slot even if replay throws, so ContentPath is not left - // corrupt for subsequent fields in the same document. - context.path().remove(); - } + parseObjectOrField(replayContext, templateMapper); } finally { for (int i = 0; i < parentMapperTuple.v1(); i++) { context.path().remove(); } } Suggestion importance[1-10]: 7 __ Why: The suggestion identifies a potentially real issue: parseObjectOrField typically handles path management internally for object mappers, and adding the field name explicitly before calling it could corrupt ContentPath for object-typed dynamic mappers. However, verification against the exact semantics of parseObjectOrField is needed.	Medium
	Verify parser position after structure copy After tryPluginInference consumes the field's structure via copyCurrentStructure, the parser is positioned at the end token (END_OBJECT/END_ARRAY) or on the value token itself. Calling parser.nextToken() here to advance past it may be correct for scalars but skip the next FIELD_NAME after objects/arrays. Verify the parser position after copyCurrentStructure matches the assumption, since copyCurrentStructure typically leaves the parser on the last token of the copied structure. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [661-664] +if (tryPluginInference(context, mapper, currentFieldName, paths)) { + token = parser.nextToken(); + continue; +} - Suggestion importance[1-10]: 4 __ Why: This is a verification-only suggestion with no concrete code change (existing_code equals improved_code). It raises a valid concern about parser positioning after copyCurrentStructure, but does not offer a concrete fix.	Low
General	Preserve builtin type alias resolution The previous behavior of XContentFieldType.fromString also accepted aliases (e.g. handled via its own logic beyond toString()). Replacing it with a toString()-only match loses alias support and may misclassify builtins as plugin types. Preserve the original resolution by attempting XContentFieldType.fromString inside a try/catch, then fall back to plugin-type handling. server/src/main/java/org/opensearch/index/mapper/DynamicTemplate.java [251-260] -for (XContentFieldType t : XContentFieldType.values()) { - if (t.toString().equals(matchMappingType)) { - xcontentFieldType = t; - break; - } +try { + xcontentFieldType = XContentFieldType.fromString(matchMappingType); +} catch (IllegalArgumentException ignored) { + // Not a builtin type — treat as plugin type } if (xcontentFieldType == null) { pluginMatchType = matchMappingType; // Validate plugin type before constructing the template if (knownPluginTypes != null && !knownPluginTypes.containsKey(pluginMatchType)) { Suggestion importance[1-10]: 3 __ Why: The claim about aliases is questionable — XContentFieldType.fromString in the original code iterates values and compares by toString(), so the new logic preserves the same matching behavior. The suggestion appears to be based on an incorrect assumption.	Low
	Verify strict-dynamic path is preserved When returning false here, control falls back to the caller's switch statement, but the caller has not called getDynamicParentMapper and will call it again through parseObject/parseArray/etc. The path slots were added and then removed here, which is correct, but the STRICT case needs to allow the normal path to throw the strict-mapping exception — verify this early return does not skip the strict-mapping check the caller expects to fire. Consider only performing the plugin lookup path once dynamic status is known non-strict/non-false, without touching getDynamicParentMapper at all for these cases. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1233-1239] +if (dynamic == ObjectMapper.Dynamic.STRICT || dynamic == ObjectMapper.Dynamic.FALSE) { + // Release path-slots added by getDynamicParentMapper before returning + for (int i = 0; i < parentMapperTuple.v1(); i++) { + context.path().remove(); + } + return false; +} - Suggestion importance[1-10]: 3 __ Why: Verification-only suggestion (existing_code equals improved_code). It raises a valid concern about strict-dynamic handling but does not provide an actionable fix.	Low

Suggestions up to commit 7863ebc

Category	Suggestion	Impact
Possible issue	Avoid side-effectful parent resolution on fast path getDynamicParentMapper can create intermediate object mappers as a side effect. Calling it here — before deciding whether the plugin path applies — may mutate the mapping (adding dynamic parents) even when the plugin ultimately doesn't claim the field and we return false to fall through. This can cause duplicate/inconsistent parent creation when the normal path runs afterwards. Consider a read-only check of the dynamic setting on parentMapper first, and only resolve the dynamic parent once you've committed to the plugin path. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1205-1214] +ObjectMapper.Dynamic dynamic = dynamicOrDefault(parentMapper, context); +if (dynamic == ObjectMapper.Dynamic.STRICT || dynamic == ObjectMapper.Dynamic.FALSE) { + return false; +} Tuple<Integer, ObjectMapper> parentMapperTuple = getDynamicParentMapper(context, resolvedPaths, parentMapper); ObjectMapper resolvedParent = parentMapperTuple.v2(); -ObjectMapper.Dynamic dynamic = dynamicOrDefault(resolvedParent, context); -if (dynamic == ObjectMapper.Dynamic.STRICT || dynamic == ObjectMapper.Dynamic.FALSE) { - // Release path-slots added by getDynamicParentMapper before returning - for (int i = 0; i < parentMapperTuple.v1(); i++) { - context.path().remove(); - } - return false; -} Suggestion importance[1-10]: 6 __ Why: Legitimate concern that getDynamicParentMapper may have side effects (creating intermediate mappers) before the plugin path is committed to. This could cause duplicate parent creation when falling through, though verification of the actual side effects is needed.	Low
	Verify parser position after buffering After tryPluginInference consumes the buffered structure, the parser is already positioned at the end token (END_OBJECT/END_ARRAY) of the consumed value. Calling parser.nextToken() here advances one token past that end, but the surrounding loop's while (token != XContentParser.Token.END_OBJECT) expects to eventually see the parent's END_OBJECT. Verify the parser position after copyCurrentStructure — if it leaves the parser on the last token of the value, this is correct; if it leaves it after, the extra nextToken() will skip a sibling field. Consider aligning explicitly with the token position after buffering to avoid subtle field-skipping bugs. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [636-639] if (tryPluginInference(context, mapper, currentFieldName, paths)) { + // copyCurrentStructure leaves the parser on the last token of the copied value; + // advance to the next token for the loop to process the next field or END_OBJECT. token = parser.nextToken(); continue; } Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about parser position after copyCurrentStructure, but only asks to verify rather than proposing a concrete fix. The improved_code merely adds a comment.	Low
General	Validate inferencer-returned type value The unchecked cast to String will throw ClassCastException if a buggy inferencer returns a non-String value for the type key, breaking document parsing with a confusing error. Given the SPI-hardening intent visible elsewhere (catching inferencer exceptions, ambiguity checks), validate the type value and either throw a clear MapperParsingException naming the offending inferencer or fall back to the existing path. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1351-1355] -String inferredType = (String) inferredFieldMapping.get("type"); +Object rawType = inferredFieldMapping.get("type"); +if (rawType != null && rawType instanceof String == false) { + throw new MapperParsingException( + "dynamic field type inferencer [" + claimingInferencer.getClass().getName() + + "] returned a non-string 'type' value [" + rawType + "] for field [" + resolvedFieldName + "]" + ); +} +String inferredType = (String) rawType; if (inferredType == null) { replayThroughExistingPath(context, resolvedParent, resolvedFieldName, rawContent, parentMapperTuple.v1()); return true; } Suggestion importance[1-10]: 4 __ Why: Type-checking suggestion for a plugin SPI return value. Adds defensive validation but is a minor robustness improvement rather than a critical bug fix.	Low
	Guard against null template parse result The prior code guarded with if (template != null) before adding. Although DynamicTemplate.parse now appears to always return non-null (or throw), removing the null check assumes that invariant. If parse can still return null in any error path (deprecation logging path was removed here), a NullPointerException will be thrown on template.getPluginMatchType(). Retain a null-check or add an assertion to make the invariant explicit. server/src/main/java/org/opensearch/index/mapper/RootObjectMapper.java [286-296] DynamicTemplate template = DynamicTemplate.parse( templateName, templateParams, parserContext.mapperService().getDynamicTemplateTypes() ); +if (template == null) { + continue; +} if (template.getPluginMatchType() == null) { validateDynamicTemplate(parserContext, template); } else { validatePluginDynamicTemplate(parserContext, template); } templates.add(template); Suggestion importance[1-10]: 3 __ Why: DynamicTemplate.parse does not appear to return null in the current implementation (it either returns a template or throws), so the null-check is likely unnecessary defensive code.	Low

Suggestions up to commit 6f6a195

Category	Suggestion	Impact
Possible issue	Ensure ContentPath cleanup on exceptions If parseObjectOrField throws, context.path().remove() calls (for the field name and the dynamic parent paths) are skipped, leaving the ContentPath in a corrupt state that affects subsequent field parsing in the same document. Wrap the path mutations in try/finally so cleanup happens even on exceptions. The same issue exists in the inferencer replay block further down. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1280-1293] try ( XContentParser replayParser = contentType.xContent() .createParser(context.parser().getXContentRegistry(), context.parser().getDeprecationHandler(), rawContent) ) { replayParser.nextToken(); ParseContext replayContext = context.switchParser(replayParser); context.path().add(resolvedFieldName); - parseObjectOrField(replayContext, templateMapper); - context.path().remove(); -} -for (int i = 0; i < parentMapperTuple.v1(); i++) { - context.path().remove(); + try { + parseObjectOrField(replayContext, templateMapper); + } finally { + context.path().remove(); + } +} finally { + for (int i = 0; i < parentMapperTuple.v1(); i++) { + context.path().remove(); + } } return true; Suggestion importance[1-10]: 7 __ Why: Valid concern — if parseObjectOrField throws, the context.path() entries added for resolvedFieldName and the dynamic parent will not be released, leaving ContentPath in a corrupt state. Wrapping in try/finally improves robustness for error paths.	Medium
	Avoid ContentPath leak on fast-path return getMapper typically walks into subobjects via paths, adding entries to context.path() that must be released; calling it here without releasing those slots will corrupt the ContentPath for the subsequent normal-path processing. Verify whether getMapper mutates context.path() and, if so, release the added slots before returning false (mirroring the parentMapperTuple cleanup pattern). server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1197-1201] // Fast path: field is already mapped — let the normal path handle it +// NOTE: If getMapper mutates context.path(), ensure any added path components are released +// before returning here to avoid corrupting ContentPath state for subsequent parsing. Mapper existingMapper = getMapper(context, parentMapper, fieldName, paths); if (existingMapper != null) { return false; } Suggestion importance[1-10]: 4 __ Why: The suggestion asks the author to verify whether getMapper mutates context.path(). It is speculative rather than a confirmed bug, and its improved_code only adds a comment without a concrete fix.	Low
General	Avoid unnecessary buffering for unclaimed fields Every unmapped field now incurs full buffering + a byte[] copy, even when no plugin ultimately claims it and the content is just replayed. For large arrays/objects this is a significant per-document allocation and CPU cost. Consider skipping the buffer entirely when no template matches AND all inferencers can quickly reject based on token type (e.g., pre-filter inferencers by expected token type) to preserve the pre-PR performance for the common no-claim case. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1219-1225] -// Buffer the complete field value — needed for replay regardless of whether -// a plugin claims the field or not (streaming parser can only be read once) +// Buffer the complete field value — needed for replay if a plugin claims OR if we must fall through. +// TODO: consider a token-type pre-filter on inferencers to avoid buffering when no plugin +// could plausibly claim this token type, restoring zero-overhead for common unmapped fields. byte[] rawContent; try (XContentBuilder bufferBuilder = XContentBuilder.builder(contentType.xContent())) { bufferBuilder.copyCurrentStructure(parser); rawContent = BytesReference.toBytes(BytesReference.bytes(bufferBuilder)); } Suggestion importance[1-10]: 6 __ Why: Valid performance concern: buffering every unmapped field adds per-document overhead. The suggestion is a reasonable optimization idea but leaves the improvement as a TODO rather than an implementable change.	Low
	Ensure paths matches current field name currentFieldName is set in the previous branch when the token was FIELD_NAME, but paths is only populated there when the field name changes. On subsequent value tokens inside the same object, paths may still refer to a prior field or be stale. Confirm paths corresponds to currentFieldName at this point, or recompute it inside tryPluginInference unconditionally rather than trusting the passed-in array. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [628-639] } else { // Before branching by token type, offer the field to plugin inferencers. - ... - if (tryPluginInference(context, mapper, currentFieldName, paths)) { + // Recompute paths from currentFieldName to avoid relying on possibly stale state. + String[] currentPaths = mapper.disableObjects() + ? new String[] { currentFieldName } + : splitAndValidatePath(currentFieldName); + if (tryPluginInference(context, mapper, currentFieldName, currentPaths)) { token = parser.nextToken(); continue; } Suggestion importance[1-10]: 3 __ Why: In the surrounding code, paths is set alongside currentFieldName in the FIELD_NAME branch, so it should correspond correctly. The suggestion's concern about staleness appears unfounded given the control flow.	Low

[github-actions]

Persistent review updated to latest commit 7863ebc

[github-actions]

Persistent review updated to latest commit 6dae7af

[github-actions]

❕ Gradle check result for 6dae7af: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[codecov]

Codecov Report

❌ Patch coverage is 80.71749% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.37%. Comparing base (0456d83) to head (6dae7af).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...va/org/opensearch/index/mapper/DocumentParser.java	75.73%	22 Missing and 11 partials ⚠️
.../org/opensearch/index/mapper/RootObjectMapper.java	70.00%	7 Missing and 2 partials ⚠️
...a/org/opensearch/index/mapper/DynamicTemplate.java	96.55%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22607      +/-   ##
============================================
- Coverage     71.39%   71.37%   -0.03%     
- Complexity    76760    76802      +42     
============================================
  Files          6148     6148              
  Lines        357951   358155     +204     
  Branches      52170    52208      +38     
============================================
+ Hits         255556   255616      +60     
- Misses        82024    82224     +200     
+ Partials      20371    20315      -56     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Persistent review updated to latest commit 2ae39c1

[github-actions]

❌ Gradle check result for 2ae39c1: 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 000d534

[github-actions]

❌ Gradle check result for 000d534: null

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?