Request for review: Add FK constraints to hobo_fields, fix migration escape issues

[DavidMikeSimon]

Note: This PR is not ready for merging! I'm submitting this now just to solicit comments and review.

This PR adds support for foreign key constraints to hobo_fields, to match the feature added in Rails 4.2. Like indexes, constraints are automatically created for belongs_to associations, but can be skipped or manually reconfigured with options to belongs_to, and can also be explicitly created in model definitions.

Currently, Rails does not have FK constraint support for its sqlite adapter; I'm working on that separately, and this new Hobo feature will not be properly testable until then.

Besides adding foreign key support, I also went through the migration generator code for hobo_fields and changed the way strings were interpolated:

• "blah blah :#{foo} blah" is changed to "blah blah #{foo.to_sym.inspect} blah" to prevent invalid code from being generated given weird symbols like :"foo bar"
• "blah blah '#{foo}' blah" is changed to "blah blah #{foo.to_s.inspect} blah" to prevent invalid code from being generated given strings containing single quotes or other escapable characters

[iox]

Hi David,

I have gone through your code and I think it's looking great. This would be a really great feature to support within Hobo. The string interpolation code also looks good to me.

Please say if you need some testers.

Warm regards, Ignacio

[DavidMikeSimon]

@iox Thanks! I'm currently waiting on https://github.com/rails/rails/pull/22236, after that's sorted out I'll revisit this PR to make sure the doctests pass. After that, some testers would be great. :-)

[DavidMikeSimon]

@iox I have been pestering the Rails devs for months now trying to get my activerecord PR for sqlite foreign keys accepted there, but it seems to be stalled out. I'll go ahead and update this PR to produce migrations that safely avoid FK actions on database adapters where they're not supported. Then I think it'll be ready for merging into hobo.

Any other feedback is absolutely welcome. Please go ahead and try out my fork on whatever FK-supporting databases you're familiar with to make sure it does what you'd expect it to.

[iox]

Thanks for your work @DavidMikeSimon , I think it's an important addition, I love sqlite :)

[DavidMikeSimon]

@iox Ok, I've added some code to my PR which skips the foreign key stuff if the database adapter doesn't support it. It warns the user about this, as it would be a bad idea to make the sqlite adapter's lack of FK support affect a migration run on another database.

Anyways, I think this is ready for merging if it meets your approval.

[iox]

@DavidMikeSimon, you have done an impressive job, congratulations. I am going to check that all tests are running and I will proceed to merge it. Thanks a lot!

[iox]

@DavidMikeSimon, the tests are failing in Codeship. Do you have any idea what the problem could be? Maybe we are using the wrong adapter in the test environment?

cd /tmp/hobo_fields_testapp
git add .
git reset --hard -q HEAD
=== Testing '/home/rof/src/github.com/Hobo/hobo/hobo_fields/test/migration_generator.rdoctest'...
-- create_table(:adverts)
-> 0.0012s
-- add_column(:adverts, :body, :text)
-> 0.0005s
-- add_column(:adverts, :published_at, :datetime)
-> 0.0004s
-- remove_column(:adverts, :published_at)
-> 0.0032s
-- add_column(:adverts, :title, :string)
-> 0.0004s
-- remove_column(:adverts, :name)
-> 0.0029s
-- change_column(:adverts, :title, :string, {:default=>"Untitled"})
-> 0.0028s
1.  FAIL | Default Test
Got:      nil
Expected: "add_foreign_key :adverts, :categories"

from /home/rof/src/github.com/Hobo/hobo/hobo_fields/test/migration_generator.rdoctest:533
32 comparisons, 1 doctests, 1 failures, 0 errors

Regards, Ignacio

[DavidMikeSimon]

@iox Sure, I'll look into this.

[DavidMikeSimon]

@iox I don't seem to have permissions to see the codeship results directly, but yeah, my guess would be that the adapter is SQLite in the tests, which will skip foreign key statement generation. Is there a way to switch the test adapter to MySQL or Postgres?

[iox]

@DavidMikeSimon Sorry for the long delay. Can you send me an email to ignacio at ihuerta dot net?

With your email address I can add you to Codeship, so you can play around with the tests.

BTW, I'm resolving a separate issue in Codeship at the moment.