ActiveRecord::Migration.maintain_test_schema! doesn't work with structure.sql by morgoth · Pull Request #15394 · rails/rails

morgoth · 2014-06-01T15:47:28Z

Introduced in rails 4.1.
Checked using sqlite3 and postgres.

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

rails/activerecord/lib/active_record/tasks/database_tasks.rb

Line 159 in 99873ca

def load_schema(format = ActiveRecord::Base.schema_format, file = nil)

which later, after choosing database adapter calls method, in example on sqlite3 adapter:

rails/activerecord/lib/active_record/tasks/sqlite_database_tasks.rb

Line 35 in 99873ca

def structure_load(filename)

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

rails/activerecord/lib/active_record/tasks/database_tasks.rb

Line 168 in 99873ca

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.

senny · 2014-05-29T11:23:18Z

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

morgoth · 2014-06-01T15:49:27Z

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

szimek · 2014-06-10T11:31:10Z

Got this issue as well.

907th · 2014-06-11T12:18:12Z

Confirm

senny · 2014-06-11T15:12:04Z

activerecord/lib/active_record/tasks/database_tasks.rb

do we need to create again? purge should already do that.

This is not true for sqlite: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/tasks/sqlite_database_tasks.rb#L17-L24

Maybe we should change purge on this adapter to create database as it is in pg and mysql?

it should have the same semantics for sure. The description in the Rake task is: "Empty the test database"
To me this implies that the database still exists. Let's change the SQLite3 adapter.

senny · 2014-06-11T15:13:34Z

@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.

morgoth · 2014-06-11T15:29:27Z

@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 · 2014-06-11T15:31:47Z

@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 · 2014-06-11T15:37:20Z

@senny I updated PR with your suggeestions

senny · 2014-06-11T15:39:09Z

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

senny · 2014-06-12T10:50:13Z

railties/test/application/test_test.rb

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

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

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.

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.

…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`

…-schema-for-sql-format ActiveRecord::Migration.maintain_test_schema! doesn't work with structure.sql Conflicts: activerecord/CHANGELOG.md

senny · 2014-06-12T13:33:07Z

@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.

rafaelfranca · 2014-06-13T22:53:20Z

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

…-schema-for-sql-format ActiveRecord::Migration.maintain_test_schema! doesn't work with structure.sql Conflicts: activerecord/CHANGELOG.md Conflicts: activerecord/CHANGELOG.md

dhh · 2014-06-29T01:11:24Z

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

…ing-test-schema-for-sql-format" This reverts commit 22e9a91. See discussion on 22e9a91 for details. Conflicts: activerecord/CHANGELOG.md

senny · 2014-07-02T12:38:07Z

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

…ntaining-test-schema-for-sql-format" This reverts commit 46139d3, reversing changes made to 8f24787. Conflicts: activerecord/CHANGELOG.md

…atic-maintaining-test-schema-for-sql-format"" This reverts commit 5c87b5c.

jonleighton · 2014-07-02T20:53:25Z

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

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

…maintaining-test-schema-for-sql-format"" This reverts commit 5c87b5c.

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.

…maintaining-test-schema-for-sql-format"" This reverts commit 5c87b5c.

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.

…maintaining-test-schema-for-sql-format"" This reverts commit 5c87b5c.

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 · 2014-08-08T19:19:29Z

@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 · 2014-08-15T07:58:34Z

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

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 · 2014-10-03T16:39:10Z

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?

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.

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.

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.

rafaelfranca added the activerecord label May 28, 2014

rafaelfranca added this to the 4.1.3 milestone May 28, 2014

Added missing test case for migrations when using sql structure

df6dc1b

senny self-assigned this Jun 11, 2014

senny reviewed Jun 11, 2014
View reviewed changes

senny mentioned this pull request Jun 11, 2014

Basic support for adding and removing foreign keys #15606

Merged

7 tasks

senny reviewed Jun 12, 2014
View reviewed changes

Fixed automatic maintaining test schema to properly handle sql struct…

ad42aae

…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 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…

46139d3

…-schema-for-sql-format ActiveRecord::Migration.maintain_test_schema! doesn't work with structure.sql Conflicts: activerecord/CHANGELOG.md

morgoth deleted the fix-automatic-maintaining-test-schema-for-sql-format branch June 12, 2014 13:42

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

Revert "Merge pull request #15394 from morgoth/fix-automatic-maintain…

4556785

…ing-test-schema-for-sql-format" This reverts commit 22e9a91. See discussion on 22e9a91 for details. 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…

99d6f93

…atic-maintaining-test-schema-for-sql-format"" This reverts commit 5c87b5c.

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

Revert "Revert "Merge pull request #15394 from morgoth/fix-automatic-…

e3dbf50

…maintaining-test-schema-for-sql-format"" This reverts commit 5c87b5c.

senny mentioned this pull request Jul 28, 2014

purge database before loading schema, take 2 #16316

Merged

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

Revert "Revert "Merge pull request #15394 from morgoth/fix-automatic-…

bc9e521

…maintaining-test-schema-for-sql-format"" This reverts commit 5c87b5c.

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

Revert "Revert "Merge pull request #15394 from morgoth/fix-automatic-…

53dba73

…maintaining-test-schema-for-sql-format"" This reverts commit 5c87b5c.

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

There is a dump/load for schema_format sql

f1c7a6f

This was referenced Aug 26, 2014

Automatically maintain test database schema #13528

Merged

maintain_test_schema doesn't honor reverse migrations #15787

Closed

metaskills mentioned this pull request Oct 3, 2014

Various Issues With 4.2.0.beta2 DB Setup/Testing #17170

Closed

calebhearth mentioned this pull request Dec 4, 2014

Rails 4.2.0.beta1 is not supported thiagopradi/octopus#273

Closed

calebhearth mentioned this pull request Dec 4, 2014

Remove environment dependency for db:schema:load #17920

Merged

senny mentioned this pull request Dec 16, 2014

Postgresql adapter rake tasks don't work with url key of database.yml #17945

Closed

Comments

Conversation

morgoth commented Jun 1, 2014

Uh oh!

senny commented May 29, 2014

Uh oh!

morgoth commented Jun 1, 2014

Uh oh!

szimek commented Jun 10, 2014

Uh oh!

907th commented Jun 11, 2014

Uh oh!

senny Jun 11, 2014

Choose a reason for hiding this comment

Uh oh!

morgoth Jun 11, 2014

Choose a reason for hiding this comment

Uh oh!

senny Jun 11, 2014

Choose a reason for hiding this comment

Uh oh!

senny commented Jun 11, 2014

Uh oh!

morgoth commented Jun 11, 2014

Uh oh!

senny commented Jun 11, 2014

Uh oh!

morgoth commented Jun 11, 2014

Uh oh!

senny commented Jun 11, 2014

Uh oh!

senny Jun 12, 2014

Choose a reason for hiding this comment

Uh oh!

morgoth Jun 12, 2014

Choose a reason for hiding this comment

Uh oh!

senny Jun 12, 2014

Choose a reason for hiding this comment

Uh oh!

morgoth Jun 12, 2014

Choose a reason for hiding this comment

Uh oh!

senny commented Jun 12, 2014

Uh oh!

rafaelfranca commented Jun 13, 2014

Uh oh!

dhh commented Jun 29, 2014

Uh oh!

senny commented Jul 2, 2014

Uh oh!

jonleighton commented Jul 2, 2014

Uh oh!

jaredbeck commented Aug 8, 2014

Uh oh!

senny commented Aug 15, 2014

Uh oh!

metaskills commented Oct 3, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants