refactor(rate_limit): adopt :eq / :re hash keys per review (ef6dc8e8) · Commits · GitLab.org / ruby / gems / labkit-ruby

lib/labkit/rate_limit/matcher.rb

+28 −23

Original line number	Diff line number	Diff line
		@@ -7,40 +7,47 @@ module Labkit
		# Matcher.build; the Evaluator calls Matcher#match? per identifier value.
		#
		# Accepted input shapes (everything else raises ArgumentError):
		# - any plain value (String, Symbol, Integer, ...) -> :equality matcher
		# - a Regexp instance -> :regex matcher (Ruby convenience)
		# - { regex: "<source>" } single-key Hash -> :regex matcher (canonical, YAML-compatible)
		# - any plain value (String, Symbol, Integer, ...) -> :eq matcher
		# - a Regexp instance -> :re matcher (Ruby convenience)
		# - { eq: <value> } / { equality: <value> } -> :eq matcher (canonical, YAML-compatible)
		# - { re: <String\|Regexp> } -> :re matcher (canonical, YAML-compatible)
		#
		# See Spec 10 (gitlab-com/gl-infra/production-engineering#28855) for the
		# full contract; glob support is intentionally out of scope.
		# Hash-key naming follows the metrics-catalog selector pattern. Glob,
		# prefix, and other matcher kinds are intentionally out of scope here; see
		# gitlab-com/gl-infra/production-engineering#28853 for that follow-up.
		class Matcher < Data.define(:type, :value)
		KNOWN_HASH_KEYS = %i[regex].freeze
		KNOWN_HASH_KEYS = %i[eq equality re].freeze
		MAX_REGEX_SOURCE_LENGTH = 200
		ERROR_INSPECT_LIMIT = 80

		def self.build(input)
		case input
		when Regexp
		new(type: :regex, value: input)
		new(type: :re, value: input)
		when Hash
		from_hash(input)
		when Array
		raise ArgumentError,
		"rate-limit match value must be a single-key Hash like {regex: \"...\"}, got #{truncate_for_error(input)}"
		"rate-limit match value must be a single-key Hash like {re: \"...\"} or {eq: ...}, got #{truncate_for_error(input)}"
		else
		new(type: :equality, value: input)
		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 {regex: \"...\"}, got #{truncate_for_error(input)}"
		"rate-limit match value must be a single-key Hash like {re: \"...\"} or {eq: ...}, got #{truncate_for_error(input)}"
		end

		type, source = input.first
		type_sym = type.to_sym

		if type_sym == :regex
		raise ArgumentError,
		"rate-limit match value uses removed key :regex; use :re instead, e.g. {re: \"^/api\"}"
		end

		unless KNOWN_HASH_KEYS.include?(type_sym)
		raise ArgumentError,
		"rate-limit match value has unknown type key #{truncate_for_error(type)}; accepted: #{KNOWN_HASH_KEYS.inspect}"
		@@ -52,21 +59,19 @@ module Labkit

		def self.compile(type_sym, source)
		case type_sym
		when :regex
		unless source.is_a?(String)
		when :eq, :equality
		new(type: :eq, value: source)
		when :re
		if source.is_a?(String) && source.length > MAX_REGEX_SOURCE_LENGTH
		raise ArgumentError,
		"rate-limit match value {regex: ...} requires a String source, got #{source.class}"
		end

		if source.length > MAX_REGEX_SOURCE_LENGTH
		raise ArgumentError,
		"rate-limit match value {regex: ...} source exceeds #{MAX_REGEX_SOURCE_LENGTH} characters"
		"rate-limit match value {re: ...} source exceeds #{MAX_REGEX_SOURCE_LENGTH} characters"
		end

		begin
		new(type: :regex, value: Regexp.new(source))
		rescue RegexpError => e
		raise ArgumentError, "rate-limit match value {regex: #{source.inspect}} failed to compile: #{e.message}"
		new(type: :re, value: Regexp.new(source))
		rescue RegexpError, TypeError => e
		raise ArgumentError,
		"rate-limit match value {re: #{truncate_for_error(source)}} failed to compile: #{e.message}"
		end
		end
		end
		@@ -80,9 +85,9 @@ module Labkit

		def match?(identifier_value)
		case type
		when :equality
		when :eq
		value == identifier_value
		when :regex
		when :re
		identifier_value.is_a?(String) && value.match?(identifier_value)
		else
		raise ArgumentError, "unknown matcher type: #{type.inspect}"

spec/labkit/rate_limit/evaluator_spec.rb

+42 −9

Original line number	Diff line number	Diff line
		@@ -402,7 +402,7 @@ RSpec.describe Labkit::RateLimit::Evaluator do
		end
		end

		describe "with an :equality matcher" do
		describe "with an :eq 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")
		@@ -414,14 +414,26 @@ RSpec.describe Labkit::RateLimit::Evaluator do
		expect(result.matched?).to be(true)
		expect(result.rule).to eq(rule)
		end

		it "matches via { eq: ... }" do
		rule = make_rule(name: "api_endpoint",
		match: { endpoint: { eq: "GET /api/v4/projects" } },
		characteristics: [:user])
		id = Labkit::RateLimit::Identifier.new(user: 42, endpoint: "GET /api/v4/projects")

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

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

		describe "with a :regex matcher" do
		describe "with a :re matcher" do
		let(:id) { Labkit::RateLimit::Identifier.new(user: 42, path: "/api/v4/projects/42") }

		it "matches via { regex: \"...\" }" do
		it "matches via { re: \"...\" }" do
		rule = make_rule(name: "projects_api",
		match: { path: { regex: "^/api/v\\d+/projects" } },
		match: { path: { re: "^/api/v\\d+/projects" } },
		characteristics: [:user])
		expect(pipe).to receive(:incr).and_return(nil)
		expect(raw_redis).to receive(:expire)
		@@ -439,9 +451,30 @@ RSpec.describe Labkit::RateLimit::Evaluator do
		expect(evaluator(rules: [rule]).check(id).matched?).to be(true)
		end

		it "matches via { re: <Regexp> }" do
		rule = make_rule(name: "projects_api",
		match: { path: { re: %r{^/api/v\d+/projects} } },
		characteristics: [:user])
		expect(pipe).to receive(:incr).and_return(nil)
		expect(raw_redis).to receive(:expire)

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

		it "AND-matches across mixed { eq: ... } and { re: ... } shapes" do
		rule = make_rule(name: "projects_api",
		match: { request_type: { eq: "api" }, path: { re: "^/api/v\\d+/projects" } },
		characteristics: [:user])
		mixed_id = Labkit::RateLimit::Identifier.new(user: 42, request_type: "api", path: "/api/v4/projects/42")
		expect(pipe).to receive(:incr).and_return(nil)
		expect(raw_redis).to receive(:expire)

		expect(evaluator(rules: [rule]).check(mixed_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" } },
		match: { path: { re: "^/api/v\\d+/projects" } },
		characteristics: [:user, :path])
		counter_id = Labkit::RateLimit::Identifier.new(user: 42, path: "/api/v4/projects")

		@@ -455,7 +488,7 @@ RSpec.describe Labkit::RateLimit::Evaluator 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" } })
		rule = make_rule(match: { path: { re: "^/admin" } })
		no_match_id = Labkit::RateLimit::Identifier.new(user: 42, path: "/api/v4/projects")
		expect(raw_redis).not_to receive(:pipelined)

		@@ -465,7 +498,7 @@ RSpec.describe Labkit::RateLimit::Evaluator do

		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" } })
		rule = make_rule(match: { user: { re: "42" } })
		int_id = Labkit::RateLimit::Identifier.new(user: 42)
		expect(raw_redis).not_to receive(:pipelined)

		@@ -474,7 +507,7 @@ RSpec.describe Labkit::RateLimit::Evaluator do
		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)
		regex_rule = make_rule(name: "regex_first", match: { user: { re: "42" } }, action: :block)
		eq_rule = make_rule(name: "eq_second", match: { user: 42 }, action: :log)
		int_id = Labkit::RateLimit::Identifier.new(user: 42)

		@@ -491,7 +524,7 @@ RSpec.describe Labkit::RateLimit::Evaluator 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" } },
		match: { request_type: "api", path: { re: "^/api/v\\d+/projects" } },
		characteristics: [:user])
		end

spec/labkit/rate_limit/matcher_spec.rb

+94 −38

Original line number	Diff line number	Diff line
		@@ -5,76 +5,121 @@ require "spec_helper"
		RSpec.describe Labkit::RateLimit::Matcher do
		describe ".build" do
		context "with plain values (equality matchers)" do
		it "wraps a String as :equality" do
		it "wraps a String as :eq" do
		m = described_class.build("/api/v4/projects")
		expect(m.type).to eq(:equality)
		expect(m.type).to eq(:eq)
		expect(m.value).to eq("/api/v4/projects")
		end

		it "wraps a Symbol as :equality" do
		it "wraps a Symbol as :eq" do
		m = described_class.build(:api)
		expect(m.type).to eq(:equality)
		expect(m.type).to eq(:eq)
		expect(m.value).to eq(:api)
		end

		it "wraps an Integer as :equality" do
		it "wraps an Integer as :eq" do
		m = described_class.build(42)
		expect(m.type).to eq(:equality)
		expect(m.type).to eq(:eq)
		expect(m.value).to eq(42)
		end

		it "wraps booleans as :equality" do
		expect(described_class.build(true)).to have_attributes(type: :equality, value: true)
		expect(described_class.build(false)).to have_attributes(type: :equality, value: false)
		it "wraps booleans as :eq" do
		expect(described_class.build(true)).to have_attributes(type: :eq, value: true)
		expect(described_class.build(false)).to have_attributes(type: :eq, value: false)
		end

		it "wraps nil as :equality" do
		expect(described_class.build(nil)).to have_attributes(type: :equality, value: nil)
		it "wraps nil as :eq" do
		expect(described_class.build(nil)).to have_attributes(type: :eq, value: nil)
		end
		end

		context "with a bare Regexp (Ruby convenience)" do
		it "stores the Regexp directly as :regex (no recompilation)" do
		it "stores the Regexp directly as :re (no recompilation)" do
		re = %r{^/api/v\d+}
		m = described_class.build(re)
		expect(m.type).to eq(:regex)
		expect(m.type).to eq(:re)
		expect(m.value).to equal(re) # same object, not a re-compiled copy
		end
		end

		context "with the canonical { regex: \"...\" } shape" do
		it "compiles the source into a Regexp at construction time" do
		m = described_class.build(regex: "^/api/v\\d+/projects")
		expect(m.type).to eq(:regex)
		context "with the canonical { eq: ... } / { equality: ... } shapes" do
		it "treats { eq: x } identically to a bare x for any plain value" do
		["api", :api, 42, true, nil].each do \|v\|
		eq_form = described_class.build(eq: v)
		plain_form = described_class.build(v)
		expect(eq_form).to eq(plain_form),
		"diverged on #{v.inspect}: #{eq_form.inspect} != #{plain_form.inspect}"
		expect(eq_form.type).to eq(:eq)
		end
		end

		it "accepts { equality: x } as an alias for { eq: x }" do
		long = described_class.build(equality: "api")
		short = described_class.build(eq: "api")
		expect(long).to eq(short)
		expect(long.type).to eq(:eq)
		end

		it "accepts a string-keyed hash too (mirrors YAML.safe_load output)" do
		m = described_class.build("eq" => "api")
		expect(m).to eq(described_class.build(eq: "api"))
		end
		end

		context "with the canonical { re: ... } shape" do
		it "compiles a String source into a Regexp at construction time" do
		m = described_class.build(re: "^/api/v\\d+/projects")
		expect(m.type).to eq(:re)
		expect(m.value).to be_a(Regexp)
		expect(m.value.source).to eq("^/api/v\\d+/projects")
		end

		it "accepts a Regexp inside { re: ... } and stores it directly" do
		re = %r{^/api/v\d+/projects}
		m = described_class.build(re: re)
		expect(m.type).to eq(:re)
		expect(m.value).to eq(re)
		end

		it "accepts a string-keyed hash too (mirrors YAML.safe_load output)" do
		m = described_class.build("regex" => "^/api/v\\d+/projects")
		expect(m.type).to eq(:regex)
		m = described_class.build("re" => "^/api/v\\d+/projects")
		expect(m.type).to eq(:re)
		expect(m.value.source).to eq("^/api/v\\d+/projects")
		end

		it "raises ArgumentError when the regex source is not a String" do
		expect { described_class.build(regex: 42) }
		.to raise_error(ArgumentError, /requires a String source/)
		it "raises ArgumentError when the source is neither a String nor a Regexp (TypeError wrapped)" do
		expect { described_class.build(re: 42) }
		.to raise_error(ArgumentError, /failed to compile/)
		end

		it "raises ArgumentError when the regex source is nil" do
		expect { described_class.build(regex: nil) }
		.to raise_error(ArgumentError, /requires a String source/)
		it "raises ArgumentError when the source is nil (TypeError wrapped)" do
		expect { described_class.build(re: nil) }
		.to raise_error(ArgumentError, /failed to compile/)
		end

		it "raises ArgumentError on invalid regex syntax (RegexpError wrapped)" do
		expect { described_class.build(regex: "[unclosed") }
		expect { described_class.build(re: "[unclosed") }
		.to raise_error(ArgumentError, /failed to compile/)
		end

		it "raises ArgumentError when the regex source exceeds 200 characters" do
		expect { described_class.build(regex: "a" * 201) }
		it "caps a String source at 200 characters" do
		expect { described_class.build(re: "a" * 201) }
		.to raise_error(ArgumentError, /exceeds 200/)
		end

		it "does not apply the 200-char cap to a Regexp source" do
		# The Regexp's source string can be longer than 200 chars without
		# tripping the cap -- it's already a compiled Regexp, not user-typed input.
		long_re = Regexp.new("a" * 201)
		expect { described_class.build(re: long_re) }.not_to raise_error
		end
		end

		context "with the removed { regex: ... } key" do
		it "raises ArgumentError naming :re as the replacement" do
		expect { described_class.build(regex: "^/api") }
		.to raise_error(ArgumentError, /removed key :regex.*use :re/m)
		end
		end

		context "with unsupported shapes" do
		@@ -89,7 +134,7 @@ RSpec.describe Labkit::RateLimit::Matcher do
		end

		it "rejects a multi-key Hash" do
		expect { described_class.build(regex: "^/a", other: "/a") }
		expect { described_class.build(re: "^/a", other: "/a") }
		.to raise_error(ArgumentError, /single-key Hash/)
		end

		@@ -135,7 +180,7 @@ RSpec.describe Labkit::RateLimit::Matcher do
		end

		describe "#match?" do
		context "with an :equality matcher" do
		context "with an :eq matcher" do
		let(:matcher) { described_class.build("api") }

		it "returns true for an equal value" do
		@@ -157,8 +202,8 @@ RSpec.describe Labkit::RateLimit::Matcher do
		end
		end

		context "with a :regex matcher" do
		let(:matcher) { described_class.build(regex: "^/api/v\\d+/projects") }
		context "with a :re matcher" do
		let(:matcher) { described_class.build(re: "^/api/v\\d+/projects") }

		it "returns true when the identifier value matches" do
		expect(matcher.match?("/api/v4/projects/42")).to be(true)
		@@ -183,9 +228,9 @@ RSpec.describe Labkit::RateLimit::Matcher do
		end
		end

		context "with a :regex matcher built from a bare Regexp" do
		it "matches identically to the canonical { regex: \"...\" } form" do
		from_hash = described_class.build(regex: "^/api/v\\d+/projects")
		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")
		from_bare = described_class.build(%r{^/api/v\d+/projects})

		["/api/v4/projects/42", "/admin", "no slashes"].each do \|v\|
		@@ -194,6 +239,17 @@ RSpec.describe Labkit::RateLimit::Matcher do
		end
		end

		context "with a :re matcher built from { re: <Regexp> }" do
		it "matches the same way as a bare Regexp" do
		from_nested = described_class.build(re: %r{^/api/v\d+/projects})
		from_bare = described_class.build(%r{^/api/v\d+/projects})

		["/api/v4/projects/42", "/admin"].each do \|v\|
		expect(from_nested.match?(v)).to eq(from_bare.match?(v)), "diverged on #{v.inspect}"
		end
		end
		end

		context "with an unknown matcher type (bypassing .build)" do
		it "raises ArgumentError" do
		matcher = described_class.new(type: :glob, value: "x")
		@@ -205,13 +261,13 @@ RSpec.describe Labkit::RateLimit::Matcher do
		describe "value equality (Data.define)" do
		it "treats two equality matchers with the same value as equal" do
		first = described_class.build("api")
		second = described_class.build("api")
		second = described_class.build(eq: "api")
		expect(first).to eq(second)
		end

		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+")
		ruby_form = described_class.build(re: "^/api/v\\d+")
		yaml_form = described_class.build("re" => "^/api/v\\d+")
		expect(ruby_form).to eq(yaml_form)
		end
		end

spec/labkit/rate_limit/rule_spec.rb

+40 −12

Original line number	Diff line number	Diff line
		@@ -129,30 +129,58 @@ RSpec.describe Labkit::RateLimit::Rule do
		end

		describe "match value normalization" do
		it "normalizes a plain value to an :equality matcher" do
		it "normalizes a plain value to an :eq matcher" do
		rule = valid_rule(match: { endpoint: "/api/v4" })
		expect(rule.match[:endpoint]).to have_attributes(type: :equality, value: "/api/v4")
		expect(rule.match[:endpoint]).to have_attributes(type: :eq, value: "/api/v4")
		end

		it "normalizes { regex: \"...\" } to a :regex matcher" do
		rule = valid_rule(match: { path: { regex: "^/api/v\\d+/projects" } })
		it "normalizes { eq: x } to an :eq matcher equivalent to a bare x" do
		from_hash = valid_rule(match: { endpoint: { eq: "/api/v4" } })
		from_plain = valid_rule(match: { endpoint: "/api/v4" })
		expect(from_hash.match[:endpoint]).to eq(from_plain.match[:endpoint])
		expect(from_hash.match[:endpoint].type).to eq(:eq)
		end

		it "normalizes { equality: x } as an alias of { eq: x }" do
		rule = valid_rule(match: { endpoint: { equality: "/api/v4" } })
		expect(rule.match[:endpoint]).to have_attributes(type: :eq, value: "/api/v4")
		end

		it "normalizes { re: \"...\" } to a :re matcher" do
		rule = valid_rule(match: { path: { re: "^/api/v\\d+/projects" } })
		matcher = rule.match[:path]
		expect(matcher.type).to eq(:regex)
		expect(matcher.type).to eq(:re)
		expect(matcher.value).to be_a(Regexp)
		expect(matcher.value.source).to eq("^/api/v\\d+/projects")
		end

		it "normalizes a bare Regexp to a :regex matcher" do
		it "normalizes a bare Regexp to a :re 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)
		expect(rule.match[:path]).to have_attributes(type: :re, value: re)
		end

		it "normalizes { re: <Regexp> } to a :re matcher" do
		re = %r{^/api/v\d+/projects}
		rule = valid_rule(match: { path: { re: re } })
		expect(rule.match[:path]).to have_attributes(type: :re, value: re)
		end

		it "raises ArgumentError on an invalid regex source" do
		expect { valid_rule(match: { path: { regex: "[unclosed" } }) }
		expect { valid_rule(match: { path: { re: "[unclosed" } }) }
		.to raise_error(ArgumentError, /failed to compile/)
		end

		it "raises ArgumentError on a non-String/non-Regexp regex source (TypeError wrapped)" do
		expect { valid_rule(match: { path: { re: 42 } }) }
		.to raise_error(ArgumentError, /failed to compile/)
		end

		it "raises ArgumentError on the removed :regex hash key, naming :re as the replacement" do
		expect { valid_rule(match: { path: { regex: "^/api" } }) }
		.to raise_error(ArgumentError, /removed key :regex.*use :re/m)
		end

		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/)
		@@ -164,7 +192,7 @@ RSpec.describe Labkit::RateLimit::Rule do
		end

		it "raises ArgumentError on a multi-key Hash" do
		expect { valid_rule(match: { path: { regex: "^/a", other: "/b" } }) }
		expect { valid_rule(match: { path: { re: "^/a", other: "/b" } }) }
		.to raise_error(ArgumentError, /single-key Hash/)
		end

		@@ -176,7 +204,7 @@ RSpec.describe Labkit::RateLimit::Rule do
		original.call(args, *kwargs)
		end

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

		# Re-reading the value should not recompile.
		@@ -190,11 +218,11 @@ RSpec.describe Labkit::RateLimit::Rule 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" } }
		ruby_form = { request_type: "api", path: { re: "^/api/v\\d+/projects" } }
		yaml_form = YAML.safe_load(<<~YAML, symbolize_names: true)
		request_type: api
		path:
		regex: "^/api/v\\\\d+/projects"
		re: "^/api/v\\\\d+/projects"
		YAML

		ruby_rule = valid_rule(match: ruby_form)