-
-
Notifications
You must be signed in to change notification settings - Fork 735
✨ Update GUID handling to use stdlib UUID.hex
instead of an int
#26
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
Conversation
Rather than integer based serialization grandfathered in from sqlalchemy, use the stdlib [`UUID.hex`](https://docs.python.org/3/library/uuid.html#uuid.UUID.hex) method. This also fixes fastapi#25
any update? |
I will just propose also adding a small test, just to catch this. I made my own small test to catch the behaviour in a fork to fix this, before finding this pr, so you can maybe work something off of this (probably want to generalize a bit - I made it really quick just to confirm this was causing the issue): def test_zero_padded_uuid():
raised = None
myid = UUID(hex='00000000000000000000000000000001')
stored_value = GUID().process_bind_param(myid, SQLiteDialect)
try:
back_to_uuid = GUID().process_result_value(stored_value, SQLiteDialect)
except ValueError as ex:
raised = ex
assert raised is None
assert myid == back_to_uuid |
@tiangolo - Do you have any other maintainers that can help get small PRs like this merged? I'm also up against this issue, and the PR was opened 2 months ago. |
Any news on this? |
I am disappointed that this simple fix was not implemented in 0.0.5, as this is causes a lot of headaches when using UUID primary keys. |
We can hope for 0.0.7 to add this @tiangolo? |
I was just about to create an issue about it and implement the exactly same solution. Looks like SQLModel got inspired by the same source of this gist: https://gist.github.com/gmolveau/7caeeefe637679005a7bb9ae1b5e421e IMHO I think SQLAlquemy-Utils uses a better approach when it provides additional native dialects besides postgresql, like msql and cockroachdb, and also uses the native UUID |
Fork 3 (Sourcery refactored)
Codecov Report
@@ Coverage Diff @@
## main #26 +/- ##
=======================================
Coverage 97.59% 97.59%
=======================================
Files 182 182
Lines 6060 6060
=======================================
Hits 5914 5914
Misses 146 146
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
UUID.hex
instead of an int
📝 Docs preview for commit 9bb4715 at: https://630a99822de66e5008e74d00--sqlmodel.netlify.app |
Great! This makes sense, thank you @andrewbolster! 🍰 And thanks everyone for the discussion! ☕ This will be available in the next version, released in the next hours, SQLModel |
Rather than integer based serialization grandfathered in from sqlalchemy, use the stdlib
UUID.hex
method.This also fixes #25