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

For PostgreSQL >= 9.4 use `gen_random_uuid()` #25395

Merged
merged 1 commit into from Nov 23, 2016

Conversation

Projects
None yet
@yawboakye
Contributor

yawboakye commented Jun 13, 2016

Since version 9.4, the PostgreSQL community recommends moving away from
uuid-ossp's UUID generation functions and using pcrypto's
gen_random_uuid() instead. It generates UUID version 4 by default,
which is what everyone tends to use by the way.

These changes use the appropriate UUID function depending on the
underlying PostgreSQL server's version, while maintaining
uuid_generate_v4() in older migrations.

@rails-bot

This comment has been minimized.

rails-bot commented Jun 13, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rafaelfranca

View changes

activerecord/lib/active_record/connection_adapters/postgresql/schema_definitions.rb Outdated
#
# Note that setting the UUID primary key default value to +nil+ will
# require you to assure that you always provide a UUID value before saving
# a record (as primary keys cannot be +nil+). This might be done via the
# +SecureRandom.uuid+ method and a +before_save+ callback, for instance.
def primary_key(name, type = :primary_key, **options)
options[:default] = options.fetch(:default, 'uuid_generate_v4()') if type == :uuid
if type == :uuid
uuid_function = ActiveRecord::Base.connection.supports_pgcrypto_uuid? ? 'gen_random_uuid()' : 'uuid_generate_v4()'

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 13, 2016

Member

This should be using the connection that it already hold instead of ActiveRecord::Base. Could you change it?

This comment has been minimized.

@yawboakye

yawboakye Jun 13, 2016

Contributor

@rafaelfranca Help me out. I can't find which connection to use :(

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 13, 2016

Member

Seems TableDefinition doesn't have access to a connection. So we should remove the connection check here and pick gen_random_uuid as default. Using ActiveRecord::Base.connection will make this code not support multi connections.

This comment has been minimized.

@yawboakye

yawboakye Jun 13, 2016

Contributor

How will that work for users on PostgreSQL 9.3 or lower since gen_random_uuid() is not available to them? Will 9.4 be the minimum required version?

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 13, 2016

Member

They will have to explicitly tell that they want to use uuid_generate_v4(). I think it is a fair trade off and since it is documented in the changelog users will know it.

@rafaelfranca

View changes

activerecord/test/cases/adapters/postgresql/uuid_test.rb Outdated
else
def test_schema_dumper_for_uuid_primary_key
schema = dump_table_schema "pg_uuids"
assert_match(/\bcreate_table "pg_uuids", id: :uuid, default: -> { "uuid_generate_v4\(\)" }/, schema)

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 13, 2016

Member

why is this uuid_generate_v4 not uuid_generate_v1?

This comment has been minimized.

@yawboakye

yawboakye Jun 13, 2016

Contributor

Two different columns. The id column, also the primary key, uses the uuid_generate_v1() function while other_uuid uses uuid_generate_v4(). Both are functions in the uuid-ossp extension.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 13, 2016

Member

Yeah, but in this test if is using uuid_generate_v4 while in the test above (that should be the same test of this) is using uuid_generate_v1

This comment has been minimized.

@yawboakye

yawboakye Jun 13, 2016

Contributor

Thanks for pointing it out. Turns out that test isn't necessary. The test for using gen_random_uuid() when no default is set is done using the pg_uuids_3 table instead.

@kamipo

This comment has been minimized.

Member

kamipo commented Jun 13, 2016

Duplicate to #21327.

@yawboakye

This comment has been minimized.

Contributor

yawboakye commented Jun 14, 2016

@kamipo Yes. This PR started life as corrections to @y-yagi's original.

@ianks

This comment has been minimized.

Contributor

ianks commented Jun 30, 2016

@yawboakye Would it be possible to include some information about migrating from uuid-ossp to gen_random_uuid()?

@yawboakye yawboakye force-pushed the yawboakye:use_gen_random_uuid_from_pgcrypto_extension branch 2 times, most recently Jul 18, 2016

@senny

This comment has been minimized.

Member

senny commented Jul 19, 2016

@ianks I don't think we need to cover migration issues. This is a guide about Active Record not the PostgreSQL database. We should just make sure that we encourage best practices in our examples.

@yawboakye

This comment has been minimized.

Contributor

yawboakye commented Jul 19, 2016

@senny As this will be my first contribution to Rails, I want to see it through. So please provide feedback and comments to help me improve it. Thanks cc/ @rafaelfranca @maclover7

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jul 20, 2016

r? @senny

@rails-bot rails-bot assigned senny and unassigned schneems Jul 20, 2016

@senny

View changes

activerecord/test/cases/adapters/postgresql/uuid_test.rb Outdated
@@ -9,6 +9,10 @@ def connection
def drop_table(name)
connection.drop_table name, if_exists: true
end
def uuid_function
connection.supports_pgcrypto_uuid? ? 'gen_random_uuid()' : 'uuid_generate_v4()'

This comment has been minimized.

@senny

senny Jul 20, 2016

Member

Wouldn't this mean that upgrading the PG database would case migrations to generate a different result? This might be unexpected at times.

Using a configuration option that's set in an initializer could do the trick. Quite a bit of work for a simple change like this though.

@matthewd any thoughts how we could make this less intrusive?

This comment has been minimized.

@matthewd

matthewd Aug 22, 2016

Member

This is just the test, so it seems fine

@yawboakye yawboakye force-pushed the yawboakye:use_gen_random_uuid_from_pgcrypto_extension branch Aug 6, 2016

@yawboakye yawboakye force-pushed the yawboakye:use_gen_random_uuid_from_pgcrypto_extension branch 2 times, most recently Aug 22, 2016

if options[:id] == :uuid && !options[:default]
options[:default] = "uuid_generate_v4()"
end
end

This comment has been minimized.

@matthewd

matthewd Aug 22, 2016

Member

This needs to be in V5_0

@kamipo

View changes

activerecord/CHANGELOG.md Outdated
* For PostgreSQL >= 9.4 use `pgcrypto`'s `gen_random_uuid()` instead of
`uuid-ossp`'s UUID generation function.
*Yaw Boakye*

This comment has been minimized.

@kamipo

kamipo Aug 23, 2016

Member

The original of this PR is #21327. I think that it should be credited the original author.

    *Yuji Yaginuma*, *Yaw Boakye*

@yawboakye yawboakye force-pushed the yawboakye:use_gen_random_uuid_from_pgcrypto_extension branch Aug 25, 2016

@yawboakye yawboakye force-pushed the yawboakye:use_gen_random_uuid_from_pgcrypto_extension branch Sep 11, 2016

@yawboakye yawboakye force-pushed the yawboakye:use_gen_random_uuid_from_pgcrypto_extension branch 2 times, most recently Nov 22, 2016

For `PostgreSQL >= 9.4` use `gen_random_uuid()`
Since 9.4, PostgreSQL recommends using `pgcrypto`'s `gen_random_uuid()`
to generate version 4 UUIDs instead of the functions in the `uuid-ossp`
extension.

These changes uses the appropriate UUID function depending on the
underlying PostgreSQL server's version, while maintaining
`uuid_generate_v4()` in older migrations.

@yawboakye yawboakye force-pushed the yawboakye:use_gen_random_uuid_from_pgcrypto_extension branch to b915b11 Nov 22, 2016

@yawboakye yawboakye closed this Nov 23, 2016

@yawboakye yawboakye reopened this Nov 23, 2016

@yawboakye

This comment has been minimized.

Contributor

yawboakye commented Nov 23, 2016

@matthewd Hi Matt, looks like we're good to go! 😄

@rafaelfranca rafaelfranca merged commit 8556ab5 into rails:master Nov 23, 2016

2 checks passed

codeclimate Code Climate didn't find any new or fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matthewd

This comment has been minimized.

Member

matthewd commented Nov 24, 2016

🎉

❤️❤️

#
# Note that setting the UUID primary key default value to +nil+ will
# require you to assure that you always provide a UUID value before saving
# a record (as primary keys cannot be +nil+). This might be done via the
# +SecureRandom.uuid+ method and a +before_save+ callback, for instance.
def primary_key(name, type = :primary_key, **options)
options[:default] = options.fetch(:default, "uuid_generate_v4()") if type == :uuid
options[:default] = options.fetch(:default, "gen_random_uuid()") if type == :uuid

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Nov 24, 2016

Member

Shouldn't this check supports_pgcrypto_uuid? and then decide whether to use gen_random_uuid or uuid_generate_v4?

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Nov 24, 2016

Member

nevermind, I got confused by PR description but checked comment by Rafael.

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Nov 24, 2016

Followup of UUID default extension in the docs [ci skip]
- Mentioned clearly that for PostgreSQL < 9.4, you need to pass the
  default option with "uuid_generate_v4()"
- Also updated PostgreSQL Active Record guide with this change.
- rails#25395 (comment)

@yawboakye yawboakye deleted the yawboakye:use_gen_random_uuid_from_pgcrypto_extension branch Nov 24, 2016

kamipo added a commit to kamipo/rails that referenced this pull request Nov 24, 2016

ensure
drop_table "pg_uuids_4"
end
else

This comment has been minimized.

@jcraigk

jcraigk Dec 7, 2016

Unnecessary else?

This comment has been minimized.

@kamipo

kamipo Dec 7, 2016

Member

Removed at #27170.

kamipo added a commit to kamipo/rails that referenced this pull request Jan 9, 2017

Fix UUID primary key with default nil in legacy migration
UUID primary key with no default value feature (rails#10404, rails#18206) was lost
in legacy migration caused by rails#25395 got merged. Restore the feature
again in legacy migration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment