refactor(usage): collapse the Usage facade into the adapter base, rename it Usage#16
refactor(usage): collapse the Usage facade into the adapter base, rename it Usage#16loks0n wants to merge 2 commits into
Conversation
Usage was a pure delegating layer: every method just forwarded to the adapter with an identical signature, duplicating Adapter's docblocks verbatim and forcing the adapters to depend on Usage for their own TYPE_* constants (a backwards dependency). The only behaviour it added was a type guard in sum(). - Delete Usage; Accumulator and Tenant now wrap an Adapter directly. - Move TYPE_EVENT/TYPE_GAUGE onto Adapter (where they belong) and add a single Adapter::assertType() that replaces the 5 duplicated event/gauge guards across Accumulator, ClickHouse and Database. - getAdapter() is gone — the adapter is the entry point. Net negative LoC, no behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
With the pass-through facade gone, the abstract contract every backend implements is the public entry point — so `Usage` is the right name for it: `new ClickHouse(...)` is a `Usage`. Concrete backends stay under the `Adapter\` namespace (ClickHouse, Database, SQL), which now describes where the implementations live rather than the type callers hold. - src/Usage/Adapter.php -> src/Usage/Usage.php (abstract class Usage) - SQL, Accumulator, Tenant and all tests reference Usage / Usage::TYPE_* - Accumulator/Tenant hold a $usage (was $adapter) Pure rename, no behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
loks0n
changed the title
Greptile SummaryThis PR collapses the pass-through
Confidence Score: 4/5Straightforward structural rename with no logic changes to query paths; the only non-mechanical delta is where assertType() fires in Database::addBatch(). The refactor is mechanically correct and the 15/15 unit tests pass. The one nuance worth watching is that Database::addBatch() now validates $type before entering the skip() closure, so an empty $metrics array paired with an invalid type will throw where it previously returned silently. Database::sum() is also the only method that does not call assertType() at its entry point while its ClickHouse counterpart now does. src/Usage/Adapter/Database.php — the assertType() placement change in addBatch() and the missing early guard in sum(). Important Files Changed
Reviews (1): Last reviewed commit: "refactor(usage): rename the Adapter base..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| public function addBatch(array $metrics, string $type, int $batchSize = 1000): bool | ||
| { | ||
| self::assertType($type); | ||
|
|
||
| $this->db->getAuthorization()->skip(function () use ($metrics, $type, $batchSize) { | ||
| $entries = []; | ||
| foreach ($metrics as $metric) { | ||
| if ($type !== Usage::TYPE_EVENT && $type !== Usage::TYPE_GAUGE) { | ||
| throw new \InvalidArgumentException("Invalid type '{$type}'. Allowed: event, gauge"); | ||
| } | ||
|
|
||
| if ($metric['value'] < 0) { |
There was a problem hiding this comment.
assertType() call moved outside the skip() closure, which subtly changes behavior: when $metrics is an empty array the old per-row guard inside foreach never ran, so addBatch([], 'invalid', 1000) returned without throwing. The new placement throws immediately even for empty batches. That's almost certainly the more correct behavior, but it is a breaking API change for any caller that relied on the old silent-pass for empty metrics.
| public function addBatch(array $metrics, string $type, int $batchSize = 1000): bool | |
| { | |
| self::assertType($type); | |
| $this->db->getAuthorization()->skip(function () use ($metrics, $type, $batchSize) { | |
| $entries = []; | |
| foreach ($metrics as $metric) { | |
| if ($type !== Usage::TYPE_EVENT && $type !== Usage::TYPE_GAUGE) { | |
| throw new \InvalidArgumentException("Invalid type '{$type}'. Allowed: event, gauge"); | |
| } | |
| if ($metric['value'] < 0) { | |
| public function addBatch(array $metrics, string $type, int $batchSize = 1000): bool | |
| { | |
| self::assertType($type); | |
| if (empty($metrics)) { | |
| return true; | |
| } | |
| $this->db->getAuthorization()->skip(function () use ($metrics, $type, $batchSize) { | |
| $entries = []; | |
| foreach ($metrics as $metric) { | |
| if ($metric['value'] < 0) { |
| public function sum(string $tenant, array $queries = [], string $attribute = 'value', string $type = self::TYPE_EVENT): int | ||
| { | ||
| /** @var array<Metric> $results */ | ||
| $results = $this->find($tenant, $queries, $type); |
There was a problem hiding this comment.
Database::sum() is the only method in the adapter hierarchy that does not call assertType() at its entry point — ClickHouse::sum() acquired the early guard in this PR. The type is still validated downstream via withTypeFilter() → assertType(), so no invalid value escapes, but the inconsistency makes the defensive contract harder to reason about at a glance.
| public function sum(string $tenant, array $queries = [], string $attribute = 'value', string $type = self::TYPE_EVENT): int | |
| { | |
| /** @var array<Metric> $results */ | |
| $results = $this->find($tenant, $queries, $type); | |
| public function sum(string $tenant, array $queries = [], string $attribute = 'value', string $type = self::TYPE_EVENT): int | |
| { | |
| self::assertType($type); | |
| /** @var array<Metric> $results */ | |
| $results = $this->find($tenant, $queries, $type); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary
Usagehad become a pure pass-through: every method forwarded to the adapter with an identical signature, duplicating the adapter's docblocks verbatim and forcing the adapters to depend onUsagefor their ownTYPE_*constants (a backwards dependency). The only behaviour it added was a type guard insum().This PR removes that layer, then renames the abstract
Adapterbase — the real contract every backend implements — toUsage, since with the facade gone it is the public entry point:new ClickHouse(...)is aUsage.End state: one
Usagecontract, concrete backends underAdapter\(ClickHouse, Database, SQL), and theAccumulator/Tenantdecorators wrapping aUsage.Changes
Usagefacade.AccumulatorandTenantwrap the contract directly.Usage::assertType(), replacing five duplicatedevent|gaugeguards acrossAccumulator,ClickHouse, andDatabase(the ClickHouse batch validator keeps itsMetric #N:prefix via the$prefixarg).TYPE_EVENT/TYPE_GAUGElive on the contract, resolving the old backwards constant dependency.Adapter→Usage(src/Usage/Adapter.php→src/Usage/Usage.php). Concrete adapters keep theUtopia\Usage\Adapter\namespace.getAdapter()is gone — the adapter is the entry point.No behaviour change: the deleted facade's signatures were identical to the contract's, and step 4 is a pure rename.
Verification
composer check(PHPStan) — cleancomposer lint(Pint) — passphp -lon all changed files — cleanClickHouse/Database integration tests need a live backend (not run locally); changes there are mechanical (
Usage::TYPE_*constants, unwrapping the facade, the rename).🤖 Generated with Claude Code