Skip to content

Add hits length in SearchRequestContext#21037

Open
shivamg2017 wants to merge 1 commit intoopensearch-project:mainfrom
shivamg2017:add-hits-length
Open

Add hits length in SearchRequestContext#21037
shivamg2017 wants to merge 1 commit intoopensearch-project:mainfrom
shivamg2017:add-hits-length

Conversation

@shivamg2017
Copy link
Copy Markdown

@shivamg2017 shivamg2017 commented Mar 29, 2026

Add hits length in SearchRequestContext

Relevant Query Insights PR to update SearchRequestContext : opensearch-project/query-insights#580

Signed-off-by: shivamg2017 <shivamgupta.6july@gmail.com>
@github-actions
Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Default Value

The hitsLength field is initialized to 0 by default. If setHitsLength is never called (e.g., in error paths or when no hits are returned), callers of getHitsLength() cannot distinguish between "0 hits returned" and "hits length was never set". Consider using Integer/Optional or documenting this behavior.

private int hitsLength;
Possible NPE

internalSearchResponse.hits().getHits() could potentially return null, causing a NullPointerException when calling .length. It should be verified that getHits() never returns null, or a null check should be added before accessing .length.

searchRequestContext.setHitsLength(internalSearchResponse.hits().getHits().length);

@github-actions
Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Guard against null array dereference

The getHits() method may return null in some cases, causing a NullPointerException
when accessing .length. A null check should be added before accessing the array
length.

server/src/main/java/org/opensearch/action/search/AbstractSearchAsyncAction.java [795]

-searchRequestContext.setHitsLength(internalSearchResponse.hits().getHits().length);
+searchRequestContext.setHitsLength(internalSearchResponse.hits().getHits() != null ? internalSearchResponse.hits().getHits().length : 0);
Suggestion importance[1-10]: 5

__

Why: The suggestion is valid - if getHits() can return null, accessing .length would cause a NullPointerException. However, in OpenSearch's SearchHits implementation, getHits() typically returns an empty array rather than null, making this a low-probability defensive check rather than a critical fix.

Low

@github-actions
Copy link
Copy Markdown
Contributor

✅ Gradle check result for de99cd0: SUCCESS

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 29, 2026

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.19%. Comparing base (dbe98aa) to head (de99cd0).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...opensearch/action/search/SearchRequestContext.java 66.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #21037      +/-   ##
============================================
- Coverage     73.21%   73.19%   -0.02%     
- Complexity    72620    72723     +103     
============================================
  Files          5849     5859      +10     
  Lines        332066   332469     +403     
  Branches      47951    48002      +51     
============================================
+ Hits         243109   243345     +236     
- Misses        69456    69635     +179     
+ Partials      19501    19489      -12     

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant