Skip to content
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

Add config.active_record.dump_schemas. #19347

Merged
merged 1 commit into from Mar 17, 2015
Merged

Conversation

@rywall
Copy link
Contributor

rywall commented Mar 15, 2015

Fixes db:structure:dump when using schema_search_path and PostgreSQL extensions.

Closes #17157.

@rywall
Copy link
Contributor Author

rywall commented Mar 15, 2015

@senny @jimmykarily Here is a first pass. Feedback appreciated. :)

# Specifies which database schemas to dump when calling db:structure:dump.
# If :schema_search_path, it will dumps any schemas listed in schema_search_path.
# Use :all to always dumps all schemas regardless of the schema_search_path.
# A string of comma separated schemas can also be used to pass a custom list of schemas.

This comment has been minimized.

Copy link
@senny

senny Mar 16, 2015

Member

The docs should mention the default value.

This comment has been minimized.

Copy link
@rywall

rywall Mar 16, 2015

Author Contributor

I was trying to model it after the documentation for schema_format (above) which doesn't specify the default in the doc. Should I change both?

This comment has been minimized.

Copy link
@senny

senny Mar 17, 2015

Member

Let's focus on this change for now. If you have time, you can update the other in a separate PR.

This comment has been minimized.

Copy link
@rywall

rywall Mar 17, 2015

Author Contributor

Done.

@senny
Copy link
Member

senny commented Mar 16, 2015

@rywall looking good. Do you think you could add test-cases for this new functionality?

@jimmykarily
Copy link

jimmykarily commented Mar 16, 2015

👍

@rywall rywall force-pushed the rywall:dump-schemas-config branch from 2f3108b to 8e10e5d Mar 16, 2015
@rywall rywall closed this Mar 16, 2015
@rywall rywall reopened this Mar 16, 2015
@rywall
Copy link
Contributor Author

rywall commented Mar 16, 2015

@senny I've added some tests. Anything else?

end

def test_structure_dump_with_dump_schemas_string
ActiveRecord::Base.dump_schemas = 'test'

This comment has been minimized.

Copy link
@senny

senny Mar 17, 2015

Member

let's use an example with two schemas as well.

This comment has been minimized.

Copy link
@rywall

rywall Mar 17, 2015

Author Contributor

Done.

ActiveRecord::Tasks::DatabaseTasks.structure_dump(@configuration, filename)
assert File.exist?(filename)
ActiveRecord::Tasks::DatabaseTasks.structure_dump(@configuration, @filename)
assert File.exist?(@filename)
ensure

This comment has been minimized.

Copy link
@senny

senny Mar 17, 2015

Member

ActiveRecord::Base.dump_schemas is currently leaking outside this test. It needs to be reset:

original_dump_schemas = ActiveRecord::Base.dump_schemas
ActiveRecord::Base.dump_schemas = "test"
# ...
ensure
  ActiveRecord::Base.dump_schemas = original_dump_schemas

def test_structure_dump_with_schema_search_path_and_dump_schemas_all
@configuration['schema_search_path'] = 'foo,public'
ActiveRecord::Base.dump_schemas = :all

This comment has been minimized.

Copy link
@senny

senny Mar 17, 2015

Member

ActiveRecord::Base.dump_schemas is currently leaking outside this test. It needs to be reset:

original_dump_schemas = ActiveRecord::Base.dump_schemas
ActiveRecord::Base.dump_schemas = :all
# ...
ensure
  ActiveRecord::Base.dump_schemas = original_dump_schemas
ensure
FileUtils.rm(filename)
FileUtils.rm_f(@filename)

This comment has been minimized.

Copy link
@senny

senny Mar 17, 2015

Member

Why do we need rm_f? For a single file rm should be sufficient, no?

This comment has been minimized.

Copy link
@rywall

rywall Mar 17, 2015

Author Contributor

I added rm_f because if the test failed and the file was not created, FileUtils.rm would raise an exception, obscuring the actual test failure. Upon reflection, the part of the test that checks if a file is created is superfluous (testing whether File.open works) so I've refactored the tests to stub out File.open and thus remove the need to cleanup.

@@ -1,3 +1,10 @@
* Add config.active_record.dump_schemas to fix db:structure:dump

This comment has been minimized.

Copy link
@senny

senny Mar 17, 2015

Member

can you put ` around "config.active_record.dump_schemas" and "db:structure:dump"?


ActiveRecord::Base.dump_schemas = :schema_search_path

This comment has been minimized.

Copy link
@senny

senny Mar 17, 2015

Member

We better not have side-effects in the setup method. I'd rather modify the global state in an individual test and have that one be responsible for resetting it.

ActiveRecord::Base.stubs(:connection).returns(@connection)
@connection.expects(:schema_search_path)

This comment has been minimized.

Copy link
@senny

senny Mar 17, 2015

Member

I don't think we should have expectations in the setup method. This is something which should be related to a test-case.

@senny
Copy link
Member

senny commented Mar 17, 2015

@rywall I added a couple comments. Thank you for your work on this 💛

Fixes db:structure:dump when using schema_search_path and PostgreSQL
extensions.

Closes #17157.
@rywall rywall force-pushed the rywall:dump-schemas-config branch from 8e10e5d to 7ab36f4 Mar 17, 2015
@rywall
Copy link
Contributor Author

rywall commented Mar 17, 2015

I've refactored the tests to avoid side effects. I think I've addressed all you other comments as well. Thanks for your feedback. The pull request is much better for it. 👍

senny added a commit that referenced this pull request Mar 17, 2015
Add config.active_record.dump_schemas.
@senny senny merged commit ec85089 into rails:master Mar 17, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@senny
Copy link
Member

senny commented Mar 17, 2015

@rywall awesome! Thank you for your work 💛

@jimmykarily
Copy link

jimmykarily commented Mar 18, 2015

Thanks!

@rywall rywall deleted the rywall:dump-schemas-config branch Apr 17, 2015
@GUI
Copy link
Contributor

GUI commented Nov 13, 2015

Thanks, @rywall! If anyone else stumbles upon this and would find this functionality useful in Rails 3 or Rails 4, I created a gem to backport this functionality to those versions of Rails: https://github.com/GUI/activerecord-postgres-dump-schemas

@njakobsen
Copy link
Contributor

njakobsen commented Jan 23, 2016

❤️ @GUI.

@NikolayS
Copy link

NikolayS commented May 18, 2017

Thanks for this.

Any plans to support pg_dump's --exclude-schema as well?

In some cases, it might be more convenient to have a "black list", not a "white" one.

@rainhead
Copy link

rainhead commented Oct 16, 2017

For anyone looking for a way to dump extensions along with a subset of schemas, or otherwise needing extra control over pg_dump, set ActiveRecord::Tasks::DatabaseTasks.structure_dump_flags:

ActiveRecord::Tasks::DatabaseTasks.structure_dump_flags = %w{
  --exclude-schema=schema2
  --exclude-schema=schema3
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants
You can’t perform that action at this time.