Fix SnapshotInfo.fromXContentInternal not Fully Consuming Parser #73268
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The parsing here was causing trouble with the new streaming deserialization because it did not fully consume the parser so if the internal buffer of the parser was just enough to finish reading the `"snapshot"` field but missed the closing bracket, then the stream behind the parser would not have been consumed fully. Also it was strangely lenient and would just read a broken in-progress `SnapshotInfo` if it ran into SMILE that contained any object field under any key that isn't "snapshot". I made it a little stricter now to enforce that we have a "snapshot" field and not just an object field by any name.
original-brownbear
added
>bug
:Distributed Coordination/Snapshot/Restore
elasticmachine
added
the
Team:Distributed (Obsolete)
Collaborator
|
Pinging @elastic/es-distributed (Team:Distributed) |
| parser.nextToken(); | ||
| } | ||
| XContentParser.Token token; | ||
| XContentParserUtils.ensureFieldName(parser, parser.currentToken(), SNAPSHOT); |
Contributor
Author
There was a problem hiding this comment.
kind of non-nonsensical to wrap in a "snapshot" field here to begin with I guess but that's a topic for another day I guess
Contributor
Author
DaveCTurner
reviewed
May 20, 2021
Contributor
DaveCTurner
left a comment
DaveCTurner
left a comment
There was a problem hiding this comment.
LGTM, taking it on trust that we never deliberately put things other than a snapshot top-level field in these blobs.
DaveCTurner
approved these changes
May 20, 2021
Contributor
Author
|
Thanks David!
I don't know of version of ES that would have done that (that would have broken the snapshot estimator for sure and we have tests literally taking snapshots from every ES version supported on Cloud for that thing). |
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
May 20, 2021
Similar to elastic#73268 we should be stricter here, especially when we are super-strict about additional fields anyway. Also, use our parser exception utils to get better exceptions if parsing fails.
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
May 20, 2021
…stic#73268) The parsing here was causing trouble with the new streaming deserialization because it did not fully consume the parser so if the internal buffer of the parser was just enough to finish reading the `"snapshot"` field but missed the closing bracket, then the stream behind the parser would not have been consumed fully. Also it was strangely lenient and would just read a broken in-progress `SnapshotInfo` if it ran into SMILE that contained any object field under any key that isn't "snapshot". I made it a little stricter now to enforce that we have a "snapshot" field and not just an object field by any name.
original-brownbear
added a commit
that referenced
this pull request
May 20, 2021
) (#73274) The parsing here was causing trouble with the new streaming deserialization because it did not fully consume the parser so if the internal buffer of the parser was just enough to finish reading the `"snapshot"` field but missed the closing bracket, then the stream behind the parser would not have been consumed fully. Also it was strangely lenient and would just read a broken in-progress `SnapshotInfo` if it ran into SMILE that contained any object field under any key that isn't "snapshot". I made it a little stricter now to enforce that we have a "snapshot" field and not just an object field by any name.
original-brownbear
added a commit
that referenced
this pull request
May 20, 2021
Similar to #73268 we should be stricter here, especially when we are super-strict about additional fields anyway. Also, use our parser exception utils to get better exceptions if parsing fails.
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
May 20, 2021
Similar to elastic#73268 we should be stricter here, especially when we are super-strict about additional fields anyway. Also, use our parser exception utils to get better exceptions if parsing fails.
original-brownbear
added a commit
that referenced
this pull request
May 21, 2021
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Jul 1, 2021
Exactly the same as elastic#73268 but for metadata and index metadata which can run into the same bug.
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Jul 13, 2021
…lastic#74844) Exactly the same as elastic#73268 but for metadata and index metadata which can run into the same bug.
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Jul 13, 2021
…lastic#74844) Exactly the same as elastic#73268 but for metadata and index metadata which can run into the same bug.
original-brownbear
added a commit
that referenced
this pull request
Jul 13, 2021
original-brownbear
added a commit
that referenced
this pull request
Jul 13, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The parsing here was causing trouble with the new streaming deserialization because it
did not fully consume the parser so if the internal buffer of the parser was just enough
to finish reading the
"snapshot"field but missed the closing bracket, then the stream behindthe parser would not have been consumed fully. Also it was strangely lenient and would just read
a broken in-progress
SnapshotInfoif it ran into SMILE that contained any object fieldunder any key that isn't "snapshot". I made it a little stricter now to enforce that
we have a "snapshot" field and not just an object field by any name.