Avoid losing type of UUID when serializing/deserializing #1575
Conversation
This file contains 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
Serializing UUIDs as strs and deserializing them as strs can lead to somewhat obscure bugs such as the one that led to the creation of this PR in django-cacheback codeinthehole/django-cacheback#100 which some would call unexpected behaviour. After all, an UUID is altogether a different type from strs and if bytes got their own serializer/deserializer for UUID the same logic should apply.
for more information, see https://pre-commit.ci
auvipy
requested changes
Jul 15, 2022
| @@ -29,7 +29,7 @@ class JSONEncoder(_encoder_cls): | |||
| def default(self, o, | |||
| dates=(datetime.datetime, datetime.date), | |||
| times=(datetime.time,), | |||
| textual=(decimal.Decimal, uuid.UUID, DjangoPromise), | |||
| textual=(decimal.Decimal, DjangoPromise), | |||
There was a problem hiding this comment.
Hmmmm, there's an entry for - JSON serializer now handles datetimes, Django promise, UUID and Decimal. in 4.0. It doesn't specify how it's handled but maybe (if accepted) this change could be documented as part of the next release?
There was a problem hiding this comment.
I pushed 6b89388 to keep the userguide (the only place I could find where JSON serialization is discussed) updated c:
thedrow
approved these changes
Jul 17, 2022
auvipy
approved these changes
Jul 17, 2022
sk-
added a commit
to sk-/kombu
that referenced
this pull request
Jun 15, 2023
In PR celery#1575 a new serializer for UUIDs was introduced, however it fails when deserializing UUIDs which version is not within 1 and 5. Given that there's a new standard (https://datatracker.ietf.org/doc/html/draft-peabody-dispatch-new-uuid-format-04) ganining traction that introduces UUIDs v6, v7 and v8, we change the code to allow such versions. In the test, we generate one UUID for each version using the package `uuid6` (https://github.com/oittaa/uuid6-python)
auvipy
pushed a commit
that referenced
this pull request
Jun 15, 2023
In PR #1575 a new serializer for UUIDs was introduced, however it fails when deserializing UUIDs which version is not within 1 and 5. Given that there's a new standard (https://datatracker.ietf.org/doc/html/draft-peabody-dispatch-new-uuid-format-04) ganining traction that introduces UUIDs v6, v7 and v8, we change the code to allow such versions. In the test, we generate one UUID for each version using the package `uuid6` (https://github.com/oittaa/uuid6-python)
samdoran
added a commit
to project-koku/koku
that referenced
this pull request
Jul 6, 2023
Kombu 5.3 fixed bugs that incorrectly cast UUID and datetime objects to strings. We are expected those to be strings in many places, so going to constrain this for now. celery/kombu#1575 celery/kombu#1515 https://github.com/celery/kombu/releases/tag/v5.3.0
samdoran
added a commit
to project-koku/koku
that referenced
this pull request
Jul 6, 2023
Kombu 5.3 fixed bugs that incorrectly cast UUID and datetime objects to strings. We expect those to be strings in many places. celery/kombu#1575 celery/kombu#1515 https://github.com/celery/kombu/releases/tag/v5.3.0
|
This PR is quite dangerous as if you have celery workers running 5.2.4, and you are deploying a new codebase using celery 5.3.1, the old workers cannot decode the arguments for the new tasks if there was a |
|
cc @auvipy |
|
Sorry, this is not the actual PR that introduced the change. I'll try to find the original one and post the warning there. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Serializing UUIDs as strs and deserializing them as strs can lead to
somewhat obscure bugs such as the one that led to the creation of this
PR in django-cacheback
codeinthehole/django-cacheback#100 which some
would call unexpected behaviour.
After all, an UUID is altogether a different type from strs and if
bytes got their own serializer/deserializer for UUID the same logic
should apply.
I added a case for every uuidX constructor found in CPython.
Lemme know what you think of it and if there's anything I'm misreading :D.
P.D: I understand that UUIDs are now serialized as a convenience for users
but perhaps it's a bit non-intuitive that after versions of not supporting them
now they're converted to a different type when deserializing?