Add cacheQuery() a fluent way to cache custom repository methods - #14
Merged
Conversation
…pository Caching a custom repository method today means calling RepositoryCache::remember() directly with 11 parameters, most of them boilerplate the repository already knows about itself (its own model, its own cacheScope()). It also silently ignores withoutCache()/lockForUpdate(), unlike the built-in fetch* methods. cacheQuery() wraps that call: a concrete repository only supplies a method name, a callback, and whatever's actually unique to that query (extra key material, relations, tier/recordId). It respects withoutCache() and skips caching while a pessimistic lock is active, matching fetchAll()/fetchById() exactly. Invalidation needs no new code as long as writes go through create()/update()/delete() — every cacheQuery() entry carries the model's base tag, the same one those already bust. flushRecordCache() is the missing granular sibling of the existing flushCache()/ flushAllCache() convenience methods, for a custom write method that wants to bust one identifiable cache entry instead of the whole model's cache.
…uery builder The first pass at cacheQuery() was a 7-parameter flat method returning mixed — the same shape of problem it was meant to fix, one size smaller. Laravel's own answer to "a few optional config knobs, one terminal action" is a fluent Pending* builder (Http's PendingRequest, Mail's PendingMail), not a wide parameter list. cacheQuery(string $method) now returns PendingCacheQuery, mirroring the protected query(): Builder entry point already established in this same class. Configuration is fluent (withKey(), with(), asRecord()); remember() is the terminal action, named to match Cache::remember() on purpose. Also drops the unused $columns parameter — neither real-world example (a public listing, a per-shop balance aggregate) varies it, so it isn't part of the common-path surface. Still reachable via the low-level RepositoryCache::remember() for the rare case that needs it. with() reuses BaseRepository::with()'s own array|string shape instead of inventing a second convention for the same concept.
…ll stack cacheQuery(string $method) still asked every call site to pass __FUNCTION__ by hand — pure boilerplate, since the value is always the calling method's own name and PHP already guarantees that's unique per class (no two methods on the same class can share a name). cacheQuery() now takes no arguments at all, matching query(): Builder's own zero-arg shape exactly, and resolves the caller via debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 2) — the same mechanism Illuminate\Support's own once() helper (Laravel 11+) already uses to identify its caller without requiring an explicit key. Added a regression test (two zero-argument, otherwise-identical Listing-tier cacheQuery() calls on the same repository) proving this is what actually keeps them from colliding on the same cache key.
…re __FUNCTION__ The backtrace-derived method name looked like free ergonomics but wasn't — checked both the performance and the risk properly: Performance: microbenchmarked at ~0.1 microsec/call, 7x slower than passing a literal. Individually trivial, but it's paid on every single cacheQuery() call INCLUDING cache hits — the one path caching exists to make fast — for zero benefit over a compile-time constant. Correctness: reproduced a real, silent collision. The moment a repository author adds one private helper to DRY up two similarly-shaped cached methods (a normal refactor, not a misuse), debug_backtrace()'s frame 1 resolves to the shared helper's name for BOTH public methods, not either of theirs — they silently share a cache key. Demonstrated with a minimal reproduction; see PR discussion. once() (the precedent cited for this) is also considerably more careful than what was implemented here — it hashes file+line+class+closure- use-variables specifically to guard against this, not just a bare function name. cacheQuery(string $method) is back, called as cacheQuery(__FUNCTION__). __FUNCTION__ is a compile-time constant (not a function call) — it costs nothing at runtime and cannot silently collide the way a shared call site can.
PendingCacheQuery is explicitly modeled on Illuminate\Http\Client\PendingRequest
in its docblock, but was missing something that class actually has:
PendingRequest itself composes `use Conditionable, Macroable;`. Added
Conditionable here for the same reason it's there — when()/unless() to
configure the pending query conditionally without breaking the fluent chain,
e.g.:
return $this->cacheQuery(__FUNCTION__)
->when($liveOnly, fn ($query) => $query->asRecord($shopId))
->remember(fn () => ...);
Skipped Macroable — nothing else in this package uses it, and core-foundation
already has its own extension mechanism (the structured ServiceProvider hooks
in base-extension-service-provider.md). Adding a second, unrelated extension
path for one class would be less consistent with the package, not more.
Added a test proving when() actually threads through PendingCacheQuery
correctly (both branches), not just that the trait compiles.
… cacheQuery() $this->cache->remember() was called twice with the same shape: once directly inside each of fetchAll()/fetchById()/fetchOneByCriteria()/getByCriteria()/ firstByCriteria()/count()/exists(), and again inside PendingCacheQuery::remember() for custom methods. Two of the eleven parameters (criteria, columns) were placeholders PendingCacheQuery faked because it had no filter/column concept of its own yet. Gave PendingCacheQuery criteria()/columns()/dontCache(), then rewrote all seven built-in methods to build one via cacheQuery() instead of calling RepositoryCache::remember() directly. Each method expresses exactly its own existing shouldCache logic via ->when(extraCondition, fn ($q) => $q->dontCache()) for whatever it needs beyond what cacheQuery() already covers (bypassCache() and an active pessimistic lock). $this->cache->remember() now has exactly one caller in the whole file: PendingCacheQuery::remember(). Verified equivalence method by method before touching anything: - fetchAll/fetchById already checked lock mode; the extra when() clause is exactly their remaining condition (cachedMethods() gate, +pagination for fetchAll). - fetchOneByCriteria/firstByCriteria used QueryType::Record with no recordId, which RepositoryCache::buildTags() already falls back to the listing tag for — identical to PendingCacheQuery's Listing default, so no asRecord() call was needed to match. - getByCriteria/firstByCriteria called applyLock() on the query but never checked lock mode before caching — a latent bug, now closed as a disclosed side effect: cacheQuery() already gates on lock mode for every caller. fetchOneByCriteria/count/exists never supported locking either way. Zero existing tests needed modification — all 391 passed unchanged, proving the rewrite is behaviorally identical apart from the disclosed fix. Added one new regression test for that fix specifically (test_get_by_criteria_skips_cache_while_a_pessimistic_lock_is_pending).
…anges Audited every method/behavior added or touched across the branch against actual test coverage and found real gaps, not just thin spots: - fetchOneByCriteria() and firstByCriteria() had ZERO test coverage, before or after the cacheQuery() migration. Added basic + not-found-case tests for both, plus getByCriteria()'s normal (non-locked) path, which was only exercised by the lock regression test. - fetchAll()'s "paginated results are never cached" behavior (the exact condition the fetchAll() rewrite's when() clause encodes) had no direct test — every existing fetchAll() test explicitly passes paginate: false. - PendingCacheQuery::columns() and criteria() (used standalone by a custom method, not through a built-in) had no coverage — only exercised indirectly via the built-ins that now route through them. - Conditionable::unless() was untested — only when() had a test. Added fixtures (countUnlessId, countByStatus, cachedCountWithColumns) to TestPostRepository and one test per gap. 401 passed (was 392), Pint clean, PHPStan 0 errors.
…e HasServiceCache bridge Raised while discussing this branch: should repository caching and service caching be merged into one class so a service result and a repository read stay coordinated? No — that coordination already exists and predates this branch entirely (HasServiceCache + CacheDependency, composed into BaseService already), it was just completely undocumented. Nothing in resources/boost/ skills mentioned it, so neither a human nor an agent would know it existed. base-repository.md: - "When to Cache a Custom Repository Method" — was only ever added to a consuming app's local skill copy, referencing the old flat $this->cache->remember() call. Added properly here using the actual cacheQuery()/PendingCacheQuery fluent API this branch ships. - New: "Choosing Invalidation Granularity" — the listing-tier-busts-on-every- write behavior is correct, not a bug (spelled out explicitly, since it's the single most likely thing someone tries to "fix" and breaks correctness instead), contrasted with asRecord()/flushRecordCache() for a genuinely per-item access pattern. Uses the "1000 products" scenario directly. base-service.md: - New: "Caching a Service Result — HasServiceCache, Not a New Cache Class" — documents rememberWithDependencies() and CacheDependency::onRecord()/ onRecords()/onListing(), explains the shared-tag-namespace mechanism (zero coupling between the two classes, the tag string is the whole contract), and shows the same "1000 products" scenario done right (onRecords() for a known subset) vs. wrong (onListing() over a computation that only actually depends on a few records). SKILL.md updated to surface both in the quick-reference and the skill's trigger description.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.