Skip to content

Commit eceda46

Browse files
authored
Fix underfilled pages in ListSources and ListRegistries (#688)
## Summary - `ListSources` fetched a single batch of up to `MaxPageSize` records and then post-filtered by caller JWT claims, causing the returned list to be underfilled when filtering removed rows and more qualifying records existed in the DB. - `ListRegistries` fetched all registries in a single query and post-filtered by claims, with the same result. Both functions now use a streaming batch loop — identical in structure to the existing `streamSkillRows` — that keeps fetching until the DB is exhausted, ensuring all qualifying records are always returned. `ListRegistries` required a small SQL change to add cursor-based pagination (`WHERE name > cursor ORDER BY name LIMIT size`) since the original query had no `LIMIT` and therefore no way to iterate in batches. Note: the `created_at` cursor used by `ListSources` is a pre-existing design in that SQL query and is left unchanged; timestamp collisions are considered acceptable at the batch sizes in use. ## Test plan - [ ] New integration tests: `TestListSources`, `TestListSources_ReturnsAllMatchingWithMixedClaims`, `TestListSources_AnonymousReturnsAll`, `TestListRegistries_ReturnsAllMatchingWithMixedClaims`, `TestListRegistries_AnonymousReturnsAll` - [ ] Smoke test "List completeness" scenarios: create 3 sources and 3 registries, verify all appear in the respective list endpoints Fixes #669 and #670 Signed-off-by: lujunsan <luisjuncaldev@gmail.com>
1 parent 8926831 commit eceda46

8 files changed

Lines changed: 383 additions & 33 deletions

File tree

.claude/skills/smoke-test/SKILL.md

Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,24 @@ Feature: Registry Server smoke tests
170170
Scenario: Publish with neither server nor skill in body returns 400
171171
When I POST /v1/entries with an empty payload
172172
Then the response status is 400
173+
174+
# ── List completeness ─────────────────────────────────────────────────────
175+
176+
Scenario: All created sources appear in list
177+
Given three managed sources are created: smoke-src-a, smoke-src-b, smoke-src-c
178+
When I GET /v1/sources
179+
Then the response status is 200
180+
And the body contains "smoke-src-a"
181+
And the body contains "smoke-src-b"
182+
And the body contains "smoke-src-c"
183+
184+
Scenario: All created registries appear in list
185+
Given three registries are created referencing the managed sources
186+
When I GET /v1/registries
187+
Then the response status is 200
188+
And the body contains "smoke-reg-a"
189+
And the body contains "smoke-reg-b"
190+
And the body contains "smoke-reg-c"
173191
```
174192

175193
---
@@ -376,10 +394,39 @@ check "POST /v1/entries with both server+skill returns 400" 400 "$SC" "$(body)"
376394
SC=$(curl_post /v1/entries '{}')
377395
check "POST /v1/entries with neither server nor skill returns 400" 400 "$SC" "$(body)"
378396

379-
# ─── Clean up managed source ──────────────────────────────────────────────────
397+
# ─── List completeness ───────────────────────────────────────────────────────
398+
echo ""
399+
echo "── List completeness ──"
400+
401+
SC=$(curl_put /v1/sources/smoke-src-a '{"managed":{}}'); check "PUT /v1/sources/smoke-src-a returns 201" 201 "$SC" "$(body)" "smoke-src-a"
402+
SC=$(curl_put /v1/sources/smoke-src-b '{"managed":{}}'); check "PUT /v1/sources/smoke-src-b returns 201" 201 "$SC" "$(body)" "smoke-src-b"
403+
SC=$(curl_put /v1/sources/smoke-src-c '{"managed":{}}'); check "PUT /v1/sources/smoke-src-c returns 201" 201 "$SC" "$(body)" "smoke-src-c"
404+
405+
SC=$(curl_get /v1/sources); BODY="$(body)"
406+
check "GET /v1/sources contains smoke-src-a" 200 "$SC" "$BODY" "smoke-src-a"
407+
check "GET /v1/sources contains smoke-src-b" 200 "$SC" "$BODY" "smoke-src-b"
408+
check "GET /v1/sources contains smoke-src-c" 200 "$SC" "$BODY" "smoke-src-c"
409+
410+
SC=$(curl_put /v1/registries/smoke-reg-a '{"sources":["smoke-src-a"]}'); check "PUT /v1/registries/smoke-reg-a returns 201" 201 "$SC" "$(body)" "smoke-reg-a"
411+
SC=$(curl_put /v1/registries/smoke-reg-b '{"sources":["smoke-src-b"]}'); check "PUT /v1/registries/smoke-reg-b returns 201" 201 "$SC" "$(body)" "smoke-reg-b"
412+
SC=$(curl_put /v1/registries/smoke-reg-c '{"sources":["smoke-src-c"]}'); check "PUT /v1/registries/smoke-reg-c returns 201" 201 "$SC" "$(body)" "smoke-reg-c"
413+
414+
SC=$(curl_get /v1/registries); BODY="$(body)"
415+
check "GET /v1/registries contains smoke-reg-a" 200 "$SC" "$BODY" "smoke-reg-a"
416+
check "GET /v1/registries contains smoke-reg-b" 200 "$SC" "$BODY" "smoke-reg-b"
417+
check "GET /v1/registries contains smoke-reg-c" 200 "$SC" "$BODY" "smoke-reg-c"
418+
419+
# ─── Clean up ─────────────────────────────────────────────────────────────────
380420
SC=$(curl_del /v1/sources/managed-test)
381421
check "DELETE /v1/sources/managed-test returns 204" 204 "$SC" "$(body)"
382422

423+
SC=$(curl_del /v1/registries/smoke-reg-a); check "DELETE /v1/registries/smoke-reg-a returns 204" 204 "$SC" "$(body)"
424+
SC=$(curl_del /v1/registries/smoke-reg-b); check "DELETE /v1/registries/smoke-reg-b returns 204" 204 "$SC" "$(body)"
425+
SC=$(curl_del /v1/registries/smoke-reg-c); check "DELETE /v1/registries/smoke-reg-c returns 204" 204 "$SC" "$(body)"
426+
SC=$(curl_del /v1/sources/smoke-src-a); check "DELETE /v1/sources/smoke-src-a returns 204" 204 "$SC" "$(body)"
427+
SC=$(curl_del /v1/sources/smoke-src-b); check "DELETE /v1/sources/smoke-src-b returns 204" 204 "$SC" "$(body)"
428+
SC=$(curl_del /v1/sources/smoke-src-c); check "DELETE /v1/sources/smoke-src-c returns 204" 204 "$SC" "$(body)"
429+
383430
# ─── Summary ──────────────────────────────────────────────────────────────────
384431
echo ""
385432
echo "══════════════════════════════════════"

database/queries/registry.sql

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,10 @@
22

33
-- name: ListRegistries :many
44
SELECT id, name, claims, creation_type, created_at, updated_at
5-
FROM registry ORDER BY name;
5+
FROM registry
6+
WHERE (sqlc.narg(cursor)::text IS NULL OR name > sqlc.narg(cursor))
7+
ORDER BY name
8+
LIMIT sqlc.arg(size)::bigint;
69

710
-- name: GetRegistryByName :one
811
SELECT id, name, claims, creation_type, created_at, updated_at

internal/db/sqlc/querier.go

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

internal/db/sqlc/registry.sql.go

Lines changed: 11 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

internal/service/db/admin_claims_test.go

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -484,6 +484,158 @@ func TestGetRegistryByName_HiddenWhenClaimsDontMatch(t *testing.T) {
484484
assert.Nil(t, reg)
485485
}
486486

487+
// ---------------------------------------------------------------------------
488+
// Mixed-claims listing tests — verify streaming batch loops return all
489+
// qualifying rows even when different claim values are interleaved.
490+
// ---------------------------------------------------------------------------
491+
492+
func TestListSources_ReturnsAllMatchingWithMixedClaims(t *testing.T) {
493+
t.Parallel()
494+
svc, cleanup := setupTestServiceWithCodecs(t)
495+
t.Cleanup(cleanup)
496+
497+
createCtx := t.Context()
498+
499+
// Create 4 sources: 2 with org=acme, 1 with org=contoso, 1 with no claims
500+
_, err := svc.CreateSource(createCtx, "lsm-acme-1", managedSourceReq(map[string]any{"org": "acme"}))
501+
require.NoError(t, err)
502+
503+
_, err = svc.CreateSource(createCtx, "lsm-acme-2", managedSourceReq(map[string]any{"org": "acme"}))
504+
require.NoError(t, err)
505+
506+
_, err = svc.CreateSource(createCtx, "lsm-contoso", managedSourceReq(map[string]any{"org": "contoso"}))
507+
require.NoError(t, err)
508+
509+
_, err = svc.CreateSource(createCtx, "lsm-open", managedSourceReq(nil))
510+
require.NoError(t, err)
511+
512+
// List with acme JWT — should see both acme sources and the open (no-claims) source
513+
listCtx := auth.ContextWithClaims(t.Context(), jwt.MapClaims{"org": "acme"})
514+
sources, err := svc.ListSources(listCtx)
515+
require.NoError(t, err)
516+
517+
names := sourceNames(sources)
518+
assert.Contains(t, names, "lsm-acme-1")
519+
assert.Contains(t, names, "lsm-acme-2")
520+
assert.NotContains(t, names, "lsm-contoso")
521+
assert.Contains(t, names, "lsm-open")
522+
}
523+
524+
func TestListRegistries_ReturnsAllMatchingWithMixedClaims(t *testing.T) {
525+
t.Parallel()
526+
svc, cleanup := setupTestServiceWithCodecs(t)
527+
t.Cleanup(cleanup)
528+
529+
createCtx := t.Context()
530+
531+
// Create 4 sources (one per registry) with different claims
532+
createSourceForRegistry(t, svc, "lrm-acme-src-1", map[string]any{"org": "acme"})
533+
createSourceForRegistry(t, svc, "lrm-acme-src-2", map[string]any{"org": "acme"})
534+
createSourceForRegistry(t, svc, "lrm-contoso-src", map[string]any{"org": "contoso"})
535+
createSourceForRegistry(t, svc, "lrm-open-src", nil)
536+
537+
// Create 4 registries with matching claims
538+
_, err := svc.CreateRegistry(createCtx, "lrm-acme-reg-1", &service.RegistryCreateRequest{
539+
Sources: []string{"lrm-acme-src-1"},
540+
Claims: map[string]any{"org": "acme"},
541+
})
542+
require.NoError(t, err)
543+
544+
_, err = svc.CreateRegistry(createCtx, "lrm-acme-reg-2", &service.RegistryCreateRequest{
545+
Sources: []string{"lrm-acme-src-2"},
546+
Claims: map[string]any{"org": "acme"},
547+
})
548+
require.NoError(t, err)
549+
550+
_, err = svc.CreateRegistry(createCtx, "lrm-contoso-reg", &service.RegistryCreateRequest{
551+
Sources: []string{"lrm-contoso-src"},
552+
Claims: map[string]any{"org": "contoso"},
553+
})
554+
require.NoError(t, err)
555+
556+
_, err = svc.CreateRegistry(createCtx, "lrm-open-reg", &service.RegistryCreateRequest{
557+
Sources: []string{"lrm-open-src"},
558+
})
559+
require.NoError(t, err)
560+
561+
// List with acme JWT — should see both acme registries and the open one
562+
listCtx := auth.ContextWithClaims(t.Context(), jwt.MapClaims{"org": "acme"})
563+
registries, err := svc.ListRegistries(listCtx)
564+
require.NoError(t, err)
565+
566+
names := registryNames(registries)
567+
assert.Contains(t, names, "lrm-acme-reg-1")
568+
assert.Contains(t, names, "lrm-acme-reg-2")
569+
assert.NotContains(t, names, "lrm-contoso-reg")
570+
assert.Contains(t, names, "lrm-open-reg")
571+
}
572+
573+
func TestListSources_AnonymousReturnsAll(t *testing.T) {
574+
t.Parallel()
575+
svc, cleanup := setupTestServiceWithCodecs(t)
576+
t.Cleanup(cleanup)
577+
578+
createCtx := t.Context()
579+
580+
// Create 3 sources with different claims
581+
_, err := svc.CreateSource(createCtx, "lsa-acme", managedSourceReq(map[string]any{"org": "acme"}))
582+
require.NoError(t, err)
583+
584+
_, err = svc.CreateSource(createCtx, "lsa-contoso", managedSourceReq(map[string]any{"org": "contoso"}))
585+
require.NoError(t, err)
586+
587+
_, err = svc.CreateSource(createCtx, "lsa-open", managedSourceReq(nil))
588+
require.NoError(t, err)
589+
590+
// List without JWT (anonymous) — should see all sources
591+
sources, err := svc.ListSources(t.Context())
592+
require.NoError(t, err)
593+
594+
names := sourceNames(sources)
595+
assert.Contains(t, names, "lsa-acme")
596+
assert.Contains(t, names, "lsa-contoso")
597+
assert.Contains(t, names, "lsa-open")
598+
}
599+
600+
func TestListRegistries_AnonymousReturnsAll(t *testing.T) {
601+
t.Parallel()
602+
svc, cleanup := setupTestServiceWithCodecs(t)
603+
t.Cleanup(cleanup)
604+
605+
createCtx := t.Context()
606+
607+
// Create 3 sources + 3 registries with different claims
608+
createSourceForRegistry(t, svc, "lra-acme-src", map[string]any{"org": "acme"})
609+
createSourceForRegistry(t, svc, "lra-contoso-src", map[string]any{"org": "contoso"})
610+
createSourceForRegistry(t, svc, "lra-open-src", nil)
611+
612+
_, err := svc.CreateRegistry(createCtx, "lra-acme-reg", &service.RegistryCreateRequest{
613+
Sources: []string{"lra-acme-src"},
614+
Claims: map[string]any{"org": "acme"},
615+
})
616+
require.NoError(t, err)
617+
618+
_, err = svc.CreateRegistry(createCtx, "lra-contoso-reg", &service.RegistryCreateRequest{
619+
Sources: []string{"lra-contoso-src"},
620+
Claims: map[string]any{"org": "contoso"},
621+
})
622+
require.NoError(t, err)
623+
624+
_, err = svc.CreateRegistry(createCtx, "lra-open-reg", &service.RegistryCreateRequest{
625+
Sources: []string{"lra-open-src"},
626+
})
627+
require.NoError(t, err)
628+
629+
// List without JWT (anonymous) — should see all registries
630+
registries, err := svc.ListRegistries(t.Context())
631+
require.NoError(t, err)
632+
633+
names := registryNames(registries)
634+
assert.Contains(t, names, "lra-acme-reg")
635+
assert.Contains(t, names, "lra-contoso-reg")
636+
assert.Contains(t, names, "lra-open-reg")
637+
}
638+
487639
// ---------------------------------------------------------------------------
488640
// Helpers
489641
// ---------------------------------------------------------------------------

internal/service/db/impl_registry.go

Lines changed: 39 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -40,24 +40,16 @@ func (s *dbService) ListRegistries(ctx context.Context) ([]service.RegistryInfo,
4040
}()
4141

4242
querier := sqlc.New(tx)
43+
callerClaims := claimsFromCtx(ctx)
4344

44-
registries, err := querier.ListRegistries(ctx)
45+
registries, err := streamRegistryRows(ctx, querier, callerClaims)
4546
if err != nil {
4647
otel.RecordError(span, err)
4748
return nil, fmt.Errorf("failed to list registries: %w", err)
4849
}
4950

50-
callerClaims := claimsFromCtx(ctx)
5151
result := make([]service.RegistryInfo, 0, len(registries))
5252
for _, reg := range registries {
53-
// In authenticated mode, only return registries whose claims the caller covers
54-
regClaims := db.DeserializeClaims(reg.Claims)
55-
if callerClaims != nil {
56-
if err := validateClaimsSubset(ctx, callerClaims, regClaims); err != nil {
57-
continue
58-
}
59-
}
60-
6153
sources, err := querier.ListRegistrySources(ctx, reg.ID)
6254
if err != nil {
6355
otel.RecordError(span, err)
@@ -565,3 +557,40 @@ func unlinkAllSources(
565557
) error {
566558
return querier.UnlinkAllRegistrySources(ctx, registryID)
567559
}
560+
561+
// streamRegistryRows fetches registry rows in batches, filtering by caller claims,
562+
// until the DB is exhausted. This avoids underfilled responses when post-filtering
563+
// drops rows that fail claims checks.
564+
func streamRegistryRows(
565+
ctx context.Context,
566+
querier *sqlc.Queries,
567+
callerClaims map[string]any,
568+
) ([]sqlc.Registry, error) {
569+
var accumulated []sqlc.Registry
570+
params := sqlc.ListRegistriesParams{
571+
Size: int64(service.MaxPageSize),
572+
}
573+
574+
for {
575+
batch, err := querier.ListRegistries(ctx, params)
576+
if err != nil {
577+
return nil, err
578+
}
579+
580+
for _, reg := range batch {
581+
if err := validateClaimsSubsetBytes(ctx, callerClaims, reg.Claims); err != nil {
582+
continue
583+
}
584+
accumulated = append(accumulated, reg)
585+
}
586+
587+
if int64(len(batch)) < params.Size {
588+
break
589+
}
590+
591+
last := batch[len(batch)-1]
592+
params.Cursor = &last.Name
593+
}
594+
595+
return accumulated, nil
596+
}

0 commit comments

Comments
 (0)