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

ActiveRecord::Migration.maintain_test_schema! doesn't work with structure.sql #15394

Merged
merged 2 commits into from Jun 12, 2014

Conversation

Projects
None yet
9 participants
@morgoth
Member

morgoth commented Jun 1, 2014

Introduced in rails 4.1.
Checked using sqlite3 and postgres.

At the end of calling ActiveRecord::Migration.maintain_test_schema! this method is called:

def load_schema(format = ActiveRecord::Base.schema_format, file = nil)
which later, after choosing database adapter calls method, in example on sqlite3 adapter:

So at the very end it runs: sqlite3 db/test.sqlite3 < db/structure.sql

This however can result in error like:

Error: near line 1: table "schema_migrations" already exists
Error: near line 2: index unique_schema_migrations already exists
Error: near line 3: table "users" already exists
Error: near line 4: UNIQUE constraint failed: schema_migrations.version

This error can happen when we created migration to add column to existing table.

It looks like it's necessary to always purge database before loading sql file and adding:

purge(current_config)
create(current_config)

before line

structure_load(current_config, file)
fixes the issue.
I'm not sure if that is the best place however.

Similar issue was noted in #13528 (comment) by @kevintyll

Please, let me know if the issue is unclear, I can submit example app then.
If you think that's the correct way of fixing it, I can prepare a PR.

Addressing to @jonleighton as he is the author of this feature.

@rafaelfranca rafaelfranca added this to the 4.1.3 milestone May 28, 2014

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny May 29, 2014

Member

I can confirm this issue. Have experienced it in my own applications.

Member

senny commented May 29, 2014

I can confirm this issue. Have experienced it in my own applications.

@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Jun 1, 2014

Member

@senny @rafaelfranca I added PR that fixes the issue. Please, let me know if some parts can be improved.

Member

morgoth commented Jun 1, 2014

@senny @rafaelfranca I added PR that fixes the issue. Please, let me know if some parts can be improved.

@szimek

This comment has been minimized.

Show comment
Hide comment
@szimek

szimek Jun 10, 2014

Contributor

Got this issue as well.

Contributor

szimek commented Jun 10, 2014

Got this issue as well.

@907th

This comment has been minimized.

Show comment
Hide comment
@907th

907th Jun 11, 2014

Contributor

Confirm

Contributor

907th commented Jun 11, 2014

Confirm

@senny senny self-assigned this Jun 11, 2014

@senny

View changes

Show outdated Hide outdated activerecord/lib/active_record/tasks/database_tasks.rb Outdated
@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Jun 11, 2014

Member

@morgoth I am working on foreign key support for Rails 4.2. Once schema.rb will be able to create foreign key, we can no longer rely on force: true from the create_table statements. I think we can use the same solution for that problem as well. Let's also purge when loading from schema.rb.

Member

senny commented Jun 11, 2014

@morgoth I am working on foreign key support for Rails 4.2. Once schema.rb will be able to create foreign key, we can no longer rely on force: true from the create_table statements. I think we can use the same solution for that problem as well. Let's also purge when loading from schema.rb.

@senny senny referenced this pull request Jun 11, 2014

Merged

Basic support for adding and removing foreign keys #15606

7 of 7 tasks complete
@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Jun 11, 2014

Member

@senny should I add purge when loading from schema.rb in this PR? Probably it will be backported to 4.1, so this change wont be needed there.

Member

morgoth commented Jun 11, 2014

@senny should I add purge when loading from schema.rb in this PR? Probably it will be backported to 4.1, so this change wont be needed there.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Jun 11, 2014

Member

@morgoth At this point I do think we should backport the purge for schema.rb as well. I've seen a couple of situations where the test db got out of sync and couldn't be fixed by the automatic maintenance. The last resort was to rake db:test:prepare which is deprecated. I know this would be a change in behavior but it will make the process more transparent for the user.

/cc @rafaelfranca

Member

senny commented Jun 11, 2014

@morgoth At this point I do think we should backport the purge for schema.rb as well. I've seen a couple of situations where the test db got out of sync and couldn't be fixed by the automatic maintenance. The last resort was to rake db:test:prepare which is deprecated. I know this would be a change in behavior but it will make the process more transparent for the user.

/cc @rafaelfranca

@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Jun 11, 2014

Member

@senny I updated PR with your suggeestions

Member

morgoth commented Jun 11, 2014

@senny I updated PR with your suggeestions

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Jun 11, 2014

Member

@morgoth thanks man 💛 I'll need some time to make sure we don't run into regressions. Will ping you back by tomorrow.

Member

senny commented Jun 11, 2014

@morgoth thanks man 💛 I'll need some time to make sure we don't run into regressions. Will ping you back by tomorrow.

assert_successful_test_run('models/user_test.rb')
end
test "sql structure migrations when adding column to existing table" do

This comment has been minimized.

@senny

senny Jun 12, 2014

Member

what promted the addition of this test? Is it actually failing when we remove the purge?

@senny

senny Jun 12, 2014

Member

what promted the addition of this test? Is it actually failing when we remove the purge?

This comment has been minimized.

@morgoth

morgoth Jun 12, 2014

Member

Yes, exactly.
To see it failing we must change structure and try to load it to existing database.

@morgoth

morgoth Jun 12, 2014

Member

Yes, exactly.
To see it failing we must change structure and try to load it to existing database.

This comment has been minimized.

@senny

senny Jun 12, 2014

Member

is this the absolute minimal test-case to reproduce the failure? There's much of overhead involved. Would be great if we can shrink it further to only guard against the purge case.

@senny

senny Jun 12, 2014

Member

is this the absolute minimal test-case to reproduce the failure? There's much of overhead involved. Would be great if we can shrink it further to only guard against the purge case.

This comment has been minimized.

@morgoth

morgoth Jun 12, 2014

Member

I removed 2 lines, but it doesn't help much ;-).
I tried to follow user steps and I think it is minimal test case this way.

It would simplify if we create db with first version of structure.sql without all these steps, but I don't how to do that in this test.

@morgoth

morgoth Jun 12, 2014

Member

I removed 2 lines, but it doesn't help much ;-).
I tried to follow user steps and I think it is minimal test case this way.

It would simplify if we create db with first version of structure.sql without all these steps, but I don't how to do that in this test.

Fixed automatic maintaining test schema to properly handle sql struct…
…ure schema format.

Additionally:
* It changes `purge` task on `sqlite3` adapter to recreate database file, to
be consistent with other adapters.
* Adds `purge` step when loading from `schema.rb`

@senny senny merged commit ad42aae into rails:master Jun 12, 2014

senny added a commit that referenced this pull request Jun 12, 2014

Merge pull request #15394 from morgoth/fix-automatic-maintaining-test…
…-schema-for-sql-format

ActiveRecord::Migration.maintain_test_schema! doesn't work with structure.sql

Conflicts:
	activerecord/CHANGELOG.md
@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Jun 12, 2014

Member

@morgoth thank you, this will solve my issues with foreign keys and makes the automatic schema maintenance more predictable.

It will add ~100ms penalty when running the tests and the schema migration needs to be performed. As this only happens here and then I don't think this is a big issue.

@rafaelfranca I'd like to backport this patch as is. This means the behavior for schema.rb would change as well. As foreign keys are coming with 4.2 this is not strictly required with 4.1. However I do think that it should work the same way wether you are using schema.rb or structure.sql.

Member

senny commented Jun 12, 2014

@morgoth thank you, this will solve my issues with foreign keys and makes the automatic schema maintenance more predictable.

It will add ~100ms penalty when running the tests and the schema migration needs to be performed. As this only happens here and then I don't think this is a big issue.

@rafaelfranca I'd like to backport this patch as is. This means the behavior for schema.rb would change as well. As foreign keys are coming with 4.2 this is not strictly required with 4.1. However I do think that it should work the same way wether you are using schema.rb or structure.sql.

@morgoth morgoth deleted the morgoth:fix-automatic-maintaining-test-schema-for-sql-format branch Jun 12, 2014

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jun 13, 2014

Member

@senny 👍 for backporting to 4-1-stable as it is. @carlosantoniodasilva will be happy 😄

Member

rafaelfranca commented Jun 13, 2014

@senny 👍 for backporting to 4-1-stable as it is. @carlosantoniodasilva will be happy 😄

senny added a commit that referenced this pull request Jun 14, 2014

Merge pull request #15394 from morgoth/fix-automatic-maintaining-test…
…-schema-for-sql-format

ActiveRecord::Migration.maintain_test_schema! doesn't work with structure.sql

Conflicts:
	activerecord/CHANGELOG.md

Conflicts:
	activerecord/CHANGELOG.md
@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jun 29, 2014

Member

This PR breaks rake db:schema:load for MySQL for development and production. I will revert for now.

Member

dhh commented Jun 29, 2014

This PR breaks rake db:schema:load for MySQL for development and production. I will revert for now.

dhh added a commit that referenced this pull request Jun 29, 2014

Revert "Merge pull request #15394 from morgoth/fix-automatic-maintain…
…ing-test-schema-for-sql-format"

This reverts commit 22e9a91. See discussion on 22e9a91 for details.

Conflicts:
	activerecord/CHANGELOG.md
@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Jul 2, 2014

Member

@jonleighton might know what's going on. While thinking about it, this could also be related to spring. (just guessing though).

Member

senny commented Jul 2, 2014

@jonleighton might know what's going on. While thinking about it, this could also be related to spring. (just guessing though).

rafaelfranca added a commit to rafaelfranca/omg-rails that referenced this pull request Jul 2, 2014

Revert "Merge pull request rails#15394 from morgoth/fix-automatic-mai…
…ntaining-test-schema-for-sql-format"

This reverts commit 46139d3, reversing
changes made to 8f24787.

Conflicts:
	activerecord/CHANGELOG.md

rafaelfranca added a commit to rafaelfranca/omg-rails that referenced this pull request Jul 2, 2014

Revert "Revert "Merge pull request rails#15394 from morgoth/fix-autom…
…atic-maintaining-test-schema-for-sql-format""

This reverts commit 5c87b5c.
@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton Jul 2, 2014

Member

I don't know sorry. You can exclude spring by setting the DISABLE_SPRING env var.

Member

jonleighton commented Jul 2, 2014

I don't know sorry. You can exclude spring by setting the DISABLE_SPRING env var.

senny added a commit that referenced this pull request Jul 24, 2014

fix, mysql `db:purge` respects `Rails.env`.
Previously this method always established a connection to the test database.
This resulted in buggy behavior when combined with other tasks like
`bin/rake db:schema:load`.

This was one of the reasons why #15394 (22e9a91)
was reverted:

> I’ve replicated it on a new app by the following commands: 1) rails
  generate model post:title, 2) rake db:migrate, 3) rake
  db:schema:load, 4) rails runner ‘puts Post.first’. The last command
  goes boom. Problem is that rake db:schema:load wipes the database,
  and then doesn’t actually restore it. This is all on MySQL. There’s
  no problem with SQLite.

  -- DHH

22e9a91#commitcomment-6834245

senny added a commit that referenced this pull request Jul 25, 2014

Revert "Revert "Merge pull request #15394 from morgoth/fix-automatic-…
…maintaining-test-schema-for-sql-format""

This reverts commit 5c87b5c.

senny added a commit that referenced this pull request Jul 28, 2014

schema rake tasks are specific about the configuration to act on.
The rake tasks and the `DatabaseTakss` adapter classes used to
assume a configuration at some places. This forced the rake
tasks to establish a specific connection before calling into
`load_schema`.

After #15394 this started to cause issues because it could
`purge` the wrong database before loading the schema.

senny added a commit that referenced this pull request Aug 5, 2014

Revert "Revert "Merge pull request #15394 from morgoth/fix-automatic-…
…maintaining-test-schema-for-sql-format""

This reverts commit 5c87b5c.

senny added a commit that referenced this pull request Aug 5, 2014

schema rake tasks are specific about the configuration to act on.
The rake tasks and the `DatabaseTakss` adapter classes used to
assume a configuration at some places. This forced the rake
tasks to establish a specific connection before calling into
`load_schema`.

After #15394 this started to cause issues because it could
`purge` the wrong database before loading the schema.

senny added a commit that referenced this pull request Aug 6, 2014

Revert "Revert "Merge pull request #15394 from morgoth/fix-automatic-…
…maintaining-test-schema-for-sql-format""

This reverts commit 5c87b5c.

senny added a commit that referenced this pull request Aug 6, 2014

schema rake tasks are specific about the configuration to act on.
The rake tasks and the `DatabaseTakss` adapter classes used to
assume a configuration at some places. This forced the rake
tasks to establish a specific connection before calling into
`load_schema`.

After #15394 this started to cause issues because it could
`purge` the wrong database before loading the schema.
@jaredbeck

This comment has been minimized.

Show comment
Hide comment
@jaredbeck

jaredbeck Aug 8, 2014

Contributor

@senny This will be fixed in 4.2?

Here's a workaround for postgres users who want to use maintain_test_schema! in the meantime.

# Put in spec_helper, before call to ActiveRecord::Migration.maintain_test_schema!
if ActiveRecord::Migrator.needs_migration?
  puts('Needs migrations, purging test database ..')
  ActiveRecord::Tasks::PostgreSQLDatabaseTasks
    .new(ActiveRecord::Base.configurations.fetch('test'))
    .purge
end
Contributor

jaredbeck commented Aug 8, 2014

@senny This will be fixed in 4.2?

Here's a workaround for postgres users who want to use maintain_test_schema! in the meantime.

# Put in spec_helper, before call to ActiveRecord::Migration.maintain_test_schema!
if ActiveRecord::Migrator.needs_migration?
  puts('Needs migrations, purging test database ..')
  ActiveRecord::Tasks::PostgreSQLDatabaseTasks
    .new(ActiveRecord::Base.configurations.fetch('test'))
    .purge
end
@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Aug 15, 2014

Member

@jaredbeck yes, this is fixed on master and will be released in 4.2

Member

senny commented Aug 15, 2014

@jaredbeck yes, this is fixed on master and will be released in 4.2

johncarney referenced this pull request in grosser/parallel_tests Aug 19, 2014

senny added a commit that referenced this pull request Aug 27, 2014

fix, mysql `db:purge` respects `Rails.env`.
Previously this method always established a connection to the test database.
This resulted in buggy behavior when combined with other tasks like
`bin/rake db:schema:load`.

This was one of the reasons why #15394 (22e9a91)
was reverted:

> I’ve replicated it on a new app by the following commands: 1) rails
  generate model post:title, 2) rake db:migrate, 3) rake
  db:schema:load, 4) rails runner ‘puts Post.first’. The last command
  goes boom. Problem is that rake db:schema:load wipes the database,
  and then doesn’t actually restore it. This is all on MySQL. There’s
  no problem with SQLite.

  -- DHH

22e9a91#commitcomment-6834245

Conflicts:
	activerecord/CHANGELOG.md
@metaskills

This comment has been minimized.

Show comment
Hide comment
@metaskills

metaskills Oct 3, 2014

Contributor

Watching this issue... in v4.2.0.beta2 I have noticed that my test db never get's populated due to maintain_test_schema! calling load_schema_if_pending! and in my situation that returns false because I have an empty db/migrate directory even tho I have a valid schema.rb and dev structure. Not sure if pending migrations should be the only method that tells if you need a schema load for test env?

Contributor

metaskills commented Oct 3, 2014

Watching this issue... in v4.2.0.beta2 I have noticed that my test db never get's populated due to maintain_test_schema! calling load_schema_if_pending! and in my situation that returns false because I have an empty db/migrate directory even tho I have a valid schema.rb and dev structure. Not sure if pending migrations should be the only method that tells if you need a schema load for test env?

calebthompson added a commit to calebthompson/rails that referenced this pull request Dec 4, 2014

Remove environment dependency for db:schema:load
All of the behavior :environment was giving (that db:schema:load needed)
was provided as well with :load_config.

This will address an issue introduced in
rails#15394. The fact that db:schema:load
now drops and creates the database causes the Octopus gem to have [an
issue](thiagopradi/octopus#273) during the drop
step for the test database (which wasn't happening in db:schema:load
before). The error looks like:

    ActiveRecord::StatementInvalid: PG::ObjectInUse: ERROR:  cannot drop the currently open database
    : DROP DATABASE IF EXISTS "app_test"

Because of the timing, this issue is present in master, 4-2-*, and
4.1.8.

A note to forlorn developers who might see this: "Additionally" in a
commit message means you should have a separate commit, with a separate
justification for changes. Small commits with big messages are your
friends.

sgrif added a commit that referenced this pull request Dec 4, 2014

Remove environment dependency for db:schema:load
All of the behavior :environment was giving (that db:schema:load needed)
was provided as well with :load_config.

This will address an issue introduced in
#15394. The fact that db:schema:load
now drops and creates the database causes the Octopus gem to have [an
issue](thiagopradi/octopus#273) during the drop
step for the test database (which wasn't happening in db:schema:load
before). The error looks like:

    ActiveRecord::StatementInvalid: PG::ObjectInUse: ERROR:  cannot drop the currently open database
    : DROP DATABASE IF EXISTS "app_test"

Because of the timing, this issue is present in master, 4-2-*, and
4.1.8.

A note to forlorn developers who might see this: "Additionally" in a
commit message means you should have a separate commit, with a separate
justification for changes. Small commits with big messages are your
friends.

sivagollapalli added a commit to sivagollapalli/rails that referenced this pull request Dec 29, 2014

Remove environment dependency for db:schema:load
All of the behavior :environment was giving (that db:schema:load needed)
was provided as well with :load_config.

This will address an issue introduced in
rails#15394. The fact that db:schema:load
now drops and creates the database causes the Octopus gem to have [an
issue](thiagopradi/octopus#273) during the drop
step for the test database (which wasn't happening in db:schema:load
before). The error looks like:

    ActiveRecord::StatementInvalid: PG::ObjectInUse: ERROR:  cannot drop the currently open database
    : DROP DATABASE IF EXISTS "app_test"

Because of the timing, this issue is present in master, 4-2-*, and
4.1.8.

A note to forlorn developers who might see this: "Additionally" in a
commit message means you should have a separate commit, with a separate
justification for changes. Small commits with big messages are your
friends.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment