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

NOISSUE - Use pgcrypto instead uuid-ossp for UUIDs generation #1208

Merged
merged 1 commit into from
Jun 18, 2020

Conversation

sprql
Copy link
Contributor

@sprql sprql commented Jun 16, 2020

According to
https://www.postgresql.org/docs/current/uuid-ossp.html

Note
If you only need randomly-generated (version 4) UUIDs, consider using the gen_random_uuid() function from the pgcrypto module instead.

@sprql sprql requested a review from a team as a code owner June 16, 2020 18:28
@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2020

Codecov Report

Merging #1208 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1208   +/-   ##
=======================================
  Coverage   77.04%   77.04%           
=======================================
  Files         104      104           
  Lines        6934     6934           
=======================================
  Hits         5342     5342           
  Misses       1206     1206           
  Partials      386      386           
Impacted Files Coverage Δ
users/postgres/init.go 88.23% <100.00%> (ø)

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 4a9c740...ffd211d. Read the comment docs.

@drasko
Copy link
Contributor

drasko commented Jun 16, 2020

@manuio @dusanb94 @mteodor please check

@manuio manuio changed the title NOISSUE — Use pgcrypto instead uuid-ossp for UUIDs generation NOISSUE - Use pgcrypto instead uuid-ossp for UUIDs generation Jun 17, 2020
@manuio
Copy link
Contributor

manuio commented Jun 17, 2020

@sprql This looks good. I didn't understand what's the problem with uuid_generate_v4 but the doc is clear about the recommendation. Just give me some time to test before to approve.

nmarcetic
nmarcetic previously approved these changes Jun 17, 2020
ALTER TABLE IF EXISTS users ADD COLUMN IF NOT EXISTS
id UUID DEFAULT uuid_generate_v4()`,
id UUID DEFAULT gen_random_uuid()`,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before I approve, can you just set id column to NOT NULL (id UUID NOT NULL DEFAULT uuid_generate_v4()), as well? We didn't include this change to any release, so it's safe to change this migration instead of adding another one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point!

…rsion 4)

According to
https://www.postgresql.org/docs/current/uuid-ossp.html
it's recommended to use
https://www.postgresql.org/docs/current/pgcrypto.html
for randomly-generated (version 4) UUIDs

Signed-off-by: Alexander Obukhov <dev@sprql.space>
Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@drasko drasko merged commit ee5c763 into absmach:master Jun 18, 2020
manuio pushed a commit that referenced this pull request Oct 12, 2020
…rsion 4) (#1208)

According to
https://www.postgresql.org/docs/current/uuid-ossp.html
it's recommended to use
https://www.postgresql.org/docs/current/pgcrypto.html
for randomly-generated (version 4) UUIDs

Signed-off-by: Alexander Obukhov <dev@sprql.space>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants