fix(api): GET /stats returns 500 whenever sourceId is omitted - #4470
Open
rancur wants to merge 1 commit into
Open
fix(api): GET /stats returns 500 whenever sourceId is omitted#4470rancur wants to merge 1 commit into
rancur wants to merge 1 commit into
Conversation
/api/stats scopes three of its four queries with `statsSourceId ?? ALL_SOURCES`
but hands the fourth, getMessagesByDayAsync, a bare `statsSourceId`. When the
caller omits sourceId that is `undefined`, which `BaseRepository.withSourceScope`
rejects by design:
withSourceScope: sourceId is required. Pass a concrete sourceId, or the
ALL_SOURCES sentinel for an intentional cross-source query.
So the endpoint throws and returns 500 / STATS_FAILED for every caller that does
not pass sourceId, even though the route comment states the intent explicitly:
"stats totals span all sources when no sourceId is specified". Only the
source-scoped call path worked; the aggregate path was dead.
Why the type system did not catch it: DatabaseService.getMessagesByDayAsync
declared `sourceId?: string`, narrower than the `SourceScope` its own repository
accepts, so passing the ALL_SOURCES symbol would not have compiled. Widening the
service signature to SourceScope makes the correct call expressible and keeps
the service in step with the repository layer.
- route: pass ALL_SOURCES when no sourceId is supplied, matching the sibling calls
- service: widen getMessagesByDayAsync's sourceId to SourceScope
- test: cover the bare /stats call, asserting all four counts receive the
sentinel. The existing test only exercised /stats?sourceId=..., which is why
this went unnoticed.
Verified against a live 4.13.0 instance: GET /api/stats returned 500, and
returns 200 with correct cross-source totals after the change. `tsc --noEmit`
clean; the new test fails without the route fix and passes with it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
The bug
GET /api/statsreturns HTTP 500 /STATS_FAILEDfor any caller that does not passsourceId.routes/dataExchangeRoutes.tsscopes three of its four queries withstatsSourceId ?? ALL_SOURCES, but hands the fourth a barestatsSourceId:With
sourceIdomitted that argument isundefined, whichBaseRepository.withSourceScoperejects by design:The throw is caught by the route's
catchand surfaced as a generic 500, so the endpoint fails with no hint of the cause. The comment immediately above the block states the intended behaviour — "intentional cross-source: stats totals span all sources when no sourceId is specified" — so this is a missed conversion, not a deliberate restriction. Only the?sourceId=...path ever worked; the aggregate path was dead.Why the type checker did not catch it
DatabaseService.getMessagesByDayAsyncdeclaredsourceId?: string, narrower than theSourceScopethat its ownmessagesRepo.getMessagesByDayaccepts. Passing theALL_SOURCESsymbol would not have compiled, so the correct call was not even expressible from the service. Widening the service signature toSourceScopefixes the leaky abstraction and keeps the service in step with the repository layer.Changes
routes/dataExchangeRoutes.ts— passALL_SOURCESwhen nosourceIdis supplied, matching the three sibling calls.services/database.ts— widengetMessagesByDayAsync'ssourceIdtoSourceScope.routes/dataExchangeRoutes.test.ts— regression test for the bare/statscall, asserting all four counts receive the sentinel. The existing test only exercised/stats?sourceId=..., which is why this went unnoticed.Verification
GET /api/stats→ 500STATS_FAILED,GET /api/stats?sourceId=<id>→ 200. After the change, the bare call returns 200 with correct cross-source totals (message/node/channel counts, and a non-emptymessagesByDay).expected "vi.fn()" to be called with arguments: [ 7, Symbol(ALL_SOURCES) ]) and passes with it.npx vitest run src/server/routes/dataExchangeRoutes.test.ts→ 7 passed.npx tsc --noEmit→ clean.No behaviour change for callers that already pass
sourceId.