has_one (unique index) vs has_many (index)

[p64]

Sorry to bother. I realize this all works the way it is. Great GEM! Thank you all!

However, in the purest sense shouldn't the index created on a foreign key (child_id) be unique if has_one and not unique if has_many?

I looked for quite a while for a way to do it. I didn't find a way other than going fully manual on the index. Did I miss something?

something like:

t.references :parent, :is_has_one

or maybe

t.references :parent, :unique

I tried

t.references :parent, index: { unique: true }

but I ended up with two indexes.

[ronen]

you're last one is right,

 t.references :parent, index: :unique

is the way to do a has_one. (that's even described in the README for https://github.com/lomba/schema_associations)

not sure why you ended up with two indexes, though -- is it possible you had an index already defined by a previous migration?

[p64]

I'm starting fresh so nothing previously existed.

I gotta think I'm doing something wrong. But here's what I tried:

made a new rails project
made two migrations (parent,child)

couldn't get away with:

        t.references :parent, index: :unique, null: false, on_delete: :cascade

as the unique wasn't there at all

had to use:

      t.references :parent, index: { unique: true }, null: false, on_delete: :cascade

which is weird

cat db/migrate/*
class Parent < ActiveRecord::Migration
  def change
    create_table :parent do |t|
      t.string :fullname, null: false
      t.integer :lock_version, null: false, default: 0
      t.timestamps
    end
  end
end
class Child < ActiveRecord::Migration
  def change
    create_table :child do |t|
      t.references :parent, index: { unique: true }, null: false, on_delete: :cascade
      t.string :fullname, null: false
      t.integer :lock_version, null: false, default: 0
      t.timestamps
    end
  end
end

and the result:

ActiveRecord::Schema.define(version: 20140622002345) do

  # These are extensions that must be enabled in order to support this database
  enable_extension "plpgsql"

  create_table "child", force: true do |t|
    t.integer  "parent_id",                null: false
    t.string   "fullname",                 null: false
    t.integer  "lock_version", default: 0, null: false
    t.datetime "created_at"
    t.datetime "updated_at"
    t.index ["parent_id"], :name => "fk__child_parent_id"
    t.index ["parent_id"], :name => "index_child_on_parent_id", :unique => true
    t.foreign_key ["parent_id"], "child", ["id"], :on_update => :no_action, :on_delete => :cascade, :name => "fk_child_parent_id"
  end

  create_table "parent", force: true do |t|
    t.string   "fullname",                 null: false
    t.integer  "lock_version", default: 0, null: false
    t.datetime "created_at"
    t.datetime "updated_at"
  end

end

and

pg_dump -s goo  | egrep INDEX\|CONSTRAINT
-- Name: child_pkey; Type: CONSTRAINT; Schema: public; Owner: sm; Tablespace: 
    ADD CONSTRAINT child_pkey PRIMARY KEY (id);
-- Name: parent_pkey; Type: CONSTRAINT; Schema: public; Owner: sm; Tablespace: 
    ADD CONSTRAINT parent_pkey PRIMARY KEY (id);
-- Name: fk__child_parent_id; Type: INDEX; Schema: public; Owner: sm; Tablespace: 
CREATE INDEX fk__child_parent_id ON child USING btree (parent_id);
-- Name: index_child_on_parent_id; Type: INDEX; Schema: public; Owner: sm; Tablespace: 
CREATE UNIQUE INDEX index_child_on_parent_id ON child USING btree (parent_id);
-- Name: unique_schema_migrations; Type: INDEX; Schema: public; Owner: sm; Tablespace: 
CREATE UNIQUE INDEX unique_schema_migrations ON schema_migrations USING btree (version);
-- Name: fk_child_parent_id; Type: FK CONSTRAINT; Schema: public; Owner: sm
    ADD CONSTRAINT fk_child_parent_id FOREIGN KEY (parent_id) REFERENCES child(id) ON DELETE CASCADE;

Thanks so much for looking at this.

[ronen]

hm, maybe you have discovered a bug. what versions of rails, schema_plus, etc. are you using?

[p64]

bundle list 
Gems included by the bundle:
  * actionmailer (4.1.1)
  * actionpack (4.1.1)
  * actionview (4.1.1)
  * activemodel (4.1.1)
  * activerecord (4.1.1)
  * activesupport (4.1.1)
  * arel (5.0.1.20140414130214)
  * builder (3.2.2)
  * bundler (1.6.2)
  * coffee-rails (4.0.1)
  * coffee-script (2.2.0)
  * coffee-script-source (1.7.0)
  * erubis (2.7.0)
  * execjs (2.2.0)
  * hike (1.2.3)
  * i18n (0.6.9)
  * jbuilder (2.1.0)
  * jquery-rails (3.1.0)
  * json (1.8.1)
  * libv8 (3.16.14.3)
  * mail (2.5.4)
  * mime-types (1.25.1)
  * minitest (5.3.4)
  * multi_json (1.10.1)
  * pg (0.17.1)
  * polyglot (0.3.5)
  * rack (1.5.2)
  * rack-test (0.6.2)
  * rails (4.1.1)
  * railties (4.1.1)
  * rake (10.3.2)
  * rdoc (4.1.1)
  * ref (1.0.5)
  * sass (3.2.19)
  * sass-rails (4.0.3)
  * schema_associations (1.2.0)
  * schema_plus (1.5.1)
  * schema_validations (1.0.0)
  * sdoc (0.4.0)
  * spring (1.1.3)
  * sprockets (2.11.0)
  * sprockets-rails (2.1.3)
  * symbolize (4.4.1)
  * therubyracer (0.12.1)
  * thor (0.19.1)
  * thread_safe (0.3.4)
  * tilt (1.4.1)
  * treetop (1.4.15)
  * turbolinks (2.2.2)
  * tzinfo (1.2.1)
  * uglifier (2.5.1)
  * valuable (0.9.8)

[p64]

I just double validated with this syntax model:

      t.references :parent, index: :unique, null: false, on_delete: :cascade

I get

    t.index ["parent_id"], :name => "fk__child_parent_id"
    t.index ["parent_id"], :name => "index_child_on_parent_id"
    t.foreign_key ["parent_id"], "child", ["id"], :on_update => :no_action, :on_delete => :cascade, :name => "fk_child_parent_id"

and with

      t.references :parent, index: { unique: true }, null: false, on_delete: :cascade

I get:

    t.index ["parent_id"], :name => "fk__child_parent_id"
    t.index ["parent_id"], :name => "index_child_on_parent_id", :unique => true
    t.foreign_key ["parent_id"], "child", ["id"], :on_update => :no_action, :on_delete => :cascade, :name => "fk_child_parent_id"

[ronen]

thanks for the info -- will look into it as soon as i get a chance...

[p64]

Very welcome. Please let me know what I can do to help.

[ronen]

In case it's not obvious, the workaround would to delete the unwanted index in a later line in the migration.

Please let me know what I can do to help.

Well, if you're motivated to dive into the code and debug it.... ;)

[p64]

I'm happy to do that. Can I ask you to point me to the file where the index should be created? It's ruby so everything is factored so well that i keep going from file to file. I'd also like a sanity check that I'm in the right place before. :) Thanks much.

[ronen]

I'd start by looking at active_record/column_options_handler.rb, which has the (mildly nasty) logic for processing options and creating an index implicitly or explicitly.

BTW I believe that some of the features that used to provided by only schema_plus are now provided by rails 4.* (my own projects are still pinned to rails 3.2 so truth is i haven't had a chance to investigate rails 4.* deeply.). In particular i think rails now supports creating an index as an option to the column definition; so conceivably what's happening is that both rails and schema_plus are processing the index option, leading to two indexes being created. That's just a SWAG, i may be entirely off the mark...

Thanks for being willing to look into it!

[p64]

Thanks for the pointers on rails 4. That sent me down the right road. Here's what I found:

At minimum in rails 4.0.2 we have code for "add_reference" that looks like this:

http://apidock.com/rails/v4.0.2/ActiveRecord/ConnectionAdapters/SchemaStatements/add_reference

     def add_reference(table_name, ref_name, options = {})
        polymorphic = options.delete(:polymorphic)
        index_options = options.delete(:index)
        add_column(table_name, "#{ref_name}_id", :integer, options)
        add_column(table_name, "#{ref_name}_type", :string, polymorphic.is_a?(Hash) ? polymorphic : options) if polymorphic
        add_index(table_name, polymorphic ? ]id type].map{ |t| "#{ref_name}_#{t}" } : "#{ref_name}_id", index_options.is_a?(Hash) ? index_options : nil) if index_options
      end

the add_index creates:

    t.index ["parent_id"], :name => "index_child_on_parent_id", :unique => true

in response to:

t.references :parent, index: { :unique => true} , null: false, on_delete: :cascade

which in my opinion makes the schema_plus index redundant:

    t.index ["parent_id"], :name => "fk__child_parent_id"
    t.index ["parent_id"], :name => "index_child_on_parent_id", :unique => true

The question is how do you want to handle such a case. I would vote for not creating the fk__child_parent_id index.

I have tested this with the following temp test:

    def column_index(table_name, column_name, options) #:nodoc:
      options = {} if options == true
      options = { :unique => true } if options == :unique
      column_name = [column_name] + Array.wrap(options.delete(:with)).compact
      #add_index(table_name, column_name, options)
    end

I would think a conditional for the rails version that started adding the index would be the best solution. What do you think?

[p64]

when I comment out "add_index" in def column_index

this:

t.references :parent, index: true , null: false, on_delete: :cascade

also works to create:

t.index ["parent_id"], :name => "index_child_on_parent_id"

[ronen]

I would think a conditional for the rails version that started adding the index would be the best solution. What do you think?

Yep, seems reasonable. I'd suggest/request going the full TDD route. That is:

1. Starting with the original code, add a test that fails because of your bug. That is, in spec/column_spec.rb add a test creates a column using

 t.references :parent, index: :unique

and checks that it creates exactly one index, and that the index is unique. Verify that the test fails (when you run ./runspecs with the relevant options).
2. To be safe, add the same test for t.integer :parent_id, index: :unique. I'm not sure if it'll succeed or fail.
3. Commit those tests.
4. Make your proposed change that's conditional on the rails version.
5. Verify that it fixes your new failing test... and that it doesn't break any of the other tests.
6. If all's good, commit & issue the PR! Otherwise, keep debugging :)

Sound good? Thanks!

[p64]

I'm not much of a TDD guy as invariably the tests are never right (as we see in this case :( ). Here's the patch:

git diff
diff --git a/vendor/bundle/ruby/2.1.0/gems/schema_plus-1.5.1/lib/schema_plus/active_record/column_options_handler.rb b/vendor/bundle/ruby/2.1.0/gems/schema_plus-1.5.1/lib/schema_plus/active_record/column_options_handler.rb
index d9b7c96..cc0bf54 100644
--- a/vendor/bundle/ruby/2.1.0/gems/schema_plus-1.5.1/lib/schema_plus/active_record/column_options_handler.rb
+++ b/vendor/bundle/ruby/2.1.0/gems/schema_plus-1.5.1/lib/schema_plus/active_record/column_options_handler.rb
@@ -81,7 +81,7 @@ module SchemaPlus::ActiveRecord
       options = {} if options == true
       options = { :unique => true } if options == :unique
       column_name = [column_name] + Array.wrap(options.delete(:with)).compact
-      add_index(table_name, column_name, options)
+      add_index(table_name, column_name, options) if Rails::VERSION::MAJOR < 4
     end

     def remove_auto_index_if_exists(table_name, column_name)

which if called like this:

t.references :parent, index: { unique: true } , null: false, on_delete: :cascade

yields:

t.index ["parent_id"], :name => "index_child_on_parent_id", :unique => true

[ronen]

No need to go off into a discussion testing philosophies. I'm not a hardcore TDD guy either. But IMHO in this case it's appropriate.

But if you're not up for it, that's OK, i'll do it myself when I get a chance. And in any case thanks for reporting the bug and tracking down the problem!

[p64]

My apologies. I didn't mean to be confrontational. I honestly wouldn't even know where to start writing the tests.

[p64]

I was thinking more about this. For rails >= 4.0 we are basically not making a "schema_plus" index in this case. So what would we be testing? Wouldn't the tests just refer to rails for testing?

[ronen]

Yeah, I agree for the most part the existing tests should cover it -- take out that line and everything should continue to work as normal. So why write a new test? A couple of reasons that make it seem appropriate for this type of bug fix:

First a sanity check, to make sure that the test rig has the same environmental setup as what caused you to see the bug. If the test doesn't fail we'd know the problem is more mysterious than we thought. That's less of an issue since you've already tracked down the problem, but still it can't hurt to doublecheck given how large and sometimes obscure rails with all its components (including schema_plus) can be.

Next, a hedge against future changes. E.g. it's possible that there are circumstances in which schema_plus will want to generate an index that rails wouldn't normally generate, or in which schema_plus might need to take over creating the index to give it options that rails doesn't support. We'll find out if that's the case now when we run the full specs, or it might come up in the future (e.g. to fix things for #159 maybe?). So at some point the simple conditional in your patch might need to change to some more complex logic -- and having the new test in place would make sure we didn't accidentally introduce a bug in that logic which would lead to indexes be created by both rails and schema_plus. (And, as you found, the existing tests don't catch that problem!)

[ronen]

OK. I've taken care of this. Thanks again for reporting and tracking down the problem.

BTW score one for TDD: The patch was indeed overbroad; ActiveRecord 4.* doesn't always create indexes where schema_plus does, and where it does create them, it doesn't support all the syntax that schema_plus does. So I indeed needed to put in some different logic and having the new test in place was very handy to make sure I navigated the logic properly to get the new tests to pass while not causing the existing tests to fail.

[p64]

Wow - Thanks so much! That's great news!