Skip to content

Coerce strings in create_join_table. #7716

Merged
merged 1 commit into from Nov 21, 2012

5 participants

@steveklabnik
Ruby on Rails member

If you accidentally pass a string and a symbol, this breaks. So
we coerce them both to strings.

Fixes #7715.

I'm not 100% sure we care to be this defensive, but the change was easy enough.

/cc @tenderlove @jonleighton

@rafaelfranca rafaelfranca commented on the diff Sep 20, 2012
activerecord/lib/active_record/migration/join_table.rb
@@ -8,7 +8,7 @@ def find_join_table_name(table_1, table_2, options = {})
end
def join_table_name(table_1, table_2)
- [table_1, table_2].sort.join("_").to_sym
+ [table_1.to_s, table_2.to_s].sort.join("_").to_sym
@rafaelfranca
Ruby on Rails member
rafaelfranca added a note Sep 20, 2012

@steveklabnik I wrote this code but I think we don't need to call to_sym in the end of chain. Could you investigate?

@steveklabnik
Ruby on Rails member
steveklabnik added a note Sep 20, 2012

Checking now.

@steveklabnik
Ruby on Rails member
steveklabnik added a note Sep 20, 2012

Gives one failure:

  1) Failure:
test_invert_create_join_table(ActiveRecord::Migration::CommandRecorderTest) [/Users/steve/src/rails/activerecord/test/cases/migration/command_recorder_test.rb:79]:
--- expected
+++ actual
@@ -1 +1 @@
-[:drop_table, [:artists_musics]]
+[:drop_table, ["artists_musics"]]

Obviously, this test could be changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva
Ruby on Rails member

Weird, should we care about this case? I think you should give both strings or symbols =/

@steveklabnik
Ruby on Rails member

@carlosantoniodasilva I'm not sure, to be honest. @slbug how did this come up?

We accept both in so many places, I figured it might be worth it. If not, no biggie.

@slbug
slbug commented Sep 20, 2012

@steveklabnik I've just wrote a migration which calls create_join_table :first_table, :second_table {|t| t.index [:first_table_id, :second_table_id] }

and migration fails. first table name was converted to string, second not. so sort failed

@rafaelfranca
Ruby on Rails member

This is weird because as far I remember we don't convert the arguments to strings. Also all the tests are passing.

@slbug
slbug commented Sep 20, 2012

will create a test app tomorrow.

@slbug
slbug commented Sep 21, 2012

@rafaelfranca @steveklabnik https://github.com/slbug/gh7715

just clone and try to run migrations. you will get 'comparison of String with :tags failed'

@steveklabnik
Ruby on Rails member

Yep! My backtrace:

$ rake db:migrate --trace                                                 ✘
** Invoke db:migrate (first_time)
** Invoke environment (first_time)
** Execute environment
** Invoke db:load_config (first_time)
** Execute db:load_config
** Execute db:migrate
==  CreateHotelsTagsJoinTable: migrating ======================================
-- create_join_table(:hotels, :tags)
rake aborted!
An error has occurred, this and all later migrations canceled:

comparison of String with :tags failed
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/migration/join_table.rb:11:in `sort'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/migration/join_table.rb:11:in `join_table_name'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/migration/join_table.rb:7:in `find_join_table_name'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb:203:in `create_join_table'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/migration.rb:488:in `block in method_missing'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/migration.rb:460:in `block in say_with_time'
/Users/steve/.rvm/rubies/ruby-1.9.3-p194/lib/ruby/1.9.1/benchmark.rb:280:in `measure'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/migration.rb:460:in `say_with_time'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/migration.rb:480:in `method_missing'
/Users/steve/tmp/gh7715/db/migrate/20120921075159_create_hotels_tags_join_table.rb:3:in `change'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/migration.rb:429:in `block (2 levels) in migrate'
/Users/steve/.rvm/rubies/ruby-1.9.3-p194/lib/ruby/1.9.1/benchmark.rb:280:in `measure'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/migration.rb:429:in `block in migrate'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:300:in `with_connection'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/migration.rb:411:in `migrate'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/migration.rb:550:in `migrate'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/migration.rb:741:in `block (2 levels) in migrate'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/migration.rb:821:in `block in ddl_transaction'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:164:in `block in transaction'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:172:in `within_new_transaction'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:164:in `transaction'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/transactions.rb:208:in `transaction'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/migration.rb:821:in `ddl_transaction'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/migration.rb:740:in `block in migrate'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/migration.rb:736:in `each'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/migration.rb:736:in `migrate'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/migration.rb:595:in `up'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/migration.rb:573:in `migrate'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bundler/gems/rails-a507c641ec78/activerecord/lib/active_record/railties/databases.rake:48:in `block (2 levels) in <top (required)>'
/Users/steve/.rvm/gems/ruby-1.9.3-p194@global/gems/rake-0.9.2.2/lib/rake/task.rb:205:in `call'
/Users/steve/.rvm/gems/ruby-1.9.3-p194@global/gems/rake-0.9.2.2/lib/rake/task.rb:205:in `block in execute'
/Users/steve/.rvm/gems/ruby-1.9.3-p194@global/gems/rake-0.9.2.2/lib/rake/task.rb:200:in `each'
/Users/steve/.rvm/gems/ruby-1.9.3-p194@global/gems/rake-0.9.2.2/lib/rake/task.rb:200:in `execute'
/Users/steve/.rvm/gems/ruby-1.9.3-p194@global/gems/rake-0.9.2.2/lib/rake/task.rb:158:in `block in invoke_with_call_chain'
/Users/steve/.rvm/rubies/ruby-1.9.3-p194/lib/ruby/1.9.1/monitor.rb:211:in `mon_synchronize'
/Users/steve/.rvm/gems/ruby-1.9.3-p194@global/gems/rake-0.9.2.2/lib/rake/task.rb:151:in `invoke_with_call_chain'
/Users/steve/.rvm/gems/ruby-1.9.3-p194@global/gems/rake-0.9.2.2/lib/rake/task.rb:144:in `invoke'
/Users/steve/.rvm/gems/ruby-1.9.3-p194@global/gems/rake-0.9.2.2/lib/rake/application.rb:116:in `invoke_task'
/Users/steve/.rvm/gems/ruby-1.9.3-p194@global/gems/rake-0.9.2.2/lib/rake/application.rb:94:in `block (2 levels) in top_level'
/Users/steve/.rvm/gems/ruby-1.9.3-p194@global/gems/rake-0.9.2.2/lib/rake/application.rb:94:in `each'
/Users/steve/.rvm/gems/ruby-1.9.3-p194@global/gems/rake-0.9.2.2/lib/rake/application.rb:94:in `block in top_level'
/Users/steve/.rvm/gems/ruby-1.9.3-p194@global/gems/rake-0.9.2.2/lib/rake/application.rb:133:in `standard_exception_handling'
/Users/steve/.rvm/gems/ruby-1.9.3-p194@global/gems/rake-0.9.2.2/lib/rake/application.rb:88:in `top_level'
/Users/steve/.rvm/gems/ruby-1.9.3-p194@global/gems/rake-0.9.2.2/lib/rake/application.rb:66:in `block in run'
/Users/steve/.rvm/gems/ruby-1.9.3-p194@global/gems/rake-0.9.2.2/lib/rake/application.rb:133:in `standard_exception_handling'
/Users/steve/.rvm/gems/ruby-1.9.3-p194@global/gems/rake-0.9.2.2/lib/rake/application.rb:63:in `run'
/Users/steve/.rvm/gems/ruby-1.9.3-p194@global/gems/rake-0.9.2.2/bin/rake:33:in `<top (required)>'
/Users/steve/.rvm/gems/ruby-1.9.3-p194@global/bin/rake:19:in `load'
/Users/steve/.rvm/gems/ruby-1.9.3-p194@global/bin/rake:19:in `<main>'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bin/ruby_noexec_wrapper:14:in `eval'
/Users/steve/.rvm/gems/ruby-1.9.3-p194/bin/ruby_noexec_wrapper:14:in `<main>'
Tasks: TOP => db:migrate
@steveklabnik
Ruby on Rails member

I bet it comes from activerecord/lib/active_record/migration.rb:488.

    connection.send(method, *arguments, &block)

which has arguments of

        arguments[0] = Migrator.proper_table_name(arguments.first)
        arguments[1] = Migrator.proper_table_name(arguments.second) if method == :rename_table

So, in proper_table_name:

    name.table_name rescue "#{ActiveRecord::Base.table_name_prefix}#{name}#{ActiveRecord::Base.table_name_suffix}"

The first one works, and it's a symbol. The second hits the rescue, which is turned into a string.

@steveklabnik
Ruby on Rails member

@rafaelfranca what do you think? Should I maybe turn both arguments[0] and arguments[1] into strings rather than doing it farther down? Seems like the bug is really here.

@rafaelfranca
Ruby on Rails member

@steveklabnik could be a solution. But we need to reproduce this failure with a test.

@rafaelfranca
Ruby on Rails member
@steveklabnik
Ruby on Rails member

I looked at this the other day, and I'm still not sure how to test it.

@carlosantoniodasilva
Ruby on Rails member

@steveklabnik that fails for me without the change:

def test_create_join_table_with_symbol_and_string
  connection.create_join_table :artists, 'musics'

  assert_equal %w(artist_id music_id), connection.columns(:artists_musics).map(&:name).sort
end
  1) Error:
test_create_join_table_with_symbol_and_string(ActiveRecord::Migration::CreateJoinTableTest):
ArgumentError: comparison of Symbol with String failed

Inverting the string/symbol arguments order also fails. I think it should be enough, wdyt?

@steveklabnik steveklabnik Coerce strings in create_join_table.
If you accidentally pass a string and a symbol, this breaks. So
we coerce them both to strings.

Fixes #7715
48a0357
@steveklabnik
Ruby on Rails member

:+1:. I've updated the commit to include this test. This is so minor that I wasn't sure about a CHANGELOG. What do you think?

@carlosantoniodasilva
Ruby on Rails member

@steveklabnik not necessary I think, create_join_table only exists in master :)

@steveklabnik
Ruby on Rails member

Awesome. I think we're good to merge then.

@rafaelfranca rafaelfranca merged commit b6793ba into rails:master Nov 21, 2012
@rafaelfranca
Ruby on Rails member

:heart: :green_heart: :blue_heart: :yellow_heart: :purple_heart:

@yahonda

I'm wondering why this test method works because I've got errors with Ruby 1.9.3-p327 and sqlite3, mysql, mysql2 and postgresql adapters.

  1) Error:
test_create_join_table_with_symbol_and_string(CopyMigrationsTest):
NameError: undefined local variable or method `connection' for #<CopyMigrationsTest:0x00000003df56a8>
    test/cases/migration_test.rb:760:in `test_create_join_table_with_symbol_and_string'
Ruby on Rails member

Hmmm, @rafaelfranca?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.