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 86c83b0)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Path corruption on template replay In attemptPluginInference, when a plugin template matches, context.path().add(resolvedFieldName) is called after switchParser, but the context.path().remove() in the finally operates on context.path(). Since switchParser likely shares the ContentPath, this is fine — but the context.addDynamicMapper(templateMapper) is called before the path slot for resolvedFieldName is added. If addDynamicMapper (or subsequent code) inspects the current path to determine the mapper's full path, the recorded path may be wrong (missing the field's own path component), leading to the dynamic mapper being registered under an incorrect path. Verify path semantics match those of the normal parseObject/parseValue code paths, which add the field name before building/registering the mapper. 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(); } } return true; } Possible NPE context.parser().contentType() is used to buffer content, but if the parser is at a position where contentType() semantics differ from the original stream, or the buffered bytes cannot be re-parsed as the original content type (e.g., SMILE/CBOR nuances), createParser on replay may misalign. Additionally, in replayThroughExistingPath, if replayToken is null after nextToken() (empty buffer), the switch defaults to no-op silently, causing the field to be dropped without diagnostic. Consider asserting or logging when a buffered field yields no token. 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); } } } } Behavior change The prior parse(name, conf) delegated to XContentFieldType.fromString(matchMappingType) which threw IllegalArgumentException with a specific message listing built-in types. The new implementation constructs a different error message and, when called via the 2-arg overload with an empty plugin registry, produces a message like "No field type matched on [text], possible values are [object, string, ...]" — the exact message and list format changed (now includes plugin types when registry is non-empty). Downstream tools or user-facing error strings that parsed the old message may break. The test update in DynamicTemplateTests acknowledges this, but the change is a subtle API-visible behavior change. String pluginMatchType = null; if (matchMappingType != null && !matchMappingType.equals("*")) { for (XContentFieldType t : XContentFieldType.values()) { if (t.toString().equals(matchMappingType)) { xcontentFieldType = t; break; } } if (xcontentFieldType == null) { // Validate the plugin type against the registry before storing it as the plugin match type. if (!knownPluginTypes.containsKey(matchMappingType)) { 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 [" + matchMappingType + "], possible values are " + allTypes ); } pluginMatchType = matchMappingType; } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 86c83b0

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Possible token skip after buffered copy After tryPluginInference consumes the buffered structure via copyCurrentStructure, the original parser is already positioned past the field value's end token (e.g. past END_OBJECT/END_ARRAY). Calling parser.nextToken() again here will advance past the next field's FIELD_NAME token, causing that field to be skipped. Since copyCurrentStructure leaves the parser positioned at the last token of the copied structure, the loop's own parser.nextToken() at the top of the next iteration should handle advancement — verify that this extra nextToken() call is correct and doesn't skip a field. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [662-665] if (tryPluginInference(context, mapper, currentFieldName, paths)) { - token = parser.nextToken(); continue; } Suggestion importance[1-10]: 7 __ Why: This raises a potentially valid concern: copyCurrentStructure leaves the parser at the last token of the copied structure, and the outer innerParseObject loop typically calls parser.nextToken() at the top. The extra nextToken() call could skip a token, which would be a real bug worth investigating.	Medium
	Avoid side effects before dynamic check Returning false after getDynamicParentMapper has been called causes the caller's normal switch path to also invoke logic that will call getDynamicParentMapper again on the same field, potentially double-counting side effects or producing inconsistent behavior on STRICT/FALSE. Consider deferring the call to getDynamicParentMapper until after the STRICT/FALSE dynamic check has been resolved via dynamicOrDefault(parentMapper, context) on the original parentMapper, only resolving the dynamic parent once the plugin path is actually going to proceed. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1231-1241] +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(); final int parentPathSlots = parentMapperTuple.v1(); -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 < parentPathSlots; i++) { - context.path().remove(); - } - return false; -} Suggestion importance[1-10]: 5 __ Why: The suggestion identifies a reasonable concern about calling getDynamicParentMapper before the dynamic check, though the current code does correctly release path slots. Reordering could be cleaner but the current implementation is functionally correct.	Low
General	Preserve builtin type-lookup error format The prior implementation used XContentFieldType.fromString(matchMappingType) which threw IllegalArgumentException for unknown types. The new manual loop silently falls through when the type is not a builtin, then checks the plugin registry. If knownPluginTypes is empty (2-arg parse path with Collections.emptyMap()), the resulting error message lists only builtin types plus an empty plugin set — but callers of the legacy 2-arg overload previously saw the exact fromString message. Ensure the error format matches expectations of existing tests and downstream tooling. server/src/main/java/org/opensearch/index/mapper/DynamicTemplate.java [253-274] XContentFieldType xcontentFieldType = null; String pluginMatchType = null; if (matchMappingType != null && !matchMappingType.equals("*")) { - for (XContentFieldType t : XContentFieldType.values()) { - if (t.toString().equals(matchMappingType)) { - xcontentFieldType = t; - break; + try { + xcontentFieldType = XContentFieldType.fromString(matchMappingType); + } catch (IllegalArgumentException e) { + if (!knownPluginTypes.containsKey(matchMappingType)) { + throw e; } + pluginMatchType = matchMappingType; } - if (xcontentFieldType == null) { - // Validate the plugin type against the registry before storing it as the plugin match type. - if (!knownPluginTypes.containsKey(matchMappingType)) { +} Suggestion importance[1-10]: 4 __ Why: Valid observation about error message format changes potentially affecting downstream tooling, but the PR tests already assert on the new format (containsString("No field type matched on [text]")), so the impact is limited.	Low
	Verify replay parser token position When the buffered field content is an OBJECT or ARRAY, parseObjectOrField on a leaf FieldMapper may consume tokens differently than expected. More importantly, calling parseObjectOrField on a non-object field mapper with START_OBJECT/START_ARRAY token semantics needs to match what a normal mapper would receive. Verify that the replay parser's token positioning (currently positioned at value start via nextToken()) matches what parseObjectOrField expects — normally the parser is positioned at the value token, but the outer parsing code sometimes expects the parser positioned at FIELD_NAME. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1334-1348] 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(); } } Suggestion importance[1-10]: 2 __ Why: The suggestion asks to verify behavior without proposing a concrete change (improved_code equals existing_code effectively), providing marginal value.	Low

---

Previous suggestions

Suggestions up to commit f719d9e

Category	Suggestion	Impact
Possible issue	Preserve builtin type matching semantics The new lookup loop silently swallows the previous behavior where XContentFieldType.fromString also accepted certain aliases/case variants. Confirm t.toString().equals(...) matches the original contract of fromString. If fromString did case-insensitive or alias matching, this change subtly breaks existing templates. Prefer calling XContentFieldType.fromString inside a try/catch to preserve the original matching semantics, then fall back to plugin lookup on failure. server/src/main/java/org/opensearch/index/mapper/DynamicTemplate.java [255-263] -for (XContentFieldType t : XContentFieldType.values()) { - if (t.toString().equals(matchMappingType)) { - xcontentFieldType = t; - break; - } +try { + xcontentFieldType = XContentFieldType.fromString(matchMappingType); +} catch (IllegalArgumentException ex) { + // Not a builtin — try plugin registry below. } if (xcontentFieldType == null) { - // Validate the plugin type against the registry before storing it as the plugin match type. if (!knownPluginTypes.containsKey(matchMappingType)) { Suggestion importance[1-10]: 7 __ Why: Valid concern: XContentFieldType.fromString may have different semantics than the new t.toString().equals(...) loop, and replacing it could subtly change behavior. Using fromString with try/catch preserves original matching semantics.	Medium
	Guard plugin hook against non-value tokens tryPluginInference is invoked for every token, including END_OBJECT, FIELD_NAME follow-ups already consumed above, and other non-value tokens. Since the hook buffers content via copyCurrentStructure, calling it on non-value tokens can cause incorrect buffering or unexpected parser state changes. Restrict the call to value/start tokens (START_OBJECT, START_ARRAY, VALUE_*) to match the switch cases below. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [662-665] -if (tryPluginInference(context, mapper, currentFieldName, paths)) { +if ((token == XContentParser.Token.START_OBJECT + || token == XContentParser.Token.START_ARRAY + || token.isValue() + || token == XContentParser.Token.VALUE_NULL) + && tryPluginInference(context, mapper, currentFieldName, paths)) { token = parser.nextToken(); continue; } Suggestion importance[1-10]: 6 __ Why: The hook is placed in the else branch where token is not FIELD_NAME, but END_OBJECT or END_ARRAY could theoretically reach it. However, in innerParseObject, END_OBJECT is the loop termination condition, so the concern may be partially mitigated. Still, adding a guard is defensive and reasonable.	Low
General	Ensure path slots released on exceptions getDynamicParentMapper may add path slots and can throw (e.g. StrictDynamicMappingException on intermediate objects). Since the try/finally that releases the added slots begins only after this call returns, an exception thrown mid-way through getDynamicParentMapper could leave ContentPath corrupted. Wrap the entire slot-tracked section in the try block, or ensure getDynamicParentMapper is exception-safe with respect to path slots. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1224-1231] -// Fast path: field is already mapped — let the normal path handle it -Mapper existingMapper = getMapper(context, parentMapper, fieldName, paths); -if (existingMapper != null) { - return false; +final String[] resolvedPaths = paths != null ? paths : splitAndValidatePath(fieldName); +Tuple<Integer, ObjectMapper> parentMapperTuple; +try { + parentMapperTuple = getDynamicParentMapper(context, resolvedPaths, parentMapper); +} catch (RuntimeException e) { + throw e; } +ObjectMapper resolvedParent = parentMapperTuple.v2(); +final int parentPathSlots = parentMapperTuple.v1(); -// Only fire for dynamic=TRUE / STRICT_ALLOW_TEMPLATES / FALSE_ALLOW_TEMPLATES -final String[] resolvedPaths = paths != null ? paths : splitAndValidatePath(fieldName); -Tuple<Integer, ObjectMapper> parentMapperTuple = getDynamicParentMapper(context, resolvedPaths, parentMapper); - Suggestion importance[1-10]: 6 __ Why: Legitimate concern about exception safety: if getDynamicParentMapper throws after adding path slots, they leak. However, the proposed improved_code simply rethrows without addressing the issue, so the fix is not effective as written.	Low
	Add path slot before switching parser The field-name path slot is added AFTER switchParser but the try block only guards parseObjectOrField. If context.path().add(resolvedFieldName) succeeds but parseObjectOrField throws, the slot is released — good. However, if the resolved field name contains dots and splitAndValidatePath was bypassed, adding a single dotted name creates a path inconsistency. Verify resolvedFieldName is the last simple path component (which it is via resolvedPaths[resolvedPaths.length - 1]), and consider adding the slot BEFORE switchParser for symmetry with parent path handling. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1339-1347] -ParseContext replayContext = context.switchParser(replayParser); context.path().add(resolvedFieldName); try { + ParseContext replayContext = context.switchParser(replayParser); 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(); } Suggestion importance[1-10]: 3 __ Why: The suggestion is largely cosmetic; switchParser is unlikely to throw and the current ordering works correctly. The rationale about dotted names is speculative and not clearly a bug.	Low

Suggestions up to commit 5ac01a0

Category	Suggestion	Impact
Possible issue	Avoid double-advancing parser after inference After tryPluginInference consumes the field (via copyCurrentStructure and/or replay through parseObject/parseArray/parseValue), the parser is already positioned at the END_OBJECT/END_ARRAY/value's end token. Calling parser.nextToken() here advances past that end token, which can skip a following FIELD_NAME or the object's own END_OBJECT, causing fields to be silently dropped or parsing to misalign. The loop's own parser.nextToken() at the top of the while should handle advancement, so simply continue without an extra advance. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [662-665] if (tryPluginInference(context, mapper, currentFieldName, paths)) { - token = parser.nextToken(); continue; } Suggestion importance[1-10]: 8 __ Why: After copyCurrentStructure consumes the field, calling parser.nextToken() in addition to the loop's own advance could skip a subsequent field. This is a plausible parsing correctness concern, though it depends on the exact post-state of copyCurrentStructure and replay logic.	Medium
	Fix path context when building inferred mapper The BuilderContext is built using context.path() before adding the field-name path slot for the field. For nested fields this yields an incorrect full path on the resulting mapper (missing the leaf field name), which can cause the dynamic mapping update to be registered at the wrong path. Add the field-name slot before constructing the builder context (and remove it after), mirroring how other dynamic-mapping paths in this file build sub-mappers. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1432-1435] -Mapper.Builder<?> builder = typeParser.parse(resolvedFieldName, inferredFieldMapping, parserContext); -Mapper.BuilderContext builderContext = new Mapper.BuilderContext(context.indexSettings().getSettings(), context.path()); -Mapper inferredMapper = builder.build(builderContext); +context.path().add(resolvedFieldName); +Mapper.Builder<?> builder; +Mapper inferredMapper; +try { + builder = typeParser.parse(resolvedFieldName, inferredFieldMapping, parserContext); + Mapper.BuilderContext builderContext = new Mapper.BuilderContext(context.indexSettings().getSettings(), context.path()); + inferredMapper = builder.build(builderContext); +} finally { + context.path().remove(); +} context.addDynamicMapper(inferredMapper); Suggestion importance[1-10]: 7 __ Why: Building the mapper's BuilderContext without the leaf field-name slot may cause the mapper's full path to be incorrect, which affects dynamic mapping updates for nested fields. This is a valid concern regarding path consistency.	Medium
	Add field path before building template mapper The templateMapper was built via templateBuilder.build(templateBuilderContext) with context.path() not yet containing the field-name slot. This means the mapper's full path is missing the leaf component, which will misregister the dynamic mapping update. Add the field-name slot to context.path() before calling templateBuilder.build(...) so the mapper is constructed with the correct full path. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1331-1348] -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 { +context.path().add(resolvedFieldName); +try { + 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); 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 { + context.path().remove(); } Suggestion importance[1-10]: 7 __ Why: Similar to suggestion 2, the template mapper is built before the field-name path slot is added, which may cause the mapper to have an incomplete full path. This is a legitimate concern for correct dynamic mapping registration.	Medium

Suggestions up to commit 6710bd9

Category	Suggestion	Impact
Possible issue	Include field name in builder path The builderContext uses context.path() without adding the field name slot before building, whereas the template path adds resolvedFieldName to the path only during replay. If the inferred mapper's builder relies on the current path for full-name construction (as most FieldMapper builders do), the field's fullname will be missing its leaf component, producing an incorrectly-named mapper. Add the field name to the path before build(). server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1432-1435] Mapper.Builder<?> builder = typeParser.parse(resolvedFieldName, inferredFieldMapping, parserContext); -Mapper.BuilderContext builderContext = new Mapper.BuilderContext(context.indexSettings().getSettings(), context.path()); -Mapper inferredMapper = builder.build(builderContext); +context.path().add(resolvedFieldName); +Mapper inferredMapper; +try { + Mapper.BuilderContext builderContext = new Mapper.BuilderContext(context.indexSettings().getSettings(), context.path()); + inferredMapper = builder.build(builderContext); +} finally { + context.path().remove(); +} context.addDynamicMapper(inferredMapper); Suggestion importance[1-10]: 7 __ Why: This is a plausible correctness concern: the template path adds the field name to context.path() during replay but the inferencer path builds the mapper before adding the field name, which could produce a mapper with an incorrect full name. Worth investigating.	Medium
	Restrict plugin hook to value/start tokens tryPluginInference is invoked for every non-FIELD_NAME token, including END_OBJECT, END_ARRAY, etc., which will cause it to buffer/parse content unnecessarily and may misbehave when currentFieldName refers to a prior sibling. Restrict the hook to value/start tokens (e.g. START_OBJECT, START_ARRAY, VALUE_*) before calling. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [662-665] -if (tryPluginInference(context, mapper, currentFieldName, paths)) { +if ((token == XContentParser.Token.START_OBJECT + || token == XContentParser.Token.START_ARRAY + || token.isValue() + || token == XContentParser.Token.VALUE_NULL) + && tryPluginInference(context, mapper, currentFieldName, paths)) { token = parser.nextToken(); continue; } Suggestion importance[1-10]: 5 __ Why: The suggestion raises a reasonable concern but in the existing innerParseObject loop the else branch is only reached for non-FIELD_NAME tokens that are already value/start tokens (END_* tokens typically terminate the loop). The added guard could still be a defensive improvement but is not clearly a bug fix.	Low
General	Preserve original error semantics without registry The previous behavior used XContentFieldType.fromString(matchMappingType) which threw an IllegalArgumentException on unknown types. The new manual loop silently sets xcontentFieldType = null for unknown types, and only later checks the plugin registry. For the 2-arg parse (no plugin registry), any unknown type now falls through to the plugin check which will always fail—make sure the error message list remains consistent and doesn't confuse users when the registry is empty. server/src/main/java/org/opensearch/index/mapper/DynamicTemplate.java [254-274] for (XContentFieldType t : XContentFieldType.values()) { if (t.toString().equals(matchMappingType)) { xcontentFieldType = t; break; } } +if (xcontentFieldType == null && knownPluginTypes.isEmpty()) { + // Preserve pre-SPI error format when no plugin registry is supplied. + XContentFieldType.fromString(matchMappingType); // throws with the canonical message +} if (xcontentFieldType == null) { Suggestion importance[1-10]: 3 __ Why: The new code does throw an equivalent IllegalArgumentException with a similar message listing all types; the test testParseUnknownMatchType was updated accordingly. The suggestion's concern about message consistency is minor.	Low
	Verify original parser positioning after replay After the try-with-resources completes, the original parser (context.parser()) is still positioned at the original field value, but the outer innerParseObject loop will call parser.nextToken() expecting to have consumed it. Since content was buffered via copyCurrentStructure, the original parser is already advanced past the value—verify token positioning is consistent, otherwise subsequent fields may be skipped or mis-parsed. Consider explicitly not switching back and ensuring the outer parser state matches expectations. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1334-1348] +// Ensure original parser is positioned correctly after replay; copyCurrentStructure +// already advanced it past the value, matching what innerParseObject's loop expects. 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(); } } Suggestion importance[1-10]: 2 __ Why: The suggestion only asks to verify positioning and does not actually change the code meaningfully, providing minimal value.	Low

Suggestions up to commit f4da253

Category	Suggestion	Impact
Possible issue	Include field name in builder path The BuilderContext is constructed with the current context.path() but the field-name slot is not added until later (inside the replay try-block). This means the mapper is built with a path that does not include the field name, which will produce an incorrect fullPath for the created mapper (parent path only). Add the field name to the path before building so fullPath() reflects the correct location, mirroring how parseObjectOrField treats a mapper's own name. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1432-1435] -Mapper.Builder<?> builder = typeParser.parse(resolvedFieldName, inferredFieldMapping, parserContext); -Mapper.BuilderContext builderContext = new Mapper.BuilderContext(context.indexSettings().getSettings(), context.path()); -Mapper inferredMapper = builder.build(builderContext); +context.path().add(resolvedFieldName); +Mapper.Builder<?> builder; +Mapper inferredMapper; +try { + builder = typeParser.parse(resolvedFieldName, inferredFieldMapping, parserContext); + Mapper.BuilderContext builderContext = new Mapper.BuilderContext(context.indexSettings().getSettings(), context.path()); + inferredMapper = builder.build(builderContext); +} finally { + context.path().remove(); +} context.addDynamicMapper(inferredMapper); Suggestion importance[1-10]: 7 __ Why: Valid observation that the BuilderContext is built without the field name in the path, which could result in an incorrect fullPath for the inferred mapper. However, this pattern is used consistently elsewhere in DocumentParser (e.g., template path) so the concern may be mitigated by how mapper builders handle names.	Medium
	Avoid duplicate dynamic mapper side effects Calling getDynamicParentMapper here duplicates the work done later by the normal path (parseObject/parseArray/parseValue also invoke it). When tryPluginInference returns false, the added path slots are released, but any side effect of getDynamicParentMapper (e.g. creation of intermediate dynamic object mappers via context.addDynamicMapper) will persist and be duplicated by the subsequent normal path, potentially causing duplicate dynamic mapper updates for the same intermediate object. Consider a non-mutating lookup for the parent, or ensure idempotence. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1231-1241] +// Consider a read-only resolution of parent/dynamic without side effects, +// or ensure getDynamicParentMapper is safely idempotent for the fall-through case. Tuple<Integer, ObjectMapper> parentMapperTuple = getDynamicParentMapper(context, resolvedPaths, parentMapper); -ObjectMapper resolvedParent = parentMapperTuple.v2(); -final int parentPathSlots = parentMapperTuple.v1(); -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 < parentPathSlots; i++) { - context.path().remove(); - } - return false; -} Suggestion importance[1-10]: 6 __ Why: Legitimate concern that getDynamicParentMapper may have side effects (e.g., creating intermediate dynamic mappers) that could be duplicated when falling through to the normal path. This could cause real bugs if not idempotent.	Low
	Verify parser position invariant after replay After tryPluginInference consumes and replays the field value, the parser is already positioned past the value's closing token. Calling parser.nextToken() here advances one more token which will typically be the next FIELD_NAME (or END_OBJECT), which is the same behavior the switch cases produce — but if tryPluginInference leaves the parser at the last token of the value (e.g. END_OBJECT/END_ARRAY of the field's structure) rather than after it, the following iteration will misinterpret tokens. Verify the parser position invariant post-replay matches what the normal switch paths leave (i.e. positioned at the value's terminal token so the outer loop's nextToken() moves to the next field). server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [662-665] +if (tryPluginInference(context, mapper, currentFieldName, paths)) { + token = parser.nextToken(); + continue; +} - Suggestion importance[1-10]: 5 __ Why: Valid concern about parser positioning invariants after tryPluginInference replay, but it only asks the author to verify rather than identifying a concrete bug. Moderate importance since incorrect positioning could cause subtle parsing issues.	Low
General	Preserve legacy error behavior for builtins The previous behavior used XContentFieldType.fromString(matchMappingType) which threw a specific IllegalArgumentException with a detailed message. The new manual loop silently falls through to the plugin registry check, and when the registry is empty (public 2-arg parse), the error message differs from historical output. Verify all callers/tests that depend on the exact prior error message are updated, and consider preserving the original fromString behavior when knownPluginTypes is empty to avoid unexpected message changes for external consumers. server/src/main/java/org/opensearch/index/mapper/DynamicTemplate.java [254-263] if (matchMappingType != null && !matchMappingType.equals("*")) { for (XContentFieldType t : XContentFieldType.values()) { if (t.toString().equals(matchMappingType)) { xcontentFieldType = t; break; } } if (xcontentFieldType == null) { - // Validate the plugin type against the registry before storing it as the plugin match type. - if (!knownPluginTypes.containsKey(matchMappingType)) { + if (knownPluginTypes.isEmpty()) { + xcontentFieldType = XContentFieldType.fromString(matchMappingType); + } else if (!knownPluginTypes.containsKey(matchMappingType)) { Suggestion importance[1-10]: 3 __ Why: Minor concern about error message compatibility. The new error message actually includes plugin types when available, and the existing test was updated to match containsString("No field type matched on [text]") which still works. Low impact.	Low

Suggestions up to commit ecb227f

Category	Suggestion	Impact
Possible issue	Guard plugin hook to value tokens only The hook fires on every non-FIELD_NAME token, including END_OBJECT (which closes the enclosing object) and other structural tokens where currentFieldName is stale or the parser is not positioned at a value. This can cause spurious buffering/inference attempts and incorrect path resolution. Restrict the invocation to value-bearing tokens (START_OBJECT, START_ARRAY, VALUE_*) only. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [662-665] -if (tryPluginInference(context, mapper, currentFieldName, paths)) { +if ((token == XContentParser.Token.START_OBJECT + || token == XContentParser.Token.START_ARRAY + || token.isValue() + || token == XContentParser.Token.VALUE_NULL) + && tryPluginInference(context, mapper, currentFieldName, paths)) { token = parser.nextToken(); continue; } Suggestion importance[1-10]: 6 __ Why: Valid concern — the hook fires on every non-FIELD_NAME token, and restricting it to value-bearing tokens would avoid unnecessary work. However, the existing switch inside tryPluginInference and innerParseObject likely already handles non-value tokens gracefully; still, guarding is a reasonable defensive improvement.	Low
	Restore original parser after replay After switchParser, the original parser in the outer context is not restored, so subsequent code that reads from context.parser() (e.g., the caller's loop calling parser.nextToken() on the outer parser) still sees the original parser — but any code path that later resolves the parser via context during replay may be inconsistent. Ensure the parser is switched back (or use the replayContext consistently) so state is not left dangling if switchParser mutates the outer context. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1334-1348] try ( XContentParser replayParser = contentType.xContent() .createParser(context.parser().getXContentRegistry(), context.parser().getDeprecationHandler(), rawContent) ) { replayParser.nextToken(); + XContentParser previousParser = context.parser(); 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(); + context.switchParser(previousParser); } } Suggestion importance[1-10]: 4 __ Why: The suggestion is speculative about switchParser semantics without clear evidence that it mutates the outer context. The replayContext is a separate context returned by switchParser, so restoring is likely unnecessary, though the concern warrants verification.	Low
	Verify no ContentPath mutation in fast path getMapper may invoke getDynamicParentMapper internally (depending on the code path) which mutates context.path(). Calling it here for the fast-path check, then calling getDynamicParentMapper again below, can double-add path slots. Verify getMapper does not add path slots, or capture and release them symmetrically, to avoid a corrupted ContentPath for the rest of the document. server/src/main/java/org/opensearch/index/mapper/DocumentParser.java [1223-1227] -// Fast path: field is already mapped — let the normal path handle it +// Fast path: field is already mapped — let the normal path handle it. +// Note: ensure getMapper does not mutate context.path(); otherwise release slots symmetrically. Mapper existingMapper = getMapper(context, parentMapper, fieldName, paths); if (existingMapper != null) { return false; } Suggestion importance[1-10]: 3 __ Why: This is a verification request rather than a concrete fix, and the improved_code is nearly identical to the existing code with only a comment added. Low impact.	Low
General	Preserve builtin type lookup semantics Previously XContentFieldType.fromString was used, which handled aliases/exact matching consistently. Replacing it with a manual toString().equals() loop may miss types that have alternative accepted names (if any) and duplicates logic. Prefer keeping fromString for the built-in lookup and only fall back to the plugin registry when it throws. server/src/main/java/org/opensearch/index/mapper/DynamicTemplate.java [254-274] -for (XContentFieldType t : XContentFieldType.values()) { - if (t.toString().equals(matchMappingType)) { - xcontentFieldType = t; - break; - } +try { + xcontentFieldType = XContentFieldType.fromString(matchMappingType); +} catch (IllegalArgumentException ex) { + // Not a builtin — check plugin registry below. } if (xcontentFieldType == null) { - // Validate the plugin type against the registry before storing it as the plugin match type. if (!knownPluginTypes.containsKey(matchMappingType)) { Suggestion importance[1-10]: 5 __ Why: Reasonable suggestion to reuse XContentFieldType.fromString for consistency and to avoid duplicating logic, though the manual loop is functionally equivalent given fromString's current implementation just iterates values.	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.

[github-actions]

Persistent review updated to latest commit 1e88a41

[naykudev]

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.

Medium (DocumentParser:1186, plugin inferencer hook): this is by design and consistent with the trusted-plugin model. Plugins already run in-process with full access to document content (analyzers, ingest processors, mapper parsers), so this hook does not expand the trust boundary. It only fires for unmapped fields when no dynamic template matches, and only for plugins the cluster admin explicitly installed. There is no untrusted-plugin threat model in OpenSearch to defend against here, so no code change is warranted. Requesting a maintainer apply skip-diff-analyzer after review.

Low (DynamicTemplateTests:58, deferred match_mapping_type validation): the relaxation is intentional and scoped. An unknown type is still rejected, just against the plugin registry (known builtin types plus registered plugin types) rather than only the builtin enum. Typos in a plain builtin type still fail. This is required so a plugin type like knn_vector is accepted. The public 2-arg DynamicTemplate.parse still fails fast on any unknown type, so the pre-SPI behavior is preserved where no registry is available.

[github-actions]

Persistent review updated to latest commit dbd28f0

[github-actions]

❌ Gradle check result for dbd28f0: 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 f0210af

[github-actions]

Persistent review updated to latest commit d8dd803

[github-actions]

❌ Gradle check result for d8dd803: 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?

[github-actions]

Persistent review updated to latest commit ecb227f

[github-actions]

❌ Gradle check result for ecb227f: TIMEOUT

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?

[shatejas]

and only for plugins the cluster admin explicitly installed
@naykudev are we sure about this, there is no path where a malicious plugin can be picked up right?

[github-actions]

Persistent review updated to latest commit f4da253

[github-actions]

Persistent review updated to latest commit 6710bd9

[naykudev]

and only for plugins the cluster admin explicitly installed
@naykudev are we sure about this, there is no path where a malicious plugin can be picked up right?

Yes, confirmed. There's no runtime/network path to install a plugin, the only plugin REST endpoint is read-only GET /_cat/plugins. Plugins load once at node startup from the on-disk plugins/ dir, so installing one requires filesystem access to the node plus a restart. A remote attacker with just cluster API access can't get a plugin picked up; it takes a cluster admin (or someone with host access) deliberately installing it.

[github-actions]

✅ Gradle check result for 6710bd9: SUCCESS

[github-actions]

Persistent review updated to latest commit 5ac01a0

[github-actions]

❌ Gradle check result for 5ac01a0: 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?

[github-actions]

Persistent review updated to latest commit f719d9e

[github-actions]

Persistent review updated to latest commit 86c83b0

[github-actions]

✅ Gradle check result for 86c83b0: SUCCESS