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

Hash expressions #31726

Merged
merged 5 commits into from Oct 25, 2019
Merged

Hash expressions #31726

merged 5 commits into from Oct 25, 2019

Conversation

@lbartoletti
Copy link
Contributor

lbartoletti commented Sep 12, 2019

Description

This PR adds hash functions to QGIS. No new dependency is required since it uses QCryptographicHash available in QtCore.

Use case is the same as PgCrypto but in client side for all supported format.

Checklist

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include Fixes #11111 at the bottom of the commit message
  • I have read the QGIS Coding Standards and this PR complies with them
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit
  • I have evaluated whether it is appropriate for this PR to be backported, backport requests are left as label or comment
@nyalldawson

This comment has been minimized.

Copy link
Contributor

nyalldawson commented Sep 12, 2019

Nice! Every addition to the qgis expression engine extends QGIS' power exponentially...

Copy link
Contributor

nyalldawson left a comment

Just gotta fix the build ;)

@nirvn

This comment has been minimized.

Copy link
Contributor

nirvn commented Sep 13, 2019

hash('keccak_224','the string to hash')

vs.

keccak_224('the string to hash') x 15ish functions...

which one is better?

@m-kuhn

This comment has been minimized.

Copy link
Member

m-kuhn commented Sep 13, 2019

My vote goes to hash(value, algorithm='keccak_224')
most time I needed a hash, the choice about which one was not that important as long as it was a reasonable one.

@nirvn

This comment has been minimized.

Copy link
Contributor

nirvn commented Sep 13, 2019

@m-kuhn , you raise a valid point, the algorithm argument would benefit from being optional, with a default algorithm automatically set. +1 for that.

@lbartoletti

This comment has been minimized.

Copy link
Contributor Author

lbartoletti commented Sep 13, 2019

hash(value, algorithm='keccak_224')

👍

I would like to keep also separated functions since I often use md5(value) / sha256(value) etc. :)
Do you agree?

@Gustry

This comment has been minimized.

Copy link
Contributor

Gustry commented Sep 13, 2019

IMHO, if it's a single function, it may fit in the already existing group Conversions with to_hash (same pattern as existing QGIS expressions) ?

@m-kuhn

This comment has been minimized.

Copy link
Member

m-kuhn commented Sep 13, 2019

I can see the use for shorthands for sha256 and md5, but I wonder if this necessarily means that we have to provide all of them. No strong opinion.
Concerning the method naming, I'd stick to hash, hasing is a one-way operation (in contrast to the usual to_xxx cast conversions)

@NathanW2

This comment has been minimized.

Copy link
Member

NathanW2 commented Sep 14, 2019

@lbartoletti

This comment has been minimized.

Copy link
Contributor Author

lbartoletti commented Sep 18, 2019

So, only one function hash(value, string). Dedicated group or existing group (which one : Conversions? )?

Thanks

@m-kuhn

This comment has been minimized.

Copy link
Member

m-kuhn commented Sep 18, 2019

How about

  • hash(value, method)
  • md5(value)
  • sha256(value)
  • and keccak256(value)?

in Conversions?

…string) are available now
@lbartoletti

This comment has been minimized.

Copy link
Contributor Author

lbartoletti commented Sep 23, 2019

@m-kuhn done. Good for you?

@m-kuhn

This comment has been minimized.

Copy link
Member

m-kuhn commented Sep 23, 2019

Yep, pretty much, thanks 👍

@m-kuhn
m-kuhn approved these changes Sep 24, 2019
@nyalldawson nyalldawson merged commit dc95936 into qgis:master Oct 25, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lbartoletti lbartoletti deleted the lbartoletti:hash_expression branch Feb 13, 2020
lbartoletti added a commit to lbartoletti/QGIS that referenced this pull request Feb 13, 2020
I realize that I didn't change it after our discussion on the PR qgis#31726.
lbartoletti added a commit to lbartoletti/QGIS that referenced this pull request Feb 13, 2020
I realize that I didn't change it after our discussion on the PR qgis#31726.
@lbartoletti lbartoletti mentioned this pull request Feb 13, 2020
nyalldawson added a commit that referenced this pull request Feb 13, 2020
I realize that I didn't change it after our discussion on the PR #31726.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.