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

Improve SHA256 equality checks #6

Merged
merged 3 commits into from
Jun 17, 2022

Conversation

Apelsinka223
Copy link
Contributor

@Apelsinka223 Apelsinka223 commented May 18, 2020

Hi! I suggest to add equal?/2 callback in sha_256 type, so when you do not change this field in changeset ecto would skip it in update (for now ecto update field all the time)

@jwaldrip
Copy link

@danielberkompas would love to see this merged as well.

@danielberkompas
Copy link
Owner

I added a version of this today but it didn't do any hashing. It just uses basic string comparison. Is there any documentation on what values Ecto will pass to the equal? function?

@Apelsinka223
Copy link
Contributor Author

Hi @danielberkompas !
If I understood your question correctly, Ecto passes as first argument the origin not updated version of the field value, and as second argument - the new updated version of the value. So if any of these versions of value is not hashed (basically the second - new version), this code will hash it and compare both hashed values.
I recognize hashed value with String.valid? function, which should return false. Maybe in your code, the unhashed value doesn't pass this check, and that's why it turning out to basic string comparison, because it recognized as hashed. Can you show with which values you tested this code?

@danielberkompas danielberkompas added feature New feature proposal A proposal for a change and removed proposal A proposal for a change labels Jun 17, 2022
@danielberkompas danielberkompas changed the title add equal?/2 in sha_256 Improve SHA256 equality checks Jun 17, 2022
@danielberkompas danielberkompas merged commit c2fe2fd into danielberkompas:master Jun 17, 2022
@danielberkompas
Copy link
Owner

It's late in coming, but thanks @Apelsinka223! ⭐

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants