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: date/time deserialization with fractional seconds #138

Merged
merged 12 commits into from
Oct 1, 2024

Conversation

ILikeToFixThings
Copy link
Contributor

Hi,

Currently on Python 3.10 and earlier there is an exception thrown if you try to parse a JSON file with more than 6 fractional seconds (the decimals) in a JSON file. ISO8601 datetimes are allowed to have any number of fractional seconds but the regex fix applied by this library to allow Python to parse ISO8601 timestamps that is needed pre Python 3.11 due to weirdness around Pythons ISO parsing requiring either 0 or 6 fractional seconds currently only pads the fractional seconds if required and does not truncate if the timestamp is too long which was causing me issues parsing a JSON SBOM file generated by cargo-cyclonedx which includes 9 decimal places on its timestamp for some reason. This is only an issue pre Python 3.11 since in 3.11 they made datetime.fromisoformat ISO compliant.

I have added some tests which showcase the issue and applied a fix to the padding code of datetime deserialization.

tests/test_helpers.py Show resolved Hide resolved
serializable/helpers.py Show resolved Hide resolved
serializable/helpers.py Outdated Show resolved Hide resolved
tests/test_helpers.py Outdated Show resolved Hide resolved
@jkowalleck jkowalleck changed the title Add support for more than 6 fractional seconds. fix: date/time deserialization with fractional seconds Oct 1, 2024
serializable/helpers.py Outdated Show resolved Hide resolved
tests/test_helpers.py Outdated Show resolved Hide resolved
@jkowalleck
Copy link
Collaborator

tried to consolidate the python 3.11 compatibility layer via 72ce000
This seams to break something, will investigate and fix

Signed-off-by: Jan Kowalleck <jan.kowalleck@gmail.com>
@jkowalleck jkowalleck added the bug Something isn't working label Oct 1, 2024
Signed-off-by: Jan Kowalleck <jan.kowalleck@gmail.com>
@jkowalleck
Copy link
Collaborator

@ILikeToFixThings I did a consolidation of your changes. could you review?

@jkowalleck
Copy link
Collaborator

@madpah could I ask you for a review?

@ILikeToFixThings
Copy link
Contributor Author

@ILikeToFixThings I did a consolidation of your changes. could you review?

Looks good to me!

Copy link
Owner

@madpah madpah left a comment

Choose a reason for hiding this comment

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

Tests appear to cover the change and change looks sane to me.

@jkowalleck jkowalleck merged commit f4b1c27 into madpah:main Oct 1, 2024
18 checks passed
@jkowalleck
Copy link
Collaborator

@ILikeToFixThings ,

Thank you so much for bringing this issue up and providing fixes for it.

Your work resulted in the published release: https://github.com/madpah/serializable/releases/tag/v1.1.2

@madpah
Copy link
Owner

madpah commented Oct 1, 2024

And thanks too @jkowalleck ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants