feat: upgrade to Rails 8.1

.github/workflows/ci.yml

@@ -50,23 +50,43 @@ jobs:
           <table>
           <thead>
             <tr>
-              <th></th>
+              <th>#</th>
               <th>Test</th>
+              <th>Failure</th>
             </tr>
           </thead>
           <tbody>
           EOF
 
           jq --slurp --raw-output '
-            map(.failed_tests|map("\(.klass)#\(.NAME)")) | flatten | unique | sort | to_entries[]
-            | "<tr><td><strong>\(.key)</strong></td><td>\(.value)</td></tr>"
+            map(.failed_tests) | flatten | map({
+              klass,
+              NAME,
+              failure: .failures[0],
+              source_url: (
+                .source_location | if (.[0] | contains("/gems/")) then
+                  (.[0] | capture("rails-(?<sha>.*?)/(?<path>.*)")) *
+                  {line: .[1], server: "https://github.com", repo: "rails/rails"}
+                else
+                  {path: .[0], line: .[1], sha: $ENV.GITHUB_SHA, repo: $ENV.GITHUB_REPOSITORY, server: $ENV.GITHUB_SERVER_URL}
+                end | "\(.server)/\(.repo)/blob/\(.sha)/\(.path)#L\(.line)"
+              )
+            }) | group_by(.) | map(.[0] * { count: length }) | sort[0:100][]
+            | "<tr>"
+            +   "<td><strong>\(.count)</strong></td>"
+            +   "<td><a href=\"\(.source_url)\">\(.klass)#\(.NAME)</a></td>"
+            +   "<td><pre>\(.failure)</pre></td>"
+            + "</tr>"
           ' reports/*/report.json >>$GITHUB_STEP_SUMMARY
 
           cat <<EOF >>$GITHUB_STEP_SUMMARY
           </tbody>
           </table>
           EOF
 
+          # Do not print json if too large.
+          [[ "$(du -s reports | cut -f1)" -gt 124 ]] && exit 0
+
           echo >>$GITHUB_STEP_SUMMARY
           echo '```json' >>$GITHUB_STEP_SUMMARY
           jq --slurp --compact-output '.' reports/*/report.json >>$GITHUB_STEP_SUMMARY

.github/workflows/flaky.yml

@@ -72,7 +72,7 @@ jobs:
           name: report-${{ matrix.crdb }}-${{ matrix.ruby }}-${{ matrix.seed }}
           path: report.json
   collect:
-    if: failure()
+    if: failure() || cancelled()
     needs: test
     runs-on: ubuntu-latest
     steps:
@@ -97,7 +97,7 @@ jobs:
           EOF
 
           jq --slurp --raw-output '
-            sort_by(.total_time)[]
+            sort_by(.total_time)[0:100][]
             | {seed, time: .total_time | strftime("%H:%M:%S"), failure: .failed_tests[0].NAME }
             | "<tr><td>\(.seed)</td><td>\(.time)</td><td>\(.failure)</td></tr>"
           ' reports/*/report.json >> $GITHUB_STEP_SUMMARY
@@ -107,6 +107,10 @@ jobs:
           </tbody>
           </table>
           EOF
+
+          # Do not print json if too large.
+          [[ "$(du -s reports | cut -f1)" -gt 124 ]] && exit 0
+
           echo >> $GITHUB_STEP_SUMMARY
           echo '```json' >> $GITHUB_STEP_SUMMARY
           jq --slurp --compact-output '.' reports/*/report.json >> $GITHUB_STEP_SUMMARY

Gemfile

@@ -52,6 +52,7 @@ group :development, :test do
   gem "msgpack", ">= 1.7.0"
   gem "mutex_m", "~> 0.2.0"
 
+  gem "tracer"
   gem "rake"
   gem "debug"
   gem "minitest-bisect", github: "BuonOmo/minitest-bisect", branch: "main"

activerecord-cockroachdb-adapter.gemspec

@@ -14,9 +14,9 @@ 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", "~> 8.0.0"
+  spec.add_dependency "activerecord", "~> 8.1.0"
   spec.add_dependency "pg", "~> 1.5"
-  spec.add_dependency "rgeo-activerecord", "~> 8.0.0"
+  spec.add_dependency "rgeo-activerecord", "~> 8.1.0"
 
   spec.add_development_dependency "benchmark-ips", "~> 2.9.1"
 

bin/start-cockroachdb

@@ -19,7 +19,7 @@ fi
 
 cockroach start-single-node \
 	--insecure --store=type=mem,size=0.25 --advertise-addr=localhost \
-	--spatial-libs="$(geos-config --includes)" \
+	--spatial-libs="$(geos-config --prefix)/lib" \
 	--pid-file "$pid_file" \
 	&> "$log_file" &
 

lib/active_record/connection_adapters/cockroachdb/column.rb

@@ -20,7 +20,7 @@ module CockroachDB
       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,
+        def initialize(name, cast_type, default, sql_type_metadata = nil, null = true,
                        default_function = nil, collation: nil, comment: nil, identity: nil,
                        serial: nil, spatial: nil, generated: nil, hidden: nil)
           @sql_type_metadata = sql_type_metadata
@@ -45,7 +45,7 @@ def initialize(name, default, sql_type_metadata = nil, null = true,
             # @geometric_type = geo_type_from_sql_type(sql_type)
             build_from_sql_type(sql_type_metadata.sql_type)
           end
-          super(name, default, sql_type_metadata, null, default_function,
+          super(name, cast_type, default, sql_type_metadata, null, default_function,
             collation: collation, comment: comment, serial: serial, generated: generated, identity: identity)
           if spatial? && @srid
             @limit = { srid: @srid, type: to_type_name(geometric_type) }

lib/active_record/connection_adapters/cockroachdb/database_statements.rb

@@ -34,7 +34,6 @@ def insert_fixtures_set(fixture_set, tables_to_delete = [])
           # our statements by calling `#execute` instead of `#execute_batch`.
           #
           # [1]: https://github.com/rails/rails/pull/52428
-
           begin # much faster without disabling referential integrity, worth trying.
             transaction(requires_new: true) do
               execute(statements, "Fixtures Load")

lib/active_record/connection_adapters/cockroachdb/database_tasks.rb

@@ -74,7 +74,7 @@ def structure_load(filename, extra_flags=nil)
               "https://github.com/cockroachdb/activerecord-cockroachdb-adapter/issues/new"
           end
 
-          run_cmd("cockroach", ["sql", "--set", "errexit=false", "--file", filename], "loading")
+          run_cmd("cockroach", "sql", "--set", "errexit=false", "--file", filename)
         end
 
         private

lib/active_record/connection_adapters/cockroachdb/oid/spatial.rb

@@ -28,6 +28,10 @@ class Spatial < Type::Value
           def initialize(oid, sql_type)
             @sql_type = sql_type
             @geo_type, @srid, @has_z, @has_m = self.class.parse_sql_type(sql_type)
+            @spatial_factory =
+              RGeo::ActiveRecord::SpatialFactoryStore.instance.factory(
+                factory_attrs
+              )
           end
 
           # sql_type: geometry, geometry(Point), geometry(Point,4326), ...
@@ -59,13 +63,6 @@ def self.parse_sql_type(sql_type)
             [geo_type, srid, has_z, has_m]
           end
 
-          def spatial_factory
-            @spatial_factory ||=
-              RGeo::ActiveRecord::SpatialFactoryStore.instance.factory(
-                factory_attrs
-              )
-          end
-
           def geographic?
             @sql_type =~ /geography/
           end
@@ -108,14 +105,14 @@ def parse_wkt(string)
           end
 
           def binary_string?(string)
-            string[0] == "\x00" || string[0] == "\x01" || string[0, 4] =~ /[0-9a-fA-F]{4}/
+            string[0] == "\x00" || string[0] == "\x01" || string[0, 4].match?(/[0-9a-fA-F]{4}/)
           end
 
           def wkt_parser(string)
             if binary_string?(string)
-              RGeo::WKRep::WKBParser.new(spatial_factory, support_ewkb: true, default_srid: @srid)
+              RGeo::WKRep::WKBParser.new(@spatial_factory, support_ewkb: true, default_srid: @srid)
             else
-              RGeo::WKRep::WKTParser.new(spatial_factory, support_ewkt: true, default_srid: @srid)
+              RGeo::WKRep::WKTParser.new(@spatial_factory, support_ewkt: true, default_srid: @srid)
             end
           end
 

lib/active_record/connection_adapters/cockroachdb/quoting.rb

@@ -34,17 +34,18 @@ module Quoting
         # but when creating objects, using RGeo features is more convenient than
         # converting to WKB, so this does it automatically.
         def quote(value)
-          if value.is_a?(Numeric)
+          case value
+          when Numeric
             # NOTE: The fact that integers are quoted is important and helps
             # mitigate a potential vulnerability.
             #
             # See
             # - https://nvd.nist.gov/vuln/detail/CVE-2022-44566
             # - https://github.com/cockroachdb/activerecord-cockroachdb-adapter/pull/280#discussion_r1288692977
             "'#{quote_string(value.to_s)}'"
-          elsif RGeo::Feature::Geometry.check_type(value)
+          when RGeo::Feature::Geometry
             "'#{RGeo::WKRep::WKBGenerator.new(hex_format: true, type_format: :ewkb, emit_ewkb_srid: true).generate(value)}'"
-          elsif value.is_a?(RGeo::Cartesian::BoundingBox)
+          when RGeo::Cartesian::BoundingBox
             "'#{value.min_x},#{value.min_y},#{value.max_x},#{value.max_y}'::box"
           else
             super

lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb

@@ -34,8 +34,47 @@ def check_all_foreign_keys_valid!
         end
 
         def disable_referential_integrity
-          foreign_keys = all_foreign_keys
+          if transaction_open? && query_value("SHOW autocommit_before_ddl") == "off"
+            begin
+              yield
+            rescue ActiveRecord::InvalidForeignKey => e
+              warn <<-WARNING
+WARNING: Rails was not able to disable referential integrity.
+
+This is due to CockroachDB's need of committing transactions
+before a schema change occurs. To bypass this, you can set
+`autocommit_before_ddl: "on"` in your database configuration.
+WARNING
+              raise e
+            end
+          else
+            foreign_keys = all_foreign_keys
+
+            remove_foreign_keys(foreign_keys)
+
+            # Prefixes and suffixes are added in add_foreign_key
+            # in AR7+ so we need to temporarily disable them here,
+            # otherwise prefixes/suffixes will be erroneously added.
+            old_prefix = ActiveRecord::Base.table_name_prefix
+            old_suffix = ActiveRecord::Base.table_name_suffix
+
+            begin
+              yield
+            ensure
+              ActiveRecord::Base.table_name_prefix = ""
+              ActiveRecord::Base.table_name_suffix = ""
+
+              add_foreign_keys(foreign_keys) # Never raises.
+
+              ActiveRecord::Base.table_name_prefix = old_prefix if defined?(old_prefix)
+              ActiveRecord::Base.table_name_suffix = old_suffix if defined?(old_suffix)
+            end
+          end
+        end
 
+        private
+
+        def remove_foreign_keys(foreign_keys)
           statements = foreign_keys.map do |foreign_key|
             # We do not use the `#remove_foreign_key` method here because it
             # checks for foreign keys existance in the schema cache. This method
@@ -46,47 +85,30 @@ def disable_referential_integrity
             schema_creation.accept(at)
           end
           execute_batch(statements, "Disable referential integrity -> remove foreign keys")
+        end
 
-          yield
-
-          # Prefixes and suffixes are added in add_foreign_key
-          # in AR7+ so we need to temporarily disable them here,
-          # otherwise prefixes/suffixes will be erroneously added.
-          old_prefix = ActiveRecord::Base.table_name_prefix
-          old_suffix = ActiveRecord::Base.table_name_suffix
-
-          ActiveRecord::Base.table_name_prefix = ""
-          ActiveRecord::Base.table_name_suffix = ""
-
-          begin
-            # 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)
-            #
-            # We avoid using `foreign_key_exists?` here because it checks the schema cache
-            # for every key. This method is performance critical for the test suite, hence
-            # we use the `#all_foreign_keys` method that only make one query to the database.
-            already_inserted_foreign_keys = all_foreign_keys
-            statements = foreign_keys.map do |foreign_key|
-              next if already_inserted_foreign_keys.any? { |fk| fk.from_table == foreign_key.from_table && fk.options[:name] == foreign_key.options[:name] }
-
-              options = foreign_key_options(foreign_key.from_table, foreign_key.to_table, foreign_key.options)
-              at = create_alter_table foreign_key.from_table
-              at.add_foreign_key foreign_key.to_table, options
-
-              schema_creation.accept(at)
-            end
-            execute_batch(statements.compact, "Disable referential integrity -> add foreign keys")
-          ensure
-            ActiveRecord::Base.table_name_prefix = old_prefix
-            ActiveRecord::Base.table_name_suffix = old_suffix
+        # NOTE: This method should never raise, otherwise we risk polluting table name
+        #   prefixes and suffixes. The good thing is: if this happens, tests will crash
+        #   hard, no way we miss it.
+        def add_foreign_keys(foreign_keys)
+          # We avoid using `foreign_key_exists?` here because it checks the schema cache
+          # for every key. This method is performance critical for the test suite, hence
+          # we use the `#all_foreign_keys` method that only make one query to the database.
+          already_inserted_foreign_keys = all_foreign_keys
+          statements = foreign_keys.map do |foreign_key|
+            next if already_inserted_foreign_keys.any? { |fk| fk.from_table == foreign_key.from_table && fk.options[:name] == foreign_key.options[:name] }
+
+            options = foreign_key_options(foreign_key.from_table, foreign_key.to_table, foreign_key.options)
+            at = create_alter_table foreign_key.from_table
+            at.add_foreign_key foreign_key.to_table, options
+
+            schema_creation.accept(at)
           end
+          execute_batch(statements.compact, "Disable referential integrity -> add foreign keys")
         end
 
-        private
-
-        # Copy/paste of the `#foreign_keys(table)` method adapted to return every single
-        # foreign key in the database.
+        # NOTE: Copy/paste of the `#foreign_keys(table)` method adapted
+        #   to return every single foreign key in the database.
         def all_foreign_keys
           fk_info = internal_exec_query(<<~SQL, "SCHEMA")
             SELECT CASE
@@ -99,14 +121,30 @@ def all_foreign_keys
               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,
-            c.conkey, c.confkey, c.conrelid, c.confrelid
+            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.conrelid, c.confrelid,
+              (
+                SELECT array_agg(a.attname ORDER BY idx)
+                FROM (
+                  SELECT idx, c.conkey[idx] AS conkey_elem
+                  FROM generate_subscripts(c.conkey, 1) AS idx
+                ) indexed_conkeys
+                JOIN pg_attribute a ON a.attrelid = t1.oid
+                AND a.attnum = indexed_conkeys.conkey_elem
+                AND NOT a.attishidden
+              ) AS conkey_names,
+              (
+                SELECT array_agg(a.attname ORDER BY idx)
+                FROM (
+                  SELECT idx, c.confkey[idx] AS confkey_elem
+                  FROM generate_subscripts(c.confkey, 1) AS idx
+                ) indexed_confkeys
+                JOIN pg_attribute a ON a.attrelid = t2.oid
+                AND a.attnum = indexed_confkeys.confkey_elem
+                AND NOT a.attishidden
+              ) AS confkey_names
             FROM pg_constraint c
             JOIN pg_class t1 ON c.conrelid = t1.oid
             JOIN pg_class t2 ON c.confrelid = t2.oid
-            JOIN pg_attribute a1 ON a1.attnum = c.conkey[1] AND a1.attrelid = t1.oid
-            JOIN pg_attribute a2 ON a2.attnum = c.confkey[1] AND a2.attrelid = t2.oid
-            JOIN pg_namespace t3 ON c.connamespace = t3.oid
             JOIN pg_namespace n1 ON t1.relnamespace = n1.oid
             JOIN pg_namespace n2 ON t2.relnamespace = n2.oid
             WHERE c.contype = 'f'
@@ -116,22 +154,16 @@ def all_foreign_keys
           fk_info.map do |row|
             from_table = PostgreSQL::Utils.unquote_identifier(row["from_table"])
             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
+
+            column = decode_string_array(row["conkey_names"])
+            primary_key = decode_string_array(row["confkey_names"])
 
             options = {
-              column: column,
+              column: column.size == 1 ? column.first : column,
               name: row["name"],
-              primary_key: primary_key
+              primary_key: primary_key.size == 1 ? primary_key.first : 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_constraint_deferrable(row["deferrable"], row["deferred"])