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

Fix `bin/rails db:migrate` with specified `VERSION` #30714

Merged
merged 1 commit into from Nov 7, 2017

Conversation

Projects
None yet
5 participants
@bogdanvlviv
Contributor

bogdanvlviv commented Sep 25, 2017

Fix bin/rails db:migrate with specified VERSION.
bin/rails db:migrate with empty VERSION behaves as without VERSION.
Check a format of VERSION: Allow a migration version number
or name of a migration file. Raise error if format of VERSION is invalid.
Raise error if target migration doesn't exist.

Related to #30707

/cc @matthewd

@rails-bot

This comment has been minimized.

rails-bot commented Sep 25, 2017

r? @eileencodes

(@rails-bot has picked a reviewer for you, use r? to override)

activerecord/lib/active_record/migration.rb Outdated
result = execute_migration_in_transaction(migration, @direction)
if invalid_target?
raise UnknownMigrationVersionError.new(@target_version)
end

This comment has been minimized.

@eileencodes

eileencodes Sep 29, 2017

Member

What does the error message say for the UnknownMigrationVersionError? I'm wondering if it's user friendly enough like "0.1.1" is an unknown migration number please run with VERSION=123. I'm concerned about inexperienced users not realizing their VERSION env var is set elsewhere and how to set it for the Rails migrations.

@eileencodes

This comment has been minimized.

Member

eileencodes commented Sep 29, 2017

I think this is fine as a bandaid fix, but I'm not sure whether this is a good long term solution. Think about this usecase:

  1. VERSION is set in an env var
  2. User goes to run migration and doesn't realize that VERSION is set
  3. They can't run rake db:migrate with VERSION manually because they don't want a version, they want to run a bunch of migrations at once
  4. What do they do now? Unset VERSION to run the migrations and then reset it?

I worry that this PR instead will completely block migrations without a clear path forward causing a lot of friction in the migration process.

@bogdanvlviv

This comment has been minimized.

Contributor

bogdanvlviv commented Oct 5, 2017

Thanks, @eileencodes. This PR doesn't fix the issue #30707 (but related to this issue), it just fix our current implementation that we have.

Currently, if run rails db:migrate and set VERSION as a negative number, it'll revert all migrations. It is a bug and this PR just fixes it.

Example:
Before:

$ rails db:migrate:status

 Status   Migration ID    Migration Name
--------------------------------------------------
   up     20171005143525  One
   up     20171005143538  Two
   up     20171005143556  Three

$ rails db:migrate VERSION=1
rails aborted!
ActiveRecord::UnknownMigrationVersionError:

No migration with version number 1.

# There is bug
$ rails db:migrate VERSION=-1
ubuntu@ubuntu-xenial:/active_projects/zz$ rails db:migrate VERSION=-1
== 20171005143556 Three: reverting ============================================
== 20171005143556 Three: reverted (0.4047s) ===================================

== 20171005143538 Two: reverting ==============================================
== 20171005143538 Two: reverted (0.0000s) =====================================

== 20171005143525 One: reverting ==============================================
== 20171005143525 One: reverted (0.0000s) =====================================

After:

$ rails db:migrate:status

 Status   Migration ID    Migration Name
--------------------------------------------------
   up     20171005143525  One
   up     20171005143538  Two
   up     20171005143556  Three

$ rails db:migrate VERSION=1
rails aborted!
ActiveRecord::UnknownMigrationVersionError:

No migration with version number 1.

# Fixed
$ rails db:migrate VERSION=-1
rails aborted!

About the case:

  • VERSION is set in an env var
  • User goes to run migration and doesn't realize that VERSION is set
  • They can't run rake db:migrate with VERSION manually because they don't want a version, they want
    to run a bunch of migrations at once
    What do they do now? Unset VERSION to run the migrations and then reset it?

I think the main reason of the problem in common name of environment variable that we use VERSION. It is a too common name and not only Rails can pretend for this variable. So I would recommend to deprecate using of VERSION in favor of new RAILS_MIGRATION_VERSION for Rails 5.2 and remove for Rails 6.0. Also, it'll look more consistent with other environment variables like RAILS_CACHE_ID, RAILS_MASTER_KEY, RAILS_ENV.

What do you think about deprecation of using VERSION in favor of RAILS_MIGRATION_VERSION?
Would love to hear your thought about it or any better solutions if exist. If my solution is good, I'll happy to open PR that will do it.
Thanks!

@matthewd

This comment has been minimized.

Member

matthewd commented Oct 5, 2017

Can we validate the supplied version before we call to_i, so that 0.1.2 will fail? (For better compatibility with what people might be doing, we should probably ignore anything following an underscore, though: AIUI, passing a full migration filename will currently work, so let's not break that.)

Renaming the variable is not appropriate: unlike the others you mentioned, it's intended to be set on the rake command line; the others are intended to suitable for setting more globally across a session. (And I believe that's the stronger convention in general: the more broadly a variable is visible, the more specifically it should be named.) Naming such a redundant variable on a db:migrate command would be difficult (more prone to typos), and ugly.

It would also be an unnecessary stopgap: the ability to pass proper arguments to commands, instead of using ugly env vars, is a large motivation for our ongoing effort to convert rake-based commands to thor. Once we've done db:migrate, this can use --target-version 123 or something instead.

We've been using VERSION for 10 years: it doesn't seem worth the disruption to make everyone adjust their invocations when we know it'll change again soon, for a complaint I don't recall ever having seen before. Stricter input validation is a proportional response... and we can focus any remaining energy on the Command-ification effort.

@matthewd

This comment has been minimized.

Member

matthewd commented Oct 5, 2017

If someone does have VERSION set in an env var, they would currently have the problem described in #30707 -- they'd never be able to migrate properly, because we'd always be aiming for whatever version we thought it was asking for. So a change from "valiantly attempting to do something they didn't intend" into "refusing to act" won't affect anyone, I don't think: either they've already worked around the variable being set, or such people don't exist.

I haven't checked, but I assume they can run the command with rails db:migrate VERSION= (i.e., blank), and that should be treated the same as if the variable wasn't set at all. That will continue to work with any change we make here. (And if it doesn't currently, let's make it.)

@bogdanvlviv

This comment has been minimized.

Contributor

bogdanvlviv commented Oct 6, 2017

@matthewd I think I got your point. I just added the changes that check VERSION.
It prevents reverting all migrations if VERSION looks like "negative number", 0.1.11, unknown etc.
If user needs revert all migrations, he should set VERSION to 0 as it is documented.
(Not the changes fix case from #30707)

This PR shouldn't break our current implementation that we have and AIUI we can apply new strict behavior(See #30714 (comment)).

Could you please review and provide feedback about behavior that this PR provides?

If this behavior doesn't fit, I'll try to find another implementation 😄 but I guess we are moving in the right direction.

Thank you for the response! ❤️

activerecord/lib/active_record/tasks/database_tasks.rb Outdated
scope.blank? || scope == migration.scope
end
ActiveRecord::Base.clear_cache!
ensure
Migration.verbose = verbose_was
end
def check_target_version
if ENV["VERSION"] && ENV["VERSION"].empty?
raise "Empty VERSION provided"

This comment has been minimized.

@matthewd

matthewd Oct 6, 2017

Member

As I mentioned, I think an empty VERSION should be treated just the same as when it's not set at all, so people can easily override a more widely-set variable. Before we kill it though, can you see any history on why we added the existing emptiness check?

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Oct 8, 2017

Contributor

Checking for empties of VERSION is added by #28485 because db:migrate:redo, db:migrate:up, db:migrate:down require VERSION,
but db:migrate doesn't require VERSION, so I think it db:migrate with empty VERSION should behave as without VERSION.

activerecord/lib/active_record/tasks/database_tasks.rb Outdated
raise "Empty VERSION provided"
end
if target_version == 0 && ENV["VERSION"].strip.length != 1

This comment has been minimized.

@matthewd

matthewd Oct 6, 2017

Member

This sounds like an overly-special case. 1.2.3 should raise too, even if there's a migration number 1.

activerecord/lib/active_record/tasks/database_tasks.rb Outdated
if target_version == 0 && ENV["VERSION"].strip.length != 1
raise(
"Invalid format of target version: \n\n `VERSION=#{ENV['VERSION']}`"

This comment has been minimized.

@matthewd

matthewd Oct 6, 2017

Member

I don't think we need the fancy spacing here

@@ -206,6 +278,46 @@ class AMigration < ActiveRecord::Migration::Current
end
end
test "raise error on any move when target migration does not exist" do
app_file "db/migrate/01_one_migration.rb", <<-MIGRATION

This comment has been minimized.

@matthewd

matthewd Oct 6, 2017

Member

I'd like to see a test somewhere (assuming we don't already have one, though I haven't checked) to ensure that VERSION=01_one_migration.rb will work, even after we make VERSION=1.2 fail.

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Oct 8, 2017

Contributor

I'd like to see a test somewhere (assuming we don't already have one, though I haven't checked) to ensure that VERSION=01_one_migration.rb will work, even after we make VERSION=1.2 fail.

I just implemented this behavior and added test cases.

@bogdanvlviv

This comment has been minimized.

Contributor

bogdanvlviv commented Oct 16, 2017

@matthewd What do you think about current behavior that is implemented by this PR?

scope.blank? || scope == migration.scope
end
ActiveRecord::Base.clear_cache!
ensure
Migration.verbose = verbose_was
end
def check_target_version
if target_version && !(Migration::MigrationFilenameRegexp.match?(ENV["VERSION"]) || /\A\d+\z/.match?(ENV["VERSION"]))

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Oct 19, 2017

Contributor

@matthewd What do you think about this condition?

@bogdanvlviv

This comment has been minimized.

Contributor

bogdanvlviv commented Oct 30, 2017

let me know if there is something i should to do.

@bogdanvlviv

This comment has been minimized.

Contributor

bogdanvlviv commented Nov 6, 2017

Could you please provide any feedback about this PR?

activerecord/lib/active_record/tasks/database_tasks.rb Outdated
end
def target_version
return ENV["VERSION"].to_i if ENV["VERSION"] && !ENV["VERSION"].empty?

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 6, 2017

Member

✂️ the return

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Nov 6, 2017

Contributor

fixed

Fix `bin/rails db:migrate` with specified `VERSION`
Ensure that `bin/rails db:migrate` with specified `VERSION` reverts
all migrations only if `VERSION` is `0`.
Raise error if target migration doesn't exist.

@rafaelfranca rafaelfranca merged commit 212481b into rails:master Nov 7, 2017

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bogdanvlviv bogdanvlviv deleted the bogdanvlviv:fix-rails_db_migrate_VERSION branch Nov 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment