fix(collections): implement ListCollections, which returned 501 - #145
Merged
Conversation
GET /v1/projects/{projectId}/collections was declared in the OpenAPI spec
and answered "not implemented" - the only documented GET that did not
work. It is also the one call that needs no prior knowledge, since you
cannot GET /collections/{id} without already holding an id, so it is what
anyone tries first from the API doc. Since restcol#143 made that doc
usable, the first impression of this API was a 501.
RESPONSE SHAPE. ListCollectionsResponse was an empty message, so this
needed one chosen. It carries `repeated GetCollectionResponse`, the same
message the single-collection GET returns, rather than a slimmer summary
type: ListByProjectID already preloads each collection's latest schema
and its fields, so a summary would discard data that has been read and
would give clients two shapes for one resource. Both endpoints now render
through newPbCollection, so they cannot drift.
No pagination fields. Adding them later is backwards-compatible in
proto3; shipping page_size and page_token that nothing honours is not.
TENANT SCOPE. The project comes from the caller's credential, never from
req.ProjectId - the rule every other handler here follows. The path
carries a projectId because the URL needs one, and honouring it would let
any caller list another tenant's collections by editing the path. There
is a test for that, and I verified it FAILS when the handler is changed
to trust the request field.
Also fixed while sharing the renderer: GetCollection guarded its schema
access with `mc.Schemas != nil` and then indexed [0]. A non-nil empty
slice panics. It is now len() > 0.
TEST ISOLATION, which this change forced. Every test here used project
9001 and the database is never reset, so collections accumulated across
tests - invisible to assertions naming a specific id, fatal to any
assertion about how many collections a project has, which is what the
list tests are. Each test now gets its own project.
The first version of that seeded from a counter and still failed on its
SECOND run: a fixed base reuses the previous run's ids and reads rows
that run left behind, which presents as flakiness rather than as leftover
state. The seed is now clock-derived, and I verified two consecutive runs
against one database both pass.
Refs #144
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BSzNfcVc1FnDCfk9AF68Eh
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.
Closes #144.
GET /v1/projects/{projectId}/collectionswas declared in the OpenAPI spec and answered 501 not implemented — the only documented GET that did not work. It is also the one call that needs no prior knowledge (you cannotGET /collections/{id}without already holding an id), so it is what anyone tries first from the API doc. Since #143 made that doc usable, the first impression of this API was a 501.The response shape — the one real decision
ListCollectionsResponsewas an empty message, so this needed a shape chosen rather than a handler filled in. It carriesrepeated GetCollectionResponse— the same message the single-collection GET returns:ListByProjectIDalready preloads each collection's latest schema and its fields. A slimmer summary type would discard data that has been read.newPbCollection, so they cannot drift.No pagination fields. Adding them later is backwards-compatible in proto3; shipping
page_size/page_tokenthat nothing honours is not — it tells a client the endpoint is paginated when it is not."Latest schema" is also what settles the ambiguity I raised on the issue: the storage query limits the preload to one schema per collection, so this lists collections, not collection-versions.
Tenant scope
The project comes from the caller's credential, never
req.ProjectId— the rule every other handler here follows. The path carries a projectId because the URL needs one; honouring it would let any caller list another tenant's collections by editing the path.There is a test for that, and I checked it actually discriminates: patching the handler to trust
req.ProjectIdmakes it fail, restoring makes it pass. A test that cannot fail is not a test.A latent panic, fixed in passing
GetCollectionguarded its schema access withmc.Schemas != niland then indexed[0]. A non-nil empty slice panics. Nowlen() > 0.Test isolation, which this change forced
Every test in this file used project
9001, and the database is never reset between them — so collections accumulated across tests. Invisible to assertions that name a specific id; fatal to any assertion about how many collections a project has, which is exactly what the list tests are. Each test now gets its own project.Worth recording that my first attempt at that was wrong: it seeded from a fixed counter, which isolates tests within a run but not across runs — the second
go testreuses the first's ids and reads back rows the earlier run left behind. It made a passing test fail on its next invocation with no code change in between, which reads as flakiness rather than leftover state. The seed is now clock-derived, and I verified two consecutive runs against one database both pass.Verification
make gen-check— clean, generated output matches the generatorsgo test ./...— full suite passes, including the 5 new tests