Fix SnapshotStats toXContent()/fromXContent() round trip serialization #128928
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
toXContent() has a check to omit the "processed" sub-object if the processedFileCount equals the incrementalFileCount. In the scenario where they are equal and greater than zero, if you were to read the JSON back in with fromXContent(), the SnapshotStats object would not be the same as the original since processedFileCount is not parsed and set. This change adds a fix and a unit test for this scenario.
JeremyDahlgren added >bug :Distributed Coordination/Snapshot/Restore 
| Hi @JeremyDahlgren, I've created a changelog YAML for you. |
JeremyDahlgren added >test | Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
DaveCTurner reviewed Jun 5, 2025
test/framework/src/main/java/org/elasticsearch/test/AbstractXContentTestCase.java Outdated Show resolved Hide resolved
mridula-s109 pushed a commit to mridula-s109/elasticsearch that referenced this pull request Jun 9, 2025
elastic#128928) toXContent() has a check to omit the "processed" sub-object if the processedFileCount equals the incrementalFileCount. In the scenario where they are equal and greater than zero, if you were to read the JSON back in with fromXContent(), the SnapshotStats object would not be the same as the original since processedFileCount is not parsed and set. This change adds a fix and adjusts the unit test to increase the probability of creating this scenario.
valeriy42 pushed a commit to valeriy42/elasticsearch that referenced this pull request Jun 12, 2025
elastic#128928) toXContent() has a check to omit the "processed" sub-object if the processedFileCount equals the incrementalFileCount. In the scenario where they are equal and greater than zero, if you were to read the JSON back in with fromXContent(), the SnapshotStats object would not be the same as the original since processedFileCount is not parsed and set. This change adds a fix and adjusts the unit test to increase the probability of creating this scenario.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 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.
toXContent()has a check to omit the "processed" sub-object if theprocessedFileCountequals theincrementalFileCount. In the scenario where they are equal and greater than zero, if you were to read the JSON back in withfromXContent(), theSnapshotStatsobject would not be the same as the original sinceprocessedFileCountis not parsed and set. You can breakSnapshotStatsTestsby forcingcreateTestInstance()to pass theincrementalFileCountvalue forprocessedFileCountwhen constructing the instance. This PR adds a fix and increases the probability of creating this scenario increateTestInstance().Since
fromXContent()is no longer called from production code, this PR also movesSnapshotStats.fromXContent()intoSnapshotStatsTests.For context, #29602 is the PR for reviewing when
getProcessedFileCount() != getIncrementalFileCount()was added inSnapshotStats.toXContent(), and #31515 is the PR for reviewing whenSnapshotStats.fromXContent()was added.This issue was found while working on tests for #128399 and noticing
XContentround trip serialization was not working as expected.