Skip to content
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

Foreign key to different table in migration does not reverse correctly #25169

Closed
lukeredpath opened this issue May 27, 2016 · 2 comments
Closed
Assignees

Comments

@lukeredpath
Copy link
Contributor

Steps to reproduce

A new feature was introduced in response to #21563 which allows a different table to be specified in a references migration where the foreign key column does not match the table it is referencing.

For example, given two tables, "buildings" and "devices", where devices have a location association which is a building.

The AR class might be:

class Device < ApplicationRecord
  belongs_to :location, class_name: 'Building'
end

The location_id column can be added with a foreign key constraint using this new feature:

change_table(:devices) do |t|
  t.references :location, foreign_key: {to_table: :buildings}
end

This works as expected. In my schema I have:

add_foreign_key "devices", "buildings", column: "location_id"

However when I try to roll back this migration, it fails.

Expected behaviour

Rolling back should remove the location_id column from devices and the foreign key constraint.

Actual behaviour

The migration fails with the error:

== 20160527152008 AssociateDeviceWithBuilding: reverting ==============
-- remove_reference(:devices, :location, {:foreign_key=>{:to_table=>:buildings}})
rake aborted!
StandardError: An error has occurred, this and all later migrations canceled:

Table 'devices' has no foreign key for locations
-e:1:in `<main>'
ArgumentError: Table 'client_devices' has no foreign key for locations

What the reverse migration should be doing is looking for a foreign key for buildings but it seems to be ignoring the to_table parameter on reverse.

System configuration

Rails version:
Rails 5 RC1

Ruby version:
ruby 2.2.3p173

@maclover7
Copy link
Contributor

Can you collate your sample code above and create an executable test script using one of the templates here, that would demonstrate the problem you are seeing?

@lukeredpath
Copy link
Contributor Author

lukeredpath commented May 27, 2016

Sure. I can't seem to reproduce it with sqlite, but I can with postgres which is the DB I'm using anyway. Executable test:

begin
  require 'bundler/inline'
rescue LoadError => e
  $stderr.puts 'Bundler version 1.10 or later is required. Please update your Bundler'
  raise e
end

gemfile(true) do
  source 'https://rubygems.org'
  gem 'rails', '>= 5.0.0.rc1', '< 5.1'
  gem 'pg'
end

require 'active_record'
require 'minitest/autorun'
require 'logger'

DB_NAME = 'test_25169'

system("dropdb #{DB_NAME}")
system("createdb #{DB_NAME}")

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: 'postgresql', database: DB_NAME)
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :buildings, force: true do |t|
  end

  create_table :devices, force: true do |t|
  end
end

class AssociateDeviceWithBuilding < ActiveRecord::Migration[5.0]
  def change
    change_table(:devices) do |t|
      t.references :location, foreign_key: {to_table: :buildings}
    end
  end
end

class BugTest < Minitest::Test
  def test_foreign_key_migration_is_reversible
    AssociateDeviceWithBuilding.migrate(:up)

    assert current_schema.match(/add_foreign_key "devices", "buildings", column: "location_id"/)

    # this should not raise
    AssociateDeviceWithBuilding.migrate(:down)

    refute current_schema.match(/add_foreign_key "devices", "buildings", column: "location_id"/)
  end

  private

  def current_schema
    output = StringIO.new
    ActiveRecord::SchemaDumper.dump(ActiveRecord::Base.connection, output)
    output.rewind
    output.read
  end
end

Output for me (backtrace omitted):

Finished in 0.027510s, 36.3503 runs/s, 36.3503 assertions/s.

  1) Error:
BugTest#test_association_stuff:
ArgumentError: Table 'devices' has no foreign key for locations

@sgrif sgrif self-assigned this May 30, 2016
@sgrif sgrif closed this as completed in 56a61e0 May 31, 2016
prathamesh-sonpatki pushed a commit to prathamesh-sonpatki/rails that referenced this issue Jul 23, 2016
The code incorrectly assumes that the option was written as
`foreign_key: true`, but that is not always the case. This now mirrors
the behavior of reverting `add_foreign_key`. The code was changed to use
kwargs while I was touching it, as well.

This could really use a refactoring to go through the same code paths as
`add_refernce` in the future, so we don't duplicate default values.

Fixes rails#25169
rafaelfranca pushed a commit that referenced this issue Jul 28, 2016
…n. (#25931)

* Respect options passed to `foreign_key` when reverting `add_reference`

The code incorrectly assumes that the option was written as
`foreign_key: true`, but that is not always the case. This now mirrors
the behavior of reverting `add_foreign_key`. The code was changed to use
kwargs while I was touching it, as well.

This could really use a refactoring to go through the same code paths as
`add_refernce` in the future, so we don't duplicate default values.

Fixes #25169

* Backport part of 1b8a7b8

- For fixing tests caused by d18fd49
- Fixes #25755

* From original commit message -

Additionally, there was a bug in `remove_foreign_key` if you passed it
an options hash containing `to_table`. This now occurs whenever removing
a reference, as we always normalize to a hash.

- The original commit can't be cherry-picked as it contains other fixes
  too which are not applicable for 5-0-stable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants