Use empty hash string for metadata default value in both Rails 4 and 5 #316

kymmt90 · 2018-04-17T13:47:06Z

fixes #313

This PR changes Statesman::GeneratorHelpers#metadata_default_value to return \"{}\" in both Rails 4 and 5 by Strint#dump.

#metadata_default_value returns "{}" when using Rails 5. This places the text default: {} to generated migration files. So if {} is passed to ActiveRecord::ConnectionAdapters::Quoting#_quote when bin/rails db:migrate, it no longer accepts Hash and raises TypeError. It seems to avoid unintentional YAML producing.

ref: rails/rails@aa31d21

I confirmed that this issue is reproduced with Rails 5.0.7, 5.1.6 and 5.2.0 and not reproduced in Rails 4.2.10.

Rails 4.2.10

$ bin/rails -v
Rails 4.2.10
$ bin/rails g statesman:active_record_transition Payment PaymentTransition
Running via Spring preloader in process 96164
      create  db/migrate/20180415142529_create_payment_transitions.rb
      create  app/models/payment_transition.rb
$ cat db/migrate/20180415142529_create_payment_transitions.rb
class CreatePaymentTransitions < ActiveRecord::Migration
  def change
    create_table :payment_transitions do |t|
      t.string :to_state, null: false
      t.text :metadata, default: "{}"
      t.integer :sort_key, null: false
      t.integer :payment_id, null: false
      t.boolean :most_recent, null: false

      # If you decide not to include an updated timestamp column in your transition
      # table, you'll need to configure the `updated_timestamp_column` setting in your
      # migration class.
      t.timestamps null: false
    end

    # Foreign keys are optional, but highly recommended
    add_foreign_key :payment_transitions, :payments

    add_index(:payment_transitions,
              [:payment_id, :sort_key],
              unique: true,
              name: "index_payment_transitions_parent_sort")
    add_index(:payment_transitions,
              [:payment_id, :most_recent],
              unique: true,
              where: 'most_recent',
              name: "index_payment_transitions_parent_most_recent")
  end
end
$ bin/rake db:migrate
Running via Spring preloader in process 96210
== 20180415142529 CreatePaymentTransitions: migrating =========================
-- create_table(:payment_transitions)
   -> 0.0017s
-- add_foreign_key(:payment_transitions, :payments)
   -> 0.0000s
-- add_index(:payment_transitions, [:payment_id, :sort_key], {:unique=>true, :name=>"index_payment_transitions_parent_sort"})
   -> 0.0007s
-- add_index(:payment_transitions, [:payment_id, :most_recent], {:unique=>true, :where=>"most_recent", :name=>"index_payment_transitions_parent_most_recent"})
   -> 0.0008s
== 20180415142529 CreatePaymentTransitions: migrated (0.0036s) ================

$ git diff
diff --git a/db/schema.rb b/db/schema.rb
index 001adb2..e45a6d6 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,20 @@
 #
 # It's strongly recommended that you check this file into your version control system.

-ActiveRecord::Schema.define(version: 20180415141825) do
+ActiveRecord::Schema.define(version: 20180415142529) do
+
+  create_table "payment_transitions", force: :cascade do |t|
+    t.string   "to_state",                   null: false
+    t.text     "metadata",    default: "{}"
+    t.integer  "sort_key",                   null: false
+    t.integer  "payment_id",                 null: false
+    t.boolean  "most_recent",                null: false
+    t.datetime "created_at",                 null: false
+    t.datetime "updated_at",                 null: false
+  end
+
+  add_index "payment_transitions", ["payment_id", "most_recent"], name: "index_payment_transitions_parent_most_recent", unique: true, where: "most_recent"
+  add_index "payment_transitions", ["payment_id", "sort_key"], name: "index_payment_transitions_parent_sort", unique: true

   create_table "payments", force: :cascade do |t|
     t.datetime "created_at", null: false

Rails 5.0.7 (same issue occurs in Rails 5.1.6 and 5.2.0)

$ bin/rails -v
Rails 5.0.7
$ bin/rails g statesman:active_record_transition Payment PaymentTransition
Running via Spring preloader in process 96563
      create  db/migrate/20180415143025_create_payment_transitions.rb
      create  app/models/payment_transition.rb
$ cat db/migrate/20180415143025_create_payment_transitions.rb
class CreatePaymentTransitions < ActiveRecord::Migration[5.0]
  def change
    create_table :payment_transitions do |t|
      t.string :to_state, null: false
      t.text :metadata, default: {}
      t.integer :sort_key, null: false
      t.integer :payment_id, null: false
      t.boolean :most_recent, null: false

      # If you decide not to include an updated timestamp column in your transition
      # table, you'll need to configure the `updated_timestamp_column` setting in your
      # migration class.
      t.timestamps null: false
    end

    # Foreign keys are optional, but highly recommended
    add_foreign_key :payment_transitions, :payments

    add_index(:payment_transitions,
              [:payment_id, :sort_key],
              unique: true,
              name: "index_payment_transitions_parent_sort")
    add_index(:payment_transitions,
              [:payment_id, :most_recent],
              unique: true,
              where: 'most_recent',
              name: "index_payment_transitions_parent_most_recent")
  end
end

$ bin/rake db:migrate
Running via Spring preloader in process 96623
== 20180415143025 CreatePaymentTransitions: migrating =========================
-- create_table(:payment_transitions)
rake aborted!
StandardError: An error has occurred, this and all later migrations canceled:

can't quote Hash
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/connection_adapters/abstract/quoting.rb:186:in `_quote'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/connection_adapters/sqlite3/quoting.rb:27:in `_quote'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/connection_adapters/abstract/quoting.rb:23:in `quote'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/connection_adapters/abstract/quoting.rb:111:in `quote_default_expression'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/connection_adapters/abstract/schema_creation.rb:17:in `quote_default_expression'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/connection_adapters/abstract/schema_creation.rb:113:in `add_column_options!'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/connection_adapters/sqlite3/schema_creation.rb:17:in `add_column_options!'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/connection_adapters/abstract/schema_creation.rb:34:in `visit_ColumnDefinition'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/connection_adapters/abstract/schema_creation.rb:14:in `accept'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/connection_adapters/abstract/schema_creation.rb:45:in `block in visit_TableDefinition'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/connection_adapters/abstract/schema_creation.rb:45:in `map'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/connection_adapters/abstract/schema_creation.rb:45:in `visit_TableDefinition'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/connection_adapters/abstract/schema_creation.rb:14:in `accept'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/connection_adapters/abstract/schema_statements.rb:278:in `create_table'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/migration.rb:846:in `block in method_missing'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/migration.rb:815:in `block in say_with_time'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/migration.rb:815:in `say_with_time'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/migration.rb:835:in `method_missing'
/Users/kymmt/app/db/migrate/20180415143025_create_payment_transitions.rb:3:in `change'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/migration.rb:789:in `exec_migration'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/migration.rb:773:in `block (2 levels) in migrate'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/migration.rb:772:in `block in migrate'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/connection_adapters/abstract/connection_pool.rb:398:in `with_connection'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/migration.rb:771:in `migrate'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/migration.rb:951:in `migrate'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/migration.rb:1232:in `block in execute_migration_in_transaction'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/migration.rb:1300:in `block in ddl_transaction'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/connection_adapters/abstract/database_statements.rb:232:in `block in transaction'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/connection_adapters/abstract/transaction.rb:189:in `within_new_transaction'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/connection_adapters/abstract/database_statements.rb:232:in `transaction'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/transactions.rb:211:in `transaction'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/migration.rb:1300:in `ddl_transaction'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/migration.rb:1231:in `execute_migration_in_transaction'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/migration.rb:1203:in `block in migrate_without_lock'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/migration.rb:1202:in `each'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/migration.rb:1202:in `migrate_without_lock'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/migration.rb:1152:in `migrate'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/migration.rb:1006:in `up'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/migration.rb:984:in `migrate'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/tasks/database_tasks.rb:163:in `migrate'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activerecord-5.0.7/lib/active_record/railties/databases.rake:58:in `block (2 levels) in <top (required)>'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activesupport-5.0.7/lib/active_support/dependencies.rb:287:in `load'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activesupport-5.0.7/lib/active_support/dependencies.rb:287:in `block in load'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activesupport-5.0.7/lib/active_support/dependencies.rb:259:in `load_dependency'
/Users/kymmt/app/vendor/bundle/ruby/2.5.0/gems/activesupport-5.0.7/lib/active_support/dependencies.rb:287:in `load'

json and jsonb types for Postgres allow to use {} in migration files, but I think it is better to use "{}" as the default value to be database-agnostic. And String#inspect is just for debugging, so I think it is better to use String#dump.

kymmt90 · 2018-04-19T10:18:57Z

@timrogers How about this PR?

timrogers

It feels like we should ideally have a test that checks our generated migrations work. It feels like a weak spot right now. What do you think?

timrogers · 2018-04-19T10:40:42Z

lib/generators/statesman/generator_helpers.rb

@@ -46,7 +46,7 @@ def database_supports_partial_indexes?
    end

    def metadata_default_value


We might as well drop this method and put the correct value directly in the template if we're convinced it works everywhere.

kymmt90 · 2018-04-19T16:32:48Z

Thank you for your review. I agree with what you are saying, so I will add tests (working in progress...). And I will put \"{}\" to migration templates directly because metadata_default_value is currently used only in those templates.

timrogers

#317 has been merged, so if you give this a rebase, it may well pass now.

I wonder if we can have tests that actually run the migration? That would be excellent.

timrogers · 2018-04-30T08:19:45Z

lib/generators/statesman/templates/update_migration.rb.erb

@@ -1,7 +1,7 @@
 class AddStatesmanTo<%= migration_class_name %> < ActiveRecord::Migration<%= "[#{ActiveRecord::Migration.current_version}]" if Statesman::Utils.rails_5_or_higher? %>
  def change
    add_column :<%= table_name %>, :to_state, :string, null: false
-    add_column :<%= table_name %>, :metadata, :text<%= ", default: #{metadata_default_value}" unless mysql? %>
+    add_column :<%= table_name %>, :metadata, :text<%= ", default: \"{}\"" unless mysql? %>


Are we sure that it's just MySQL that works this way? Would we be better off with a whitelist of adapters that need this?

It makes sense to introduce a whitelist of adapters for more flexibility. However, now Statesman is tested for using Active Record with Postgres/MySQL and only MySQL needs this fix, so I think it is enough to use this fix as-is at now.

timrogers · 2018-04-30T08:21:03Z

spec/spec_helper.rb

@@ -27,6 +27,10 @@ def connection_failure
    end
  end

+  def mysql?


Is this getting defined on Object? I'm not sure what happens with this stuff inside RSpec.configure?

This is getting defined as the class method of example groups. But I noticed that only doing extend Statesman::GeneratorHelpers in describe blocks enables to use helper methods, so I fixed it in that clearer way.

Fixed GeneratorHelpers::migration_class_name because a migration file name and its migration class name must match each other

timrogers suggested changes Apr 19, 2018

View reviewed changes

kymmt90 force-pushed the fix-metadata-default-value branch from 999e955 to 7115a10 Compare April 19, 2018 14:27

kymmt90 force-pushed the fix-metadata-default-value branch 4 times, most recently from 8c6431e to cd28db0 Compare April 24, 2018 13:47

kymmt90 mentioned this pull request Apr 28, 2018

Set mysql2 version '~> 0.4.0' to fix rails-429-mysql CI builds #317

Merged

timrogers suggested changes Apr 30, 2018

View reviewed changes

kymmt90 force-pushed the fix-metadata-default-value branch from cd28db0 to 441cb13 Compare April 30, 2018 10:43

kymmt90 force-pushed the fix-metadata-default-value branch 2 times, most recently from 1e81f29 to c643460 Compare May 12, 2018 07:12

kymmt90 added 2 commits June 16, 2018 19:29

Set the default value of metadata as empty hash string

e1b4b2f

Check migrations properly work in tests

47a3158

Fixed GeneratorHelpers::migration_class_name because a migration file name and its migration class name must match each other

kymmt90 force-pushed the fix-metadata-default-value branch from 0e6686d to 47a3158 Compare June 16, 2018 10:31

rubocop -a

9ef9740

kymmt90 closed this Oct 22, 2019

kymmt90 deleted the fix-metadata-default-value branch October 22, 2019 09:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use empty hash string for metadata default value in both Rails 4 and 5 #316

Use empty hash string for metadata default value in both Rails 4 and 5 #316

kymmt90 commented Apr 17, 2018

kymmt90 commented Apr 19, 2018

timrogers left a comment

timrogers Apr 19, 2018

kymmt90 commented Apr 19, 2018

timrogers left a comment

timrogers Apr 30, 2018

kymmt90 May 12, 2018

timrogers Apr 30, 2018

kymmt90 May 12, 2018

		@@ -46,7 +46,7 @@ def database_supports_partial_indexes?
		end

		def metadata_default_value

Use empty hash string for metadata default value in both Rails 4 and 5 #316

Use empty hash string for metadata default value in both Rails 4 and 5 #316

Conversation

kymmt90 commented Apr 17, 2018

kymmt90 commented Apr 19, 2018

timrogers left a comment

Choose a reason for hiding this comment

timrogers Apr 19, 2018

Choose a reason for hiding this comment

kymmt90 commented Apr 19, 2018

timrogers left a comment

Choose a reason for hiding this comment

timrogers Apr 30, 2018

Choose a reason for hiding this comment

kymmt90 May 12, 2018

Choose a reason for hiding this comment

timrogers Apr 30, 2018

Choose a reason for hiding this comment

kymmt90 May 12, 2018

Choose a reason for hiding this comment