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

Adding UUID support to hashing #1861

Merged
merged 4 commits into from
Aug 9, 2018

Conversation

szoupanos
Copy link
Contributor

Fixes issue #1771
UUID is not hashed properly under SQLA and this is fixed by this PR.

@szoupanos szoupanos added this to the v0.12.2 milestone Aug 8, 2018
@szoupanos szoupanos self-assigned this Aug 8, 2018
@szoupanos szoupanos requested a review from sphuber August 8, 2018 15:23
Copy link
Contributor

@sphuber sphuber left a 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-io
Copy link

codecov-io commented Aug 8, 2018

Codecov Report

Merging #1861 into release_v0.12.2 will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@                 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
Impacted Files Coverage Δ
aiida/common/hashing.py 84.37% <100%> (+0.5%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98602cf...cdeb613. Read the comment docs.

@szoupanos
Copy link
Contributor Author

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 believe that QB should return a unicode UUID as the Node.uuid method does.
(and the behaviour should be uniform in both backends)
But I leave this as a separate issue for @lekah

@lekah
Copy link
Contributor

lekah commented Aug 9, 2018

I'm OK with this as it is. @sphuber if you judge your requested changes enough, go ahead and merge.

@sphuber sphuber merged commit b0cf7c9 into aiidateam:release_v0.12.2 Aug 9, 2018
@szoupanos szoupanos deleted the issue_1771 branch February 9, 2019 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants