Deprecate boolean constructor of FetchSourceContext

[urmichm]

Description

Deprecate the boolean constructor of FetchSourceContext in favour of FetchSourceContext.FETCH_SOURCE and FetchSourceContext.DO_NOT_FETCH_SOURCE

Related Issues

Resolves #20743
Relates to #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 f1ea800)

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 Missing Deprecation The three-argument constructor FetchSourceContext(boolean fetchSource, String[] includes, String[] excludes) is still used in many places throughout the codebase (e.g., with new FetchSourceContext(true, includes, excludes)). Consider whether this constructor also needs a deprecation notice or if only the single-boolean constructor is being deprecated. The PR description mentions deprecating the boolean constructor, but the three-argument constructor is not deprecated, which may cause confusion. 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(); } Logic Change In randomInnerHits(), when randomInt == 0, the old code used new FetchSourceContext(true, Strings.EMPTY_ARRAY, Strings.EMPTY_ARRAY) (always fetch source with empty arrays), but the new code uses FetchSourceContext.FETCH_SOURCE. These should be equivalent, but it's worth verifying that FETCH_SOURCE has the same empty includes/excludes arrays. Additionally, when randomInt == 2, the old code used new FetchSourceContext(randomBoolean()) which could produce either fetch or no-fetch, while the new code uses randomBoolean() ? FETCH_SOURCE : DO_NOT_FETCH_SOURCE — this is equivalent and correct. randomFetchSourceContext = FetchSourceContext.FETCH_SOURCE; } else if (randomInt == 1) { randomFetchSourceContext = new FetchSourceContext( true, generateRandomStringArray(12, 16, false), generateRandomStringArray(12, 16, false) ); } else { randomFetchSourceContext = randomBoolean() ? FetchSourceContext.FETCH_SOURCE : FetchSourceContext.DO_NOT_FETCH_SOURCE; Missing Assignment In the old code, consumer.accept(new FetchSourceContext(fetchSource)) was called before the if (fetchSource == false) check. In the new code, consumer.accept(fetchSourceContext) is called correctly before the check. However, verify that the ternary assignment and consumer call are placed correctly relative to the expectedParams update — the logic appears correct but should be validated that the consumer is called in all cases where it was called before. FetchSourceContext fetchSourceContext = fetchSource ? FetchSourceContext.FETCH_SOURCE : FetchSourceContext.DO_NOT_FETCH_SOURCE; consumer.accept(fetchSourceContext); if (fetchSource == false) { expectedParams.put("_source", "false"); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to f1ea800
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Remove duplicate test case reducing coverage diversity In the else branch (randomInt == 2), using FetchSourceContext.FETCH_SOURCE duplicates the randomInt == 0 case, reducing test coverage diversity. The else branch should only use FetchSourceContext.DO_NOT_FETCH_SOURCE to ensure all three branches produce distinct contexts. server/src/test/java/org/opensearch/index/query/InnerHitBuilderTests.java [182-191] if (randomInt == 0) { randomFetchSourceContext = FetchSourceContext.FETCH_SOURCE; } else if (randomInt == 1) { randomFetchSourceContext = new FetchSourceContext( true, generateRandomStringArray(12, 16, false), generateRandomStringArray(12, 16, false) ); } else { - randomFetchSourceContext = randomBoolean() ? FetchSourceContext.FETCH_SOURCE : FetchSourceContext.DO_NOT_FETCH_SOURCE; + randomFetchSourceContext = FetchSourceContext.DO_NOT_FETCH_SOURCE; } Suggestion importance[1-10]: 5 __ Why: The else branch (randomInt == 2) randomly picks between FETCH_SOURCE and DO_NOT_FETCH_SOURCE, which can duplicate the randomInt == 0 case. Using only DO_NOT_FETCH_SOURCE in the else branch ensures each of the three branches produces a distinct, deterministic context, improving test coverage diversity.	Low
	Enforce usage of shared constants over deprecated constructor The deprecated constructor is still fully functional, which means callers can still create new FetchSourceContext instances with arbitrary boolean values. To enforce immutability of the shared FETCH_SOURCE and DO_NOT_FETCH_SOURCE constants and prevent misuse, consider making the constructor package-private or throwing an AssertionError/logging a warning when called, to guide users toward the constants. server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java [92-94] +/** + * @deprecated use {@link #FETCH_SOURCE} or {@link #DO_NOT_FETCH_SOURCE} instead + */ @Deprecated public FetchSourceContext(boolean fetchSource) { this(fetchSource, Strings.EMPTY_ARRAY, Strings.EMPTY_ARRAY); + assert false : "Use FetchSourceContext.FETCH_SOURCE or FetchSourceContext.DO_NOT_FETCH_SOURCE instead"; } Suggestion importance[1-10]: 2 __ Why: Adding an assert statement to a deprecated constructor would break existing callers that still use it (including tests in this very PR that use new FetchSourceContext(true, ...)) and would be overly aggressive. The @Deprecated annotation already signals the intent. The improved_code also adds a comment that was already present in the existing code.	Low

---

Previous suggestions

Suggestions up to commit fa77808

Category	Suggestion	Impact
General	Deprecated constructor should return shared constants The deprecated single-boolean constructor still delegates to the three-argument constructor, which means callers using new FetchSourceContext(true) or new FetchSourceContext(false) won't get the same singleton instances as FETCH_SOURCE / DO_NOT_FETCH_SOURCE. Consider having the deprecated constructor return (or delegate to) the constants, or at minimum document that equality checks against the constants may fail for instances created via this constructor. server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java [91-94] +/** + * @deprecated use {@link #FETCH_SOURCE} or {@link #DO_NOT_FETCH_SOURCE} instead + */ @Deprecated -public FetchSourceContext(boolean fetchSource) { - this(fetchSource, Strings.EMPTY_ARRAY, Strings.EMPTY_ARRAY); +public static FetchSourceContext of(boolean fetchSource) { + return fetchSource ? FETCH_SOURCE : DO_NOT_FETCH_SOURCE; } Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about identity equality vs. value equality when using the deprecated constructor vs. the constants. However, the improved_code changes the constructor to a static factory method, which is a more significant API change than what's described. The suggestion is more of a design observation than a critical bug fix, and the improved_code doesn't accurately reflect a minimal fix to the existing constructor.	Low

[urmichm]

The related Pull Requests:

• Refactor and simplify the _source parsing in FetchSourceContext #21086
• Validation of the _source object in request and deprecation of ambiguous usage  #21203

This is the final Pull Request related to this series. Only refactoring. No logic changes.

cc: @andrross

[urmichm]

The three-argument constructor FetchSourceContext(boolean fetchSource, String[] includes, String[] excludes) is still used in many places...

Yes, it would require a lot of refactoring to reduce the usage of this constructor. This is outside of the scope of this PR

In randomInnerHits, when randomInt == 0, the context is always set to FETCH_SOURCE, but when randomInt == 2 (the else branch), it randomly picks between...

True, there are a few places where the random branches have suspicious probabilities, as well as duplicated cases in the switch block. However, this is also outside of the PR's scope.

This PR focuses on refactoring and deprecation only.

[github-actions]

❌ Gradle check result for fa77808: 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 f1ea800

[github-actions]

✅ Gradle check result for f1ea800: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.29%. Comparing base (feb6941) to head (f1ea800).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21235      +/-   ##
============================================
+ Coverage     73.27%   73.29%   +0.01%     
+ Complexity    73347    73344       -3     
============================================
  Files          5910     5910              
  Lines        334421   334421              
  Branches      48207    48207              
============================================
+ Hits         245054   245103      +49     
+ Misses        69762    69699      -63     
- Partials      19605    19619      +14     

☔ 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.