Validation of the _source to reject contradicting and ambiguous requests

[urmichm]

Description

Validation of the _source object added to avoid confusion and reject contradicting requests:

• "_source": { "includes": "text", "excludes": ["title", "text"] } The text field is defined in both includes and excludes. Contradiction. 🚫

Deprecation logs added for ambiguous requests:

• "_source": {} At lease one of includes or excludes shall be defined.
• "_source": [] Explicitly defined empty array of includes is ambiguous.
• "_source": { "includes": [], "excludes": ["title"] } or _source: { "includes": ["title"], "excludes": [] } Explicitly defined empty array of excludes or includes is ambiguous.

To include the whole _source object or to exclude it completely, the existing boolean logic is encouraged.
"_source": true and "_source": false

Unit tests and yml tests added to test behaviour of parsing implementation.
Existing tests have been extended.

Follow-up to Pull Request #21086

Related Issues

Resolves #20612

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 884af1c)

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: Core _source validation logic and unit tests Relevant files: server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java server/src/test/java/org/opensearch/search/fetch/subphase/FetchSourceContextTests.java Sub-PR theme: Integration and gRPC proto tests for _source validation Relevant files: rest-api-spec/src/main/resources/rest-api-spec/test/search/10_source_filtering.yml modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/common/FetchSourceContextProtoUtilsTests.java

⚡ Recommended focus areas for review Breaking Change The validateAmbiguousFields() method is called in the constructor FetchSourceContext(boolean fetchSource, String[] includes, String[] excludes). This is a public constructor, so any existing code that passes overlapping includes/excludes will now throw an OpenSearchException at construction time. This could be a breaking change for existing callers who previously relied on this behavior (even if it was logically incorrect). The deserialization path via StreamInput does NOT call validateAmbiguousFields(), creating an inconsistency where deserialized objects can bypass validation. public FetchSourceContext(boolean fetchSource, String[] includes, String[] excludes) { this.fetchSource = fetchSource; this.includes = includes == null ? Strings.EMPTY_ARRAY : includes; this.excludes = excludes == null ? Strings.EMPTY_ARRAY : excludes; validateAmbiguousFields(); } public FetchSourceContext(boolean fetchSource) { this(fetchSource, Strings.EMPTY_ARRAY, Strings.EMPTY_ARRAY); } public FetchSourceContext(StreamInput in) throws IOException { fetchSource = in.readBoolean(); includes = in.readStringArray(); excludes = in.readStringArray(); } /** * The same entry cannot be both included and excluded in _source. * Since the constructors are public, this validation is required to be called in the constructor. * */ private void validateAmbiguousFields() { Set<String> includeSet = new HashSet<>(Arrays.asList(this.includes)); for (String exclude : this.excludes) { if (includeSet.contains(exclude)) { throw new OpenSearchException(AMBIGUOUS_FIELD_MESSAGE, exclude); } } } Deprecation vs Error Empty arrays and empty objects emit deprecation warnings rather than errors, while conflicting includes/excludes throw exceptions. The PR description says empty arrays/objects are "not allowed" (🚫), but the implementation only warns. This inconsistency between the stated intent and the actual behavior may cause confusion. Consider whether these cases should be errors or warnings, and ensure the PR description matches the implementation. if (includes.isEmpty()) { deprecationLogger.deprecate( "empty_source_array", "An empty array was provided as [_source]. Provide at least one field pattern or use `_source: true` to fetch the entire source." ); } return new FetchSourceContext(true, includes.toArray(new String[0]), null); } static FetchSourceContext parseSourceObject(XContentParser parser) throws IOException { XContentParser.Token token = parser.currentToken(); Set<String> includes = Collections.emptySet(); Set<String> excludes = Collections.emptySet(); String currentFieldName = null; if (token != XContentParser.Token.START_OBJECT) { throw new ParsingException( parser.getTokenLocation(), "Expected a " + XContentParser.Token.START_OBJECT + " but got a " + token + " in [" + parser.currentName() + "]." ); } while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = parser.currentName(); continue; // only field name is required in this iteration } if (currentFieldName == null) { throw new ParsingException( parser.getTokenLocation(), "Expected a field name but got a " + token + " in [" + parser.currentName() + "]." ); } // process field value switch (token) { case XContentParser.Token.START_ARRAY -> { if (INCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { includes = parseSourceFieldArray(parser, INCLUDES_FIELD, excludes); } else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { excludes = parseSourceFieldArray(parser, EXCLUDES_FIELD, includes); } else { throw new ParsingException( parser.getTokenLocation(), "Unknown key for a " + token + " in [" + currentFieldName + "]." ); } } case XContentParser.Token.VALUE_STRING -> { if (INCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { String includeEntry = parser.text(); if (excludes.contains(includeEntry)) { throw new ParsingException(parser.getTokenLocation(), AMBIGUOUS_FIELD_MESSAGE, includeEntry); } includes = Collections.singleton(includeEntry); } else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { String excludeEntry = parser.text(); if (includes.contains(excludeEntry)) { throw new ParsingException(parser.getTokenLocation(), AMBIGUOUS_FIELD_MESSAGE, excludeEntry); } excludes = Collections.singleton(excludeEntry); } else { throw new ParsingException( parser.getTokenLocation(), "Unknown key for a " + token + " in [" + currentFieldName + "]." ); } } default -> { throw new ParsingException(parser.getTokenLocation(), "Unknown key for a " + token + " in [" + currentFieldName + "]."); } } } if (includes.isEmpty() && excludes.isEmpty()) { // no valid field names -> empty or unrecognized fields; deprecated deprecationLogger.deprecate( "empty_source_object", "An empty object was provided as [_source]. Provide at least one of [" + INCLUDES_FIELD.getPreferredName() + "] or [" + EXCLUDES_FIELD.getPreferredName() + "] or use `_source: true` to fetch the entire source." ); } Immutable Set Issue In parseSourceObject, includes and excludes are initialized to Collections.emptySet() (immutable). When a VALUE_STRING token is encountered, they are replaced with Collections.singleton(...) (also immutable). If the same field appears twice (e.g., two includes string values), the second assignment would silently overwrite the first. Additionally, Collections.singleton cannot be used as a mutable accumulator. The logic should use a mutable set or handle multiple values for the same field properly. Set<String> includes = Collections.emptySet(); Set<String> excludes = Collections.emptySet(); String currentFieldName = null; if (token != XContentParser.Token.START_OBJECT) { throw new ParsingException( parser.getTokenLocation(), "Expected a " + XContentParser.Token.START_OBJECT + " but got a " + token + " in [" + parser.currentName() + "]." ); } while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = parser.currentName(); continue; // only field name is required in this iteration } if (currentFieldName == null) { throw new ParsingException( parser.getTokenLocation(), "Expected a field name but got a " + token + " in [" + parser.currentName() + "]." ); } // process field value switch (token) { case XContentParser.Token.START_ARRAY -> { if (INCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { includes = parseSourceFieldArray(parser, INCLUDES_FIELD, excludes); } else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { excludes = parseSourceFieldArray(parser, EXCLUDES_FIELD, includes); } else { throw new ParsingException( parser.getTokenLocation(), "Unknown key for a " + token + " in [" + currentFieldName + "]." ); } } case XContentParser.Token.VALUE_STRING -> { if (INCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { String includeEntry = parser.text(); if (excludes.contains(includeEntry)) { throw new ParsingException(parser.getTokenLocation(), AMBIGUOUS_FIELD_MESSAGE, includeEntry); } includes = Collections.singleton(includeEntry); } else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { String excludeEntry = parser.text(); if (includes.contains(excludeEntry)) { throw new ParsingException(parser.getTokenLocation(), AMBIGUOUS_FIELD_MESSAGE, excludeEntry); } excludes = Collections.singleton(excludeEntry); Deprecation Key Collision The deprecation key "empty_source_" + parseField.getPreferredName() dynamically generates keys like "empty_source_includes" and "empty_source_excludes". If deprecation keys are expected to be stable/registered identifiers, dynamic generation could cause issues. Also, the deprecation message for empty field arrays does not end with a period, inconsistent with other messages. if (sourceArr.isEmpty()) { deprecationLogger.deprecate( "empty_source_" + parseField.getPreferredName(), "Expected at least one value for an array of [" + parseField.getPreferredName() + "]" ); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 884af1c

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Enforce validation in deserialization constructor The StreamInput constructor does not call validateAmbiguousFields(), meaning deserialized instances can bypass the ambiguity check. Since the validation is intended to be enforced in all constructors, it should be called here as well. server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java [93-97] public FetchSourceContext(StreamInput in) throws IOException { fetchSource = in.readBoolean(); includes = in.readStringArray(); excludes = in.readStringArray(); + validateAmbiguousFields(); } Suggestion importance[1-10]: 6 __ Why: The StreamInput constructor skips validateAmbiguousFields(), allowing deserialized instances to bypass the ambiguity check. However, data read from a stream was presumably already validated when written, so this is a lower-risk gap, but still worth fixing for consistency.	Low
	Prevent silent data loss for multiple string values Using Collections.emptySet() for includes and excludes means they are immutable. When a VALUE_STRING token is encountered, the code replaces them with Collections.singleton(...), but if a second string value is provided for the same field, the previous singleton set is silently discarded. This could cause data loss. Use a LinkedHashSet instead to allow accumulation of multiple string values. server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java [233-234] -Set<String> includes = Collections.emptySet(); -Set<String> excludes = Collections.emptySet(); +Set<String> includes = new LinkedHashSet<>(); +Set<String> excludes = new LinkedHashSet<>(); Suggestion importance[1-10]: 5 __ Why: Using Collections.emptySet() and then replacing with Collections.singleton() means a second VALUE_STRING for the same field would silently overwrite the first. Using LinkedHashSet would be more robust, though in practice JSON objects rarely have duplicate field names.	Low
	Fix message placeholder format in exception The AMBIGUOUS_FIELD_MESSAGE uses {} as a placeholder, which is the SLF4J/log4j style. OpenSearchException formats messages using String.format style (%s), so the placeholder {} will not be substituted and the exception message will literally contain {} instead of the field name. Either use %s as the placeholder or manually substitute the value before passing to the exception. server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java [75] -private static final String AMBIGUOUS_FIELD_MESSAGE = "The same entry [{}] cannot be both included and excluded in _source."; +private static final String AMBIGUOUS_FIELD_MESSAGE = "The same entry [%s] cannot be both included and excluded in _source."; ... private void validateAmbiguousFields() { Set<String> includeSet = new HashSet<>(Arrays.asList(this.includes)); for (String exclude : this.excludes) { if (includeSet.contains(exclude)) { - throw new OpenSearchException(AMBIGUOUS_FIELD_MESSAGE, exclude); + throw new OpenSearchException(String.format(AMBIGUOUS_FIELD_MESSAGE, exclude)); } } } Suggestion importance[1-10]: 3 __ Why: The test at line 373 in the test file asserts "The same entry [theSameEntry] cannot be both included and excluded in _source.", which suggests OpenSearchException does substitute {} placeholders correctly (likely via its own formatting). The suggestion may be incorrect about OpenSearchException's formatting behavior, and the existing tests appear to pass with {}.	Low

---

Previous suggestions

Suggestions up to commit 19b9aa7

Category	Suggestion	Impact
Possible issue	Fix placeholder format for exception message The AMBIGUOUS_FIELD_MESSAGE uses {} as a placeholder, which is the SLF4J/log4j style. However, OpenSearchException (used in validateAmbiguousFields) formats messages differently than ParsingException. Verify that OpenSearchException correctly substitutes {} placeholders; if not, the error message will contain a literal {} instead of the field name. server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java [75] -private static final String AMBIGUOUS_FIELD_MESSAGE = "The same entry [{}] cannot be both included and excluded in _source."; +private static final String AMBIGUOUS_FIELD_MESSAGE = "The same entry [%s] cannot be both included and excluded in _source."; Suggestion importance[1-10]: 7 __ Why: The AMBIGUOUS_FIELD_MESSAGE uses {} placeholders, but OpenSearchException may not support this format, potentially resulting in a literal {} in the error message. The test in FetchSourceContextProtoUtilsTests checks for the formatted message "The same entry [theSameEntry] cannot be both included and excluded in _source.", so if OpenSearchException doesn't substitute {}, the test would fail.	Medium
	Validate ambiguous fields after deserialization The StreamInput constructor does not call validateAmbiguousFields(), meaning deserialized instances bypass the ambiguity check. This could allow invalid states to be reconstructed from serialized data. Add the validation call here as well. server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java [93-97] public FetchSourceContext(StreamInput in) throws IOException { fetchSource = in.readBoolean(); includes = in.readStringArray(); excludes = in.readStringArray(); + validateAmbiguousFields(); } Suggestion importance[1-10]: 6 __ Why: The StreamInput constructor bypasses validateAmbiguousFields(), allowing deserialized instances to skip the ambiguity check. While this may be acceptable for trusted serialized data, adding validation ensures consistency across all construction paths.	Low
General	Use mutable sets to avoid immutability issues Using Collections.emptySet() for includes and excludes means that when a VALUE_STRING token is encountered, the field is replaced with Collections.singleton(...), which is immutable and cannot be updated if a subsequent array is also parsed for the same field. While the current logic may not hit this case in practice, using mutable sets from the start is safer and more consistent with the array-parsing path. server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java [233-234] -Set<String> includes = Collections.emptySet(); -Set<String> excludes = Collections.emptySet(); +Set<String> includes = new LinkedHashSet<>(); +Set<String> excludes = new LinkedHashSet<>(); Suggestion importance[1-10]: 5 __ Why: Using Collections.emptySet() for includes and excludes could cause issues if the same field is parsed multiple times or if the logic evolves. Using LinkedHashSet from the start is more consistent with the array-parsing path and avoids potential UnsupportedOperationException.	Low

Suggestions up to commit 2a92628

Category	Suggestion	Impact
Possible issue	Fix message placeholder format for exception types The AMBIGUOUS_FIELD_MESSAGE uses {} as a placeholder, which is the SLF4J/log4j style. However, OpenSearchException (used in validateAmbiguousFields) formats messages differently than ParsingException. Verify that OpenSearchException correctly substitutes {} placeholders; if not, the error message will contain a literal {} instead of the field name. server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java [75] -private static final String AMBIGUOUS_FIELD_MESSAGE = "The same entry [{}] cannot be both included and excluded in _source."; +private static final String AMBIGUOUS_FIELD_MESSAGE = "The same entry [%s] cannot be both included and excluded in _source."; Suggestion importance[1-10]: 7 __ Why: The test in FetchSourceContextProtoUtilsTests asserts "The same entry [theSameEntry] cannot be both included and excluded in _source." which implies OpenSearchException does substitute {} placeholders. However, OpenSearchException uses its own formatting mechanism that may not handle {} the same way as logging frameworks, making this a valid concern worth verifying.	Medium
	Validate ambiguous fields after deserialization The StreamInput constructor does not call validateAmbiguousFields(), meaning deserialized instances bypass the ambiguity check. This could allow invalid states to be reconstructed from serialized data. Add the validation call here as well. server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java [93-97] public FetchSourceContext(StreamInput in) throws IOException { fetchSource = in.readBoolean(); includes = in.readStringArray(); excludes = in.readStringArray(); + validateAmbiguousFields(); } Suggestion importance[1-10]: 6 __ Why: The StreamInput constructor bypasses validateAmbiguousFields(), allowing invalid states to be reconstructed from serialized data. This is a valid concern, though in practice serialized data should already be valid if it was validated on creation.	Low
General	Use mutable sets to avoid potential runtime errors Using Collections.emptySet() for includes and excludes means that when only one of them is set via a VALUE_STRING token (using Collections.singleton()), the other remains an immutable empty set. If later code tries to add to these sets (e.g., if parsing order changes), it will throw an UnsupportedOperationException. Consider using mutable sets initialized as empty LinkedHashSet instances for consistency with the array parsing path. server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java [233-234] -Set<String> includes = Collections.emptySet(); -Set<String> excludes = Collections.emptySet(); +Set<String> includes = new LinkedHashSet<>(); +Set<String> excludes = new LinkedHashSet<>(); Suggestion importance[1-10]: 5 __ Why: Using Collections.emptySet() for includes and excludes could cause UnsupportedOperationException if code tries to mutate them. However, looking at the current code, includes and excludes are reassigned (not mutated) via Collections.singleton() or parseSourceFieldArray(), so the risk is lower but still present if the code evolves.	Low

Suggestions up to commit ea4f7b1

Category	Suggestion	Impact
General	Use mutable sets for consistent field parsing Using Collections.emptySet() for includes and excludes means that when the first field is parsed as a VALUE_STRING, it is replaced with Collections.singleton(...), which is also immutable. If a second string value is encountered for the same field, calling .contains() works but the set cannot be updated. More critically, if the order of fields in the JSON is excludes first then includes, the cross-check against the opposite set works correctly, but if includes is parsed first as a string and then excludes as an array containing the same entry, the includes set is a singleton and the check in parseSourceFieldArray correctly reads it. However, using mutable sets from the start would be safer and more consistent with the array-parsing path that uses LinkedHashSet. server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java [233-234] -Set<String> includes = Collections.emptySet(); -Set<String> excludes = Collections.emptySet(); +Set<String> includes = new LinkedHashSet<>(); +Set<String> excludes = new LinkedHashSet<>(); Suggestion importance[1-10]: 7 __ Why: Using Collections.emptySet() and then replacing with Collections.singleton() for VALUE_STRING cases creates immutable sets that could cause issues if a field appears multiple times or in different orders. Using LinkedHashSet from the start would be safer and more consistent with the array-parsing path.	Medium
Possible issue	Enforce validation in deserialization constructor The StreamInput constructor does not call validateAmbiguousFields(), meaning deserialized instances can bypass the ambiguity check. Since the validation is intended to be enforced in all constructors, it should be called here as well. server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java [93-97] public FetchSourceContext(StreamInput in) throws IOException { fetchSource = in.readBoolean(); includes = in.readStringArray(); excludes = in.readStringArray(); + validateAmbiguousFields(); } Suggestion importance[1-10]: 6 __ Why: The StreamInput constructor skips validateAmbiguousFields(), allowing deserialized instances to bypass the ambiguity check. While this is a valid concern, data read from a stream was presumably already validated when written, making this a lower-priority defensive measure.	Low
	Fix message placeholder inconsistency across exception types The AMBIGUOUS_FIELD_MESSAGE uses {} as a placeholder, which is the SLF4J/log4j style. However, OpenSearchException (used in validateAmbiguousFields) formats messages differently than ParsingException. OpenSearchException uses {} via its own formatting, but the resulting getMessage() may include the literal {} instead of the substituted value if the formatting is not applied. Verify that OpenSearchException supports {} placeholder substitution, or use String.format / concatenation to avoid inconsistent error messages. server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java [75] +// Use a format compatible with both OpenSearchException and ParsingException +private static final String AMBIGUOUS_FIELD_MESSAGE_TEMPLATE = "The same entry [%s] cannot be both included and excluded in _source."; + +// In validateAmbiguousFields: +throw new OpenSearchException(String.format(java.util.Locale.ROOT, AMBIGUOUS_FIELD_MESSAGE_TEMPLATE, exclude)); + +// In parseSourceObject/parseSourceFieldArray (ParsingException uses {} style): private static final String AMBIGUOUS_FIELD_MESSAGE = "The same entry [{}] cannot be both included and excluded in _source."; Suggestion importance[1-10]: 5 __ Why: The test in FetchSourceContextProtoUtilsTests asserts e.getMessage() equals the formatted string (without {}), suggesting OpenSearchException does support {} substitution. However, the concern about potential inconsistency is valid and worth verifying, though the improved_code introduces two separate constants which adds complexity.	Low

Suggestions up to commit 959c5ef

Category	Suggestion	Impact
Possible issue	Fix placeholder format for exception message The AMBIGUOUS_FIELD_MESSAGE uses {} as a placeholder, which is the SLF4J/log4j style. However, OpenSearchException (thrown in validateAmbiguousFields) uses Java's String.format-style %s placeholders, while ParsingException uses its own positional {} style. Using {} with OpenSearchException will result in a message like "The same entry [{}] cannot be both included and excluded in _source." instead of substituting the actual field name. server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java [75] -private static final String AMBIGUOUS_FIELD_MESSAGE = "The same entry [{}] cannot be both included and excluded in _source."; +private static final String AMBIGUOUS_FIELD_MESSAGE = "The same entry [%s] cannot be both included and excluded in _source."; Suggestion importance[1-10]: 8 __ Why: The AMBIGUOUS_FIELD_MESSAGE uses {} placeholder style, but OpenSearchException uses %s-style formatting. This means when validateAmbiguousFields() throws an OpenSearchException, the field name won't be substituted, producing a broken error message. The test in the PR verifies the exact message text, so this would cause test failures.	Medium
	Validate ambiguous fields after deserialization The StreamInput constructor does not call validateAmbiguousFields(), meaning deserialized instances can bypass the ambiguity check. Since the other constructors all call validateAmbiguousFields(), this constructor should too for consistency and correctness. server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java [93-97] public FetchSourceContext(StreamInput in) throws IOException { fetchSource = in.readBoolean(); includes = in.readStringArray(); excludes = in.readStringArray(); + validateAmbiguousFields(); } Suggestion importance[1-10]: 7 __ Why: The StreamInput constructor bypasses validateAmbiguousFields(), allowing deserialized instances to have conflicting includes/excludes. Adding the validation call ensures consistency with other constructors and prevents invalid states from being introduced via deserialization.	Medium
	Use mutable sets to avoid silent overwrites Using Collections.emptySet() for includes and excludes means that when a VALUE_STRING token is encountered, the field is replaced with a Collections.singleton(...), which is immutable and cannot be updated if a second value is later encountered. If the parser encounters a second string value for the same field, it would silently overwrite the previous singleton. Consider using LinkedHashSet from the start to allow accumulation of values. server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java [234-235] -Set<String> includes = Collections.emptySet(); -Set<String> excludes = Collections.emptySet(); +Set<String> includes = new LinkedHashSet<>(); +Set<String> excludes = new LinkedHashSet<>(); Suggestion importance[1-10]: 6 __ Why: Using Collections.emptySet() and then replacing with Collections.singleton(...) for VALUE_STRING tokens means a second string value for the same field would silently overwrite the previous one. Using LinkedHashSet from the start would allow proper accumulation, though in practice JSON objects rarely have duplicate field names.	Low

Suggestions up to commit 58a7b18

Category	Suggestion	Impact
Possible issue	Fix placeholder substitution in exception message The AMBIGUOUS_FIELD_MESSAGE uses {} as a placeholder (SLF4J/log-style), but OpenSearchException uses %s-style or direct string formatting. This means the exception message will literally contain {} instead of the field name. Use string concatenation or String.format to produce the correct message. server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java [103-110] private void validateAmbiguousFields() { Set<String> includeSet = new HashSet<>(Arrays.asList(this.includes)); for (String exclude : this.excludes) { if (includeSet.contains(exclude)) { - throw new OpenSearchException(AMBIGUOUS_FIELD_MESSAGE, exclude); + throw new OpenSearchException( + "The same entry [" + exclude + "] cannot be both included and excluded in _source." + ); } } } Suggestion importance[1-10]: 8 __ Why: The AMBIGUOUS_FIELD_MESSAGE uses {} placeholder style, but OpenSearchException does not perform SLF4J-style substitution, so the exception message would literally contain {} instead of the field name. The test in FetchSourceContextProtoUtilsTests asserts the message contains the actual field name (e.g., [theSameEntry]), confirming this is a real bug.	Medium
	Validate ambiguous fields after deserialization The StreamInput constructor does not call validateAmbiguousFields(), meaning deserialized instances bypass the ambiguity check. If a serialized payload somehow contains conflicting includes/excludes, it would be accepted silently. Add the validation call here as well to ensure consistency. server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java [93-97] public FetchSourceContext(StreamInput in) throws IOException { fetchSource = in.readBoolean(); includes = in.readStringArray(); excludes = in.readStringArray(); + validateAmbiguousFields(); } Suggestion importance[1-10]: 6 __ Why: The StreamInput constructor bypasses validateAmbiguousFields(), which could allow deserialized instances with conflicting includes/excludes to exist silently. Adding validation here ensures consistency, though in practice such data would only come from trusted internal serialization.	Low
General	Use empty set instead of null for clarity When parsing a top-level START_ARRAY (i.e., _source: ["field1", "field2"]), opposite is passed as null to parseSourceFieldArray, so no cross-check is performed. This is fine since there are no excludes in this case, but the resulting FetchSourceContext constructor will still call validateAmbiguousFields() with empty excludes, which is harmless. However, the null passed as opposite is a special-cased path — consider passing Collections.emptySet() for clarity and consistency. server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java [174-175] -includes = parseSourceFieldArray(parser, INCLUDES_FIELD, null).toArray(new String[0]); +includes = parseSourceFieldArray(parser, INCLUDES_FIELD, Collections.emptySet()).toArray(new String[0]); return new FetchSourceContext(true, includes, null); Suggestion importance[1-10]: 3 __ Why: Passing Collections.emptySet() instead of null is a minor style/clarity improvement. The parseSourceFieldArray method already handles null with a null check, so this is purely cosmetic and has no functional impact.	Low

[github-actions]

❌ Gradle check result for c2718e8: 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 418c9b7

[github-actions]

Persistent review updated to latest commit 8f222c4

[github-actions]

Persistent review updated to latest commit 49d359c

[github-actions]

Persistent review updated to latest commit ea4f7b1

[github-actions]

❌ Gradle check result for ea4f7b1: 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 2a92628

[github-actions]

❌ Gradle check result for 2a92628: 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 19b9aa7

[github-actions]

Persistent review updated to latest commit 884af1c

[urmichm]

closed in favour of #21203

cc: @bowenlan-amzn

[github-actions]

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