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

create_join_table should include indexes and foreign key contraints #22960

Closed
AlexVPopov opened this Issue Jan 7, 2016 · 3 comments

Comments

Projects
None yet
5 participants
@AlexVPopov
Contributor

AlexVPopov commented Jan 7, 2016

Currently, if one generates a join table like this:

rails g migration create_join_table_showroom_user showroom user

the following migration will be created:

class CreateJoinTableShowroomUser < ActiveRecord::Migration
  def change
    create_join_table :showrooms, :users do |t|
      # t.index [:showroom_id, :user_id]
      # t.index [:user_id, :showroom_id]
    end
  end
end

Running this migration directly will create a very simple join table:

create_table "showrooms_users", id: false, force: :cascade do |t|
  t.integer "showroom_id", null: false
  t.integer "user_id",     null: false
end

So no indexes, no foreign keys etc. If one uncomments the two lines in the migration, now one gets:

create_table "showrooms_users", id: false, force: :cascade do |t|
  t.integer "showroom_id", null: false
  t.integer "user_id",     null: false
end

add_index "showrooms_users", ["showroom_id", "user_id"], name: "index_showrooms_users_on_showroom_id_and_user_id", using: :btree
add_index "showrooms_users", ["user_id", "showroom_id"], name: "index_showrooms_users_on_user_id_and_showroom_id", using: :btree

I would argue, that the generated migration should look like this:

class CreateJoinTableShowroomUser < ActiveRecord::Migration
  def change
    create_join_table :showrooms, :users do |t|
      # t.references :showroom, index: true, null: false, foreign_key: true
      # t.references :user, index: true, null: false, foreign_key: true
      # t.index [:showroom_id, :user_id]
      # t.index [:user_id, :showroom_id]
    end
  end
end

If one uncomments the two references lines, the produced table will look like this:

create_table "showrooms_users", id: false, force: :cascade do |t|
  t.integer "showroom_id", null: false
  t.integer "user_id",     null: false
end

add_index "showrooms_users", ["showroom_id"], name: "index_showrooms_users_on_showroom_id", using: :btree
add_index "showrooms_users", ["user_id"], name: "index_showrooms_users_on_user_id", using: :btree

add_foreign_key "showrooms_users", "showrooms"
add_foreign_key "showrooms_users", "users"

Why wouldn't somebody want foreign keys and indexes on them in a join table? @rafaelfranca

@heptaman

This comment has been minimized.

Show comment
Hide comment
@heptaman

heptaman Jan 21, 2016

I agree! I have to do this all the time. Maybe we could add options to the generator, like:
rails g migration ... --fk --idx
or include them by default, and have options to disable them, like:
rails g migration ... --no_fk --no_idx

heptaman commented Jan 21, 2016

I agree! I have to do this all the time. Maybe we could add options to the generator, like:
rails g migration ... --fk --idx
or include them by default, and have options to disable them, like:
rails g migration ... --no_fk --no_idx

@erullmann

This comment has been minimized.

Show comment
Hide comment
@erullmann

erullmann Feb 1, 2016

Contributor

I don't know, the migrations mostly don't use options like that. You can already enable the index by using :index. So running

rails g migration create_join_table_showroom_user showroom:index user:index

Produces

class CreateJoinTableShowroomUser < ActiveRecord::Migration
  def change
    create_join_table :showrooms, :users do |t|
      t.index [:showroom_id, :user_id]
      t.index [:user_id, :showroom_id]
    end
  end
end

And you can use references to create an index as well when creating models. Like so:

rails g migration CreatePost title:string user:references:index

Which produces:

class CreatePost < ActiveRecord::Migration
  def change
    create_table :posts do |t|
      t.string :title
      t.references :user, index: true, foreign_key: true
    end
  end
end

So maybe it would be best to allow something like:

rails g migration create_join_table_showroom_user showroom:references:index user:references:index

Creating this:

class CreateJoinTableShowroomUser < ActiveRecord::Migration
  def change
    create_join_table :showrooms, :users do |t|
      t.references :showroom, index: true, foreign_key: true
      t.references :user, index: true, foreign_key: true
      # t.index [:showroom_id, :user_id]
      # t.index [:user_id, :showroom_id]
    end
  end
end

And then the default rails g migration create_join_table_showroom_user showroom user would produce:

class CreateJoinTableShowroomUser < ActiveRecord::Migration
  def change
    create_join_table :showrooms, :users do |t|
      # t.references :showroom, index: true, foreign_key: true
      # t.references :user, index: true, foreign_key: true
      # t.index [:showroom_id, :user_id]
      # t.index [:user_id, :showroom_id]
    end
  end
end
Contributor

erullmann commented Feb 1, 2016

I don't know, the migrations mostly don't use options like that. You can already enable the index by using :index. So running

rails g migration create_join_table_showroom_user showroom:index user:index

Produces

class CreateJoinTableShowroomUser < ActiveRecord::Migration
  def change
    create_join_table :showrooms, :users do |t|
      t.index [:showroom_id, :user_id]
      t.index [:user_id, :showroom_id]
    end
  end
end

And you can use references to create an index as well when creating models. Like so:

rails g migration CreatePost title:string user:references:index

Which produces:

class CreatePost < ActiveRecord::Migration
  def change
    create_table :posts do |t|
      t.string :title
      t.references :user, index: true, foreign_key: true
    end
  end
end

So maybe it would be best to allow something like:

rails g migration create_join_table_showroom_user showroom:references:index user:references:index

Creating this:

class CreateJoinTableShowroomUser < ActiveRecord::Migration
  def change
    create_join_table :showrooms, :users do |t|
      t.references :showroom, index: true, foreign_key: true
      t.references :user, index: true, foreign_key: true
      # t.index [:showroom_id, :user_id]
      # t.index [:user_id, :showroom_id]
    end
  end
end

And then the default rails g migration create_join_table_showroom_user showroom user would produce:

class CreateJoinTableShowroomUser < ActiveRecord::Migration
  def change
    create_join_table :showrooms, :users do |t|
      # t.references :showroom, index: true, foreign_key: true
      # t.references :user, index: true, foreign_key: true
      # t.index [:showroom_id, :user_id]
      # t.index [:user_id, :showroom_id]
    end
  end
end

erullmann added a commit to erullmann/rails that referenced this issue Feb 1, 2016

Added references option to join tables
Fixes issue #22960
When creating join tables with the command

    rails g migration CreateJoinTableShowroomUser showroom:references user:references

The migration will use references to create the joins and output:

     class CreateJoinTableShowroomUser < ActiveRecord::Migration
       def change
         create_join_table :showrooms, :users do |t|
           t.references :showroom, index: true, foreign_key: true
           t.references :user, index: true, foreign_key: true
         end
       end
     end

This allows for proper refrences with indexes and foreign keys to be easily used when
adding join tables. Without `:refrences` the normal output is generated:

    class CreateJoinTableShowroomUser < ActiveRecord::Migration[5.0]
      def change
        create_join_table :showrooms, :users do |t|
          # t.index [:showroom_id, :user_id]
          # t.index [:user_id, :showroom_id]
        end
      end
    end
@AlexVPopov

This comment has been minimized.

Show comment
Hide comment
@AlexVPopov

AlexVPopov Feb 1, 2016

Contributor

+1 @erullmann This is a very nice solution. 😃

Contributor

AlexVPopov commented Feb 1, 2016

+1 @erullmann This is a very nice solution. 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment