refactor(abuse)!: make adapters immutable - #14056
ChiragAgg5k wants to merge 16 commits into
Conversation
…rams Adapters cached the window timestamp and count at construction and mutated their key via setParam, so a reused instance kept counting into a stale window and leaked params between callers. Adapter is now readonly with withParams clones, check/peek return a Result, the window is derived from now() on every call, and the Redis family hits through one atomic Lua script.
Adds the readonly Adapter with withParams/key, the Result value object, the TimeLimit base with a now() clock seam, and the Redis/RedisCluster/RedisPool/None adapters behind a shared RedisBase that hits through one atomic Lua script. Unit tests drive the base through an in-memory adapter with a movable clock.
…utable contract hit() now returns the pre-hit count and only writes below the limit, count() is a read-only lookup, and nothing is cached on the instance so withParams() clones stay correct across windows.
Window and elapsed fraction are derived from a now() seam on every call, so a reused instance never carries stale window state; the Lua script returns the pre-hit estimate so Result can be built uniformly across algorithms.
Adapters no longer cache the timestamp or bucket state, so a reused or cloned instance always reads a fresh refill. check() returns a Result, and both Lua scripts return the integer tokens used before the call, with every key passed in KEYS for Dragonfly. The refillRate guard now applies only when tokens > 0, so limit 0 (unlimited) can be built from limit / interval.
ReCaptcha never fit the rate-limit Adapter contract (reset/getLogs/cleanup all threw), so it becomes a final readonly verifier with an injectable PSR-18 client, defaulting to utopia-php/client over cURL. The package now targets PHP 8.5 and the README examples follow the immutable withParams()/Result API.
Cover remaining, peek, reset time, withParams isolation and window rollover on a single instance; align assertions to window starts so clock-sensitive checks cannot straddle a boundary.
…ch to immutable API Reuse one adapter instance across windows and refills so the suites catch state frozen at construction, and cover withParams clones counting independently.
Callers now pass params through withParams() clones and read check()/peek() Results, so headers come from one atomic round trip instead of separate remaining()/limit()/time() reads, and key/API-key paths emit headers via peek() without recording a hit.
…TokenBucket Match TimeLimit\RedisBase so a broken eval surfaces instead of silently allowing the request; fail-open stays the job of subclasses that override eval (e.g. Cloud Pool breakers).
The pool suites prefix keys in getAdapter, so hard-coded keys failed RedisPoolTest and RedisPoolClusterTest.
The immutable rewrite typed the scan and log replies that the 44 baseline entries covered.
Each check now reads the clock, so three database round trips in a one-second window could straddle a second boundary on a cold MySQL and flake testStaticKey.
🟢 Tier S · Ready to merge
Refactors the abuse package to immutable adapters with parameter-copy methods and a shared Result value object, and migrates Appwrite callers to the new API. Separates ReCaptcha verification from rate limiting and updates package configuration, documentation, and regression coverage, including fractional token-bucket reset calculations. The newest change checks decoded ReCaptcha form fields instead of an exact serialized request body. Latest changes: Updates the ReCaptcha request test to decode the form body and assert field values rather than requiring one exact URL-encoded string.
📂 Walkthrough · 19
Reviewed the commits since |
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after)
|
The Redis scripts floored the balance before returning, so reset overstated the time until the bucket refills, and peek() reported the post-consume refill time. Adapters now return the refilled balance and TokenBucket subtracts the consumed token only on check().
Closes #14046
Summary
utopia-php/abuseadapters were stateful.setParam()mutated them,parseKey()overwrote the key pattern so later params were ignored, and TimeLimit cached its count and window start on the instance, so a reused adapter counted into an expired window. The adapters are now immutable, matchingpackages/client,circuit-breaker,scheduleandsmtp. This is a breaking change, released as abuse 3.0.0.Changes
setParam()mutates and returns$thiswithParams()/withParam()return a clone; the key is resolved per call withstrtr()check(): bool, plusremaining(),limit(),time()check(): Resultandpeek(): Result$countand$timestampon the instanceprotectedpropertiesreadonlyclasses with promoted constructor properties; concrete adapters arefinalUtopia\Abuse\AbusewrapperUtopia\Abuse\Adapters\*Utopia\Abuse\Adapter\*ReCaptcha extends Adapter, raw curlfinal readonly ReCaptcha::verify()onutopia-php/clientRuntimeException, like TimeLimitAppwrite call sites migrated:
shared/api.php(init + shutdown),init/resources.php,realtime.php,Mqtt/Handler.php,Functions/Create.php,Workers/Deletes.php.X-RateLimit-*headers are unchanged. The oldremainingwas read before the hit aslimit - (count + 1), which equals the post-hit remaining thatcheck()now returns. When_APP_OPTIONS_ABUSEis disabled, headers come frompeek(), so no hit is recorded (same as before).Test plan
APPWRITE_ENDPOINT)withParams, and window rollover on the same instancecomposer lint,analyze,refactor:checkabuseEnabledgroupFollow-up (Cloud, after 3.0.0 is tagged)
Pool/Nonesubclasses of TimeLimit, SlidingWindow and TokenBucket to the newhit/count/setcontract; the breaker fallback must cover the new throw-on-bad-reply behaviour.app/http.php:(new Abuse($timeLimit))->check()→$timeLimit->check()->limited.app/realtime.phpgetTimelimit(): stop caching one adapter per coroutine; build it, or callwithParams(), per request.