Fix bug that `extract_limit` is not correctly overridden

[r7kamura]

Thank you @jlebray for getting this adapter ready for activerecord 7.

Problem

I encountered a problem in our Rails app that WHERE clause is incorrectly generated when it takes a large integer value as a bigint type column value.

Here is an example test code to reproduce this problem:

# frozen_string_literal: true

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'

  gem 'activerecord'
  gem 'activerecord7-redshift-adapter'
end

require 'active_record'
require 'minitest/autorun'
require 'logger'

ActiveRecord::Base.establish_connection(
  ENV.fetch('DATABASE_URL')
)
ActiveRecord::Base.logger = Logger.new($stdout)

ActiveRecord::Schema.define do
  create_table :users, force: true do |t|
    t.bigint :value_bigint, null: false
  end
end

class User < ActiveRecord::Base
end

class BugTest < Minitest::Test
  def test_where_bigint_is_small_value
    value = ('1' * 10).to_i
    assert_equal(
      %(SELECT "users".* FROM "users" WHERE "users"."value_bigint" = #{value}),
      User.where(value_bigint: value).to_sql
    )
  end

  def test_where_bigint_is_large_value
    value = ('1' * 11).to_i
    assert_equal(
      %(SELECT "users".* FROM "users" WHERE "users"."value_bigint" = #{value}),
      User.where(value_bigint: value).to_sql
    )
  end
end

and this results in:

$ ruby example.rb
-- create_table(:users, {:force=>true})
D, [2022-10-18T12:06:35.928713 #5811] DEBUG -- :    (34.7ms)  DROP TABLE "users"
D, [2022-10-18T12:06:35.975404 #5811] DEBUG -- :    (46.5ms)  CREATE TABLE "users" ("id" integer identity primary key, "value_bigint" bigint NOT NULL)
   -> 0.2019s
DEPRECATION WARNING: #table_exists? currently checks both tables and views. This behavior is deprecated and will be changed with Rails 5.1 to only check tables. Use #data_source_exists? instead. (called from <main> at example.rb:21)
D, [2022-10-18T12:06:36.042591 #5811] DEBUG -- :   ActiveRecord::InternalMetadata Load (7.8ms)  SELECT "ar_internal_metadata".* FROM "ar_internal_metadata" WHERE "ar_internal_metadata"."key" = 'environment' LIMIT 1
D, [2022-10-18T12:06:36.066754 #5811] DEBUG -- :    (6.1ms)  BEGIN
D, [2022-10-18T12:06:36.073342 #5811] DEBUG -- :    (5.9ms)  COMMIT
Run options: --seed 53551

# Running:

F.

Finished in 0.041459s, 48.2407 runs/s, 48.2407 assertions/s.

  1) Failure:
BugTest#test_where_bigint_is_large_value [example.rb:41]:
--- expected
+++ actual
@@ -1 +1 @@
-"SELECT \"users\".* FROM \"users\" WHERE \"users\".\"value_bigint\" = 11111111111"
+"SELECT \"users\".* FROM \"users\" WHERE 1=0"

2 runs, 2 assertions, 1 failures, 0 errors, 0 skips

Solution

After some investigation, I found that the #extract_limit was not rewritten correctly in https://github.com/pennylane-hq/activerecord7-redshift-adapter/pull/1.

As you know, #initialize_type_map was rewritten to .initialize_type_map in https://github.com/rails/rails/pull/42773. At this time, #extract_limit was also rewritten to .extract_limit. This change also had to be handled.

This Pull Request fixes this problem.

With this change, the above test will succeed.

$ ruby example.rb
-- create_table(:users, {:force=>true})
D, [2022-10-18T12:13:21.917765 #5990] DEBUG -- :    (36.4ms)  DROP TABLE "users"
D, [2022-10-18T12:13:21.966484 #5990] DEBUG -- :    (48.5ms)  CREATE TABLE "users" ("id" integer identity primary key, "value_bigint" bigint NOT NULL)
   -> 0.2156s
DEPRECATION WARNING: #table_exists? currently checks both tables and views. This behavior is deprecated and will be changed with Rails 5.1 to only check tables. Use #data_source_exists? instead. (called from <main> at example.rb:21)
D, [2022-10-18T12:13:22.029215 #5990] DEBUG -- :   ActiveRecord::InternalMetadata Load (7.0ms)  SELECT "ar_internal_metadata".* FROM "ar_internal_metadata" WHERE "ar_internal_metadata"."key" = 'environment' LIMIT 1
D, [2022-10-18T12:13:22.052618 #5990] DEBUG -- :    (6.3ms)  BEGIN
D, [2022-10-18T12:13:22.058396 #5990] DEBUG -- :    (5.3ms)  COMMIT
Run options: --seed 50465

# Running:

..

Finished in 0.025294s, 79.0708 runs/s, 79.0708 assertions/s.

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

[leonid-shevtsov]

I did a different approach with https://github.com/pennylane-hq/activerecord7-redshift-adapter/pull/8 by simply bypassing the limit extraction. It's not hard to do for just 3 types.

[quentindemetz]

Thanks @r7kamura for raising this. In the end #8 was merged, so I think we can close this.