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

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

Closed
3 tasks done
compwron opened this issue Sep 16, 2021 · 11 comments
Closed
3 tasks done

Comments

@compwron
Copy link

compwron commented Sep 16, 2021

  • This is not a usage question, this is a bug report
  • This bug can be reproduced with the script I provide below
  • This bug can be reproduced in the latest release of the paper_trail gem

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
Copy link

pauldub commented Oct 13, 2021

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:

if mysql?
", { null: false, limit: 191 }"
else
", { null: false }"
end
to:

      if mysql?
        ", null: false, limit: 191 "
      else
        ", null: false"
      end

@vhiairrassary
Copy link

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

@duffyjp
Copy link

duffyjp commented Jan 3, 2022

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
Copy link

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
Copy link

duffyjp commented Jan 6, 2022

@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
Copy link

@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
Copy link

duffyjp commented Jan 10, 2022

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

@vhiairrassary
Copy link

#1366 might be a solution 🥳

@jaredbeck
Copy link
Member

Fixed by #1366

@estebanz01
Copy link

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
Copy link

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.

tobyprivett added a commit to ministryofjustice/hmpps-book-secure-move-api that referenced this issue Sep 27, 2024
This column name broke the ERD generator. It was introduced by
this issue (now resolved) in PaperTrail:

paper-trail-gem/paper_trail#1347
tobyprivett added a commit to ministryofjustice/hmpps-book-secure-move-api that referenced this issue Oct 1, 2024
* Ensure random_move_type creates a valid move_type

`UNKNOWN` is not in the list of move_types

* Remove column `{:null=>false}` from schema

This column name broke the ERD generator. It was introduced by
this issue (now resolved) in PaperTrail:

paper-trail-gem/paper_trail#1347

* Update README

Adds notes that:
- Reference data should be loaded before generating Auth tokens
- Locations are cached in Redis
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

No branches or pull requests

8 participants