-
Notifications
You must be signed in to change notification settings - Fork 192
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
Adding UUID support to hashing #1861
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add a test? If this was broken but was not caught by the current tests
Codecov Report
@@ Coverage Diff @@
## release_v0.12.2 #1861 +/- ##
===================================================
+ Coverage 54.48% 54.48% +<.01%
===================================================
Files 246 246
Lines 32438 32441 +3
===================================================
+ Hits 17674 17677 +3
Misses 14764 14764
Continue to review full report at Codecov.
|
Tests were added. The problem was a corner case: When the UUID is asked as a result from the QueryBuilder, the UUID is returned as a uuid.UUID type in SQLA. When this results is passed in another QB query as a filtering argument, the query fails because hashing fails (it doesn't support UUID hashing). I fixed hashing to work also in the case that a uuid.UUID argument is passed. |
I'm OK with this as it is. @sphuber if you judge your requested changes enough, go ahead and merge. |
Fixes issue #1771
UUID is not hashed properly under SQLA and this is fixed by this PR.