activerecord-jdbc-adapter icon indicating copy to clipboard operation
activerecord-jdbc-adapter copied to clipboard

simple.rb:test_foreign_keys has type mismatch error

Open enebo opened this issue 8 years ago • 7 comments

This line:

    connection.add_foreign_key :entries, :users, :name => 'entries_user_id_fk'

fails with:

  ActiveRecord::MismatchedForeignKey: Column `user_id` on table `entries` has a type of `int(11)`.
  This does not match column `id` on `users`, which has type `bigint(20)`.
  To resolve this issue, change the type of the `user_id` column on `entries` to be :integer. (For example `t.integer user_id`).
  
  Original message: ActiveRecord::JDBCError: Can't create table `arjdbc_test`.`#sql-55a_f7` (errno: 150 "Foreign key constraint is incorrectly formed"): ALTER TABLE `entries` ADD CONSTRAINT `entries_user_id_fk`
  FOREIGN KEY (`user_id`)
    REFERENCES `users` (`id`)

It is unclear why this is the case since AR has many foreign key tests and we pass all of them. Evaluate if our particular migrations should allow a fk here or not?

enebo avatar Nov 16 '17 17:11 enebo

Hi,

I think this is related with bigint support in Rails 5.1. To create foreign keys, it looks like both data types need identical. However, users table id column is now bigint(20) and entries user_id is int(11).

  • entries
mysql> show create table entries\G
*************************** 1. row ***************************
       Table: entries
Create Table: CREATE TABLE `entries` (
  `id` bigint(20) NOT NULL AUTO_INCREMENT,
  `title` varchar(100) DEFAULT NULL,
  `content` text,
  `status` varchar(255) DEFAULT 'unknown',
  `rating` decimal(10,2) DEFAULT NULL,
  `user_id` int(11) DEFAULT NULL,
  `updated_on` datetime DEFAULT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8
1 row in set (0.00 sec)
  • users
mysql> show create table users\G
*************************** 1. row ***************************
       Table: users
Create Table: CREATE TABLE `users` (
  `id` bigint(20) NOT NULL AUTO_INCREMENT,
  `login` varchar(100) NOT NULL,
  `created_at` datetime NOT NULL,
  `updated_at` datetime NOT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8
1 row in set (0.00 sec)

mysql>

A quick (but dirty) fix is to replace :integer with :bigint, but there must be some more accurate fix at Rails (I do not recall it now).

$ git diff
diff --git a/test/models/entry.rb b/test/models/entry.rb
index 9a035d6e..b661a41c 100644
--- a/test/models/entry.rb
+++ b/test/models/entry.rb
@@ -5,7 +5,7 @@ class CreateEntries < ActiveRecord::Migration[4.2]
       t.column :content, :text
       t.column :status, :string, :default => 'unknown'
       t.column :rating, :decimal, :precision => 10, :scale => 2
-      t.column :user_id, :integer
+      t.column :user_id, :bigint
       t.column :updated_on, :datetime # treated as date "_on" convention
     end
   end

I'll update when I recall it.

Thank you.

yahonda avatar Nov 16 '17 19:11 yahonda

@yahonda so is the test basically bad because the two migrations do not match and AR is just getting more strict in what it allows now? If this test case is currently bad for that reason I am inclined to remove it since AR seems to have quite a bit of coverage for fk.

enebo avatar Nov 16 '17 19:11 enebo

I'd suggest not to remove this test, it is ok to pending it tentatively.

I think the correct fix is to create entries table primary key as integer not bigint. If ActiveRecord::Migration specifies [4.2], primary key column should be integer.

There are a couple of pull requests related: https://github.com/rails/rails/pull/27389 https://github.com/rails/rails/pull/27384 https://github.com/rails/rails/pull/27334

yahonda avatar Nov 16 '17 19:11 yahonda

@yahonda I am confused on whether I should change something or whether Rails is fixing something? (Also are you at rubyconf?)

enebo avatar Nov 16 '17 20:11 enebo

Unfortunately, I am not RubyConf this year.

I think activerecord-jdbc-adapter will need to change some code to support migration versions.

If you have run this test at Rails 5.0 or lower, it should have been successfully finished since there was no bigint support. Both id primary key and user_id should have the same data type integer.

In 5.1 primary key data type is bigint by default. Then new migrations after 5.1 it is fine to rewrite user_id to bigint. However, to consider the backward compatibility of migrations, the migrations created and executed at the lower version of Rails should create the same models and database tables. In this test, users primary key should be integer. not bigint as they were created in 5.0 or lower.

Then migration can specify its versions such as ActiveRecord::Migration[4.2]. I think activerecord-jdbc-adapter will need to support it. Unless, if someone executes the migrations created in 4.2 and executed in 5.1, it would create different primary key definitions.

This is one of the most complex part of 5.1, then I am not sure if I can explain it well.

yahonda avatar Nov 16 '17 20:11 yahonda

@yahonda if we are not properly supporting the older style then this must be an issue with our Java as we have no Ruby code related to this in arjdbc (for mysql). mysql is almost entirely the AR code. Thanks for letting me know specifically what is wrong though. I will try and figure out why we make the wrong type when 4.2 is specified.

enebo avatar Nov 16 '17 20:11 enebo

Raiis has ActiveRecord::Migration::Compatibility module to support migration versions.

https://github.com/rails/rails/blob/5-1-stable/activerecord/lib/active_record/migration/compatibility.rb

This commit may help you to understand more detail.

https://github.com/rails/rails/commit/3e452b12043ecfd983c6132d6eb97eb79db952b7#diff-2a8be25f82da6b3935cc6a41300a1b01

yahonda avatar Nov 16 '17 20:11 yahonda