Skip to content

fix(collections): implement ListCollections, which returned 501 - #145

Merged
hsinatfootprintai merged 1 commit into
mainfrom
fix/144-list-collections
Aug 18, 2026
Merged

fix(collections): implement ListCollections, which returned 501#145
hsinatfootprintai merged 1 commit into
mainfrom
fix/144-list-collections

Conversation

@hsinatfootprintai

Copy link
Copy Markdown
Contributor

Closes #144.

GET /v1/projects/{projectId}/collections was 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 cannot GET /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

ListCollectionsResponse was an empty message, so this needed a shape chosen rather than a handler filled in. It carries repeated GetCollectionResponse — the same message the single-collection GET returns:

  • ListByProjectID already preloads each collection's latest schema and its fields. A slimmer summary type would discard data that has been read.
  • One resource should not have two shapes. Both endpoints now render through newPbCollection, so they cannot drift.

No pagination fields. Adding them later is backwards-compatible in proto3; shipping page_size/page_token that 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.ProjectId makes it fail, restoring makes it pass. A test that cannot fail is not a test.

A latent panic, fixed in passing

GetCollection guarded its schema access with mc.Schemas != nil and then indexed [0]. A non-nil empty slice panics. Now len() > 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 test reuses 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 generators
  • go test ./... — full suite passes, including the 5 new tests
  • Two consecutive runs against the same postgres, both green

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
@hsinatfootprintai
hsinatfootprintai merged commit 740b6ad into main Aug 18, 2026
2 checks passed
@hsinatfootprintai
hsinatfootprintai deleted the fix/144-list-collections branch August 18, 2026 03:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ListCollections is documented but returns 501 — and the API doc now makes that the first thing a user clicks

2 participants