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.

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

try to fix issue where _numberString is used despite having old value #1389

Closed
wants to merge 6 commits into from

Conversation

pjfanning
Copy link
Member

@cowtowncoder
Copy link
Member

Sounds reasonable. But we really need a test or two here, to reproduce the problem and guard against regression. Test in jackson-databind not ideal, but hopefully can be translated to sequence of nextToken()s and number access calls.

@pjfanning
Copy link
Member Author

  • the test that I added seems valid enough but actually works without the code fix
  • I will continue seeing if I can produce a jackson-core that fails without the change
  • this change does seem to work in that it fixes the test in test for issue #4917 jackson-databind#4918

@cowtowncoder
Copy link
Member

@pjfanning I think the difference is that thanks to @JsonCreator, number value will be buffered in TokenBuffer and is probably accessed with JsonParser.getNumberValueDeferred() instead of getDecimalValue().

@pjfanning
Copy link
Member Author

@cowtowncoder the test now reproduces the issue - I put a summary in FasterXML/jackson-databind#4918 (comment)

@pjfanning
Copy link
Member Author

@cowtowncoder While this change fixes the immediate issue with clearing the _numberString value in the affected code path, I'm wondering if it might be better to do this at a different point. Some method like public JsonToken nextToken() so that when we advance past a number that we clear some of the number state in the parser instance. Can you think of a good stage to do this clearing of number state?

@cowtowncoder
Copy link
Member

@cowtowncoder While this change fixes the immediate issue with clearing the _numberString value in the affected code path, I'm wondering if it might be better to do this at a different point.
Some method like public JsonToken nextToken() so that when we advance past a number that we clear some of the number state in the parser instance. Can you think of a good stage to do this clearing of number state?

Places that clear _numTypesValid are the ones to look for -- I think this means methods resetInt, resetFloat and resetAsNaN? That might be enough.
nextToken() and others probably need not be checked since _numberString is only considered for cases where _currToken is one of numbers (int or float).

@pjfanning
Copy link
Member Author

I've created #1391 and targeted 2.17 branch. I've kept the changes as is because I think they are less risky for a patch release in this form. What I can do after these changes are merged is that I can do a larger change in the 2.19 branch that more explicitly manages the parser state so that it is less likely that old state can be read back in at the wrong time.

@pjfanning pjfanning closed this Jan 25, 2025
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.

2 participants