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

include migration statements on ActiveRecord::Migration #125

Conversation

samrjenkins
Copy link

@samrjenkins samrjenkins commented Oct 11, 2023

Currently, projects with old migration files containing Paperclip schema statements do not perform the same schema changes both before and after an ActiveRecord version upgrade.

Consider the following migration file which will add one datetime column using ActiveRecord's add_column and another datetime column as part of Paperclip's add_attachment (note that is is written as an ActiveRecord 6.1 migration).

class AddFooAttachmentToBars < ActiveRecord::Migration[6.1]
  def change
    add_column :bars, :baz, :datetime
    add_attachment :bars, :foo
  end
end

We would expect the two added datetime columns to be identical in everything but name.

When run on Rails 6.1.7.6 it produces the following schema entry as expected: baz and foo_updated_at both have the same format

  create_table "bars", force: :cascade do |t|
    t.datetime "baz"
    t.string "foo_content_type"
    t.string "foo_file_name"
    t.bigint "foo_file_size"
    t.datetime "foo_updated_at"
  end

When run on Rails 7.0.8 it produces the following schema entry: baz and foo_updated_at have different formats (note that foo_updated_at is no longer added with precision: nil as it would have in Rails 6)

  create_table "bars", force: :cascade do |t|
    t.datetime "baz", precision: nil
    t.string "foo_content_type"
    t.string "foo_file_name"
    t.bigint "foo_file_size"
    t.datetime "foo_updated_at"
  end

This is because Paperclip schema statements do not currently respect ActiveRecord::Migration::Compatibility versioning; the logic which ensures that schema statements from old migrations retain consistent behaviour when run within newer versions of ActiveRecord.


This pull request includes Paperclip::Schema::Statements on ActiveRecord::Migration to ensure that Paperclip schema statements are consistent with ActiveRecord::Migration::Compatibility versioning.

When run on Rails 6.1.7.6 it produces the following schema entry as expected: baz and foo_updated_at both have the same format

  create_table "bars", force: :cascade do |t|
    t.datetime "baz"
    t.string "foo_content_type"
    t.string "foo_file_name"
    t.bigint "foo_file_size"
    t.datetime "foo_updated_at"
  end

When run on Rails 7.0.8 it produces the following schema entry as expected: baz and foo_updated_at both have the same format

  create_table "bars", force: :cascade do |t|
    t.datetime "baz", precision: nil
    t.string "foo_content_type"
    t.string "foo_file_name"
    t.bigint "foo_file_size"
    t.datetime "foo_updated_at", precision: nil
  end

@samrjenkins samrjenkins force-pushed the include-migration-statements-on-ar-migration-current branch from b5dd695 to 43aa5f9 Compare October 12, 2023 12:54
@samrjenkins samrjenkins changed the title include migration statements on ActiveRecord::Migration::Current include migration statements on ActiveRecord::Migration Oct 12, 2023
@samrjenkins samrjenkins force-pushed the include-migration-statements-on-ar-migration-current branch from 43aa5f9 to e4ea04e Compare October 12, 2023 13:03
@samrjenkins samrjenkins marked this pull request as ready for review October 12, 2023 14:53
@ssinghi
Copy link

ssinghi commented Dec 15, 2023

@samrjenkins can you please sign your commits

@samrjenkins samrjenkins force-pushed the include-migration-statements-on-ar-migration-current branch from 623c839 to 512a35e Compare December 15, 2023 11:44
@samrjenkins
Copy link
Author

@ssinghi I have rebased with signed commits

@ssinghi ssinghi merged commit 1c6dfe6 into kreeti:master Dec 16, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants