feat: Support Rails 7.1

This PR includes loads of small changes that won't be mentionned here to adapt to the new Rails code. Loads of tests ignored tests have been added back as they were not error prone anymore. There are likely more of these. We removed the deprecated `configure_connection` method override. It is now unnecessary as CockroachDB supports `SET TIMEZONE <...>`. We dropped support for CockroachDB version before 22.X.X. We removed a lot of copy-pasted code in the test thanks to new file separation in Rails 7.1 test configuration. Signed-off-by: Ulysse Buonomo <buonomo.ulysse@gmail.com>
cockroachdb · Jan 24, 2024 · 0003518 · 0003518
1 parent 2d6771e
commit 0003518
Show file tree

Hide file tree

Showing 72 changed files with 821 additions and 1,474 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -15,7 +15,7 @@ on:
 
 # This allows a subsequently queued workflow run to interrupt previous runs.
 concurrency:
-  group: '${{ github.workflow }} @ ${{ github.ref }}'
+  group: "${{ github.workflow }} @ ${{ github.ref }}"
   cancel-in-progress: true
 
 jobs:
@@ -39,7 +39,7 @@ jobs:
     strategy:
       matrix:
         crdb: [v23.1.11]
-        ruby: [ruby-head]
+        ruby: [head]
     name: Test (crdb=${{ matrix.crdb }} ruby=${{ matrix.ruby }})
     steps:
       - name: Set Up Actions
@@ -49,8 +49,8 @@ jobs:
       - name: Set Up Ruby
         uses: ruby/setup-ruby@v1
         with:
-           ruby-version: ${{ matrix.ruby }}
-           bundler-cache: true
+          ruby-version: ${{ matrix.ruby }}
+          bundler-cache: true
       - name: Install and Start Cockroachdb
         run: |
           # Download CockroachDB
@@ -95,4 +95,4 @@ jobs:
             SET CLUSTER SETTING sql.defaults.experimental_temporary_tables.enabled = 'true';
           "
       - name: Test
-        run: bundle exec rake test
+        run: bundle exec rake test TESTOPTS='--profile=3'
diff --git a/Gemfile b/Gemfile
@@ -48,12 +48,13 @@ group :development, :test do
   # a specific rails codebase.
   gem "rails", github: "rails/rails", tag: RailsTag.call
 
-  # Needed before rails update for ruby 3.4
-  gem "mutex_m"
+  # Needed for the test suite
+  gem "msgpack", ">= 1.7.0"
 
   gem "rake"
-  gem "byebug"
+  gem "debug"
   gem "minitest-excludes", "~> 2.0.1"
+  gem "minitest-github_action_reporter", github: "BuonOmo/minitest-github_action_reporter", require: "minitest/github_action_reporter_plugin"
 
   # Gems used by the ActiveRecord test suite
   gem "bcrypt", "~> 3.1.18"

diff --git a/activerecord-cockroachdb-adapter.gemspec b/activerecord-cockroachdb-adapter.gemspec
@@ -14,7 +14,7 @@ Gem::Specification.new do |spec|
   spec.description   = "Allows the use of CockroachDB as a backend for ActiveRecord and Rails apps."
   spec.homepage      = "https://github.com/cockroachdb/activerecord-cockroachdb-adapter"
 
-  spec.add_dependency "activerecord", "~> 7.0.3"
+  spec.add_dependency "activerecord", "~> 7.1.0"
   spec.add_dependency "pg", "~> 1.2"
   spec.add_dependency "rgeo-activerecord", "~> 7.0.0"
 

diff --git a/lib/active_record/connection_adapters/cockroachdb/column.rb b/lib/active_record/connection_adapters/cockroachdb/column.rb
@@ -1,11 +1,11 @@
 module ActiveRecord
   module ConnectionAdapters
     module CockroachDB
-      class Column < PostgreSQLColumn
+      class Column < PostgreSQL::Column
         # most functions taken from activerecord-postgis-adapter spatial_column
         # https://github.com/rgeo/activerecord-postgis-adapter/blob/master/lib/active_record/connection_adapters/postgis/spatial_column.rb
         def initialize(name, default, sql_type_metadata = nil, null = true,
-                       default_function = nil, collation: nil, comment: nil,
+                       default_function = nil, collation: nil, comment: nil, identity: nil,
                        serial: nil, spatial: nil, generated: nil, hidden: nil)
           @sql_type_metadata = sql_type_metadata
           @geographic = !!(sql_type_metadata.sql_type =~ /geography\(/i)
@@ -30,7 +30,7 @@ def initialize(name, default, sql_type_metadata = nil, null = true,
             build_from_sql_type(sql_type_metadata.sql_type)
           end
           super(name, default, sql_type_metadata, null, default_function,
-            collation: collation, comment: comment, serial: serial, generated: generated)
+            collation: collation, comment: comment, serial: serial, generated: generated, identity: identity)
           if spatial? && @srid
             @limit = { srid: @srid, type: to_type_name(geometric_type) }
             @limit[:has_z] = true if @has_z
@@ -53,10 +53,6 @@ def limit
           spatial? ? @limit : super
         end
 
-        def virtual?
-          @generated.present?
-        end
-
         def hidden?
           @hidden
         end

diff --git a/lib/active_record/connection_adapters/cockroachdb/column_methods.rb b/lib/active_record/connection_adapters/cockroachdb/column_methods.rb
@@ -45,6 +45,14 @@ def st_point(name, options = {})
         def st_polygon(name, options = {})
           column(name, :st_polygon, **options)
         end
+
+        private
+
+        def valid_column_definition_options
+          spatial = [:srid, :has_z, :has_m, :geographic, :spatial_type]
+          crdb = [:hidden]
+          super + spatial + crdb
+        end
       end
     end
 

diff --git a/lib/active_record/connection_adapters/cockroachdb/database_statements.rb b/lib/active_record/connection_adapters/cockroachdb/database_statements.rb
@@ -2,21 +2,6 @@ module ActiveRecord
   module ConnectionAdapters
     module CockroachDB
       module DatabaseStatements
-        # Since CockroachDB will run all transactions with serializable isolation,
-        # READ UNCOMMITTED, READ COMMITTED, and REPEATABLE READ are all aliases
-        # for SERIALIZABLE. This lets the adapter support all isolation levels,
-        # but READ UNCOMMITTED has been removed from this list because the
-        # ActiveRecord transaction isolation test fails for READ UNCOMMITTED.
-        # See https://www.cockroachlabs.com/docs/v19.2/transactions.html#isolation-levels
-        def transaction_isolation_levels
-          {
-            read_committed:   "READ COMMITTED",
-            repeatable_read:  "REPEATABLE READ",
-            serializable:     "SERIALIZABLE",
-            read_uncommitted: "READ UNCOMMITTED"
-          }
-        end
-
         # Overridden to avoid using transactions for schema creation.
         def insert_fixtures_set(fixture_set, tables_to_delete = [])
           fixture_inserts = build_fixture_statements(fixture_set)
@@ -29,74 +14,6 @@ def insert_fixtures_set(fixture_set, tables_to_delete = [])
             end
           end
         end
-
-        private
-          def execute_batch(statements, name = nil)
-            statements.each do |statement|
-              execute(statement, name)
-            end
-          end
-
-          DEFAULT_INSERT_VALUE = Arel.sql("DEFAULT").freeze
-          private_constant :DEFAULT_INSERT_VALUE
-
-          def default_insert_value(column)
-            DEFAULT_INSERT_VALUE
-          end
-
-          def build_fixture_sql(fixtures, table_name)
-            columns = schema_cache.columns_hash(table_name)
-
-            values_list = fixtures.map do |fixture|
-              fixture = fixture.stringify_keys
-
-              unknown_columns = fixture.keys - columns.keys
-              if unknown_columns.any?
-                raise Fixture::FixtureError, %(table "#{table_name}" has no columns named #{unknown_columns.map(&:inspect).join(', ')}.)
-              end
-
-              columns.map do |name, column|
-                if fixture.key?(name)
-                  type = lookup_cast_type_from_column(column)
-                  with_yaml_fallback(type.serialize(fixture[name]))
-                else
-                  default_insert_value(column)
-                end
-              end
-            end
-
-            table = Arel::Table.new(table_name)
-            manager = Arel::InsertManager.new
-            manager.into(table)
-
-            if values_list.size == 1
-              values = values_list.shift
-              new_values = []
-              columns.each_key.with_index { |column, i|
-                unless values[i].equal?(DEFAULT_INSERT_VALUE)
-                  new_values << values[i]
-                  manager.columns << table[column]
-                end
-              }
-              values_list << new_values
-            else
-              columns.each_key { |column| manager.columns << table[column] }
-            end
-
-            manager.values = manager.create_values_list(values_list)
-            manager.to_sql
-          end
-
-          def build_fixture_statements(fixture_set)
-            fixture_set.map do |table_name, fixtures|
-              next if fixtures.empty?
-              build_fixture_sql(fixtures, table_name)
-            end.compact
-          end
-
-          def with_multi_statements
-            yield
-          end
       end
     end
   end

diff --git a/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb b/lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb
@@ -15,7 +15,7 @@ module ReferentialIntegrity
         # referential integrity (e.g: adding a foreign key with invalid data
         # raises).
         # So foreign keys should always be valid for that matter.
-        def all_foreign_keys_valid?
+        def check_all_foreign_keys_valid!
           true
         end
 
@@ -39,16 +39,12 @@ def disable_referential_integrity
 
           begin
             foreign_keys.each do |foreign_key|
-              begin
-                add_foreign_key(foreign_key.from_table, foreign_key.to_table, **foreign_key.options)
-              rescue ActiveRecord::StatementInvalid => error
-                if error.cause.class == PG::DuplicateObject
-                  # This error is safe to ignore because the yielded caller
-                  # already re-added the foreign key constraint.
-                else
-                  raise error
-                end
-              end
+              # Avoid having PG:DuplicateObject error if a test is ran in transaction.
+              # TODO: verify that there is no cache issue related to running this (e.g: fk
+              #   still in cache but not in db)
+              next if foreign_key_exists?(foreign_key.from_table, name: foreign_key.options[:name])
+
+              add_foreign_key(foreign_key.from_table, foreign_key.to_table, **foreign_key.options)
             end
           ensure
             ActiveRecord::Base.table_name_prefix = old_prefix

diff --git a/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb b/lib/active_record/connection_adapters/cockroachdb/schema_statements.rb
@@ -46,7 +46,8 @@ def foreign_keys(table_name)
               THEN ''
               ELSE n2.nspname || '.'
             END || t2.relname AS to_table,
-            a1.attname AS column, a2.attname AS primary_key, c.conname AS name, c.confupdtype AS on_update, c.confdeltype AS on_delete, c.convalidated AS valid, c.condeferrable AS deferrable, c.condeferred AS deferred
+            a1.attname AS column, a2.attname AS primary_key, c.conname AS name, c.confupdtype AS on_update, c.confdeltype AS on_delete, c.convalidated AS valid, c.condeferrable AS deferrable, c.condeferred AS deferred,
+            c.conkey, c.confkey, c.conrelid, c.confrelid
             FROM pg_constraint c
             JOIN pg_class t1 ON c.conrelid = t1.oid
             JOIN pg_class t2 ON c.confrelid = t2.oid
@@ -61,15 +62,26 @@ def foreign_keys(table_name)
           SQL
 
           fk_info.map do |row|
+            to_table = PostgreSQL::Utils.unquote_identifier(row["to_table"])
+            conkey = row["conkey"].scan(/\d+/).map(&:to_i)
+            confkey = row["confkey"].scan(/\d+/).map(&:to_i)
+
+            if conkey.size > 1
+              column = column_names_from_column_numbers(row["conrelid"], conkey)
+              primary_key = column_names_from_column_numbers(row["confrelid"], confkey)
+            else
+              column = PostgreSQL::Utils.unquote_identifier(row["column"])
+              primary_key = row["primary_key"]
+            end
+
             options = {
-              column: PostgreSQL::Utils.unquote_identifier(row["column"]),
+              column: column,
               name: row["name"],
-              primary_key: row["primary_key"]
+              primary_key: primary_key
             }
-
             options[:on_delete] = extract_foreign_key_action(row["on_delete"])
             options[:on_update] = extract_foreign_key_action(row["on_update"])
-            options[:deferrable] = extract_foreign_key_deferrable(row["deferrable"], row["deferred"])
+            options[:deferrable] = extract_constraint_deferrable(row["deferrable"], row["deferred"])
 
             options[:validate] = row["valid"]
             to_table = PostgreSQL::Utils.unquote_identifier(row["to_table"])
@@ -87,16 +99,20 @@ def default_sequence_name(table_name, pk = "id")
 
         # override
         # https://github.com/rails/rails/blob/6-0-stable/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb#L624
-        def new_column_from_field(table_name, field)
-          column_name, type, default, notnull, oid, fmod, collation, comment, generated, hidden = field
+        def new_column_from_field(table_name, field, _definition)
+          column_name, type, default, notnull, oid, fmod, collation, comment, identity, attgenerated, hidden = field
           type_metadata = fetch_type_metadata(column_name, type, oid.to_i, fmod.to_i)
           default_value = extract_value_from_default(default)
-          default_function = extract_default_function(default_value, default)
 
-          serial =
-            if (match = default_function&.match(/\Anextval\('"?(?<sequence_name>.+_(?<suffix>seq\d*))"?'::regclass\)\z/))
-              sequence_name_from_parts(table_name, column_name, match[:suffix]) == match[:sequence_name]
-            end
+          if attgenerated.present?
+            default_function = default
+          else
+            default_function = extract_default_function(default_value, default)
+          end
+
+          if match = default_function&.match(/\Anextval\('"?(?<sequence_name>.+_(?<suffix>seq\d*))"?'::regclass\)\z/)
+            serial = sequence_name_from_parts(table_name, column_name, match[:suffix]) == match[:sequence_name]
+          end
 
           # {:dimension=>2, :has_m=>false, :has_z=>false, :name=>"latlon", :srid=>0, :type=>"GEOMETRY"}
           spatial = spatial_column_info(table_name).get(column_name, type_metadata.sql_type)
@@ -110,8 +126,9 @@ def new_column_from_field(table_name, field)
             collation: collation,
             comment: comment.presence,
             serial: serial,
+            identity: identity.presence,
             spatial: spatial,
-            generated: generated,
+            generated: attgenerated,
             hidden: hidden
           )
         end

diff --git a/lib/active_record/connection_adapters/cockroachdb/type.rb b/lib/active_record/connection_adapters/cockroachdb/type.rb
@@ -1,15 +1,16 @@
 module ActiveRecord
   module Type
-    class << self
+    module CRDBExt
       # Return :postgresql instead of :cockroachdb for current_adapter_name so
       # we can continue using the ActiveRecord::Types defined in
       # PostgreSQLAdapter.
       def adapter_name_from(model)
-        name = model.connection_db_config.adapter.to_sym
+        name = super
         return :postgresql if name == :cockroachdb
 
         name
       end
     end
+    singleton_class.prepend CRDBExt
   end
 end