Commit 9a9fc079 authored by Max Woolf's avatar Max Woolf
Browse files

chore(rate_limit): address GitLabDuo MR review comments 

1. count_distinct ArgumentError now lists String alongside Symbol/nil
   (the validation does accept String — coerced to Symbol).
2. SADD-mode count is now Float-typed, matching the INCR path.
   Callers see a consistent Result::Info#count type regardless of mode.
3. Rename a misleadingly-named `first` variable in the
   "enforces :block when the cardinality exceeds the limit" integration
   test (it actually held the fourth check result).

Co-Authored-By: default avatarClaude Opus 4.7 (1M context) <noreply@anthropic.com>
parent bb474c4d

lib/labkit/rate_limit/evaluator.rb

+7 −3
Original line number Diff line number Diff line
@@ -239,23 +239,27 @@ module Labkit 
      end



      # Atomic SADD + SCARD + conditional EXPIRE in one Redis operation via Lua.

      # See SADD_SCRIPT for the body. Mirrors incr_with_ttl's shape.

      # See SADD_SCRIPT for the body. Mirrors incr_with_ttl's shape, including

      # the Float-typed count for uniformity with the INCR path.

      def sadd_with_ttl(redis_key, member, period)

        member_str = encode_char_value(member.to_s)

        @redis.with do |conn|

          raw_count, ttl = SADD_SCRIPT.eval(conn, keys: [redis_key], argv: [period, member_str])

          [Integer(raw_count), ttl]

          [Float(raw_count), ttl]

        end

      end



      # Pipelined SCARD + TTL. SCARD on a missing key returns 0, so no

      # explicit nil handling is needed (unlike GET in read_with_ttl).

      # SCARD is integer-valued; coerced to Float for type uniformity with

      # the INCR path so callers see a consistent count type.

      def scard_with_ttl(redis_key)

        @redis.with do |conn|

          conn.pipelined do |pipe|

          scard, ttl = conn.pipelined do |pipe|

            pipe.scard(redis_key)

            pipe.ttl(redis_key)

          end

          [Float(scard), ttl]

        end

      end

lib/labkit/rate_limit/rule.rb

+1 −1
Original line number Diff line number Diff line
@@ -34,7 +34,7 @@ module Labkit 
          when Symbol then value

          when String then value.to_sym

          else

            raise ArgumentError, "count_distinct must be a Symbol or nil, got #{value.class}"

            raise ArgumentError, "count_distinct must be a Symbol, String, or nil, got #{value.class}"

          end



        raise ArgumentError, "count_distinct #{sym.inspect} must not overlap characteristics #{characteristics_arr.inspect}" if sym && characteristics_arr.include?(sym)

spec/labkit/rate_limit/evaluator_spec.rb

+5 −5
Original line number Diff line number Diff line
@@ -894,7 +894,7 @@ RSpec.describe Labkit::RateLimit::Evaluator do 


      expect(result.exceeded?).to be(true)

      expect(result.action).to eq(:block)

      expect(result.info.count).to eq(6)

      expect(result.info.count).to eq(6.0)

    end



    it "fails open on Redis error", :aggregate_failures do
@@ -923,7 +923,7 @@ RSpec.describe Labkit::RateLimit::Evaluator do 


      expect(result.exceeded?).to be(true)

      expect(result.action).to eq(:block)

      expect(result.info.count).to eq(6)

      expect(result.info.count).to eq(6.0)

    end



    it "treats a :log rule as non-terminating and continues to the next rule" do
@@ -1030,7 +1030,7 @@ RSpec.describe Labkit::RateLimit::Evaluator do 
      result = evaluator(rules: [rule]).peek(identifier)



      expect(result.matched?).to be(true)

      expect(result.info.count).to eq(3)

      expect(result.info.count).to eq(3.0)

      expect(raw_redis.scard(key)).to eq(3) # no SADD happened

      expect(raw_redis.ttl(key)).to be_between(1, 7) # TTL not extended

    end
@@ -1042,7 +1042,7 @@ RSpec.describe Labkit::RateLimit::Evaluator do 


      expect(result.matched?).to be(true)

      expect(result.exceeded?).to be(false)

      expect(result.info.count).to eq(0)

      expect(result.info.count).to eq(0.0)

      expect(result.info.remaining).to eq(5)

    end
@@ -1080,7 +1080,7 @@ RSpec.describe Labkit::RateLimit::Evaluator do 
      result = evaluator(rules: [rule]).peek(id)



      expect(result.matched?).to be(true)

      expect(result.info.count).to eq(3)

      expect(result.info.count).to eq(3.0)

    end

  end

end

spec/labkit/rate_limit/limiter_spec.rb

+1 −1
Original line number Diff line number Diff line
@@ -252,7 +252,7 @@ RSpec.describe Labkit::RateLimit::Limiter do 
        .check({ user: 42, project: 99 })



      expect(result.matched?).to be(true)

      expect(result.info.count).to eq(4)

      expect(result.info.count).to eq(4.0)

      expect(result.info.remaining).to eq(6)

    end

  end

spec/labkit/rate_limit/rule_spec.rb

+1 −1
Original line number Diff line number Diff line
@@ -253,7 +253,7 @@ RSpec.describe Labkit::RateLimit::Rule do 


    it "raises when set to a non-Symbol, non-String, non-nil value" do

      expect { valid_rule(count_distinct: 42) }

        .to raise_error(ArgumentError, /count_distinct must be a Symbol or nil/)

        .to raise_error(ArgumentError, /count_distinct must be a Symbol, String, or nil/)

    end



    it "raises when it overlaps characteristics" do