Verified Commit c9bc814f authored by Hercules Merscher's avatar Hercules Merscher 🌴 Committed by GitLab
Browse files

Merge branch 'sankalp/rate-limit-oneof-matcher' into 'master'

feat: add oneOf set-membership matcher to rate-limit rules

See merge request !341

Merged-by: Hercules Merscher's avatarHercules Merscher <hmerscher@gitlab.com>
Approved-by: Hercules Merscher's avatarHercules Merscher <hmerscher@gitlab.com>
Reviewed-by: Bob Van Landuyt's avatarBob Van Landuyt <bob@gitlab.com>
Reviewed-by: default avatarGitLab Duo <gitlab-duo@gitlab.com>
Co-authored-by: default avatarSankalp <sdas@gitlab.com>
parents 0b5dc535 507d6519
Loading
Loading
Loading
Loading
Loading
+7 −0
Original line number Diff line number Diff line
@@ -174,10 +174,17 @@ A `match` hash gates whether a rule applies. Each value is normalised through
| `Regexp`          | `re`     | `match: { endpoint: %r{\A/api/} }`                     |
| `{ eq: <value> }` | `eq`     | `match: { method: { eq: "POST" } }` (YAML-friendly)    |
| `{ re: <source> }`| `re`     | `match: { endpoint: { re: '\A/api/' } }` (YAML-friendly) |
| `{ oneOf: <Array> }` | `oneOf` | `match: { user: { oneOf: ["7", "42"] } }` (YAML-friendly) |

`re` coerces the identifier value via `#to_s` before matching, so it can be
used against non-String values (e.g. matching a 503 status against `{ re: '^5' }`).

`oneOf` is set membership via `Set#include?` (`eql?`/`hash` equality, no
coercion): it agrees with `eq` for Strings, Symbols, booleans, nil, and
same-class numerics, but cross-type numerics differ (`1 == 1.0`, yet
`{ oneOf: [1] }` does not match `1.0`). A bare Array is rejected; membership must
be spelled `{ oneOf: [...] }` explicitly.

Glob and prefix matchers are intentionally out of scope.

### Evaluation flow
+28 −7
Original line number Diff line number Diff line
# frozen_string_literal: true

require "set"

module Labkit
  module RateLimit
    # Matcher is the internal representation of a single key/value predicate in
@@ -11,6 +13,14 @@ module Labkit
    #   - a Regexp instance                              -> :re matcher (Ruby convenience)
    #   - { eq: <value> }                                -> :eq matcher (canonical, YAML-compatible)
    #   - { re: <String|Regexp> }                        -> :re matcher (canonical, YAML-compatible)
    #   - { oneOf: <Array> }                             -> :oneOf matcher (set membership, YAML-compatible)
    #
    # A bare Array is rejected rather than treated as :oneOf, so a value that
    # was meant as a single matcher hash but arrived as an Array fails loudly
    # instead of silently becoming a membership test.
    #
    # :oneOf membership is Set#include? (eql?/hash equality), not :eq's ==, so
    # cross-type numerics differ (1 == 1.0, but {oneOf: [1]} does not match 1.0).
    #
    # Hash-key naming follows the metrics-catalog selector pattern. Glob,
    # prefix, and other matcher kinds are intentionally out of scope here; see
@@ -20,7 +30,7 @@ module Labkit
    # the regex, so callers can match non-String identifier values such as
    # Integer status codes (e.g. {status: { re: "^5" }} against status: 503).
    class Matcher < Data.define(:type, :value)
      KNOWN_HASH_KEYS = %i[eq re].freeze
      KNOWN_HASH_KEYS = %i[eq re oneOf].freeze
      MAX_REGEX_SOURCE_LENGTH = 200
      ERROR_INSPECT_LIMIT = 80

@@ -42,18 +52,14 @@ module Labkit
        when Hash
          from_hash(input)
        when Array
          raise ArgumentError,
            "rate-limit match value must be a single-key Hash like {re: \"...\"} or {eq: ...}, got #{truncate_for_error(input)}"
          raise ArgumentError, invalid_shape_error(input)
        else
          new(type: :eq, value: input)
        end
      end

      def self.from_hash(input)
        if input.size != 1
          raise ArgumentError,
            "rate-limit match value must be a single-key Hash like {re: \"...\"} or {eq: ...}, got #{truncate_for_error(input)}"
        end
        raise ArgumentError, invalid_shape_error(input) if input.size != 1

        type, source = input.first
        type_sym = type.to_sym
@@ -71,6 +77,13 @@ module Labkit
        case type_sym
        when :eq
          new(type: :eq, value: source)
        when :oneOf
          unless source.is_a?(Array)
            raise ArgumentError,
              "rate-limit match value {oneOf: ...} must be an Array, got #{truncate_for_error(source)}"
          end

          new(type: :oneOf, value: Set.new(source).freeze)
        when :re
          if source.to_s.length > MAX_REGEX_SOURCE_LENGTH
            raise ArgumentError,
@@ -113,6 +126,12 @@ module Labkit

      private_class_method :with_match_timeout

      def self.invalid_shape_error(input)
        "rate-limit match value must be a single-key Hash like {re: \"...\"}, {eq: ...} or {oneOf: [...]}, " \
          "got #{truncate_for_error(input)}"
      end
      private_class_method :invalid_shape_error

      def self.truncate_for_error(value)
        s = value.inspect
        s.length > ERROR_INSPECT_LIMIT ? "#{s[0, ERROR_INSPECT_LIMIT]}...(truncated)" : s
@@ -123,6 +142,8 @@ module Labkit
        case type
        when :eq
          value == identifier_value
        when :oneOf
          value.include?(identifier_value)
        when :re
          value.match?(identifier_value.to_s)
        else
+59 −0
Original line number Diff line number Diff line
@@ -137,6 +137,36 @@ RSpec.describe Labkit::RateLimit::Matcher do
      end
    end

    context "with the canonical { oneOf: [...] } shape" do
      it "builds a :oneOf matcher holding the members as a Set" do
        m = described_class.build(oneOf: %w[7 42])
        expect(m.type).to eq(:oneOf)
        expect(m.value).to eq(Set.new(%w[7 42]))
        expect(m.value).to be_frozen
      end

      it "accepts a string-keyed hash too (mirrors YAML.safe_load output)" do
        m = described_class.build("oneOf" => %w[7 42])
        expect(m).to eq(described_class.build(oneOf: %w[7 42]))
      end

      it "accepts an empty Array (matches nothing)" do
        m = described_class.build(oneOf: [])
        expect(m.match?("7")).to be(false)
        expect(m.match?(nil)).to be(false)
      end

      it "raises ArgumentError when the source is not an Array" do
        expect { described_class.build(oneOf: "7,42") }
          .to raise_error(ArgumentError, /must be an Array/)
      end

      it "raises ArgumentError when the source is nil" do
        expect { described_class.build(oneOf: nil) }
          .to raise_error(ArgumentError, /must be an Array/)
      end
    end

    context "with unsupported shapes" do
      it "rejects an Array" do
        expect { described_class.build(["/a", "/b"]) }
@@ -261,6 +291,35 @@ RSpec.describe Labkit::RateLimit::Matcher do
      end
    end

    context "with a :oneOf matcher" do
      let(:matcher) { described_class.build(oneOf: ["7", "42", 99]) }

      it "returns true for a member value" do
        expect(matcher.match?("7")).to be(true)
        expect(matcher.match?(99)).to be(true)
      end

      it "returns false for a non-member value" do
        expect(matcher.match?("8")).to be(false)
      end

      it "compares without coercion" do
        # 99 is a member as an Integer; the String "99" is a different value.
        expect(matcher.match?("99")).to be(false)
        expect(matcher.match?(7)).to be(false)
      end

      it "uses eql?/hash equality, so cross-type numerics do not match (unlike :eq's ==)" do
        expect(described_class.build(eq: 1).match?(1.0)).to be(true)
        expect(described_class.build(oneOf: [1]).match?(1.0)).to be(false)
      end

      it "returns false for nil unless nil is a member" do
        expect(matcher.match?(nil)).to be(false)
        expect(described_class.build(oneOf: [nil]).match?(nil)).to be(true)
      end
    end

    context "with a :re matcher built from a bare Regexp" do
      it "matches identically to the canonical { re: \"...\" } form" do
        from_hash = described_class.build(re: "^/api/v\\d+/projects")