feat(npm): datastore remote-repository finder (S15 Step 5, part 1/4)

David Fernandezrequested to merge
10io/s15-npm-remote-step-5-part1of4 into main

📚 Stacked MRs

S15 Step 5 — the read side of the npm-remote datastore layer — is split into 4 stacked MRs to keep each within the review size target (~600 reviewable LoC). Each part targets the previous one (part 1 targets main); review and merge proceed bottom-up. The four parts together supersede the change originally proposed in !1050 (closed), and have since taken review feedback on top of it.

Stacked MRs (review/merge bottom-up)

📦 What this adds (part 1 of 4)

The remote-repository finder: FindRemoteByNameInNamespace resolves a kind=2 (caching-proxy) npm repository by name and returns the two-id pair the proxy handlers key on (npm_remote_repositories.id for child lookups, repositories.id for visibility) plus the remote behavioral fields (upstream URL, interim bearer token, both cache-validity windows, health columns). The parent repositories.soft_deleted_at is gated on the JOIN, so a hosted, virtual, soft-deleted, or cross-namespace row all resolve as ErrNotFound. Both partitioned tables carry the partition key explicitly rather than relying on the planner to propagate it through the JOIN.

NpmRemoteRepositoryResolution carries the interim bearer token as a *crypto.RedactedString, so redaction is a property of the value rather than of every struct that holds it: fmt verbs, slog attributes (top-level and nested), and encoding/json all render [REDACTED], while an anonymous pull stays distinguishable as <nil> / null. The plaintext is reachable only through Reveal, which is the one greppable audit point where this layer's token is read.

🧪 Spec coverage

Spec: docs/specs/S15-npm-remote.md

This part is the datastore read layer. S15's acceptance criteria are stated at the HTTP boundary, so most are owned by later parts and steps; the rows below are the ones this MR's code can satisfy. AC numbering follows the spec's own per-subsection numbering.

Acceptance criteria

# Criterion Tests
Repository kind dispatch A kind=2 npm repository resolves to the remote read path; kind=1 (virtual) and kind=0 (hosted) do not TestNpmRemoteRepositoryStore_FindRemoteByNameInNamespace: resolves a kind=2 npm repository…, …ErrNotFound for a hosted (kind=0) npm repository, …ErrNotFound for a virtual (kind=1) repository even with a remote binding
Data model — npm_remote_repositories The finder returns the binding surrogate, the foundation repositories.id, and every remote behavioral column …resolves a kind=2 npm repository and returns the id pair plus remote fields (whole-struct cmp.Diff, so a new column fails the assertion)
Credentials and health AC-1 (Bearer attached) The set/unset token distinction the Authorization: Bearer decision keys on is surfaced; attaching the header is a later part …resolves a kind=2 npm repository…, returns a nil token when tmp_plaintext_auth_token is NULL
Name resolution Repository name resolution is namespace-scoped (ADR-001 isolation) does not resolve a repository from a different namespace
Write unavailability, Packument proxy, Tarball proxy, Error mapping, Health probe Not in this MR — HTTP-boundary behavior owned by parts 2–4 and later steps

Error cases

# Condition Tests
Resolver not-found A name that belongs to no matching row — wrong format, wrong kind, soft-deleted, or another namespace — resolves as ErrNotFound rather than a partial result …ErrNotFound for a hosted (kind=0) npm repository, …for a virtual (kind=1) repository…, …for a maven repository with an npm remote binding, …when the parent repository is soft-deleted, …when no repository matches the name, does not resolve a repository from a different namespace
Transient DB failure A non-qrm.ErrNoRows DB error surfaces as itself, never as ErrNotFound, so a live remote repository is not routed to the wrong path a transient DB failure is not misreported as ErrNotFound
Caller wiring mistakes Nil context, zero-UUID namespace, and empty repository name each reject with the store's own sentinel before any DB access; a nil client panics at construction TestNpmRemoteRepositoryStore_FindRemoteByNameInNamespace_ArgumentGuards, TestNewNpmRemoteRepositoryStore_NilClientPanics
HTTP error envelope (405 unsupported, 404, 503 upstream_unavailable, upstream 5xx) Not in this MR — owned by parts 2–4 and later steps

Security considerations

# Concern Tests
Credential hygiene The upstream bearer token never appears in logs, metrics, traces, or API responses; it is read only when composing an upstream request Owned by internal/crypto: crypto.RedactedString and internal/crypto/redact_test.go cover every rendering path. This layer's only plaintext read is Reveal.
Credentials at rest Plaintext during the interim Not in this MR — GA-blocking follow-up https://gitlab.com/gitlab-org/ops/artifact-registry/-/work_items/68
SSRF, cross-origin redirect token stripping, outbound path-segment safety, error-payload hygiene Not in this MR — this layer issues no upstream request and builds no URL

🔗 References

Database Review Evidence

Queries

Note

Plans are from EXPLAIN (ANALYZE, BUFFERS) against an ephemeral PostgreSQL 17 container (matching GL_PG_CURR_VERSION from .gitlab-ci-other-versions.yml), with synthesized seed data rolled back per query and the container torn down at the end of the run. Numbers reflect moderate cardinality and do not capture production-scale effects. See Database review evidence for seed sizing, methodology, and the anomalies the skill flags. Expand each row's details for the seed shape, rendered SQL, bound args, and raw plan.

Method Plan node Index Rows (plan / actual) Cost Time Buffers (hit / read) Partitions
datastore.NpmRemoteRepositoryStore.FindRemoteByNameInNamespace Limit → Nested Loop repositories_p20_namespace_id_format_name_idx, npm_remote_repositories_p20_namespace_id_repository_id_idx 1 / 1 16.62 0.023ms 6 / 0 1 of 64 per table
datastore.NpmRemoteRepositoryStore.FindRemoteByNameInNamespace

Summary: The plan matches the method's intent. The driving scan is an Index Scan over the partial index (namespace_id, format, name) WHERE soft_deleted_at IS NULL, which absorbs the namespace, format, name, and soft-delete predicates in the index condition and leaves only the low-cardinality kind = 2 as a Filter on an already single-row result; the binding is then resolved through a Nested Loop Index Scan on the (namespace_id, repository_id) unique index. Passing namespace_id to both tables explicitly prunes each to exactly one of its 64 hash partitions (repositories_p20, npm_remote_repositories_p20), estimates match reality exactly (1 / 1), and the 6 execution-time buffer accesses are all cache hits with no reads. No anomalies against the flagged criteria, with one number worth reading carefully: planning dominates execution by 41x (1.772ms against 0.043ms), and planning touches shared hit=747 read=1 opening 64 partitions' catalog entries before pruning discards 63 of them. Because simple query protocol leaves no server-side prepared statements, every call re-plans, so the realistic per-request cost is the 1.815ms total rather than the 0.023ms in the Time column above. That is still well inside the 100 ms budget and warrants no code change.

Seed shape: namespaces=1, repositories=5000, npm_remote_repositories=5000

Rendered SQL:

SELECT npm_remote_repositories.id AS "npm_remote_repositories.id",
     npm_remote_repositories.repository_id AS "npm_remote_repositories.repository_id",
     npm_remote_repositories.url AS "npm_remote_repositories.url",
     npm_remote_repositories.tmp_plaintext_auth_token AS "npm_remote_repositories.tmp_plaintext_auth_token",
     npm_remote_repositories.cache_validity_hours AS "npm_remote_repositories.cache_validity_hours",
     npm_remote_repositories.metadata_cache_validity_hours AS "npm_remote_repositories.metadata_cache_validity_hours",
     npm_remote_repositories.last_health_status AS "npm_remote_repositories.last_health_status",
     npm_remote_repositories.last_health_checked_at AS "npm_remote_repositories.last_health_checked_at"
FROM public.npm_remote_repositories
     INNER JOIN public.repositories ON ((repositories.id = npm_remote_repositories.repository_id) AND (repositories.namespace_id = npm_remote_repositories.namespace_id))
WHERE (((((npm_remote_repositories.namespace_id = $1::uuid) AND (repositories.namespace_id = $2::uuid)) AND (repositories.name = $3::text)) AND (repositories.format = $4)) AND (repositories.kind = $5)) AND (repositories.soft_deleted_at IS NULL)
LIMIT $6;

Bound args: ['8c21f7a7-7fa0-44fa-bf3f-fff2538f8e50', '8c21f7a7-7fa0-44fa-bf3f-fff2538f8e50', 'review-prep-repo-002500', 2, 2, 1]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Limit  (cost=0.56..16.62 rows=1 width=91) (actual time=0.022..0.023 rows=1 loops=1)
   Buffers: shared hit=6
   ->  Nested Loop  (cost=0.56..16.62 rows=1 width=91) (actual time=0.022..0.022 rows=1 loops=1)
         Buffers: shared hit=6
         ->  Index Scan using repositories_p20_namespace_id_format_name_idx on repositories_p20 repositories  (cost=0.28..8.31 rows=1 width=32) (actual time=0.012..0.012 rows=1 loops=1)
               Index Cond: ((namespace_id = '8c21f7a7-7fa0-44fa-bf3f-fff2538f8e50'::uuid) AND (format = '2'::smallint) AND (name = 'review-prep-repo-002500'::text))
               Filter: (kind = '2'::smallint)
               Buffers: shared hit=3
         ->  Index Scan using npm_remote_repositories_p20_namespace_id_repository_id_idx on npm_remote_repositories_p20 npm_remote_repositories  (cost=0.28..8.30 rows=1 width=107) (actual time=0.009..0.009 rows=1 loops=1)
               Index Cond: ((namespace_id = '8c21f7a7-7fa0-44fa-bf3f-fff2538f8e50'::uuid) AND (repository_id = repositories.id))
               Buffers: shared hit=3
 Planning:
   Buffers: shared hit=747 read=1
 Planning Time: 1.772 ms
 Execution Time: 0.043 ms

Timings: planning 1.772ms, execution 0.043ms, total 1.815ms.

Related to #342 (closed)

Edited by David Fernandez

Merge request reports