Skip to content

Handle missing content in incremental bulk #133685

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
Merged
merged 6 commits into from
Aug 29, 2025
Merged

Handle missing content in incremental bulk #133685

merged 6 commits into from
Aug 29, 2025

Conversation

@mhl-b
Copy link
Contributor

@mhl-b mhl-b commented Aug 27, 2025 •
edited
Loading

Incremental bulk does not check if content is missing, and can throw NPE on xContentType.

java.lang.NullPointerException: Cannot invoke "org.elasticsearch.xcontent.XContentType.xContent()" because "xContentType" is null
	at [email protected]/org.elasticsearch.action.bulk.BulkRequestParser$IncrementalParser.<init>(BulkRequestParser.java:248)
	at [email protected]/org.elasticsearch.action.bulk.BulkRequestParser.incrementalParser(BulkRequestParser.java:177)

Non-incremental path checks through ReleasableBytesReference content = request.requiredContent();.

This PR adds explicit check for content presence in the incremental processing path.

ES-12591

@mhl-b mhl-b requested review from a team and DaveCTurner August 27, 2025 20:33
@mhl-b mhl-b added >non-issue :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. Team:Distributed Indexing Meta label for Distributed Indexing team v9.2.0 labels Aug 27, 2025
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Aug 27, 2025

Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing)

@mhl-b mhl-b requested a review from JeremyDahlgren August 28, 2025 04:12
DaveCTurner
DaveCTurner reviewed Aug 28, 2025
View reviewed changes
DaveCTurner
DaveCTurner approved these changes Aug 29, 2025
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

server/src/test/java/org/elasticsearch/rest/action/document/RestBulkActionTests.java

public void testIncrementalBulkMissingContent() {
assertThrows(
ElasticsearchParseException.class,
Copy link
Contributor

@DaveCTurner DaveCTurner Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could we also assert the exception message here? We're catching the empty body, not the lack of a content-type.

Copy link
Contributor Author

@mhl-b mhl-b Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, pressed merge button too soon. Addressed in new PR #133855

@mhl-b mhl-b merged commit 36de99b into elastic:main Aug 29, 2025
33 checks passed
This was referenced Aug 29, 2025
Merged
Merged
@mhl-b
Copy link
Contributor Author

mhl-b commented Aug 29, 2025

💔 Some backports could not be created

Status Branch Result
✅ 9.1
❌ 8.19 Conflict resolution was aborted by the user

Manual backport

To create the backport manually run:

backport --pr 133685

Questions ?

Please refer to the Backport tool documentation

mhl-b added a commit to mhl-b/elasticsearch that referenced this pull request Aug 29, 2025
@mhl-b
Handle missing content in incremental bulk (elastic#133685)
4d64d10 
(cherry picked from commit 36de99b)
@mhl-b
Copy link
Contributor Author

mhl-b commented Aug 29, 2025

@DaveCTurner, I'm backporting to 9.1, should I backport to 8.19?

@DaveCTurner
Copy link
Contributor

DaveCTurner commented Aug 29, 2025

should I backport to 8.19?

Yes I think so, we want to reinstate the old behaviour in 8.19 too.

mhl-b added a commit that referenced this pull request Aug 29, 2025
@mhl-b
Handle missing content in incremental bulk (#133685) (#133856)
6e810fb 
(cherry picked from commit 36de99b)
@DaveCTurner
Copy link
Contributor

DaveCTurner commented Sep 1, 2025

Yes I think so, we want to reinstate the old behaviour in 8.19 too.

Actually I haven't checked whether this is needed in 8.19, and there were certainly some changes in this area that only landed in 8.19.

@mhl-b
Copy link
Contributor Author

mhl-b commented Sep 2, 2025

Incremental stuff is disabled by default in 8.x, but NPE can happen. The problem is missing machinery for content detection at Rest layer. This part landed together with moving aggregator into Rest layer. 8.x still aggregates by netty. I would fix it if anyone complains about it in 8.x.

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

Reviewers

@DaveCTurner DaveCTurner DaveCTurner approved these changes

@JeremyDahlgren JeremyDahlgren Awaiting requested review from JeremyDahlgren

Assignees

No one assigned

Labels

:Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. >non-issue Team:Distributed Indexing Meta label for Distributed Indexing team v9.2.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mhl-b @elasticsearchmachine @DaveCTurner