← back to Cli Printing Press
fix(cli): writeThroughCache caches single-object responses keyed by non-id PKs (#1531)
c96ac5da6a9f7b58146ad45ee569c30d9f0ce85c · 2026-05-16 15:53:25 -0700 · Trevin Chow
* fix(cli): writeThroughCache caches single-object responses keyed by non-id PKs
writeThroughCache guarded its single-object cache write with
`if _, ok := envelope["id"]; ok` — APIs whose primary key is named
CertNo / sku / invoiceId / etc. silently fell through every lookup.
The user saw HTTP 200 and correct stdout while every downstream
feature that reads from the local SQLite store (smart presets,
holdings analysis, offline search) got no data.
Drop the hardcoded "id" guard and delegate primary-key resolution
to UpsertBatch's existing resourceIDFieldOverrides path, which the
parser already populates from `x-resource-id` and the response-schema
inference chain.
Tightened the guard to skip list-shaped envelopes whose array
happened to be empty — `{"items": []}` must not write a row keyed
by the whole envelope. Caught by TestClientBasePathLiveRequest on
the first pass; explicit regression added.
Closes #1439
* fix(cli): verify list-wrapper field is an array before skipping cache write
Greptile flagged that the list-envelope guard checked for KEY presence
only — any detail object whose own field happened to be named data,
results, or items (with a scalar/object/null value) would be treated
as an empty list envelope and silently dropped. That regressed the
prior envelope["id"] path, which would have cached the row.
Tightened the guard to verify the value at the wrapper key actually
unmarshals as a JSON array. A scalar-valued field with a colliding
name is a regular response field, not a list envelope, and the row
still goes through UpsertBatch.
Added TestWriteThroughCacheCachesObjectWithListWrapperFieldName as
explicit coverage for the collision case Greptile called out.
* fix(cli): distinguish JSON null from empty array on list-wrapper field
Greptile noted that json.Unmarshal("null", &arr) succeeds with arr
left nil, so the previous unmarshal-only check would misclassify
{"CertNo":"x","items":null} as an empty list envelope and silently
drop the row. Require arr != nil so true empty arrays stay in the
skip branch while JSON null falls through to UpsertBatch as a
regular field collision.
Added TestWriteThroughCacheCachesObjectWithNullWrapperFieldName as
the missing coverage Greptile called out.
* chore(cli): empty commit to force CI rerun after flaky text file busy
* fix(cli): require list envelope to be fully metadata-shaped before skipping
Greptile flagged round-3: a detail object whose own field happened to
be named items/data/results AND holds an empty array (e.g.
{"id":"order_123","items":[],"status":"pending"}) was being treated
as an empty list envelope and silently dropped. The previous "any
wrapper key with array value" heuristic was too permissive.
Switch to a structural check: an envelope is list-shaped only when
EVERY top-level key is either the wrapper itself (results/data/items)
or one of a known pagination-metadata key (next_cursor, has_more,
total, links, meta, response_metadata, etc.). A single non-metadata
key signals real per-row data and the row gets cached.
Codifies the metadata allowlist as a package-level var so it stays
auditable. Adds explicit regression tests for the multi-key detail
shape and for the real list-envelope-with-metadata shape.
---------
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Files touched
M internal/generator/generator_test.goM internal/generator/templates/data_source.go.tmpl
Diff
commit c96ac5da6a9f7b58146ad45ee569c30d9f0ce85c
Author: Trevin Chow <trevin@trevinchow.com>
Date: Sat May 16 15:53:25 2026 -0700
fix(cli): writeThroughCache caches single-object responses keyed by non-id PKs (#1531)
* fix(cli): writeThroughCache caches single-object responses keyed by non-id PKs
writeThroughCache guarded its single-object cache write with
`if _, ok := envelope["id"]; ok` — APIs whose primary key is named
CertNo / sku / invoiceId / etc. silently fell through every lookup.
The user saw HTTP 200 and correct stdout while every downstream
feature that reads from the local SQLite store (smart presets,
holdings analysis, offline search) got no data.
Drop the hardcoded "id" guard and delegate primary-key resolution
to UpsertBatch's existing resourceIDFieldOverrides path, which the
parser already populates from `x-resource-id` and the response-schema
inference chain.
Tightened the guard to skip list-shaped envelopes whose array
happened to be empty — `{"items": []}` must not write a row keyed
by the whole envelope. Caught by TestClientBasePathLiveRequest on
the first pass; explicit regression added.
Closes #1439
* fix(cli): verify list-wrapper field is an array before skipping cache write
Greptile flagged that the list-envelope guard checked for KEY presence
only — any detail object whose own field happened to be named data,
results, or items (with a scalar/object/null value) would be treated
as an empty list envelope and silently dropped. That regressed the
prior envelope["id"] path, which would have cached the row.
Tightened the guard to verify the value at the wrapper key actually
unmarshals as a JSON array. A scalar-valued field with a colliding
name is a regular response field, not a list envelope, and the row
still goes through UpsertBatch.
Added TestWriteThroughCacheCachesObjectWithListWrapperFieldName as
explicit coverage for the collision case Greptile called out.
* fix(cli): distinguish JSON null from empty array on list-wrapper field
Greptile noted that json.Unmarshal("null", &arr) succeeds with arr
left nil, so the previous unmarshal-only check would misclassify
{"CertNo":"x","items":null} as an empty list envelope and silently
drop the row. Require arr != nil so true empty arrays stay in the
skip branch while JSON null falls through to UpsertBatch as a
regular field collision.
Added TestWriteThroughCacheCachesObjectWithNullWrapperFieldName as
the missing coverage Greptile called out.
* chore(cli): empty commit to force CI rerun after flaky text file busy
* fix(cli): require list envelope to be fully metadata-shaped before skipping
Greptile flagged round-3: a detail object whose own field happened to
be named items/data/results AND holds an empty array (e.g.
{"id":"order_123","items":[],"status":"pending"}) was being treated
as an empty list envelope and silently dropped. The previous "any
wrapper key with array value" heuristic was too permissive.
Switch to a structural check: an envelope is list-shaped only when
EVERY top-level key is either the wrapper itself (results/data/items)
or one of a known pagination-metadata key (next_cursor, has_more,
total, links, meta, response_metadata, etc.). A single non-metadata
key signals real per-row data and the row gets cached.
Codifies the metadata allowlist as a package-level var so it stays
auditable. Adds explicit regression tests for the multi-key detail
shape and for the real list-envelope-with-metadata shape.
---------
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
---
internal/generator/generator_test.go | 227 +++++++++++++++++++++++
internal/generator/templates/data_source.go.tmpl | 72 ++++++-
2 files changed, 296 insertions(+), 3 deletions(-)
diff --git a/internal/generator/generator_test.go b/internal/generator/generator_test.go
index 2d14e6dc..b469ffa2 100644
--- a/internal/generator/generator_test.go
+++ b/internal/generator/generator_test.go
@@ -3573,6 +3573,233 @@ func TestWriteThroughCachePopulatesTypedTable(t *testing.T) {
runGoCommandRequired(t, outputDir, "test", "-run", "TestWriteThroughCachePopulatesTypedTable", "./internal/cli")
}
+// TestWriteThroughCacheNonIDPrimaryKeyResponse guards #1439: the previous
+// `envelope["id"]` guard silently dropped single-object detail responses
+// whose primary key field was named anything other than "id" (PCGS uses
+// CertNo, Stripe uses object ids prefixed with their type, many ERP APIs
+// use sku / invoiceId / etc.). After the fix the row reaches UpsertBatch
+// and the existing resourceIDFieldOverrides path resolves the PK.
+func TestLiveFetchWriteThroughCacheNonIDPrimaryKeyResponse(t *testing.T) {
+ t.Parallel()
+
+ apiSpec := &spec.APISpec{
+ Name: "pcgs",
+ Version: "0.1.0",
+ BaseURL: "https://api.example.com",
+ Auth: spec.AuthConfig{Type: "none"},
+ Config: spec.ConfigSpec{
+ Format: "toml",
+ Path: "~/.config/pcgs-pp-cli/config.toml",
+ },
+ Resources: map[string]spec.Resource{
+ "certs": {
+ Description: "Look up coin certs by CertNo",
+ Endpoints: map[string]spec.Endpoint{
+ // list endpoint triggers typed-table emission (UpsertBatch
+ // dispatches to it); lookup endpoint is the single-object
+ // detail path the bug fires on at runtime.
+ "list": {
+ Method: "GET",
+ Path: "/certs",
+ Description: "List recent certs",
+ IDField: "CertNo",
+ Response: spec.ResponseDef{Type: "array", Item: "Cert"},
+ },
+ "lookup": {
+ Method: "GET",
+ Path: "/certs/{cert_no}",
+ Description: "Fetch a single cert by CertNo",
+ IDField: "CertNo",
+ Response: spec.ResponseDef{Type: "object", Item: "Cert"},
+ },
+ },
+ },
+ },
+ Types: map[string]spec.TypeDef{
+ "Cert": {
+ Fields: []spec.TypeField{
+ {Name: "CertNo", Type: "string"},
+ {Name: "PCGSNo", Type: "string"},
+ {Name: "Grade", Type: "string"},
+ },
+ },
+ },
+ }
+ outputDir := filepath.Join(t.TempDir(), naming.CLI(apiSpec.Name))
+ gen := New(apiSpec, outputDir)
+ gen.VisionSet = VisionTemplateSet{Store: true}
+ require.NoError(t, gen.Generate())
+
+ inlineTest := `package cli
+
+import (
+ "context"
+ "encoding/json"
+ "testing"
+
+ "` + naming.CLI(apiSpec.Name) + `/internal/store"
+)
+
+func TestWriteThroughCacheNonIDPrimaryKey(t *testing.T) {
+ t.Setenv("HOME", t.TempDir())
+
+ // Single-object detail response keyed by CertNo (not "id"). Before
+ // #1439, the envelope["id"] guard caused writeThroughCache to drop
+ // this on the floor. After the fix the row reaches UpsertBatch and
+ // the resourceIDFieldOverrides mechanism resolves the PK.
+ writeThroughCache(context.Background(), "certs", json.RawMessage(` + "`" + `{"CertNo":"12345678","PCGSNo":"7280","Grade":"MS65"}` + "`" + `))
+
+ db, err := store.Open(defaultDBPath("pcgs-pp-cli"))
+ if err != nil {
+ t.Fatalf("open cache store: %v", err)
+ }
+ defer db.Close()
+
+ var typedCount int
+ if err := db.DB().QueryRow(` + "`" + `SELECT COUNT(*) FROM certs WHERE id = '12345678'` + "`" + `).Scan(&typedCount); err != nil {
+ t.Fatalf("query certs: %v", err)
+ }
+ if typedCount != 1 {
+ t.Fatalf("typed certs count = %d, want 1 (non-id primary key was dropped)", typedCount)
+ }
+}
+
+// TestWriteThroughCacheSkipsEmptyListEnvelope guards the tighter
+// single-object branch from misfiring on list responses whose array
+// happened to be empty. {"items": []} must NOT write a row keyed by
+// the entire envelope; the existing wrapper-key loop already handled
+// this implicitly and the relaxed guard must preserve the invariant.
+func TestWriteThroughCacheSkipsEmptyListEnvelope(t *testing.T) {
+ t.Setenv("HOME", t.TempDir())
+
+ writeThroughCache(context.Background(), "certs", json.RawMessage(` + "`" + `{"items": []}` + "`" + `))
+
+ db, err := store.Open(defaultDBPath("pcgs-pp-cli"))
+ if err != nil {
+ t.Fatalf("open cache store: %v", err)
+ }
+ defer db.Close()
+
+ var count int
+ if err := db.DB().QueryRow(` + "`" + `SELECT COUNT(*) FROM certs` + "`" + `).Scan(&count); err != nil {
+ t.Fatalf("query certs: %v", err)
+ }
+ if count != 0 {
+ t.Fatalf("certs count after empty-list envelope = %d, want 0 (envelope must not be upserted as a single object)", count)
+ }
+}
+
+// TestWriteThroughCacheCachesObjectWithListWrapperFieldName guards the
+// list-envelope guard from a field-name collision: a detail object whose
+// own field happens to be named "data" / "results" / "items" with a
+// scalar value (not an array) is a regular response, not an empty list
+// envelope, and must still be cached. Skipping it would be a regression
+// from the prior envelope["id"] path.
+func TestWriteThroughCacheCachesObjectWithListWrapperFieldName(t *testing.T) {
+ t.Setenv("HOME", t.TempDir())
+
+ // "data" key is present but holds a string. Old code would have written
+ // this via the id guard; new guard must still write it because the
+ // wrapper key isn't a list.
+ writeThroughCache(context.Background(), "certs", json.RawMessage(` + "`" + `{"CertNo":"55555","data":"opaque-token","Grade":"AU58"}` + "`" + `))
+
+ db, err := store.Open(defaultDBPath("pcgs-pp-cli"))
+ if err != nil {
+ t.Fatalf("open cache store: %v", err)
+ }
+ defer db.Close()
+
+ var count int
+ if err := db.DB().QueryRow(` + "`" + `SELECT COUNT(*) FROM certs WHERE id = '55555'` + "`" + `).Scan(&count); err != nil {
+ t.Fatalf("query certs: %v", err)
+ }
+ if count != 1 {
+ t.Fatalf("certs count after field-name-collision response = %d, want 1 (scalar-valued wrapper-named field must not trip the list-envelope skip)", count)
+ }
+}
+
+// TestWriteThroughCacheCachesObjectWithNullWrapperFieldName guards the
+// specific JSON-null case Greptile flagged: json.Unmarshal("null", &arr)
+// succeeds with arr=nil, so without an explicit arr != nil check the
+// guard would misclassify {"CertNo":"x","items":null} as an empty-list
+// envelope and silently drop the row.
+func TestWriteThroughCacheCachesObjectWithNullWrapperFieldName(t *testing.T) {
+ t.Setenv("HOME", t.TempDir())
+
+ writeThroughCache(context.Background(), "certs", json.RawMessage(` + "`" + `{"CertNo":"77777","items":null,"Grade":"PR70"}` + "`" + `))
+
+ db, err := store.Open(defaultDBPath("pcgs-pp-cli"))
+ if err != nil {
+ t.Fatalf("open cache store: %v", err)
+ }
+ defer db.Close()
+
+ var count int
+ if err := db.DB().QueryRow(` + "`" + `SELECT COUNT(*) FROM certs WHERE id = '77777'` + "`" + `).Scan(&count); err != nil {
+ t.Fatalf("query certs: %v", err)
+ }
+ if count != 1 {
+ t.Fatalf("certs count after null-wrapper-field response = %d, want 1 (JSON null is not an empty array)", count)
+ }
+}
+
+// TestWriteThroughCacheCachesObjectWithEmptyListWrapperAndOtherFields
+// covers the multi-key case Greptile flagged in round 3: a detail object
+// with real per-row fields PLUS a wrapper-named field that happens to be
+// an empty array. The skip branch must only fire when EVERY top-level
+// key is a list wrapper or known pagination metadata.
+func TestWriteThroughCacheCachesObjectWithEmptyListWrapperAndOtherFields(t *testing.T) {
+ t.Setenv("HOME", t.TempDir())
+
+ writeThroughCache(context.Background(), "certs", json.RawMessage(` + "`" + `{"CertNo":"88888","items":[],"Grade":"PR69"}` + "`" + `))
+
+ db, err := store.Open(defaultDBPath("pcgs-pp-cli"))
+ if err != nil {
+ t.Fatalf("open cache store: %v", err)
+ }
+ defer db.Close()
+
+ var count int
+ if err := db.DB().QueryRow(` + "`" + `SELECT COUNT(*) FROM certs WHERE id = '88888'` + "`" + `).Scan(&count); err != nil {
+ t.Fatalf("query certs: %v", err)
+ }
+ if count != 1 {
+ t.Fatalf("certs count after detail-with-empty-wrapper-field response = %d, want 1 (envelope has real per-row fields and must be cached)", count)
+ }
+}
+
+// TestWriteThroughCacheSkipsListEnvelopeWithPaginationMetadata guards the
+// flip side: a real list envelope with pagination metadata fields
+// (next_cursor, has_more, etc.) plus an empty array must still skip
+// the single-object upsert path. The metadata allowlist is what keeps
+// these envelopes recognized as lists.
+func TestWriteThroughCacheSkipsListEnvelopeWithPaginationMetadata(t *testing.T) {
+ t.Setenv("HOME", t.TempDir())
+
+ writeThroughCache(context.Background(), "certs", json.RawMessage(` + "`" + `{"items":[],"next_cursor":"","has_more":false}` + "`" + `))
+
+ db, err := store.Open(defaultDBPath("pcgs-pp-cli"))
+ if err != nil {
+ t.Fatalf("open cache store: %v", err)
+ }
+ defer db.Close()
+
+ var count int
+ if err := db.DB().QueryRow(` + "`" + `SELECT COUNT(*) FROM certs` + "`" + `).Scan(&count); err != nil {
+ t.Fatalf("query certs: %v", err)
+ }
+ if count != 0 {
+ t.Fatalf("certs count after list-envelope-with-metadata response = %d, want 0 (no real per-row data, must skip)", count)
+ }
+}
+`
+ testPath := filepath.Join(outputDir, "internal", "cli", "write_through_cache_non_id_test.go")
+ require.NoError(t, os.WriteFile(testPath, []byte(inlineTest), 0o644))
+
+ runGoCommandRequired(t, outputDir, "mod", "tidy")
+ runGoCommandRequired(t, outputDir, "test", "-run", "TestWriteThroughCacheNonIDPrimaryKey|TestWriteThroughCacheSkipsEmptyListEnvelope|TestWriteThroughCacheCachesObjectWithListWrapperFieldName|TestWriteThroughCacheCachesObjectWithNullWrapperFieldName|TestWriteThroughCacheCachesObjectWithEmptyListWrapperAndOtherFields|TestWriteThroughCacheSkipsListEnvelopeWithPaginationMetadata", "./internal/cli")
+}
+
func TestSyncDiscriminatorDispatchRoutesMixedItemsToTypedTables(t *testing.T) {
t.Parallel()
diff --git a/internal/generator/templates/data_source.go.tmpl b/internal/generator/templates/data_source.go.tmpl
index 9a83d1b7..934a338e 100644
--- a/internal/generator/templates/data_source.go.tmpl
+++ b/internal/generator/templates/data_source.go.tmpl
@@ -159,6 +159,36 @@ func resolvePaginatedRead(ctx context.Context, c *client.Client, flags *rootFlag
}
}
+// listEnvelopeMetadataKeys are top-level keys that, when accompanying a
+// list-wrapper array, suggest the response is a paginated list envelope
+// rather than a detail object. Used by writeThroughCache to decide
+// whether to upsert a single-object body. Any envelope key NOT in this
+// set (and not a list wrapper itself) signals real per-row data, and
+// the envelope is treated as a detail object even when one of its
+// wrapper-named fields happens to be an empty array.
+var listEnvelopeMetadataKeys = map[string]bool{
+ // list wrappers themselves
+ "results": true, "data": true, "items": true,
+ // pagination cursors / tokens
+ "next_cursor": true, "nextCursor": true,
+ "next_page_token": true, "nextPageToken": true,
+ "page_token": true, "pageToken": true,
+ "end_cursor": true, "endCursor": true,
+ "start_cursor": true, "startCursor": true,
+ "cursor": true, "after": true, "before": true,
+ // has-more flags and page numbers
+ "has_more": true, "hasMore": true, "has_next": true, "hasNext": true,
+ "next_page": true, "previous_page": true,
+ "page": true, "page_size": true, "per_page": true,
+ // counts / totals
+ "total": true, "count": true, "size": true, "total_count": true, "totalCount": true,
+ // wrapper objects
+ "links": true, "meta": true, "pagination": true,
+ "response_metadata": true, "paging": true,
+ // links shape
+ "next": true, "prev": true, "previous": true, "first": true, "last": true,
+}
+
// writeThroughCache upserts live API results into the local SQLite store so
// FTS search covers everything the user has looked up — not just explicit syncs.
// Best-effort: failures are silently ignored (the live result already succeeded).
@@ -187,9 +217,45 @@ func writeThroughCache(ctx context.Context, resourceType string, data json.RawMe
}
}
}
- // Single object with an id field (e.g., detail response)
- if items == nil {
- if _, ok := envelope["id"]; ok {
+ // Single object detail response: let UpsertBatch's existing
+ // resourceIDFieldOverrides mechanism resolve the primary key.
+ // Guarding on envelope["id"] dropped any API whose PK is named
+ // CertNo / sku / invoiceId / etc. on the floor (#1439).
+ //
+ // Treat the envelope as a list-shaped response only when EVERY
+ // top-level key is either a list-wrapper (results/data/items)
+ // holding a real array, or a known pagination-metadata key.
+ // A detail object that happens to carry an empty wrapper-named
+ // field alongside real data (e.g. {"id":"order","items":[],
+ // "status":"pending"}) must still cache as a single row.
+ if items == nil && len(envelope) > 0 {
+ looksLikeListEnvelope := false
+ hasListWrapperArray := false
+ for _, key := range []string{"results", "data", "items"} {
+ raw, ok := envelope[key]
+ if !ok {
+ continue
+ }
+ // json.Unmarshal("null", &arr) succeeds with arr=nil,
+ // so require arr != nil to keep true empty arrays in
+ // the skip branch while letting scalar/null values fall
+ // through as regular field-name collisions.
+ var arr []json.RawMessage
+ if json.Unmarshal(raw, &arr) == nil && arr != nil {
+ hasListWrapperArray = true
+ break
+ }
+ }
+ if hasListWrapperArray {
+ looksLikeListEnvelope = true
+ for k := range envelope {
+ if !listEnvelopeMetadataKeys[k] {
+ looksLikeListEnvelope = false
+ break
+ }
+ }
+ }
+ if !looksLikeListEnvelope {
_, _, _ = db.UpsertBatch(resourceType, []json.RawMessage{data})
return
}
← 5b8a8141 fix(ci): scope settle delay to synchronize, strip head_sha f
·
back to Cli Printing Press
·
fix(ci): use job exit-code instead of POST/PATCH for check_r 94332417 →