paper-trail-gem / paper_trail

Track changes to your rails models
MIT License
6.76k stars 895 forks source link

running migration creates invalid schema.rb diff t.string "{:null=>false}" #1347

Closed compwron closed 2 years ago

compwron commented 2 years ago

In the repo https://github.com/rubyforgood/casa we use paper_trail

Our gem versions are https://github.com/rubyforgood/casa/blob/cf7a4078812d556d28cf37152dd96e78951ef706/Gemfile.lock with paper_trail (12.1.0) When we run rake db:drop && rake db:create && rake db:migrate we see a diff as below:

   create_table "versions", force: :cascade do |t|
-    t.string "item_type", null: false
+    t.string "item_type"
+    t.string "{:null=>false}"
     t.bigint "item_id", null: false
     t.string "event", null: false
     t.string "whodunnit"

Screen Shot 2021-09-15 at 6 40 07 PM

Our previously generated migration is at https://github.com/rubyforgood/casa/blob/cf7a4078812d556d28cf37152dd96e78951ef706/db/migrate/20200329085225_create_versions.rb and shown below:

# This migration creates the `versions` table, the only schema PT requires.
# All other migrations PT provides are optional.
class CreateVersions < ActiveRecord::Migration[6.0]
  # The largest text column available in all supported RDBMS is
  # 1024^3 - 1 bytes, roughly one gibibyte.  We specify a size
  # so that MySQL will use `longtext` instead of `text`.  Otherwise,
  # when serializing very large objects, `text` might not be big enough.
  TEXT_BYTES = 1_073_741_823

  def change
    create_table :versions do |t|
      t.string :item_type, {null: false}
      t.integer :item_id, null: false, limit: 8
      t.string :event, null: false
      t.string :whodunnit
      t.text :object, limit: TEXT_BYTES

      # Known issue in MySQL: fractional second precision
      # -------------------------------------------------
      #
      # MySQL timestamp columns do not support fractional seconds unless
      # defined with "fractional seconds precision". MySQL users should manually
      # add fractional seconds precision to this migration, specifically, to
      # the `created_at` column.
      # (https://dev.mysql.com/doc/refman/5.6/en/fractional-seconds.html)
      #
      # MySQL users should also upgrade to at least rails 4.2, which is the first
      # version of ActiveRecord with support for fractional seconds in MySQL.
      # (https://github.com/rails/rails/pull/14359)
      #
      t.datetime :created_at
    end
    add_index :versions, %i[item_type item_id]
  end
end
pauldub commented 2 years ago

I can confirm that this is happening, it seems to be related to how kwargs are handled by ruby. The following example illustrates why:

[5] pry(main)> def foo(*one_or_many, **options)
[5] pry(main)*   puts "one_or_many: #{one_or_many}"   
[5] pry(main)*   puts "option: #{options}"  
[5] pry(main)* end  
=> :foo
[6] pry(main)> foo(:item_type)
one_or_many: [:item_type]
option: {}
=> nil
[7] pry(main)> foo(:item_type, null: false)
one_or_many: [:item_type]
option: {:null=>false}
=> nil
[8] pry(main)> foo(:item_type, {:null => false})
one_or_many: [:item_type, {:null=>false}]
option: {}
=> nil
[9] pry(main)>

A fix would be change the following options declaration from: https://github.com/paper-trail-gem/paper_trail/blob/7e50b841ac97866adcfb1866b053acdfaf3427d1/lib/generators/paper_trail/install/install_generator.rb#L43-L47 to:

      if mysql?
        ", null: false, limit: 191 "
      else
        ", null: false"
      end
vhiairrassary commented 2 years ago

On our code base, it happens only when upgrading from Ruby 2.x to 3.0.

duffyjp commented 2 years ago

I hit this with a 2.x to 3.0 upgrade as well, with an sqlite3 database. Loading the schema adds a string field named {:null=>false}

$ rails db:schema:load:audit

Screen Shot 2022-01-03 at 1 02 52 PM
tinacious commented 2 years ago

I am on Ruby version 3.0.2 and I'm also experiencing this issue. I've tried with Rails versions 6.1.3.1 and 6.1.4.4 and tried upgrading paper_trail (12.0.0 and 12.1.0) and the issue exists for both versions of Rails and both versions of paper_trail. I needed to upgrade Ruby to support M1 laptops on my team and this didn't happen before the upgrade.

This is some resulting output:

# would be `item_type` field
t.string "{:null=>false}"

And sometimes:

# would be `object` field
t.text "{:null=>false}"

As others have already mentioned, I would also presume that it has to do with the way named arguments have changed in Ruby 3.

It looks to be an issue specifically with a trailing null: false being added to a line and how instead of being added to that same line, it's added as the field name for a subsequent line.

Potentially related but splatting and double splatting appears to have changed in Ruby 3. Here's an example argument-related change we've needed to make in order for our app to work with Ruby v3:

-  def self.call(**args)
-    new(args).call
+  def self.call(args)
+    new(**args).call

Please let me know if you need any additional information. Thanks!

duffyjp commented 2 years ago

@tinacious What was your previous Ruby version? My team recently moved to M1 laptops and found the later point releases of 2.6 and 2.7 install and work fine (earlier / mid release versions don't). FYI

$ rvm list
   ruby-2.6.5 [ x86_64 ]
   ruby-2.6.8 [ x86_64 ]
   ruby-2.6.9 [ arm64 ]
   ruby-2.7.2 [ x86_64 ]
   ruby-2.7.5 [ x86_64 ]
=* ruby-3.0.3 [ arm64 ]
tinacious commented 2 years ago

@duffyjp we were on 2.6.6 and it did not work on M1, and we also tried other 2.x versions higher than our initial version that were also supported by Heroku (a requirement) and they did not work on M1. The lowest working version we found that works for both was 3.0.2. Our project's Ruby will need to keep up with Heroku's Ruby support lifecycle so, even if older versions did work (and the ones supported on Heroku at the time of upgrade did not work on M1), downgrading is not ideal as 2.x versions are nearing EOL. I'm not sure if you're a maintainer of this library or if you're just trying to offer your advice but I assure you we've already considered and tried all available Ruby 2.x versions before deciding to do a major version upgrade. Your questions also seem out of scope for this bug report as going into details about which versions of Ruby we tried on M1 and why is not relevant for reproducing this bug. Thanks.

duffyjp commented 2 years ago

@tinacious I'm just a user trying to be helpful.

vhiairrassary commented 2 years ago

https://github.com/paper-trail-gem/paper_trail/pull/1366 might be a solution 🥳

jaredbeck commented 2 years ago

Fixed by #1366

estebanz01 commented 2 years ago

I don't like writing on closed issues, but I'm getting the same problem with paper_trail 12.3.0. The table was created before this fix was in place, so not sure how can we apply that change in my codebase.

Here's the output after running migrations:

@ db/schema.rb:934 @ ActiveRecord::Schema[7.0].define(version: 2022_05_12_084303) do
  end

  create_table "versions", force: :cascade do |t|
-    t.string "item_type", null: false
+    t.string "item_type"
+    t.string "{:null=>false}"
    t.bigint "item_id", null: false
    t.string "event", null: false
    t.string "whodunnit"
samgooi4189 commented 2 years ago

There are certain version of papertrail generate different migration script

The papertrail I use that time:

ruby '2.6.6'
gem 'rails', '~> 6.0.3', '>= 6.0.3.4'
gem "paper_trail", "~> 11.1"

migration script generated

create_table :versions do |t|
      t.string   :item_type, {:null => false}

which creates schema

create_table "versions", force: :cascade do |t|
    t.string "item_type"
    t.string "{:null=>false}"

How I fix: t.string :item_type, :null => false or t.string :item_type, null: false

Change the migration script to ignore curly bracket works for me.