Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix Duplicate entries in db/schema.rb (Postgres) #10328

Closed
wants to merge 4 commits into from

9 participants

@codeprimate

Closes #10327

When a schema.rb dump is created from a Postgres database containing multiple schemas, duplicate statements will be generated in db/schema.rb, which will cause errors when loading the schema.rb.

This patch simply adds "uniq" statements to the method chain to ensure duplicate statements are not output.

I would like to add a test for this, but am unsure how/if y'all would like to handle database-specific tests. I am unaware of how this issue might be duplicated with other RDMS's

@rafaelfranca

We will for sure need tests for this because people in the future can see that uniq call and remove it since it doesn't seems to make sense.

You can add database specific tests for PostgreSQL here https://github.com/rails/rails/tree/master/activerecord/test/cases/adapters/postgresql

@codeprimate

Thank you. I will update a bit later with tests.

@codeprimate codeprimate reopened this
activerecord/test/cases/schema_dumper_test.rb
@@ -85,6 +85,16 @@ def test_no_dump_errors
assert_no_match %r{\# Could not dump table}, output
end
+ def test_schema_dump_has_no_duplicate_tables
+ output_lines = standard_dump.split("\n")
+ assert output_lines.select{|i| i.match(/create_table "duplicate_table"/)}.size == 1
@neerajdotname Collaborator

I think it is better to do assert_equal because in case of failure you have more feedback .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
activerecord/test/schema/postgresql_specific_schema.rb
((10 lines not shown))
+ create_table :duplicate_table do |t|
+ t.integer :field1
+ t.index :field1
+ end
+
+ # Temporarily switch search_path to schema_2 and create a duplicate table
+ conn = ActiveRecord::Base.connection
+ old_search_path = conn.schema_search_path
+ conn.schema_search_path = 'schema_2'
+
+ create_table :duplicate_table do |t|
+ t.integer :field1
+ t.index :field1
+ end
+
+ # Revert search_path
@neerajdotname Collaborator

I think an ensure will be nice here .

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

@codeprimate The two tests that you added are failing for me :confused: (Expected: 1, Actual: 0 for both)

@kennyj
Collaborator

@codeprimate I merged only two commits about testcase to master to prepare testing your PR, but tests were succeeded.

@codeprimate

I ran the tests using the following, to ensure the usage of the PostgreSQL database, otherwise they are run using Sqlite:

ARCONN=postgresql ruby -Itest test/cases/schema_dumper_test.rb test/schema/postgresql_specific_schema.rb

The failures in test/cases/schema_dumper_test.rb:90,95 were due to the test expecting tables to be created in a second schema, which would not exist when running the test suite against Sqlite. I added conditionals to test/cases/schema_dumper_test.rb:90,95 to check for the PostgresSQL adapter before running these assertions.

@kennyj
Collaborator

@codeprimate I've already ran the following command line.

ARCONN=postgresql ruby -Itest:lib test/cases/schema_dumper_test.rb test/schema/postgresql_specific_schema.rb

But I couldn't reproduce your problem.

@senny
Owner

ping: what is the status on this PR?

@rafaelfranca

I also applied the tests without the proposed fix and everything is green.

git diff HEAD~3
diff --git a/activerecord/test/cases/schema_dumper_test.rb b/activerecord/test/cases/schema_dumper_test.rb
index 32f86f9..85c1368 100644
--- a/activerecord/test/cases/schema_dumper_test.rb
+++ b/activerecord/test/cases/schema_dumper_test.rb
@@ -85,6 +85,20 @@ class SchemaDumperTest < ActiveRecord::TestCase
     assert_no_match %r{\# Could not dump table}, output
   end

+  def test_schema_dump_has_no_duplicate_tables
+    if ActiveRecord::Base.connection.adapter_name == 'PostgreSQL'
+      output_lines = standard_dump.split("\n")
+      assert_equal 1, output_lines.select{|i| i.match(/create_table "duplicate_table"/)}.size
+    end
+  end
+
+  def test_schema_dump_has_no_duplicate_indexes
+    if ActiveRecord::Base.connection.adapter_name == 'PostgreSQL'
+      output_lines = standard_dump.split("\n")
+      assert_equal 1, output_lines.select{|i| i.match(/add_index "duplicate_table"/)}.size
+    end
+  end
+
   def test_schema_dump_includes_not_null_columns
     stream = StringIO.new

diff --git a/activerecord/test/schema/postgresql_specific_schema.rb b/activerecord/test/schema/postgresql_specific_schema.rb
index 6b7012a..a64d204 100644
--- a/activerecord/test/schema/postgresql_specific_schema.rb
+++ b/activerecord/test/schema/postgresql_specific_schema.rb
@@ -1,7 +1,7 @@
 ActiveRecord::Schema.define do

   %w(postgresql_ranges postgresql_tsvectors postgresql_hstores postgresql_arrays postgresql_moneys postgresql_numbers postgresql_times postgresql_network_addresses postgresql_bit_strings postgresql_uuids postgresql_ltrees
-      postgresql_oids postgresql_xml_data_type defaults geometrics postgresql_timestamp_with_zones postgresql_partitioned_table postgresql_partitioned_table_parent postgresql_json_data_type).each do |table_name|
+      postgresql_oids postgresql_xml_data_type defaults geometrics postgresql_timestamp_with_zones postgresql_partitioned_table postgresql_partitioned_table_parent postgresql_json_data_type duplicate_table).each do |table_name|
     execute "DROP TABLE IF EXISTS #{quote_table_name table_name}"
   end

@@ -13,6 +13,7 @@ ActiveRecord::Schema.define do
   execute 'DROP FUNCTION IF EXISTS partitioned_insert_trigger()'

   execute "DROP SCHEMA IF EXISTS schema_1 CASCADE"
+  execute "DROP SCHEMA IF EXISTS schema_2 CASCADE"

   %w(accounts_id_seq developers_id_seq projects_id_seq topics_id_seq customers_id_seq orders_id_seq).each do |seq_name|
     execute "SELECT setval('#{seq_name}', 100)"
@@ -220,5 +221,35 @@ _SQL
     t.binary :binary, limit: 100_000
     t.text :text, limit: 100_000
   end
+
+  begin
+    conn = ActiveRecord::Base.connection
+    old_search_path = conn.schema_search_path
+
+    execute "CREATE SCHEMA schema_2"
+    execute "CREATE DOMAIN schema_2.text AS text"
+    execute "CREATE DOMAIN schema_2.varchar AS varchar"
+    execute "CREATE DOMAIN schema_2.bpchar AS bpchar"
+
+    # Create table in schema_1
+    create_table :duplicate_table do |t|
+      t.integer :field1
+      t.index :field1
+    end
+
+    # Temporarily switch search_path to schema_2
+    conn.schema_search_path = 'schema_2'
+
+    # Create table in schema_2
+    create_table :duplicate_table do |t|
+      t.integer :field1
+      t.index :field1
+    end
+
+  ensure
+    # Revert search_path
+    conn.schema_search_path = old_search_path if conn
+  end
+
 end
ARCONN=postgresql be ruby -Itest test/cases/schema_dumper_test.rb test/schema/postgresql_specific_schema.rb
Using postgresql
Run options: --seed 2586

# Running:

...................................

Finished in 20.127223s, 1.7389 runs/s, 34.9278 assertions/s.

35 runs, 703 assertions, 0 failures, 0 errors, 0 skips
@laurocaetano
Collaborator

I've added only the tests on master and it failed here.

ARCONN=postgresql be ruby -Itest test/cases/schema_dumper_test.rb test/schema/postgresql_specific_schema.rb
Using postgresql
Run options: --seed 26626

# Running:

......FF............................

Finished in 16.979065s, 2.1203 runs/s, 41.9340 assertions/s.

  1) Failure:
SchemaDumperTest#test_schema_dump_has_no_duplicate_indexes [test/cases/schema_dumper_test.rb:45]:
Expected: 1
  Actual: 0


  2) Failure:
SchemaDumperTest#test_schema_dump_has_no_duplicate_tables [test/cases/schema_dumper_test.rb:40]:
Expected: 1
  Actual: 0

36 runs, 712 assertions, 2 failures, 0 errors, 0 skips

The tests that I've applied

diff --git a/activerecord/test/cases/schema_dumper_test.rb b/activerecord/test/cases/schema_dumper_test.rb
index 1ee8e60..293a759 100644
--- a/activerecord/test/cases/schema_dumper_test.rb
+++ b/activerecord/test/cases/schema_dumper_test.rb
@@ -35,6 +35,16 @@ class SchemaDumperTest < ActiveRecord::TestCase
     assert_no_match %r{create_table "schema_migrations"}, output
   end

+  def test_schema_dump_has_no_duplicate_tables
+    output_lines = standard_dump.split("\n")
+    assert_equal 1, output_lines.select{|i| i.match(/create_table "duplicate_table"/)}.size
+  end
+
+  def test_schema_dump_has_no_duplicate_indexes
+    output_lines = standard_dump.split("\n")
+    assert_equal 1, output_lines.select{|i| i.match(/add_index "duplicate_table"/)}.size
+  end
+
   def test_schema_dump_excludes_sqlite_sequence
     output = standard_dump
     assert_no_match %r{create_table "sqlite_sequence"}, output

@rafaelfranca could you double check it? It seems that the fix was present in your test.

codeprimate added some commits
@codeprimate codeprimate Prevent the creation of duplicate table and index statements when cre…
…ating a schema dump from a Postgres Database containing multiple schemas.
82288dc
@codeprimate codeprimate Added tests for Postgres schema dump fix 3b7f176
@codeprimate codeprimate Use assert_equal in test_schema_dump_has_no_duplicate_values and test…
…_schema_dump_has_no_duplicate_indexes. Wrap associated postgresql-specific schema code to ensure that failure during setup does not change the database connection's schema_search_path
eb6c724
@codeprimate codeprimate Add conditionals to schema_dumper duplicate_table test to ensure asse…
…rtions are only made when running with the PostgreSQL database adapter.
e86f86f
@arthurnn
Collaborator

@laurocaetano the tests will not fail if you add the test change and the schema change for the tests(activerecord/test/schema/postgresql_specific_schema.rb) .

@laurocaetano
Collaborator

@arthurnn now I see :sweat_smile:
Sorry for the noise guys :(

@rafaelfranca rafaelfranca modified the milestone: 4.0.5, 4.0.4, 4.0.6
@romatr

@codeprimate Do you use pg_power gem?

@rafaelfranca rafaelfranca modified the milestone: 4.0.10, 4.1.7
@rafaelfranca rafaelfranca modified the milestone: 4.1.9, 4.0.13
@rafaelfranca

I applied the tests again without the fix and everything is green in my machine. That said I'm closing this pull request.

Thank you so much for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 5, 2013
  1. @codeprimate

    Prevent the creation of duplicate table and index statements when cre…

    codeprimate authored
    …ating a schema dump from a Postgres Database containing multiple schemas.
  2. @codeprimate
  3. @codeprimate

    Use assert_equal in test_schema_dump_has_no_duplicate_values and test…

    codeprimate authored
    …_schema_dump_has_no_duplicate_indexes. Wrap associated postgresql-specific schema code to ensure that failure during setup does not change the database connection's schema_search_path
  4. @codeprimate

    Add conditionals to schema_dumper duplicate_table test to ensure asse…

    codeprimate authored
    …rtions are only made when running with the PostgreSQL database adapter.
This page is out of date. Refresh to see the latest.
View
4 activerecord/lib/active_record/schema_dumper.rb
@@ -91,7 +91,7 @@ def extensions(stream)
end
def tables(stream)
- @connection.tables.sort.each do |tbl|
+ @connection.tables.sort.uniq.each do |tbl|
next if ['schema_migrations', ignore_tables].flatten.any? do |ignored|
case ignored
when String; remove_prefix_and_suffix(tbl) == ignored
@@ -207,7 +207,7 @@ def indexes(table, stream)
' ' + statement_parts.join(', ')
end
- stream.puts add_index_statements.sort.join("\n")
+ stream.puts add_index_statements.sort.uniq.join("\n")
stream.puts
end
end
View
14 activerecord/test/cases/schema_dumper_test.rb
@@ -85,6 +85,20 @@ def test_no_dump_errors
assert_no_match %r{\# Could not dump table}, output
end
+ def test_schema_dump_has_no_duplicate_tables
+ if ActiveRecord::Base.connection.adapter_name == 'PostgreSQL'
+ output_lines = standard_dump.split("\n")
+ assert_equal 1, output_lines.select{|i| i.match(/create_table "duplicate_table"/)}.size
+ end
+ end
+
+ def test_schema_dump_has_no_duplicate_indexes
+ if ActiveRecord::Base.connection.adapter_name == 'PostgreSQL'
+ output_lines = standard_dump.split("\n")
+ assert_equal 1, output_lines.select{|i| i.match(/add_index "duplicate_table"/)}.size
+ end
+ end
+
def test_schema_dump_includes_not_null_columns
stream = StringIO.new
View
33 activerecord/test/schema/postgresql_specific_schema.rb
@@ -1,7 +1,7 @@
ActiveRecord::Schema.define do
%w(postgresql_ranges postgresql_tsvectors postgresql_hstores postgresql_arrays postgresql_moneys postgresql_numbers postgresql_times postgresql_network_addresses postgresql_bit_strings postgresql_uuids postgresql_ltrees
- postgresql_oids postgresql_xml_data_type defaults geometrics postgresql_timestamp_with_zones postgresql_partitioned_table postgresql_partitioned_table_parent postgresql_json_data_type).each do |table_name|
+ postgresql_oids postgresql_xml_data_type defaults geometrics postgresql_timestamp_with_zones postgresql_partitioned_table postgresql_partitioned_table_parent postgresql_json_data_type duplicate_table).each do |table_name|
execute "DROP TABLE IF EXISTS #{quote_table_name table_name}"
end
@@ -13,6 +13,7 @@
execute 'DROP FUNCTION IF EXISTS partitioned_insert_trigger()'
execute "DROP SCHEMA IF EXISTS schema_1 CASCADE"
+ execute "DROP SCHEMA IF EXISTS schema_2 CASCADE"
%w(accounts_id_seq developers_id_seq projects_id_seq topics_id_seq customers_id_seq orders_id_seq).each do |seq_name|
execute "SELECT setval('#{seq_name}', 100)"
@@ -220,5 +221,35 @@
t.binary :binary, limit: 100_000
t.text :text, limit: 100_000
end
+
+ begin
+ conn = ActiveRecord::Base.connection
+ old_search_path = conn.schema_search_path
+
+ execute "CREATE SCHEMA schema_2"
+ execute "CREATE DOMAIN schema_2.text AS text"
+ execute "CREATE DOMAIN schema_2.varchar AS varchar"
+ execute "CREATE DOMAIN schema_2.bpchar AS bpchar"
+
+ # Create table in schema_1
+ create_table :duplicate_table do |t|
+ t.integer :field1
+ t.index :field1
+ end
+
+ # Temporarily switch search_path to schema_2
+ conn.schema_search_path = 'schema_2'
+
+ # Create table in schema_2
+ create_table :duplicate_table do |t|
+ t.integer :field1
+ t.index :field1
+ end
+
+ ensure
+ # Revert search_path
+ conn.schema_search_path = old_search_path if conn
+ end
+
end
Something went wrong with that request. Please try again.