Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #8340: Prevent null reference exception in SearchResponse<T> #8466

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix #8340: Prevent null reference exception in SearchResponse<T> #8466

wants to merge 1 commit into from
+3 −2

Conversation

sadeghpour
Copy link

@sadeghpour sadeghpour commented Mar 4, 2025 •
edited by flobernd
Loading

  • Added null checks for HitsMetadata in SearchResponse<T>.Documents
  • Ensured HitsMetadata and Hits are properly validated before accessing them
  • Prevented potential crashes when the response contains no hits

Closes #8340

@cla-checker-service CLA Checker Service
Copy link

cla-checker-service bot commented Mar 4, 2025 •
edited
Loading

💚 CLA has been signed

@sadeghpour
Copy link
Author

Hi team, I have signed the CLA, but the check still seems to be failing. Could you please verify? Let me know if I need to take any further action. Thanks!

@pquentin
Copy link
Member

pquentin commented Mar 4, 2025

Hello, and thank you for your contribution! I have no idea about the code itself. I can confirm that you signed the CLA check correctly, but the issue is that your commit does not use an email address. See https://github.com/elastic/elasticsearch-net/commit/4a91b26f1095bd82681c825b441960f2dba0df07.patch.

@sadeghpour
Fix elastic#8340: Prevent null reference exception in SearchResponse<T>
330d842 
- Added null checks for `HitsMetadata` in `SearchResponse<T>.Documents`
- Ensured `HitsMetadata` and `Hits` are properly validated before accessing them
- Prevented potential crashes when the response contains no hits
@flobernd
Copy link
Member

flobernd commented Mar 4, 2025

Thanks for the PR @sadeghpour !

Besides this fix, I would like to investigate the case a little bit more in depth. Would it be possible for you to send me the JSON request and response that triggers this behavior?

In our specification, both, ResponseBody<TDocument>.hits (this is HitsMetadata in the .NET client) and HitsMetadata<T>.hits are not marked as optional. I wonder if this should be changed cc @pquentin @l-trotta

@l-trotta
Copy link

l-trotta commented Mar 4, 2025

@flobernd never found any evidence that hits could be null, is there a special case where it happens?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SearchResponse.Documents throws null reference exception on successful call
4 participants
@sadeghpour @pquentin @flobernd @l-trotta