Fix field_caps returning empty results for disable_objects mappings

[newtonne]

Description

_field_caps returns an empty response for indexes where a disable_objects: true object field has been populated with a child field. A second bug causes such child fields to be corrupted after any subsequent document is indexed into the same index - for example attributes.foo.bar becomes attributes.foo.foo.bar.

Root causes

When _field_caps walks the parent chain of a flattened leaf field (e.g. attributes.foo.bar), it looks up an ObjectMapper for each intermediate path. Under disable_objects: true, intermediate paths like attributes.foo have no ObjectMapper by design - the fix adds a null check to skip them and continue up the chain.

The field name corruption is caused by ParametrizedFieldMapper.merge() using name().lastIndexOf('.') to reconstruct the parent ContentPath when rebuilding a mapper. For a field with simpleName foo.bar and full name attributes.foo.bar, this returns the position of the dot before bar rather than the dot before foo.bar, so the parent path is computed as attributes.foo instead of attributes. The fix computes the boundary from simpleName.length() instead.

Related Issues

Resolves #20811

Check List

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

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

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit e212d0d)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Fix field_caps empty results for disable_objects mappings Relevant files: server/src/main/java/org/opensearch/action/fieldcaps/TransportFieldCapabilitiesIndexAction.java rest-api-spec/src/main/resources/rest-api-spec/test/field_caps/40_disable_objects.yml server/src/test/java/org/opensearch/action/fieldcaps/TransportFieldCapabilitiesIndexActionTests.java Sub-PR theme: Fix field name corruption in ParametrizedFieldMapper.merge() for disable_objects Relevant files: server/src/main/java/org/opensearch/index/mapper/ParametrizedFieldMapper.java

⚡ Recommended focus areas for review Infinite Loop Risk When a null ObjectMapper is found and dotIndex is recomputed via parentField.lastIndexOf('.'), if parentField has no dot, dotIndex becomes -1 and the loop condition dotIndex > 0 will terminate correctly. However, if there is a chain of multiple consecutive intermediate paths with no ObjectMapper (all under disable_objects), the loop will keep iterating and recomputing dotIndex correctly. This should be verified to ensure the loop always terminates and does not skip the responseMap.containsKey(parentField) check for intermediate paths that do have an ObjectMapper. if (mapper == null) { // parentField is part of a literal dotted field name under a disable_objects=true parent // No ObjectMapper exists for this intermediate path so skip it and continue up the chain dotIndex = parentField.lastIndexOf('.'); continue; } Edge Case: simpleName longer than name In parentPath, if simpleName.length() is greater than name.length(), endPos would be a large negative number (not just -1), and the check endPos < 0 correctly handles this. However, if simpleName equals name exactly (top-level field with no parent), endPos would be -1, which is handled. The case where simpleName is longer than name should not occur in practice but could indicate a data integrity issue worth validating. private static ContentPath parentPath(String name, String simpleName) { // Use simpleName to compute the parent path so that fields whose simpleName contains dots // (because of disable_objects) get the correct parent path int endPos = name.length() - simpleName.length() - 1; if (endPos < 0) { return new ContentPath(0); } return new ContentPath(name.substring(0, endPos)); Index Name Reuse Both testFieldCapsDisableObjects and testFieldCapsDisableObjectsAfterUnrelatedDocument use the same index name "test". If these tests run in the same test class instance without cleanup between tests, the second test may fail or produce unexpected results due to the pre-existing index from the first test. assertAcked(client().admin().indices().prepareCreate("test").setMapping(mapping)); client().prepareIndex("test") .setId("1") .setSource( XContentFactory.jsonBuilder() .startObject() .startObject("attributes") .startObject("foo") .field("bar", "baz") .endObject() .endObject() .endObject() ) .get(); client().admin().indices().prepareRefresh("test").get(); FieldCapabilitiesResponse response = client().fieldCaps(new FieldCapabilitiesRequest().fields("*").indices("test")).actionGet(); Map<String, Map<String, FieldCapabilities>> fields = response.get(); assertTrue("expected attributes.foo.bar in field caps", fields.containsKey("attributes.foo.bar")); assertTrue("expected attributes in field caps", fields.containsKey("attributes")); assertEquals("object", fields.get("attributes").values().iterator().next().getType()); // phantom intermediate path has no ObjectMapper and must not exist assertFalse("attributes.foo should not exist in field caps", fields.containsKey("attributes.foo")); } // Indexing a second document after a disable_objects field must not corrupt the flattened field // name. Previously "attributes.foo.bar" became "attributes.foo.foo.bar" after a mapping merge // triggered by an unrelated document. public void testFieldCapsDisableObjectsAfterUnrelatedDocument() throws Exception { String mapping = XContentFactory.jsonBuilder() .startObject() .startObject("properties") .startObject("attributes") .field("type", "object") .field("disable_objects", true) .endObject() .endObject() .endObject() .toString(); assertAcked(client().admin().indices().prepareCreate("test").setMapping(mapping));

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to e212d0d

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Use unique index names to avoid test interference All three test methods create an index named "test". Since OpenSearchSingleNodeTestCase reuses a single node across tests in the same class, if tests run in sequence without cleanup, the second and third test will fail trying to create an already-existing index. Use unique index names per test method (e.g., "test_disable_objects", "test_disable_objects_merge", "test_normal_nested") or ensure the index is deleted in a @After method. server/src/test/java/org/opensearch/action/fieldcaps/TransportFieldCapabilitiesIndexActionTests.java [36] -assertAcked(client().admin().indices().prepareCreate("test").setMapping(mapping)); -... -assertAcked(client().admin().indices().prepareCreate("test").setMapping(mapping)); -... -assertAcked(client().admin().indices().prepareCreate("test").setMapping(mapping)); +// In testFieldCapsDisableObjects: +assertAcked(client().admin().indices().prepareCreate("test_disable_objects").setMapping(mapping)); +// update all subsequent references to "test" → "test_disable_objects" +// In testFieldCapsDisableObjectsAfterUnrelatedDocument: +assertAcked(client().admin().indices().prepareCreate("test_disable_objects_merge").setMapping(mapping)); +// update all subsequent references to "test" → "test_disable_objects_merge" + +// In testFieldCapsNormalNestedObjectUnaffected: +assertAcked(client().admin().indices().prepareCreate("test_normal_nested").setMapping(mapping)); +// update all subsequent references to "test" → "test_normal_nested" + Suggestion importance[1-10]: 7 __ Why: All three test methods use the same index name "test", which will cause failures when tests run sequentially on a shared node without cleanup. This is a real correctness issue that would cause test failures, making it an important fix.	Medium
	Prevent potential infinite loop on null mapper When mapper == null, the code updates dotIndex to parentField.lastIndexOf('.') and then continues the loop. However, the outer loop likely recomputes parentField from dotIndex at the top of each iteration. If dotIndex is not updated before the continue, the loop may revisit the same parentField indefinitely. Verify that the loop uses the updated dotIndex to recompute parentField at the start of each iteration; if not, the parentField must also be recomputed here to avoid an infinite loop. server/src/main/java/org/opensearch/action/fieldcaps/TransportFieldCapabilitiesIndexAction.java [183-188] if (mapper == null) { // parentField is part of a literal dotted field name under a disable_objects=true parent // No ObjectMapper exists for this intermediate path so skip it and continue up the chain dotIndex = parentField.lastIndexOf('.'); + parentField = dotIndex > 0 ? parentField.substring(0, dotIndex) : ""; continue; } Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about potential infinite loops, but without seeing the full loop body it's unclear whether parentField is recomputed from dotIndex at the top of each iteration. The fix adds parentField recomputation as a safety measure, but this may be redundant if the loop already handles it. The risk is real but the fix may be unnecessary depending on the loop structure.	Low
General	Guard against incorrect suffix assumption in path computation The computation name.length() - simpleName.length() - 1 assumes that simpleName is always a suffix of name. If simpleName is longer than name or is not actually a suffix (e.g., due to an unexpected mapping state), endPos could be negative or point to the wrong position, silently producing an incorrect parent path. Add a guard to verify that name actually ends with simpleName before using this arithmetic. server/src/main/java/org/opensearch/index/mapper/ParametrizedFieldMapper.java [136-139] int endPos = name.length() - simpleName.length() - 1; -if (endPos < 0) { +if (endPos < 0 || !name.endsWith(simpleName)) { return new ContentPath(0); } Suggestion importance[1-10]: 4 __ Why: Adding a !name.endsWith(simpleName) check is a reasonable defensive guard, but in practice simpleName should always be a suffix of name in valid mapper state. This is a minor defensive improvement that prevents silent incorrect behavior in edge cases.	Low

---

Previous suggestions

Suggestions up to commit 22919db

Category	Suggestion	Impact
Possible issue	Use unique index names per test to avoid conflicts All three test methods create an index named "test", which will cause conflicts if the tests run in the same node/cluster context (as is typical with OpenSearchSingleNodeTestCase). Each test should use a unique index name, or the index should be deleted in a tearDown method to prevent IndexAlreadyExistsException failures. server/src/test/java/org/opensearch/action/fieldcaps/TransportFieldCapabilitiesIndexActionTests.java [36] -assertAcked(client().admin().indices().prepareCreate("test").setMapping(mapping)); -... -assertAcked(client().admin().indices().prepareCreate("test").setMapping(mapping)); -... -assertAcked(client().admin().indices().prepareCreate("test").setMapping(mapping)); +// testFieldCapsDisableObjects +assertAcked(client().admin().indices().prepareCreate("test_disable_objects_unit").setMapping(mapping)); +// testFieldCapsDisableObjectsAfterUnrelatedDocument +assertAcked(client().admin().indices().prepareCreate("test_disable_objects_merge_unit").setMapping(mapping)); +// testFieldCapsNormalNestedObjectUnaffected +assertAcked(client().admin().indices().prepareCreate("test_normal_nested_unit").setMapping(mapping)); Suggestion importance[1-10]: 7 __ Why: This is a valid and important concern — all three test methods use the same index name "test" in a OpenSearchSingleNodeTestCase, which shares a single node. This can cause IndexAlreadyExistsException failures when tests run sequentially. Using unique index names per test would prevent such conflicts.	Medium
	Guard against infinite loop when no more parent segments exist When mapper == null, the code updates dotIndex to parentField.lastIndexOf('.') and then continues the loop. However, the outer loop likely recomputes parentField from dotIndex at the top of each iteration. If dotIndex is not updated before the continue, the loop may revisit the same parentField indefinitely. Verify that the loop uses the updated dotIndex value at the start of each iteration; if not, the dotIndex assignment must happen before continue (which it does here), but also confirm the loop condition and variable update order to avoid an infinite loop when multiple consecutive intermediate paths have no ObjectMapper. server/src/main/java/org/opensearch/action/fieldcaps/TransportFieldCapabilitiesIndexAction.java [183-188] if (mapper == null) { // parentField is part of a literal dotted field name under a disable_objects=true parent // No ObjectMapper exists for this intermediate path so skip it and continue up the chain dotIndex = parentField.lastIndexOf('.'); + if (dotIndex == -1) { + break; // no more parent segments to walk + } continue; } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that when dotIndex becomes -1 (no more parent segments), the loop could behave unexpectedly. Adding a break when dotIndex == -1 is a valid defensive guard, though the outer loop likely already handles dotIndex == -1 as a termination condition. This is a minor safety improvement.	Low
General	Validate simpleName is a proper suffix of name The new formula endPos = name.length() - simpleName.length() - 1 assumes that simpleName is always a suffix of name and that the separator character (.) occupies exactly one position. If simpleName is longer than name (e.g., due to an unexpected state), endPos would be negative and the early return handles it, but if simpleName is not actually a suffix of name, name.substring(0, endPos) would silently return a wrong parent path. Add an assertion or validation that name ends with "." + simpleName to catch such inconsistencies early. server/src/main/java/org/opensearch/index/mapper/ParametrizedFieldMapper.java [136-140] int endPos = name.length() - simpleName.length() - 1; if (endPos < 0) { return new ContentPath(0); } +assert name.charAt(endPos) == '.' && name.endsWith(simpleName) + : "simpleName [" + simpleName + "] is not a dot-separated suffix of name [" + name + "]"; return new ContentPath(name.substring(0, endPos)); Suggestion importance[1-10]: 3 __ Why: Adding an assertion to validate that simpleName is a proper dot-separated suffix of name is a reasonable defensive check, but it's a debug-only assertion that won't affect production behavior. The improvement is marginal and mainly aids in catching bugs during development.	Low

Suggestions up to commit b4eb2f0

Category	Suggestion	Impact
Possible issue	Use unique index names to avoid test interference All three test methods create an index named "test". Since OpenSearchSingleNodeTestCase reuses a single node across tests in the same class, if the index is not deleted between tests, the second and third test will fail with an "index already exists" error. Use unique index names per test method or add teardown logic to delete the index after each test. server/src/test/java/org/opensearch/action/fieldcaps/TransportFieldCapabilitiesIndexActionTests.java [36] -assertAcked(client().admin().indices().prepareCreate("test").setMapping(mapping)); -... -assertAcked(client().admin().indices().prepareCreate("test").setMapping(mapping)); -... -assertAcked(client().admin().indices().prepareCreate("test").setMapping(mapping)); +// In testFieldCapsDisableObjects: +assertAcked(client().admin().indices().prepareCreate("test_disable_objects_unit").setMapping(mapping)); +// In testFieldCapsDisableObjectsAfterUnrelatedDocument: +assertAcked(client().admin().indices().prepareCreate("test_disable_objects_merge_unit").setMapping(mapping)); +// In testFieldCapsNormalNestedObjectUnaffected: +assertAcked(client().admin().indices().prepareCreate("test_normal_nested_unit").setMapping(mapping)); Suggestion importance[1-10]: 7 __ Why: This is a real bug: all three test methods create an index named "test" in a single-node test case without cleanup, which will cause the second and third tests to fail with an index-already-exists error. Using unique index names per test is the correct fix.	Medium
	Guard against incorrect parent path computation The computation name.length() - simpleName.length() - 1 assumes that simpleName is always a suffix of name. If simpleName is longer than name or is not actually a suffix (e.g., due to an unexpected mapping state), endPos could be negative or point to the wrong position, silently producing an incorrect parent path. Add a guard to verify that name actually ends with simpleName before using this arithmetic. server/src/main/java/org/opensearch/index/mapper/ParametrizedFieldMapper.java [136-139] +if (!name.endsWith(simpleName)) { + // Fallback: simpleName is not a suffix of name, use dot-based split + int endPos = name.lastIndexOf('.'); + if (endPos < 0) { + return new ContentPath(0); + } + return new ContentPath(name.substring(0, endPos)); +} int endPos = name.length() - simpleName.length() - 1; if (endPos < 0) { return new ContentPath(0); } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that the arithmetic name.length() - simpleName.length() - 1 assumes simpleName is always a suffix of name. Adding a guard with name.endsWith(simpleName) would make the code more robust against unexpected mapping states, though in practice the invariant should hold for well-formed field names.	Low
	Prevent potential infinite loop on null mapper When mapper == null, the code updates dotIndex to parentField.lastIndexOf('.') and then continues the loop. However, the outer loop likely recomputes parentField from dotIndex at the top of each iteration. If dotIndex is not updated before the continue, the loop may revisit the same parentField indefinitely. Verify that the loop uses the updated dotIndex to recompute parentField on the next iteration, or explicitly recompute parentField before the continue to avoid an infinite loop. server/src/main/java/org/opensearch/action/fieldcaps/TransportFieldCapabilitiesIndexAction.java [183-188] if (mapper == null) { // parentField is part of a literal dotted field name under a disable_objects=true parent // No ObjectMapper exists for this intermediate path so skip it and continue up the chain dotIndex = parentField.lastIndexOf('.'); + parentField = dotIndex > 0 ? parentField.substring(0, dotIndex) : null; + if (parentField == null) break; continue; } Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about potential infinite loops, but looking at the PR code, dotIndex is updated to parentField.lastIndexOf('.') before continue, which should allow the outer loop to recompute parentField correctly on the next iteration. Without seeing the full loop body, the risk is uncertain, but the suggestion's improved_code adds explicit parentField recomputation and a break guard which could be a safer approach.	Low

Suggestions up to commit 0042299

Category	Suggestion	Impact
Possible issue	Use unique index names across test methods All three test methods create an index named "test", which will cause conflicts if the tests run in the same node/cluster context (as is typical with OpenSearchSingleNodeTestCase). The second and third test will fail with an index-already-exists error. Each test should use a unique index name (e.g., "test_disable_objects", "test_disable_objects_merge", "test_normal_nested") matching the REST test names. server/src/test/java/org/opensearch/action/fieldcaps/TransportFieldCapabilitiesIndexActionTests.java [36] -assertAcked(client().admin().indices().prepareCreate("test").setMapping(mapping)); -... -assertAcked(client().admin().indices().prepareCreate("test").setMapping(mapping)); -... -assertAcked(client().admin().indices().prepareCreate("test").setMapping(mapping)); +// testFieldCapsDisableObjects +assertAcked(client().admin().indices().prepareCreate("test_disable_objects").setMapping(mapping)); +// ... use "test_disable_objects" throughout this test +// testFieldCapsDisableObjectsAfterUnrelatedDocument +assertAcked(client().admin().indices().prepareCreate("test_disable_objects_merge").setMapping(mapping)); +// ... use "test_disable_objects_merge" throughout this test + +// testFieldCapsNormalNestedObjectUnaffected +assertAcked(client().admin().indices().prepareCreate("test_normal_nested").setMapping(mapping)); +// ... use "test_normal_nested" throughout this test + Suggestion importance[1-10]: 8 __ Why: This is a valid and important bug - all three test methods use the same index name "test" in OpenSearchSingleNodeTestCase, which shares a single node. This will cause index-already-exists failures for the second and third tests, making the test suite unreliable. Using unique index names per test method is necessary for correctness.	Medium
General	Add explicit break when no parent dot found When mapper == null and parentField contains no dot, lastIndexOf('.') returns -1, which will cause the outer while (dotIndex > -1) loop to exit correctly. However, if dotIndex is already -1 before this block is reached (e.g., a top-level field with no dot), the code inside the loop would still execute and set dotIndex = -1 again, which is harmless but worth verifying. More critically, the continue statement re-evaluates the while condition with the newly assigned dotIndex, so if parentField itself has no dot, the loop exits — this is correct. The logic appears safe, but adding a guard to break out early when dotIndex == -1 after the assignment would make the intent explicit and prevent any future confusion. server/src/main/java/org/opensearch/action/fieldcaps/TransportFieldCapabilitiesIndexAction.java [183-188] if (mapper == null) { // parentField is part of a literal dotted field name under a disable_objects=true parent // No ObjectMapper exists for this intermediate path so skip it and continue up the chain dotIndex = parentField.lastIndexOf('.'); + if (dotIndex == -1) { + break; + } continue; } Suggestion importance[1-10]: 4 __ Why: The suggestion is logically sound - when dotIndex becomes -1 after lastIndexOf, the while (dotIndex > -1) condition would already cause the loop to exit on the next iteration, making the explicit break redundant but clearer. The improvement is minor since the existing code already handles this case correctly through the loop condition.	Low
	Assert expected dot separator in parent path calculation The calculation endPos = name.length() - simpleName.length() - 1 assumes that simpleName is always a suffix of name separated by a dot. If simpleName is not a suffix of name (e.g., due to an unexpected mapper state), name.substring(0, endPos) could silently return a wrong parent path instead of failing fast. Adding an assertion or a validation check that name ends with "." + simpleName would make this assumption explicit and catch bugs early. server/src/main/java/org/opensearch/index/mapper/ParametrizedFieldMapper.java [133-141] private static ContentPath parentPath(String name, String simpleName) { // Use simpleName to compute the parent path so that fields whose simpleName contains dots // (because of disable_objects) get the correct parent path int endPos = name.length() - simpleName.length() - 1; if (endPos < 0) { return new ContentPath(0); } + assert name.charAt(endPos) == '.' : "Expected '.' separator between parent path and simpleName in: " + name; return new ContentPath(name.substring(0, endPos)); } Suggestion importance[1-10]: 3 __ Why: Adding an assertion to validate that name ends with "." + simpleName is a defensive programming practice that could help catch unexpected mapper states early. However, this is a minor improvement that only aids debugging and doesn't fix any known issue.	Low

Suggestions up to commit bd5b6aa

Category	Suggestion	Impact
Possible issue	Use unique index names per test All three test methods create an index named "test". Since OpenSearchSingleNodeTestCase reuses the same node across tests in the class, if tests run in sequence without cleanup, the second and third test will fail trying to create an already-existing index. Use unique index names per test method or add a @After teardown that deletes the index. server/src/test/java/org/opensearch/action/fieldcaps/TransportFieldCapabilitiesIndexActionTests.java [36] -assertAcked(client().admin().indices().prepareCreate("test").setMapping(mapping)); -... -assertAcked(client().admin().indices().prepareCreate("test").setMapping(mapping)); -... -assertAcked(client().admin().indices().prepareCreate("test").setMapping(mapping)); +// In testFieldCapsDisableObjects: +assertAcked(client().admin().indices().prepareCreate("test_disable_objects_unit").setMapping(mapping)); +// In testFieldCapsDisableObjectsAfterUnrelatedDocument: +assertAcked(client().admin().indices().prepareCreate("test_disable_objects_merge_unit").setMapping(mapping)); +// In testFieldCapsNormalNestedObjectUnaffected: +assertAcked(client().admin().indices().prepareCreate("test_normal_nested_unit").setMapping(mapping)); Suggestion importance[1-10]: 7 __ Why: All three test methods create an index named "test", which will cause failures when tests run sequentially on the same node in OpenSearchSingleNodeTestCase. Using unique index names per test is a real correctness issue that would cause test failures.	Medium
	Guard against infinite loop on missing dot When mapper == null, dotIndex is updated to parentField.lastIndexOf('.'), but parentField is a local variable derived from the previous dotIndex. The loop then recomputes parentField using the updated dotIndex at the top of the next iteration, which is correct. However, if dotIndex becomes -1 (no more dots), the loop condition should handle termination properly. Verify that the outer loop correctly terminates when dotIndex reaches -1 after this continue, to avoid an infinite loop or incorrect behavior. server/src/main/java/org/opensearch/action/fieldcaps/TransportFieldCapabilitiesIndexAction.java [183-188] if (mapper == null) { // parentField is part of a literal dotted field name under a disable_objects=true parent // No ObjectMapper exists for this intermediate path so skip it and continue up the chain dotIndex = parentField.lastIndexOf('.'); + if (dotIndex == -1) { + break; + } continue; } Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid concern about loop termination when dotIndex becomes -1 after the continue. However, the outer loop likely already checks dotIndex > 0 or similar as its condition, so this may already be handled. Adding an explicit break when dotIndex == -1 is a reasonable defensive improvement to prevent potential infinite loops.	Low
General	Validate simpleName is suffix of name The calculation name.length() - simpleName.length() - 1 assumes that simpleName is always a suffix of name. If simpleName is not a suffix of name (e.g., due to an unexpected mapper state), endPos could point to a wrong position in the string, producing a silently incorrect parent path. Add an assertion or validation to ensure name ends with simpleName before using this arithmetic. server/src/main/java/org/opensearch/index/mapper/ParametrizedFieldMapper.java [136-139] int endPos = name.length() - simpleName.length() - 1; if (endPos < 0) { return new ContentPath(0); } +assert name.endsWith(simpleName) : "Field name [" + name + "] does not end with simpleName [" + simpleName + "]"; Suggestion importance[1-10]: 4 __ Why: The assertion adds a useful defensive check to catch unexpected mapper states where simpleName is not a suffix of name. However, this is a minor defensive improvement and the arithmetic is logically sound given the existing mapper contract.	Low

Suggestions up to commit 39e8551

Category	Suggestion	Impact
Possible issue	Use unique index names per test method All three test methods create an index named "test". Since these are JUnit tests in a single test class that may share state (depending on the test framework's lifecycle), if the index is not deleted between tests, the second and third test methods will fail because the index already exists. Each test should use a unique index name or ensure cleanup between tests. server/src/test/java/org/opensearch/action/fieldcaps/TransportFieldCapabilitiesIndexActionTests.java [36] -assertAcked(client().admin().indices().prepareCreate("test").setMapping(mapping)); -... -assertAcked(client().admin().indices().prepareCreate("test").setMapping(mapping)); -... -assertAcked(client().admin().indices().prepareCreate("test").setMapping(mapping)); +// testFieldCapsDisableObjects +assertAcked(client().admin().indices().prepareCreate("test_disable_objects_unit").setMapping(mapping)); +// testFieldCapsDisableObjectsAfterUnrelatedDocument +assertAcked(client().admin().indices().prepareCreate("test_disable_objects_merge_unit").setMapping(mapping)); +// testFieldCapsNormalNestedObjectUnaffected +assertAcked(client().admin().indices().prepareCreate("test_normal_nested_unit").setMapping(mapping)); Suggestion importance[1-10]: 7 __ Why: All three test methods use the same index name "test", which can cause test failures if the index persists between tests. Using unique index names per test method is important for test isolation and reliability in OpenSearchSingleNodeTestCase.	Medium
	Guard against infinite loop when no dot found When mapper == null, the code updates dotIndex to parentField.lastIndexOf('.') and then continues the loop. However, parentField is derived from name.substring(0, dotIndex) earlier in the loop, so dotIndex should be updated based on parentField's last dot position before continuing. If the outer loop recomputes parentField from the updated dotIndex correctly this is fine, but it's worth verifying that the loop variable dotIndex is being used consistently and that this continue doesn't skip the update of dotIndex that would normally happen at the bottom of the loop, potentially causing an infinite loop or incorrect traversal. server/src/main/java/org/opensearch/action/fieldcaps/TransportFieldCapabilitiesIndexAction.java [183-188] if (mapper == null) { // parentField is part of a literal dotted field name under a disable_objects=true parent // No ObjectMapper exists for this intermediate path so skip it and continue up the chain dotIndex = parentField.lastIndexOf('.'); + if (dotIndex == -1) { + break; + } continue; } Suggestion importance[1-10]: 6 __ Why: If parentField has no dot (i.e., lastIndexOf('.') returns -1), the loop could behave incorrectly or loop infinitely. Adding a break when dotIndex == -1 is a valid defensive guard, though the outer loop likely handles this case already via its own condition check.	Low
General	Validate simpleName is suffix of name The calculation name.length() - simpleName.length() - 1 assumes that simpleName is always a suffix of name. If simpleName is not a suffix of name (e.g., due to an unexpected state), endPos could be a positive but incorrect index, leading to a wrong parent path being returned. Adding an assertion or validation that name ends with simpleName would make this safer and easier to debug. server/src/main/java/org/opensearch/index/mapper/ParametrizedFieldMapper.java [136-139] int endPos = name.length() - simpleName.length() - 1; if (endPos < 0) { return new ContentPath(0); } +assert name.endsWith(simpleName) : "simpleName [" + simpleName + "] is not a suffix of name [" + name + "]"; Suggestion importance[1-10]: 3 __ Why: Adding an assertion that name ends with simpleName is a minor defensive improvement. The assumption is inherent to the design, and this would only help during development/debugging rather than fixing a real bug.	Low

[github-actions]

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

[gaobinlong]

The DCO check failed, please amend the commit message with your sign-off, add also add some change log for this PR. In addition, this bug fix is better to have some e2e yaml rest tests, see https://github.com/opensearch-project/OpenSearch/blob/main/TESTING.md#testing-the-rest-layer, could you add it, thanks! @newtonne

[newtonne]

Thanks @gaobinlong. Sure, I can take a look at that. Would the rest tests be in addition to the current tests?

[gaobinlong]

Thanks @gaobinlong. Sure, I can take a look at that. Would the rest tests be in addition to the current tests?

Yeah, It's better to have yaml rest tests for API changes.

[github-actions]

Persistent review updated to latest commit 39e8551

[github-actions]

❌ Gradle check result for 39e8551: 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 9dace53

[github-actions]

Persistent review updated to latest commit bd5b6aa

[github-actions]

❌ Gradle check result for bd5b6aa: 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 0042299

[github-actions]

❌ Gradle check result for 0042299: 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 b4eb2f0

[github-actions]

✅ Gradle check result for b4eb2f0: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.17%. Comparing base (9142d0e) to head (e212d0d).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #20814      +/-   ##
============================================
- Coverage     73.27%   73.17%   -0.10%     
+ Complexity    72546    72431     -115     
============================================
  Files          5819     5819              
  Lines        331357   331360       +3     
  Branches      47877    47878       +1     
============================================
- Hits         242796   242475     -321     
- Misses        69077    69362     +285     
- Partials      19484    19523      +39     

☔ View full report in Codecov by Sentry.
📢 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 22919db

[github-actions]

❌ Gradle check result for 22919db: 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 e212d0d

[github-actions]

✅ Gradle check result for e212d0d: SUCCESS