ENV variable usage in migrations

[plribeiro3000]

So, i just upgraded to version 3.0.0 and got an issue that does not exist on version 2.0.0.

We have a codebase with thousands of migrations and several tables. Over the years we got to a solution to help us maintain all those migrations and facilitate deployments and migrations through heroku for different database sizes (Yes, we have multiple databases as some big clients asked and payed for it)

So we got to work with different table sizes using ENV vars like so:

# frozen_string_literal: true

class AddUserIdentifiersCompanyUniqueIndex < ActiveRecord::Migration[6.0]
  disable_ddl_transaction!
  set_statement_timeout(statement_timeout)

  def up
    return if index_exists?(:user_identifiers, %i[company_id value])

    add_index :user_identifiers, %i[company_id value], algorithm: :concurrently, unique: true, where: 'subsidiary_id is null'
  end

  def down
    return unless index_exists?(:user_identifiers, %i[company_id value])

    remove_index :user_identifiers, %i[company_id value]
  end

  def self.statement_timeout
    ENV.fetch('MIGRATION_20201014221845', 250)
  end
end

Any ideas on why it does not work on version 3.0.0?

[nickcampbell18]

I'm not sure. Apologies, we seem to have bungled the 3.0 release a bit - I've just pushed the tag for it. Here's the diff between the versions: https://github.com/gocardless/activerecord-safer_migrations/compare/v2.0.0...v3.0.0

From here it looks like we only applied Rubocop style adjustments and updated some of the dependencies, but clearly something else has gone awry...

Looking at the code snippet: does set_statement_timeout run before you've overridden the class method? i.e. statement_timeout on line 5 might evaluate to the class attribute rather than the overriden method at the end, because of the way that this code is executed as it is parsed? What if you try overriding it before set_statement_timeout is called? Or, inline it like this: set_statement_timeout(ENV.fetch(..))

[plribeiro3000]

Hello @nickcampbell18 . Thanks for the fast reply.

I did try some options without success:

• Define statement_timeout as an instance method
• Use the content of the statement_timeout method as an argument to set_statement_timeout (set_statement_timeout(ENV.fetch('MIGRATION_20201014221845', 250))
• Use Env['MIGRATION_20201014221845'] inside set_statement_timeout

All of them failed with the result being nil which explodes on the gem verification for .zero?.

As my first thought i went into figuring out how rails 6.0.3.4 works or that it could be related to ruby 2.7.2. But the only thing that made it back to work was to rollback from activerecord-safer_migrations 3.0.0 to 2.0.0.

Dunno what it could be as it seems weirdo to me what is happening exactly. If you give me some directions i can try to give it a try.

EDIT: Could it be related to the way ruby define instance and class methods besides the new rails loading mechanism?

[nickcampbell18]

OK so it turns out there were two separate issues here!

Your first code was broken as I expected, because statement_timeout == nil when the set_statement_timeout line evaluates. The exception is because of this Rubocop change we changed statement_timeout == 0 to statement_timeout.zero?: however nil doesn't respond to .zero? and it throws an exception.

The code in your follow-up comments would have worked except for one crucial change - you need to cast your environment variable to an integer (.to_i)! Everything accessed through ENV[...] gets cast as a String, which (again) doesn't have a .zero? method we can use and it blows up.

Here is some code which works (I ran it locally to test on a fresh Rails install):

class Testing < ActiveRecord::Migration[6.0]
  disable_ddl_transaction!
  set_statement_timeout(
    ENV
      .fetch("MIGRATION_20201014221845", 250)
      .to_i
  )

  def up
    create_table :foos
  end

  def down
    drop_table :foos
  end
end

$ MIGRATION_20201014221845=99 rails db:migrate

== 20201019151748 Testing: migrating ==========================================
-- set_setting(:lock_timeout, 750)
-- set_setting(:statement_timeout, 99)
-- create_table(:foos)
   -> 0.0039s
-- set_setting(:statement_timeout, 0)
-- set_setting(:lock_timeout, 0)
== 20201019151748 Testing: migrated (0.0109s) =================================

[plribeiro3000]

Cool. Will check it out asap!

Thanks for the detailed explanation.

[nickcampbell18]

Gonna mark this issue as closed but please reopen if you encounter further problems!

[plribeiro3000]

@nickcampbell18 I just checked what you suggested with no luck.

What you said about casting to integer makes sense but even if i add to_i to my statement_timeout class name if still returns nil.

It seems to be related to how the method set_statement_timeout is evaluating my method.

So first i thought about statement_timeout being used internally by this gem and found this

Tried changing to asd and then i got a different issue.

It does not matter if i define as a class method or as an instance method, it always raise NoMethodError: undefined method asd' for #ActiveRecord::Migration:0x00007f9edbac1cb0`

So in the end what differs from version 2.0.0 and 3.0.0 is how it evaluates methods in my migration. It seems that in 2.0.0 it correctly evaluates whether in 3.0.0 it does not.

Ideas?

EDIT: Can't reopen the issue. It seems this repo is configured to not allow it.

[plribeiro3000]

Ok, i figured out how to fix my issue.

As i stated in my last comment set_timeout is not being executed in the context of the migration but rather ActiveRecord::Migration so i suspect it related to the way the code is being inject into ActiveRecord.

Since i was always defining a class method called statement_timeout and then using the DSL set_statement_timeout passing the method like:

class AddUserIdentifiersCompanyUniqueIndex < ActiveRecord::Migration[6.0]
  disable_ddl_transaction!
  set_statement_timeout(statement_timeout)

  def up
    return if index_exists?(:user_identifiers, %i[company_id value])

    add_index :user_identifiers, %i[company_id value], algorithm: :concurrently, unique: true, where: 'subsidiary_id is null'
  end

  def down
    return unless index_exists?(:user_identifiers, %i[company_id value])

    remove_index :user_identifiers, %i[company_id value]
  end

  def self.statement_timeout
    ENV.fetch('MIGRATION_20201014221845', 250)
  end
end

it was not getting my method definition but rather the attribute reader of ActiveRecord::Migration instead which was blank. And thats why i got the second error of undefined method when i used anything but statement_timeout.

When i removed the DSL, it worked correctly.

Now i'm just not sure if it is running in the context of ActiveRecord::Migration because of the way this gem is loading inside ActiveRecord or if is how it is supposed to work anyway.

The solution (which i don't like much because seems to much of magic going on) is to remove the DSL:

class AddUserIdentifiersCompanyUniqueIndex < ActiveRecord::Migration[6.0]
  disable_ddl_transaction!

  def up
    return if index_exists?(:user_identifiers, %i[company_id value])

    add_index :user_identifiers, %i[company_id value], algorithm: :concurrently, unique: true, where: 'subsidiary_id is null'
  end

  def down
    return unless index_exists?(:user_identifiers, %i[company_id value])

    remove_index :user_identifiers, %i[company_id value]
  end

  def self.statement_timeout
    ENV.fetch('MIGRATION_20201014221845', 250)
  end
end

[plribeiro3000]

BTW, i got it working without type casting ENV var to integer @nickcampbell18.

I did some diggings to try to understand the behaviour and could not reproduce the method being executed from the context of the parent:

class Vehicle
  def self.wheels(wheels)
    puts self.name
    @wheels = wheels
  end

  def wheels
    puts self.class.name
    self.class.instance_variable_get(:@wheels)
  end
end

class Car < Vehicle
  wheels 4
end

# Car
#  => 4

class Motorcycle < Vehicle
  wheels 2
end

# Motorcycle
#  => 2

Car.new.wheels # Car
Motorcycle.new.wheels # Motorcycle

Is this behaviour of executing in the context of ActiveRecord::Migration intended?