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

Fix(#291): InstantDeserializer fails to parse negative numeric timestamp strings for pre-1970 values #362

Merged
merged 3 commits into from
Mar 5, 2025

Conversation

Strongbeard
Copy link
Contributor

@Strongbeard Strongbeard commented Mar 3, 2025

Fix for #291:

  • Updated InstantDeserializer._countPeriods(String) to check for - and + at beginning of numeric string.
  • Additionally, added check if ALLOW_LEADING_PLUS_SIGN_FOR_NUMBERS is enabled. If disabled, throws same exception as before change from _countPeriods(...).
  • Added associated tests to failing directory and then moved to deser directory.

Full Issue Title: InstantDeserializer fails to parse negative numeric
timestamp strings for pre-1970 values.

- Updated `InstantDeserializer._countPeriods(String)` to check for `-`
  and `+` at beginning of numeric string.
- Additionally, added check if `ALLOW_LEADING_PLUS_SIGN_FOR_NUMBERS` is
  enabled. If disabled, throws same exception as before change from
  `_countPeriods(...)`.
- Moved associated tests out of `failing` directory
Copy link
Member

@JooHyukKim JooHyukKim left a comment

Choose a reason for hiding this comment

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

Looks good

@Strongbeard
Copy link
Contributor Author

Would it be better to explicitly throw a JsonParseException when a + is encountered and p.isEnabled(ALLOW_LEADING_PLUS_SIGN_FOR_NUMBERS.mappedFeature()) is false? I didn't think about it when writing the PR, but a more straightforward error message might be better for users...

@cowtowncoder
Copy link
Member

Would it be better to explicitly throw a JsonParseException when a + is encountered and p.isEnabled(ALLOW_LEADING_PLUS_SIGN_FOR_NUMBERS.mappedFeature()) is false? I didn't think about it when writing the PR, but a more straightforward error message might be better for users...

I think this is ok. Parsing numbers from Strings is bit icky anyway.

Changed the function signature of `_countPeriods(JsonParser, String)` to
`_countPeriods(String, boolean)` per suggestion from user cowtowncoder
@cowtowncoder cowtowncoder added the cla-received Marker to denote that there is a CLA for pr label Mar 5, 2025
@cowtowncoder cowtowncoder merged commit adf848b into FasterXML:2.18 Mar 5, 2025
4 checks passed
@cowtowncoder cowtowncoder added this to the 2.18.4 milestone Mar 5, 2025
@Strongbeard Strongbeard deleted the fix-291 branch March 5, 2025 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-received Marker to denote that there is a CLA for pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants