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

Restore the behaviour of the compatibility layer for integer-like PKs #27389

Merged
merged 3 commits into from Feb 7, 2017

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Dec 17, 2016

Follow up to #26266.

Currently master branch was lost 1fa6c9e (#22090) and #18220 due
to #26266 and #26266 has another regression #27374.

primary key definition before #26266 after #26266
id: :primary_key (default) int auto_increment PRIMARY KEY bigint auto_increment PRIMARY KEY
id: :integer int PRIMARY KEY int PRIMARY KEY
id: :integer, default: nil int PRIMARY KEY int PRIMARY KEY
id: :bigint bigint auto_increment PRIMARY KEY bigint PRIMARY KEY
id: :bigint, default: nil bigint PRIMARY KEY bigint PRIMARY KEY

1fa6c9e (#22090) and #18220 are to ease to create an auto incremental
(or not) bigint primary key and to dump the primary key correctly.

Since #26266, changed default primary key, and 1fa6c9e (#22090)
and #18220 were lost. And so currently an auto incremental int primary
key cannot dump (lose auto increment).

To fix the issue, integer and bigint primary key has auto increment
as implicit default, and legacy migration still keep its previous
behavior as before #26266.

primary key definition suggested fix
id: :primary_key (default) bigint auto_increment PRIMARY KEY
id: :integer int auto_increment PRIMARY KEY
id: :integer, default: nil int PRIMARY KEY
id: :bigint bigint auto_increment PRIMARY KEY
id: :bigint, default: nil bigint PRIMARY KEY

The PR #27384 changed migration compatibility behaviour.

class CreateMasterData < ActiveRecord::Migration[5.0]
  def change
    create_table :master_data, id: :integer do |t|
      t.string :name
    end
  end
end

Previously this migration created non-autoincremental primary key
expected. But after the PR, the primary key changed to autoincremental,
it is unexpected.

This change restores the behaviour of the compatibility layer.

@rails-bot
Copy link

r? @chancancode

(@rails-bot has picked a reviewer for you, use r? to override)

@maclover7
Copy link
Contributor

cc @matthewd

@eileencodes eileencodes assigned matthewd and unassigned chancancode Dec 17, 2016
@kamipo kamipo force-pushed the fix_mysql_pk_dumping_correctly branch from 17fd5df to 67bc8b3 Compare December 30, 2016 17:11
@kamipo kamipo added the MySQL label Jan 6, 2017
@kamipo kamipo force-pushed the fix_mysql_pk_dumping_correctly branch 2 times, most recently from facc301 to fb9a1b1 Compare January 19, 2017 17:54
@kamipo kamipo changed the title Fix MySQL PK dumping correctly Restore the behaviour of the compatibility layer for integer-like PKs Jan 19, 2017
@rafaelfranca rafaelfranca added this to the 5.1.0 milestone Jan 19, 2017
@kamipo kamipo force-pushed the fix_mysql_pk_dumping_correctly branch 4 times, most recently from 01b9b16 to 38c936c Compare January 22, 2017 22:54
The PR rails#27384 changed integer-like primary key to be autoincrement
unless an explicit default. This means that integer-like primary key is
restored as autoincrement unless dumping the default nil explicitly.
We should dump integer-like primary key with default nil correctly.
The PR rails#27384 changed migration compatibility behaviour.

```ruby
class CreateMasterData < ActiveRecord::Migration[5.0]
  def change
    create_table :master_data, id: :integer do |t|
      t.string :name
    end
  end
end

```

Previously this migration created non-autoincremental primary key
expected. But after the PR, the primary key changed to autoincremental,
it is unexpected.

This change restores the behaviour of the compatibility layer.
Some custom primary key tests (added at rails#17631, rails#17696, rails#18220, rails#18228)
were lost at rails#26266. Restore the tests to improve test coverage.
@kamipo kamipo force-pushed the fix_mysql_pk_dumping_correctly branch from 38c936c to 29c70ab Compare February 4, 2017 12:05
tgxworld pushed a commit to tgxworld/rails that referenced this pull request Feb 7, 2017
…ctly

Restore the behaviour of the compatibility layer for integer-like PKs

* kamipo/fix_mysql_pk_dumping_correctly:
  Restore custom primary key tests lost at rails#26266
  Restore the behaviour of the compatibility layer for integer-like PKs
  Correctly dump integer-like primary key with default nil
@jeremy jeremy merged commit 29c70ab into rails:master Feb 7, 2017
@kamipo kamipo deleted the fix_mysql_pk_dumping_correctly branch February 7, 2017 05:19
@kamipo
Copy link
Member Author

kamipo commented Feb 7, 2017

Legacy migration still have the regression #27334.
Could you also review #27334?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants