Permalink
Browse files

Set active_record config for always creating uuids in generators

  • Loading branch information...
jmccartie committed Sep 25, 2015
1 parent 7b92798 commit fb42c492a7eff58f45867ea50440d938648cdb48
@@ -1,3 +1,10 @@
* Add ability to default to `uuid` as primary key when generating database migrations
Set `Rails.application.config.active_record.primary_key = :uuid`
or `config.active_record.primary_key = :uuid` in config/application.rb
*Jon McCartie*
* Qualify column name inserted by `group` in calculation
Giving `group` an unqualified column name now works, even if the relation
@@ -1,6 +1,6 @@
class <%= migration_class_name %> < ActiveRecord::Migration
def change
create_table :<%= table_name %> do |t|
create_table :<%= table_name %><%= id_kind %> do |t|
<% attributes.each do |attribute| -%>
<% if attribute.password_digest? -%>
t.string :password_digest<%= attribute.inject_options %>
@@ -30,6 +30,11 @@ def next_migration_number(dirname)
end
end
def id_kind
kind = Rails.application.config.active_record.primary_key rescue nil

This comment has been minimized.

Show comment
Hide comment
@samnang

samnang Oct 22, 2015

@jmccartie is there any reason why we need rescue nil here? or I am missing something here? Because what I thought, Rails.application.config.active_record.primary_key either return assigned value or nil. rescue nil will catch everything even NoMethodError or NameError which usually happen because of typo.

@samnang

samnang Oct 22, 2015

@jmccartie is there any reason why we need rescue nil here? or I am missing something here? Because what I thought, Rails.application.config.active_record.primary_key either return assigned value or nil. rescue nil will catch everything even NoMethodError or NameError which usually happen because of typo.

This comment has been minimized.

Show comment
Hide comment
@jmccartie

jmccartie Oct 22, 2015

Contributor

@samnang Trying to remember now ... I think there was an edge case where Rails.application.config.active_record was not set (maybe in engines? or when AR is not included?), so I was avoiding chaining try's together. I suppose it could specifically catch an undefined method error in a future iteration.

@jmccartie

jmccartie Oct 22, 2015

Contributor

@samnang Trying to remember now ... I think there was an edge case where Rails.application.config.active_record was not set (maybe in engines? or when AR is not included?), so I was avoiding chaining try's together. I suppose it could specifically catch an undefined method error in a future iteration.

This comment has been minimized.

Show comment
Hide comment
@samnang

samnang Oct 22, 2015

@jmccartie Oh, I see. In that case, we probably need to put a guard clause there. 👍

@samnang

samnang Oct 22, 2015

@jmccartie Oh, I see. In that case, we probably need to put a guard clause there. 👍

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Oct 22, 2015

Member

👍 for removing rescue nil - we've been bitten by that too many times before.

@pixeltrix

pixeltrix Oct 22, 2015

Member

👍 for removing rescue nil - we've been bitten by that too many times before.

This comment has been minimized.

Show comment
Hide comment
@jmccartie

jmccartie Oct 22, 2015

Contributor

@pixeltrix @samnang Thanks for your feedback -- I'll make that change real quick

@jmccartie

jmccartie Oct 22, 2015

Contributor

@pixeltrix @samnang Thanks for your feedback -- I'll make that change real quick

This comment has been minimized.

Show comment
Hide comment
@jmccartie

jmccartie Oct 22, 2015

Contributor

@pixeltrix @samnang Found it. One of the tests (test/generators/plugin_generator_test.rb) fails because Rails.application is not set. Thoughts on this?

def id_kind
  return unless Rails.application.try(:config)
  kind = Rails.application.config.active_record.primary_key
  ", id: :#{kind}" if kind
end
@jmccartie

jmccartie Oct 22, 2015

Contributor

@pixeltrix @samnang Found it. One of the tests (test/generators/plugin_generator_test.rb) fails because Rails.application is not set. Thoughts on this?

def id_kind
  return unless Rails.application.try(:config)
  kind = Rails.application.config.active_record.primary_key
  ", id: :#{kind}" if kind
end

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Oct 22, 2015

Member

That test that should be fixed.

@rafaelfranca

rafaelfranca Oct 22, 2015

Member

That test that should be fixed.

This comment has been minimized.

Show comment
Hide comment
@jmccartie

jmccartie Oct 22, 2015

Contributor

Opened #22033

@jmccartie

jmccartie Oct 22, 2015

Contributor

Opened #22033

", id: :#{kind}" if kind
end
def create_migration(destination, data, config = {}, &block)
action Rails::Generators::Actions::CreateMigration.new(self, destination, block || data.to_s, config)
end
@@ -221,6 +221,19 @@ def test_create_table_migration
end
end
def test_add_uuid_to_create_table_migration
previous_value = Rails.application.config.active_record.primary_key
Rails.application.config.active_record.primary_key = :uuid
run_generator ["create_books"]
assert_migration "db/migrate/create_books.rb" do |content|
assert_method :change, content do |change|
assert_match(/create_table :books, id: :uuid/, change)
end
end
Rails.application.config.active_record.primary_key = previous_value
end
def test_should_create_empty_migrations_if_name_not_start_with_add_or_remove_or_create
migration = "delete_books"
run_generator [migration, "title:string", "content:text"]

11 comments on commit fb42c49

@vincentopensourcetaiwan

This comment has been minimized.

Show comment
Hide comment
@jmccartie

This comment has been minimized.

Show comment
Hide comment
@jmccartie

jmccartie Oct 21, 2015

Contributor

Thanks @vincentopensourcetaiwan! I hope it helps you!

Contributor

jmccartie replied Oct 21, 2015

Thanks @vincentopensourcetaiwan! I hope it helps you!

@samnang

This comment has been minimized.

Show comment
Hide comment

Nice job! @jmccartie

@rizidoro

This comment has been minimized.

Show comment
Hide comment

👏

@emaiax

This comment has been minimized.

Show comment
Hide comment
@emaiax

emaiax Oct 23, 2015

incredible! 👏

incredible! 👏

@corck

This comment has been minimized.

Show comment
Hide comment
@corck

corck Oct 23, 2015

love it!

love it!

@jpwynn

This comment has been minimized.

Show comment
Hide comment
@jpwynn

jpwynn Jun 10, 2016

Rail 5 only, is that correct?

Rail 5 only, is that correct?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
Member

rafaelfranca replied Jun 11, 2016

Yes

@helperhaps

This comment has been minimized.

Show comment
Hide comment
@helperhaps

helperhaps Sep 27, 2016

@jmccartie Couse the default generate method of uuid is uuid_generate_v4 (ref:#20518) ,when use postgresql's pgcrypto extension, the migration will failed with function uuid_generate_v4() does not exist obviously. To fix that need to edit the edit phase by hands (add " default: 'gen_random_uuid()' by hands")

create_table :tables, id: :uuid, default: 'gen_random_uuid()' do |t|

So if use pgcrypto to generate uuid,

config.generators do |g|
    g.orm :active_record, primary_key_type: :uuid
end

this is not so good, why not make it like

config.generators do |g|
    g.orm :active_record, primary_key_type: :uuid, generate_method: 'gen_random_uuid()'
end

Or it can handle that but i did't find?

helperhaps replied Sep 27, 2016

@jmccartie Couse the default generate method of uuid is uuid_generate_v4 (ref:#20518) ,when use postgresql's pgcrypto extension, the migration will failed with function uuid_generate_v4() does not exist obviously. To fix that need to edit the edit phase by hands (add " default: 'gen_random_uuid()' by hands")

create_table :tables, id: :uuid, default: 'gen_random_uuid()' do |t|

So if use pgcrypto to generate uuid,

config.generators do |g|
    g.orm :active_record, primary_key_type: :uuid
end

this is not so good, why not make it like

config.generators do |g|
    g.orm :active_record, primary_key_type: :uuid, generate_method: 'gen_random_uuid()'
end

Or it can handle that but i did't find?

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Sep 27, 2016

Member

@helperhaps you'd be better off opening a ticket than commenting on an 12 month old commit. We should probably handle that in the PostgreSQL adapter rather than adding another config.

Member

pixeltrix replied Sep 27, 2016

@helperhaps you'd be better off opening a ticket than commenting on an 12 month old commit. We should probably handle that in the PostgreSQL adapter rather than adding another config.

@helperhaps

This comment has been minimized.

Show comment
Hide comment
@helperhaps

helperhaps Sep 27, 2016

@pixeltrix Thanks for your comment

@pixeltrix Thanks for your comment

Please sign in to comment.