From 1d3f21301a71509a30c10038dda110f9c95f3682 Mon Sep 17 00:00:00 2001 From: Ulysse Buonomo Date: Wed, 2 Oct 2024 10:24:10 +0200 Subject: [PATCH] chore: cleanup todos --- .../cockroachdb/schema_statements.rb | 10 +--- .../cockroachdb_adapter.rb | 5 +- test/cases/helper_cockroachdb.rb | 2 +- test/cases/migration/columns_test.rb | 50 ------------------- test/excludes/ActiveRecord/AdapterTest.rb | 8 +-- .../ActiveRecord/Migration/ColumnsTest.rb | 4 -- test/excludes/AttributeMethodsTest.rb | 2 - test/excludes/BulkAlterTableMigrationsTest.rb | 9 ++-- test/excludes/PostgresqlNetworkTest.rb | 2 +- 9 files changed, 15 insertions(+), 77 deletions(-) delete mode 100644 test/cases/migration/columns_test.rb delete mode 100644 test/excludes/ActiveRecord/Migration/ColumnsTest.rb delete mode 100644 test/excludes/AttributeMethodsTest.rb diff --git a/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb b/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb index 8f059bb0..1e5c2c76 100644 --- a/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb +++ b/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb @@ -77,11 +77,7 @@ def pk_and_sequence_for(table) # :nodoc: pg_attribute attr, pg_depend dep, pg_constraint cons, - pg_namespace nsp, - -- TODO: use the pg_catalog.pg_attribute(attishidden) column when - -- it is added instead of joining on crdb_internal. - -- See https://github.com/cockroachdb/cockroach/pull/126397 - crdb_internal.table_columns tc + pg_namespace nsp WHERE seq.oid = dep.objid AND seq.relkind = 'S' AND attr.attrelid = dep.refobjid @@ -89,9 +85,7 @@ def pk_and_sequence_for(table) # :nodoc: AND attr.attrelid = cons.conrelid AND attr.attnum = cons.conkey[1] AND seq.relnamespace = nsp.oid - AND attr.attrelid = tc.descriptor_id - AND attr.attname = tc.column_name - AND tc.hidden = false + AND attr.attishidden = false AND cons.contype = 'p' AND dep.classid = 'pg_class'::regclass AND dep.refobjid = #{quote(quote_table_name(table))}::regclass diff --git a/lib/active_record/connection_adapters/cockroachdb_adapter.rb b/lib/active_record/connection_adapters/cockroachdb_adapter.rb index 5b42506a..1b421401 100644 --- a/lib/active_record/connection_adapters/cockroachdb_adapter.rb +++ b/lib/active_record/connection_adapters/cockroachdb_adapter.rb @@ -414,11 +414,8 @@ def column_definitions(table_name) fields.delete_if do |field| # Don't include rowid column if it is hidden and the primary key # is not defined (meaning CRDB implicitly created it). - if field[0] == CockroachDBAdapter::DEFAULT_PRIMARY_KEY + field[0] == CockroachDBAdapter::DEFAULT_PRIMARY_KEY && field[10] && !primary_key(table_name) - else - false # Keep this entry. - end end end diff --git a/test/cases/helper_cockroachdb.rb b/test/cases/helper_cockroachdb.rb index c78f6f59..9afbcd1b 100644 --- a/test/cases/helper_cockroachdb.rb +++ b/test/cases/helper_cockroachdb.rb @@ -26,7 +26,7 @@ def current_adapter?(...) module LoadSchemaHelperExt def load_schema # TODO: Remove this const_set mess once https://github.com/rails/rails/commit/d5c2ff8345c9d23b7326edb2bbe72b6e86a63140 - # is part of a rails release. + # is part of a rails release (likely 8.0.0). old_helper = ActiveRecord::TestCase ActiveRecord.const_set(:TestCase, NoPGSchemaTestCase.new(ActiveRecord::TestCase)) return if ENV['COCKROACH_LOAD_FROM_TEMPLATE'] diff --git a/test/cases/migration/columns_test.rb b/test/cases/migration/columns_test.rb deleted file mode 100644 index 64f88c71..00000000 --- a/test/cases/migration/columns_test.rb +++ /dev/null @@ -1,50 +0,0 @@ -# frozen_string_literal: true - -require "cases/migration/helper" - -module ActiveRecord - module CockroachDB - class Migration - class ColumnsTest < ActiveRecord::TestCase - include ActiveRecord::Migration::TestHelper - - self.use_transactional_tests = false - - # This file replaces the same tests that have been excluded from ColumnsTest - # (see test/excludes/ActiveRecord/Migration/ColumnsTest.rb). New, unsaved - # records won't have string default values if the default has been changed - # in the database. This happens because once a column default is changed - # in CockroachDB, the type information on the value is missing. - # We can still verify the desired behavior by persisting the test records. - # When ActiveRecord fetches the records from the database, they'll have - # their default values. - - def test_change_column_default - add_column "test_models", "first_name", :string - connection.change_column_default "test_models", "first_name", "Tester" - TestModel.reset_column_information - - # Instead of using an unsaved TestModel record, persist one and fetch - # it from the database to get the new default value for type. - - TestModel.create! - test_model = TestModel.last - assert_equal "Tester", test_model.first_name - end - - def test_change_column_default_with_from_and_to - add_column "test_models", "first_name", :string - connection.change_column_default "test_models", "first_name", from: nil, to: "Tester" - TestModel.reset_column_information - - # Instead of using an unsaved TestModel record, persist one and fetch - # it from the database to get the new default value for type. - - TestModel.create! - test_model = TestModel.last - assert_equal "Tester", test_model.first_name - end - end - end - end -end diff --git a/test/excludes/ActiveRecord/AdapterTest.rb b/test/excludes/ActiveRecord/AdapterTest.rb index c8c31d6e..7a06a5d0 100644 --- a/test/excludes/ActiveRecord/AdapterTest.rb +++ b/test/excludes/ActiveRecord/AdapterTest.rb @@ -1,3 +1,5 @@ -exclude :test_indexes, "Rails transactional tests are being used while making schema changes. See https://www.cockroachlabs.com/docs/stable/online-schema-changes.html#limited-support-for-schema-changes-within-transactions." -exclude :test_remove_index_when_name_and_wrong_column_name_specified, "Rails transactional tests are being used while making schema changes. See https://www.cockroachlabs.com/docs/stable/online-schema-changes.html#limited-support-for-schema-changes-within-transactions." -exclude :test_remove_index_when_name_and_wrong_column_name_specified_positional_argument, "Rails transactional tests are being used while making schema changes. See https://www.cockroachlabs.com/docs/stable/online-schema-changes.html#limited-support-for-schema-changes-within-transactions." +transactional_test_msg = "Rails transactional tests are being used while making schema changes. " \ + "See https://www.cockroachlabs.com/docs/stable/online-schema-changes.html#limited-support-for-schema-changes-within-transactions." +exclude :test_indexes, transactional_test_msg +exclude :test_remove_index_when_name_and_wrong_column_name_specified, transactional_test_msg +exclude :test_remove_index_when_name_and_wrong_column_name_specified_positional_argument, transactional_test_msg diff --git a/test/excludes/ActiveRecord/Migration/ColumnsTest.rb b/test/excludes/ActiveRecord/Migration/ColumnsTest.rb deleted file mode 100644 index f772a6ff..00000000 --- a/test/excludes/ActiveRecord/Migration/ColumnsTest.rb +++ /dev/null @@ -1,4 +0,0 @@ -exclude :test_change_column, "This operation is not currently supported in CockroachDB, see cockroachdb/cockroach#9851" -exclude :test_change_column_default, "This test fails because type information is stripped from string column default values when the default is changed in the database. Possibly caused by https://github.com/cockroachdb/cockroach/issues/47285." -exclude :test_change_column_default_with_from_and_to, "This test fails because type information is stripped from string column default values when the default is changed in the database. Possibly caused by https://github.com/cockroachdb/cockroach/issues/47285." -exclude :test_remove_column_with_multi_column_index, "This test fails because it lacks the requisite CASCADE clause to fully remove the column" diff --git a/test/excludes/AttributeMethodsTest.rb b/test/excludes/AttributeMethodsTest.rb deleted file mode 100644 index fec54a47..00000000 --- a/test/excludes/AttributeMethodsTest.rb +++ /dev/null @@ -1,2 +0,0 @@ -# TODO: Rails 7.2 remove this exclusion -exclude "test_#undefine_attribute_methods_undefines_alias_attribute_methods", "The test will be fixed in 7.2 (https://github.com/rails/rails/commit/a0993f81d0450a191da1ee35282f60fc2899135c)" diff --git a/test/excludes/BulkAlterTableMigrationsTest.rb b/test/excludes/BulkAlterTableMigrationsTest.rb index b36ec7a3..286ae542 100644 --- a/test/excludes/BulkAlterTableMigrationsTest.rb +++ b/test/excludes/BulkAlterTableMigrationsTest.rb @@ -1,7 +1,8 @@ exclude :test_changing_columns, "Type conversion from DATE to TIMESTAMP requires overwriting existing values which is not yet implemented. https://github.com/cockroachdb/cockroach/issues/9851" exclude :test_changing_column_null_with_default, "Type conversion from DATE to TIMESTAMP requires overwriting existing values which is not yet implemented. https://github.com/cockroachdb/cockroach/issues/9851" -exclude :test_adding_indexes, "Need to reference the specific query count for CockroachDB" -exclude :test_removing_index, "Need to reference the specific query count for CockroachDB" -exclude :test_adding_multiple_columns, "Need to reference the specific query count for CockroachDB" -exclude :test_changing_index, "Need to reference the specific query count for CockroachDB" +query_count_msg = "Need to reference the specific query count for CockroachDB. Fixed in test/cases/migration_test.rb" +exclude :test_adding_indexes, query_count_msg +exclude :test_removing_index, query_count_msg +exclude :test_adding_multiple_columns, query_count_msg +exclude :test_changing_index, query_count_msg diff --git a/test/excludes/PostgresqlNetworkTest.rb b/test/excludes/PostgresqlNetworkTest.rb index dfcef4a6..25a07576 100644 --- a/test/excludes/PostgresqlNetworkTest.rb +++ b/test/excludes/PostgresqlNetworkTest.rb @@ -36,7 +36,7 @@ def on_send(node) case node in [:send, [:const, nil, :PostgresqlNetworkAddress], /create|new/, [:hash, *pairs]] - pairs.each_with_index do |pair, i| + pairs.each do |pair| if pair in [:pair, [:sym, /(cidr|mac)_address/], *] expr = pair.location.expression large_expr = expr.resize(expr.size + 1)