Commit e57d5121 authored by Sam Wiskow's avatar Sam Wiskow
Browse files

refactor(rate_limit): address review feedback on Spec 10 Matcher 

- Refactor Matcher from `Data.define(...) do ... end` block form to
  `class Matcher < Data.define(:type, :value); end` so internal
  constants (KNOWN_HASH_KEYS, MAX_REGEX_SOURCE_LENGTH, the new
  ERROR_INSPECT_LIMIT) live inside the class instead of polluting
  the surrounding RateLimit namespace. Removes the now-redundant
  module-level MATCHER_KNOWN_HASH_KEYS / MATCHER_MAX_REGEX_SOURCE_LENGTH.
- Drop the `# @api private` line — the codebase doesn't use that
  convention.
- Add an explicit `else raise ArgumentError, "unknown matcher type:
  ..."` arm to `Matcher#match?` so a hand-constructed
  `Matcher.new(type: :glob, ...)` (bypassing `.build`) explodes loudly
  on first use rather than silently returning nil.
- Add `Matcher.truncate_for_error` private helper and apply it to the
  three rejection branches (Array, multi-key Hash, unknown type key).
  Bounds the resulting `ArgumentError` message length, defending
  against multi-megabyte garbage inputs reaching logs / error
  trackers. Regex-source branches already capped at 200 chars.
- Test name rebuild: drop "Spec 10 / Scenario X:" prefixes from
  describe / context / it strings across matcher_spec, rule_spec, and
  evaluator_spec. Tests now describe behaviour, not their position in
  the spec.
- Restructure evaluator_spec's regex blocks into behaviour-named
  describes ("with an :equality matcher", "with a :regex matcher",
  "when the rule has multiple match keys"); fold counter-key,
  no-match-falls-through, and quiet-skip cases into the regex
  describe as it/contexts of the same code path.
- Add spec coverage for the new bypass guard and the bounded-error
  helper (Array / unknown-type-key / multi-key-Hash).

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

lib/labkit/rate_limit/matcher.rb

+19 −11
Original line number Diff line number Diff line
@@ -2,9 +2,6 @@ 


module Labkit

  module RateLimit

    MATCHER_KNOWN_HASH_KEYS = %i[regex].freeze

    MATCHER_MAX_REGEX_SOURCE_LENGTH = 200



    # Matcher is the internal representation of a single key/value predicate in

    # a Rule#match hash. Rule.new normalizes every match value through

    # Matcher.build; the Evaluator calls Matcher#match? per identifier value.
@@ -16,8 +13,11 @@ module Labkit 
    #

    # See Spec 10 (gitlab-com/gl-infra/production-engineering#28855) for the

    # full contract; glob support is intentionally out of scope.

    # @api private

    Matcher = Data.define(:type, :value) do

    class Matcher < Data.define(:type, :value)

      KNOWN_HASH_KEYS = %i[regex].freeze

      MAX_REGEX_SOURCE_LENGTH = 200

      ERROR_INSPECT_LIMIT = 80



      def self.build(input)

        case input

        when Regexp
@@ -26,7 +26,7 @@ module Labkit 
          from_hash(input)

        when Array

          raise ArgumentError,

            "rate-limit match value must be a single-key Hash like {regex: \"...\"}, got #{input.inspect}"

            "rate-limit match value must be a single-key Hash like {regex: \"...\"}, got #{truncate_for_error(input)}"

        else

          new(type: :equality, value: input)

        end
@@ -35,15 +35,15 @@ module Labkit 
      def self.from_hash(input)

        if input.size != 1

          raise ArgumentError,

            "rate-limit match value must be a single-key Hash like {regex: \"...\"}, got #{input.inspect}"

            "rate-limit match value must be a single-key Hash like {regex: \"...\"}, got #{truncate_for_error(input)}"

        end



        type, source = input.first

        type_sym = type.to_sym



        unless MATCHER_KNOWN_HASH_KEYS.include?(type_sym)

        unless KNOWN_HASH_KEYS.include?(type_sym)

          raise ArgumentError,

            "rate-limit match value has unknown type key #{type.inspect}; accepted: #{MATCHER_KNOWN_HASH_KEYS.inspect}"

            "rate-limit match value has unknown type key #{truncate_for_error(type)}; accepted: #{KNOWN_HASH_KEYS.inspect}"

        end



        compile(type_sym, source)
@@ -58,9 +58,9 @@ module Labkit 
              "rate-limit match value {regex: ...} requires a String source, got #{source.class}"

          end



          if source.length > MATCHER_MAX_REGEX_SOURCE_LENGTH

          if source.length > MAX_REGEX_SOURCE_LENGTH

            raise ArgumentError,

              "rate-limit match value {regex: ...} source exceeds #{MATCHER_MAX_REGEX_SOURCE_LENGTH} characters"

              "rate-limit match value {regex: ...} source exceeds #{MAX_REGEX_SOURCE_LENGTH} characters"

          end



          begin
@@ -72,12 +72,20 @@ module Labkit 
      end

      private_class_method :compile



      def self.truncate_for_error(value)

        s = value.inspect

        s.length > ERROR_INSPECT_LIMIT ? "#{s[0, ERROR_INSPECT_LIMIT]}...(truncated)" : s

      end

      private_class_method :truncate_for_error



      def match?(identifier_value)

        case type

        when :equality

          value == identifier_value

        when :regex

          identifier_value.is_a?(String) && value.match?(identifier_value)

        else

          raise ArgumentError, "unknown matcher type: #{type.inspect}"

        end

      end

    end

spec/labkit/rate_limit/evaluator_spec.rb

+49 −51
Original line number Diff line number Diff line
@@ -402,8 +402,8 @@ RSpec.describe Labkit::RateLimit::Evaluator do 
    end

  end



  describe "Spec 10 Scenario A: equality semantics unchanged" do

    it "matches an :equality matcher and increments the counter" do

  describe "with an :equality matcher" do

    it "matches and increments the counter when the identifier value equals the rule value" do

      rule = make_rule(name: "api_endpoint", match: { endpoint: "GET /api/v4/projects" }, characteristics: [:user])

      id = Labkit::RateLimit::Identifier.new(user: 42, endpoint: "GET /api/v4/projects")
@@ -416,7 +416,7 @@ RSpec.describe Labkit::RateLimit::Evaluator do 
    end

  end



  describe "Spec 10 Scenario B: regex matcher (canonical and Ruby-convenience forms)" do

  describe "with a :regex matcher" do

    let(:id) { Labkit::RateLimit::Identifier.new(user: 42, path: "/api/v4/projects/42") }



    it "matches via { regex: \"...\" }" do
@@ -438,26 +438,64 @@ RSpec.describe Labkit::RateLimit::Evaluator do 


      expect(evaluator(rules: [rule]).check(id).matched?).to be(true)

    end



    it "uses the identifier value (not the regex source) in the compound counter key" do

      rule = make_rule(name: "projects_api",

        match: { path: { regex: "^/api/v\\d+/projects" } },

        characteristics: [:user, :path])

      counter_id = Labkit::RateLimit::Identifier.new(user: 42, path: "/api/v4/projects")



      expect(pipe).to receive(:incr)

        .with("labkit:rl:rack_request:projects_api:user:42:path:/api/v4/projects")

        .and_return(nil)

      expect(raw_redis).to receive(:expire)



      evaluator(rules: [rule]).check(counter_id)

    end



  describe "Spec 10 Scenario C: regex non-match falls through" do

    it "does not match when the regex does not match the identifier value" do

    context "when the identifier value does not match" do

      it "does not call Redis and the rule does not match" do

        rule = make_rule(match: { path: { regex: "^/admin" } })

      id = Labkit::RateLimit::Identifier.new(user: 42, path: "/api/v4/projects")

        no_match_id = Labkit::RateLimit::Identifier.new(user: 42, path: "/api/v4/projects")

        expect(raw_redis).not_to receive(:pipelined)



      expect(evaluator(rules: [rule]).check(id).matched?).to be(false)

        expect(evaluator(rules: [rule]).check(no_match_id).matched?).to be(false)

      end

    end



    context "when the identifier value is not a String" do

      it "does not match and does not raise" do

        rule = make_rule(match: { user: { regex: "42" } })

        int_id = Labkit::RateLimit::Identifier.new(user: 42)

        expect(raw_redis).not_to receive(:pipelined)



        expect { evaluator(rules: [rule]).check(int_id) }.not_to raise_error

        expect(evaluator(rules: [rule]).check(int_id).matched?).to be(false)

      end



      it "falls through to a subsequent equality rule that does match" do

        regex_rule = make_rule(name: "regex_first", match: { user: { regex: "42" } }, action: :block)

        eq_rule    = make_rule(name: "eq_second",   match: { user: 42 }, action: :log)

        int_id = Labkit::RateLimit::Identifier.new(user: 42)



        expect(pipe).to receive(:incr).and_return(nil)

        expect(raw_redis).to receive(:expire)



        result = evaluator(rules: [regex_rule, eq_rule]).check(int_id)

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

        expect(result.rule).to eq(eq_rule)

      end

    end

  end



  describe "Spec 10 Scenario D: AND across mixed equality + regex matchers" do

  describe "when the rule has multiple match keys" do

    let(:rule) do

      make_rule(name: "mixed",

        match: { request_type: "api", path: { regex: "^/api/v\\d+/projects" } },

        characteristics: [:user])

    end



    it "matches when both predicates pass" do

    it "matches when every predicate passes" do

      id = Labkit::RateLimit::Identifier.new(user: 42, request_type: "api", path: "/api/v4/projects")

      expect(pipe).to receive(:incr).and_return(nil)

      expect(raw_redis).to receive(:expire)
@@ -465,58 +503,18 @@ RSpec.describe Labkit::RateLimit::Evaluator do 
      expect(evaluator(rules: [rule]).check(id).matched?).to be(true)

    end



    it "does not match when only the equality predicate passes" do

    it "does not match when one predicate fails (equality passes, regex fails)" do

      id = Labkit::RateLimit::Identifier.new(user: 42, request_type: "api", path: "/admin")

      expect(raw_redis).not_to receive(:pipelined)



      expect(evaluator(rules: [rule]).check(id).matched?).to be(false)

    end



    it "does not match when only the regex predicate passes" do

    it "does not match when one predicate fails (regex passes, equality fails)" do

      id = Labkit::RateLimit::Identifier.new(user: 42, request_type: "web", path: "/api/v4/projects")

      expect(raw_redis).not_to receive(:pipelined)



      expect(evaluator(rules: [rule]).check(id).matched?).to be(false)

    end

  end



  describe "Spec 10 Scenario E: counter key uses the identifier value, not the regex source" do

    it "writes the identifier's path value into the compound key" do

      rule = make_rule(name: "projects_api",

        match: { path: { regex: "^/api/v\\d+/projects" } },

        characteristics: [:user, :path])

      id = Labkit::RateLimit::Identifier.new(user: 42, path: "/api/v4/projects")



      expect(pipe).to receive(:incr)

        .with("labkit:rl:rack_request:projects_api:user:42:path:/api/v4/projects")

        .and_return(nil)

      expect(raw_redis).to receive(:expire)



      evaluator(rules: [rule]).check(id)

    end

  end



  describe "Spec 10 Scenario G: non-String identifier vs regex matcher (quiet skip)" do

    it "does not match and does not raise when the identifier value is an Integer" do

      rule = make_rule(match: { user: { regex: "42" } })

      id = Labkit::RateLimit::Identifier.new(user: 42)

      expect(raw_redis).not_to receive(:pipelined)



      expect { evaluator(rules: [rule]).check(id) }.not_to raise_error

      expect(evaluator(rules: [rule]).check(id).matched?).to be(false)

    end



    it "falls through to the next rule when the regex matcher quiet-skips" do

      regex_rule = make_rule(name: "regex_first", match: { user: { regex: "42" } }, action: :block)

      eq_rule    = make_rule(name: "eq_second",   match: { user: 42 }, action: :log)

      id = Labkit::RateLimit::Identifier.new(user: 42)



      expect(pipe).to receive(:incr).and_return(nil)

      expect(raw_redis).to receive(:expire)



      result = evaluator(rules: [regex_rule, eq_rule]).check(id)

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

      expect(result.rule).to eq(eq_rule)

    end

  end

end

spec/labkit/rate_limit/matcher_spec.rb

+39 −3
Original line number Diff line number Diff line
@@ -77,7 +77,7 @@ RSpec.describe Labkit::RateLimit::Matcher do 
      end

    end



    context "with unsupported shapes (Scenario H)" do

    context "with unsupported shapes" do

      it "rejects an Array" do

        expect { described_class.build(["/a", "/b"]) }

          .to raise_error(ArgumentError, /single-key Hash/)
@@ -103,6 +103,35 @@ RSpec.describe Labkit::RateLimit::Matcher do 
          .to raise_error(ArgumentError, /unknown type key/)

      end

    end



    context "with unbounded user-controlled rejection input" do

      it "produces a bounded ArgumentError message for a giant Array" do

        big = Array.new(50_000) { "x" * 100 } # ~5 MB when inspected



        expect { described_class.build(big) }.to raise_error(ArgumentError) do |error|

          expect(error.message.length).to be < 200

          expect(error.message).to include("(truncated)")

        end

      end



      it "produces a bounded ArgumentError message for a giant unknown type key" do

        giant_symbol = ("a" * 5_000).to_sym



        expect { described_class.build(giant_symbol => "x") }.to raise_error(ArgumentError) do |error|

          expect(error.message.length).to be < 200

          expect(error.message).to include("(truncated)")

        end

      end



      it "produces a bounded ArgumentError message for a giant multi-key Hash" do

        big_hash = (1..5_000).to_h { |i| [:"k#{i}", "v#{i}"] }



        expect { described_class.build(big_hash) }.to raise_error(ArgumentError) do |error|

          expect(error.message.length).to be < 200

          expect(error.message).to include("(truncated)")

        end

      end

    end

  end



  describe "#match?" do
@@ -139,7 +168,7 @@ RSpec.describe Labkit::RateLimit::Matcher do 
        expect(matcher.match?("/admin")).to be(false)

      end



      context "when the identifier value is not a String (Scenario G -- quiet skip)" do

      context "when the identifier value is not a String" do

        it "returns false for an Integer (no exception, no log)" do

          expect(matcher.match?(42)).to be(false)

        end
@@ -164,6 +193,13 @@ RSpec.describe Labkit::RateLimit::Matcher do 
        end

      end

    end



    context "with an unknown matcher type (bypassing .build)" do

      it "raises ArgumentError" do

        matcher = described_class.new(type: :glob, value: "x")

        expect { matcher.match?("x") }.to raise_error(ArgumentError, /unknown matcher type/)

      end

    end

  end



  describe "value equality (Data.define)" do
@@ -173,7 +209,7 @@ RSpec.describe Labkit::RateLimit::Matcher do 
      expect(first).to eq(second)

    end



    it "treats two regex matchers with the same compiled source as equal (Scenario I parity)" do

    it "treats two regex matchers with the same compiled source as equal" do

      ruby_form = described_class.build(regex: "^/api/v\\d+")

      yaml_form = described_class.build("regex" => "^/api/v\\d+")

      expect(ruby_form).to eq(yaml_form)

spec/labkit/rate_limit/rule_spec.rb

+8 −8
Original line number Diff line number Diff line
@@ -128,13 +128,13 @@ RSpec.describe Labkit::RateLimit::Rule do 
    end

  end



  describe "match value normalization (Spec 10)" do

  describe "match value normalization" do

    it "normalizes a plain value to an :equality matcher" do

      rule = valid_rule(match: { endpoint: "/api/v4" })

      expect(rule.match[:endpoint]).to have_attributes(type: :equality, value: "/api/v4")

    end



    it "normalizes { regex: \"...\" } to a :regex matcher (Scenario B canonical)" do

    it "normalizes { regex: \"...\" } to a :regex matcher" do

      rule = valid_rule(match: { path: { regex: "^/api/v\\d+/projects" } })

      matcher = rule.match[:path]

      expect(matcher.type).to eq(:regex)
@@ -142,28 +142,28 @@ RSpec.describe Labkit::RateLimit::Rule do 
      expect(matcher.value.source).to eq("^/api/v\\d+/projects")

    end



    it "normalizes a bare Regexp to a :regex matcher (Scenario B Ruby convenience)" do

    it "normalizes a bare Regexp to a :regex matcher" do

      re = %r{^/api/v\d+/projects}

      rule = valid_rule(match: { path: re })

      expect(rule.match[:path]).to have_attributes(type: :regex, value: re)

    end



    it "raises ArgumentError on an invalid regex source (Scenario F)" do

    it "raises ArgumentError on an invalid regex source" do

      expect { valid_rule(match: { path: { regex: "[unclosed" } }) }

        .to raise_error(ArgumentError, /failed to compile/)

    end



    it "raises ArgumentError on { glob: \"...\" } as an unknown type (Scenario H)" do

    it "raises ArgumentError on { glob: \"...\" } as an unknown type" do

      expect { valid_rule(match: { path: { glob: "/api/v*/projects" } }) }

        .to raise_error(ArgumentError, /unknown type key/)

    end



    it "raises ArgumentError on an Array match value (Scenario H)" do

    it "raises ArgumentError on an Array match value" do

      expect { valid_rule(match: { path: ["/a", "/b"] }) }

        .to raise_error(ArgumentError, /single-key Hash/)

    end



    it "raises ArgumentError on a multi-key Hash (Scenario H)" do

    it "raises ArgumentError on a multi-key Hash" do

      expect { valid_rule(match: { path: { regex: "^/a", other: "/b" } }) }

        .to raise_error(ArgumentError, /single-key Hash/)

    end
@@ -187,7 +187,7 @@ RSpec.describe Labkit::RateLimit::Rule do 
    end

  end



  describe "cross-format parity (Scenario I)" do

  describe "cross-format parity (Ruby Hash and YAML)" do

    it "produces equal Matcher hashes from a Ruby Hash and a YAML.safe_load Hash" do

      require "yaml"

      ruby_form = { request_type: "api", path: { regex: "^/api/v\\d+/projects" } }