From b4b94875c8aea35d9b6ab4bfc4277da3b5c6c9ec Mon Sep 17 00:00:00 2001 From: dygerydoo Date: Sun, 15 Mar 2026 08:58:32 +0100 Subject: [PATCH] fix: make registry project field required for gitlab/github types Use Zod discriminated union in RegistryEntrySchema so project is validated at schema level instead of runtime. Narrow ResolvedRegistry into typed variants (ResolvedGitLabRegistry, ResolvedGitHubRegistry, ResolvedDefaultRegistry) so constructors enforce the constraint at compile time, eliminating runtime null checks. Closes #93 --- src/registry/clients/github-browser.ts | 8 +-- src/registry/clients/github.test.ts | 69 +++++++--------------- src/registry/clients/github.ts | 10 +--- src/registry/clients/gitlab-browser.ts | 8 +-- src/registry/clients/gitlab.test.ts | 81 ++++++++++---------------- src/registry/clients/gitlab.ts | 8 +-- src/registry/factory.test.ts | 13 +---- src/registry/registry.types.ts | 25 ++++++-- src/registry/resolver.ts | 19 +++--- src/schemas/index.test.ts | 38 +++++++++--- src/schemas/index.ts | 30 ++++++++-- 11 files changed, 147 insertions(+), 162 deletions(-) diff --git a/src/registry/clients/github-browser.ts b/src/registry/clients/github-browser.ts index 257e596..faff6a9 100644 --- a/src/registry/clients/github-browser.ts +++ b/src/registry/clients/github-browser.ts @@ -12,7 +12,7 @@ */ import type { HttpClient } from "#/core"; -import type { ResolvedRegistry } from "../registry.types"; +import type { ResolvedGitHubRegistry } from "../registry.types"; import type { RegistryBrowser, BrowseResult, BrowsedArtifact } from "../browse.types"; import { ArtifactManifestSchema } from "#/schemas"; import { parse as parseYaml } from "yaml"; @@ -48,11 +48,7 @@ export class GitHubRepositoryBrowser implements RegistryBrowser { private token?: string; private http: HttpClient; - constructor(registry: ResolvedRegistry, http: HttpClient) { - if (!registry.project) { - throw new Error("GitHub browser requires 'project' field (owner/repo format)"); - } - + constructor(registry: ResolvedGitHubRegistry, http: HttpClient) { this.apiHost = resolveApiHost(registry.host); this.token = registry.token; this.http = http; diff --git a/src/registry/clients/github.test.ts b/src/registry/clients/github.test.ts index 9c21ad5..e887c9b 100644 --- a/src/registry/clients/github.test.ts +++ b/src/registry/clients/github.test.ts @@ -7,14 +7,18 @@ import { createMockTarOperations, jsonResponse, } from "#/test-utils/mocks"; -import type { ResolvedRegistry } from "../registry.types"; +import type { ResolvedGitHubRegistry } from "../registry.types"; describe("GitHubRegistryClient", () => { - const createClient = (registry: Partial = {}) => { - const fullRegistry: ResolvedRegistry = { - type: "github", - host: "ghcr.io", - project: "myorg", + const defaultGitHubRegistry: ResolvedGitHubRegistry = { + type: "github", + host: "ghcr.io", + project: "myorg", + }; + + const createClient = (registry: Partial = {}) => { + const fullRegistry: ResolvedGitHubRegistry = { + ...defaultGitHubRegistry, ...registry, }; const http = createMockHttpClient(); @@ -31,37 +35,8 @@ describe("GitHubRegistryClient", () => { }; }; - describe("constructor", () => { - test("throws when project field is missing", () => { - const registry: ResolvedRegistry = { - type: "github", - host: "ghcr.io", - // project is missing - }; - const http = createMockHttpClient(); - const fs = createMockFileSystem(); - const shell = createMockShellExecutor(); - - expect( - () => new GitHubRegistryClient(registry, http, fs, shell, createMockTarOperations()) - ).toThrow("GitHub registry requires 'project' field in config"); - }); - - test("accepts registry with project field", () => { - const registry: ResolvedRegistry = { - type: "github", - host: "ghcr.io", - project: "myorg", - }; - const http = createMockHttpClient(); - const fs = createMockFileSystem(); - const shell = createMockShellExecutor(); - - expect( - () => new GitHubRegistryClient(registry, http, fs, shell, createMockTarOperations()) - ).not.toThrow(); - }); - }); + // constructor: project is now required by the type system (discriminated union) + // so runtime null checks are no longer needed describe("prefix configuration", () => { test("prepends prefix to repository name when configured", async () => { @@ -78,7 +53,7 @@ describe("GitHubRegistryClient", () => { return jsonResponse({ tags: [] }); }; - const registry: ResolvedRegistry = { + const registry: ResolvedGitHubRegistry = { type: "github", host: "ghcr.io", project: "myorg", @@ -104,7 +79,7 @@ describe("GitHubRegistryClient", () => { return jsonResponse({ tags: [] }); }; - const registry: ResolvedRegistry = { + const registry: ResolvedGitHubRegistry = { type: "github", host: "ghcr.io", project: "myorg", @@ -130,7 +105,7 @@ describe("GitHubRegistryClient", () => { return jsonResponse({ tags: [] }); }; - const registry: ResolvedRegistry = { + const registry: ResolvedGitHubRegistry = { type: "github", host: "ghcr.io", project: "myorg", @@ -175,7 +150,7 @@ describe("GitHubRegistryClient", () => { const shell = createMockShellExecutor({ tar: "" }); fs.files.set("/target/file.md", { content: "content", isDirectory: false }); - const registry: ResolvedRegistry = { + const registry: ResolvedGitHubRegistry = { type: "github", host: "ghcr.io", project: "myorg", @@ -201,7 +176,7 @@ describe("GitHubRegistryClient", () => { return jsonResponse({}); }; - const registry: ResolvedRegistry = { + const registry: ResolvedGitHubRegistry = { type: "github", host: "ghcr.io", project: "myorg", @@ -221,7 +196,7 @@ describe("GitHubRegistryClient", () => { return jsonResponse({}); }; - const registry: ResolvedRegistry = { + const registry: ResolvedGitHubRegistry = { type: "github", host: "ghcr.io", project: "myorg", @@ -290,7 +265,7 @@ describe("GitHubRegistryClient", () => { }); }; - const registry: ResolvedRegistry = { + const registry: ResolvedGitHubRegistry = { type: "github", host: "ghcr.io", project: "myorg", @@ -362,7 +337,7 @@ describe("GitHubRegistryClient", () => { return new Response("Forbidden", { status: 403 }); }; - const registry: ResolvedRegistry = { + const registry: ResolvedGitHubRegistry = { type: "github", host: "ghcr.io", project: "myorg", @@ -395,7 +370,7 @@ describe("GitHubRegistryClient", () => { }); test("returns error when oras is not installed", async () => { - const registry: ResolvedRegistry = { + const registry: ResolvedGitHubRegistry = { type: "github", host: "ghcr.io", project: "myorg", @@ -426,7 +401,7 @@ describe("GitHubRegistryClient", () => { }); test("passes relative tarball path to oras", async () => { - const registry: ResolvedRegistry = { + const registry: ResolvedGitHubRegistry = { type: "github", host: "ghcr.io", project: "myorg", diff --git a/src/registry/clients/github.ts b/src/registry/clients/github.ts index 6ad4cc8..2e77bde 100644 --- a/src/registry/clients/github.ts +++ b/src/registry/clients/github.ts @@ -14,7 +14,7 @@ import { relative } from "path"; import { validateTarballContents, generateSecureTempPath, type FileSystem, type HttpClient, type ShellExecutor, type TarOperations } from "#/core"; import type { RegistryClient, - ResolvedRegistry, + ResolvedGitHubRegistry, DownloadResult, PublishResult, RegistryArtifactInfo, @@ -38,18 +38,12 @@ export class GitHubRegistryClient implements RegistryClient { private ociClient: OciClient; constructor( - registry: ResolvedRegistry, + registry: ResolvedGitHubRegistry, http: HttpClient, fs: FileSystem, shell: ShellExecutor, tar: TarOperations ) { - if (!registry.project) { - throw new Error( - "GitHub registry requires 'project' field in config (your GHCR namespace, e.g., 'myorg' for ghcr.io/myorg/*)" - ); - } - this.host = registry.host || DEFAULT_GHCR_HOST; this.token = registry.token; this.prefix = registry.prefix; diff --git a/src/registry/clients/gitlab-browser.ts b/src/registry/clients/gitlab-browser.ts index e7f2cc5..25fbbde 100644 --- a/src/registry/clients/gitlab-browser.ts +++ b/src/registry/clients/gitlab-browser.ts @@ -12,7 +12,7 @@ */ import type { HttpClient } from "#/core"; -import type { ResolvedRegistry } from "../registry.types"; +import type { ResolvedGitLabRegistry } from "../registry.types"; import type { RegistryBrowser, BrowseResult, BrowsedArtifact } from "../browse.types"; import { ArtifactManifestSchema } from "#/schemas"; import { parse as parseYaml } from "yaml"; @@ -34,11 +34,7 @@ export class GitLabRepositoryBrowser implements RegistryBrowser { private token?: string; private http: HttpClient; - constructor(registry: ResolvedRegistry, http: HttpClient) { - if (!registry.project) { - throw new Error("GitLab browser requires 'project' field in config"); - } - + constructor(registry: ResolvedGitLabRegistry, http: HttpClient) { this.host = normalizeHost(registry.host); this.encodedProject = encodeURIComponent(normalizeProject(registry.project)); this.token = registry.token; diff --git a/src/registry/clients/gitlab.test.ts b/src/registry/clients/gitlab.test.ts index df95472..313747e 100644 --- a/src/registry/clients/gitlab.test.ts +++ b/src/registry/clients/gitlab.test.ts @@ -8,17 +8,21 @@ import { binaryResponse, errorResponse, } from "#/test-utils/mocks"; -import type { ResolvedRegistry } from "../registry.types"; +import type { ResolvedGitLabRegistry } from "../registry.types"; describe("GitLabRegistryClient", () => { + const defaultGitLabRegistry: ResolvedGitLabRegistry = { + type: "gitlab", + host: "gitlab.com", + project: "group/project", + }; + const createClient = ( - registry: Partial = {}, + registry: Partial = {}, httpResponses = new Map Response)>() ) => { - const fullRegistry: ResolvedRegistry = { - type: "gitlab", - host: "gitlab.com", - project: "group/project", + const fullRegistry: ResolvedGitLabRegistry = { + ...defaultGitLabRegistry, ...registry, }; const http = createMockHttpClient(httpResponses); @@ -34,31 +38,6 @@ describe("GitLabRegistryClient", () => { }; describe("constructor", () => { - test("throws when project field is missing", () => { - const registry: ResolvedRegistry = { - type: "gitlab", - host: "gitlab.com", - // project is missing - }; - const http = createMockHttpClient(); - const fs = createMockFileSystem(); - expect( - () => new GitLabRegistryClient(registry, http, fs, createMockTarOperations()) - ).toThrow("GitLab registry requires 'project' field in config"); - }); - - test("accepts registry with project field", () => { - const registry: ResolvedRegistry = { - type: "gitlab", - host: "gitlab.com", - project: "group/project", - }; - const http = createMockHttpClient(); - const fs = createMockFileSystem(); - expect( - () => new GitLabRegistryClient(registry, http, fs, createMockTarOperations()) - ).not.toThrow(); - }); test("normalizes host by stripping https:// prefix", async () => { let requestedUrl = ""; @@ -69,7 +48,7 @@ describe("GitLabRegistryClient", () => { return jsonResponse([]); }; - const registry: ResolvedRegistry = { + const registry: ResolvedGitLabRegistry = { type: "gitlab", host: "https://gitlab.example.com", project: "group/project", @@ -91,7 +70,7 @@ describe("GitLabRegistryClient", () => { return jsonResponse([]); }; - const registry: ResolvedRegistry = { + const registry: ResolvedGitLabRegistry = { type: "gitlab", host: "http://gitlab.internal", project: "group/project", @@ -113,7 +92,7 @@ describe("GitLabRegistryClient", () => { return jsonResponse([]); }; - const registry: ResolvedRegistry = { + const registry: ResolvedGitLabRegistry = { type: "gitlab", host: "gitlab.com", project: "group/subgroup/project", @@ -134,7 +113,7 @@ describe("GitLabRegistryClient", () => { return jsonResponse([]); }; - const registry: ResolvedRegistry = { + const registry: ResolvedGitLabRegistry = { type: "gitlab", host: "gitlab.com", project: "/group/subgroup/project", @@ -155,7 +134,7 @@ describe("GitLabRegistryClient", () => { throw new Error("fetch failed"); }; - const registry: ResolvedRegistry = { + const registry: ResolvedGitLabRegistry = { type: "gitlab", host: "gitlab.unreachable.com", project: "group/project", @@ -172,7 +151,7 @@ describe("GitLabRegistryClient", () => { const http = createMockHttpClient(); http.fetch = async () => errorResponse(401, "Unauthorized"); - const registry: ResolvedRegistry = { + const registry: ResolvedGitLabRegistry = { type: "gitlab", host: "gitlab.com", project: "group/project", @@ -269,7 +248,7 @@ describe("GitLabRegistryClient", () => { const fs = createMockFileSystem(); - const registry: ResolvedRegistry = { + const registry: ResolvedGitLabRegistry = { type: "gitlab", host: "gitlab.mycompany.com", project: "team/artifacts", @@ -328,7 +307,7 @@ describe("GitLabRegistryClient", () => { return errorResponse(404, "Not Found"); }; - const registry: ResolvedRegistry = { + const registry: ResolvedGitLabRegistry = { type: "gitlab", host: "gitlab.com", project: "group/project", @@ -361,7 +340,7 @@ describe("GitLabRegistryClient", () => { "/path/to/tarball.tar.gz": Buffer.from("tarball content"), }); - const registry: ResolvedRegistry = { + const registry: ResolvedGitLabRegistry = { type: "gitlab", host: "gitlab.com", project: "group/project", @@ -480,7 +459,7 @@ describe("GitLabRegistryClient", () => { return errorResponse(404, "Not Found"); }; - const registry: ResolvedRegistry = { + const registry: ResolvedGitLabRegistry = { type: "gitlab", host: "gitlab.com", project: "group/project", @@ -496,7 +475,7 @@ describe("GitLabRegistryClient", () => { const http = createMockHttpClient(); http.fetch = async () => errorResponse(404, "Not Found"); - const registry: ResolvedRegistry = { + const registry: ResolvedGitLabRegistry = { type: "gitlab", host: "gitlab.com", project: "group/project", @@ -519,7 +498,7 @@ describe("GitLabRegistryClient", () => { return new Response(null, { status: 200 }); }; - const registry: ResolvedRegistry = { + const registry: ResolvedGitLabRegistry = { type: "gitlab", host: "gitlab.com", project: "group/project", @@ -600,7 +579,7 @@ describe("GitLabRegistryClient", () => { return jsonResponse([]); }; - const registry: ResolvedRegistry = { + const registry: ResolvedGitLabRegistry = { type: "gitlab", host: "gitlab.com", project: "group/project", @@ -623,7 +602,7 @@ describe("GitLabRegistryClient", () => { return jsonResponse([]); }; - const registry: ResolvedRegistry = { + const registry: ResolvedGitLabRegistry = { type: "gitlab", host: "gitlab.com", project: "group/project", @@ -646,7 +625,7 @@ describe("GitLabRegistryClient", () => { return jsonResponse([]); }; - const registry: ResolvedRegistry = { + const registry: ResolvedGitLabRegistry = { type: "gitlab", host: "gitlab.com", project: "group/project", @@ -671,7 +650,7 @@ describe("GitLabRegistryClient", () => { return jsonResponse([]); }; - const registry: ResolvedRegistry = { + const registry: ResolvedGitLabRegistry = { type: "gitlab", host: "gitlab.com", project: "group/project", @@ -694,7 +673,7 @@ describe("GitLabRegistryClient", () => { return jsonResponse([]); }; - const registry: ResolvedRegistry = { + const registry: ResolvedGitLabRegistry = { type: "gitlab", host: "gitlab.com", project: "group/project", @@ -730,7 +709,7 @@ describe("GitLabRegistryClient", () => { const fs = createMockFileSystem(); fs.files.set("/target/file.md", { content: "content", isDirectory: false }); - const registry: ResolvedRegistry = { + const registry: ResolvedGitLabRegistry = { type: "gitlab", host: "gitlab.com", project: "group/project", @@ -762,7 +741,7 @@ describe("GitLabRegistryClient", () => { "/path/to/tarball.tar.gz": Buffer.from("tarball content"), }); - const registry: ResolvedRegistry = { + const registry: ResolvedGitLabRegistry = { type: "gitlab", host: "gitlab.com", project: "group/project", @@ -786,7 +765,7 @@ describe("GitLabRegistryClient", () => { return jsonResponse([]); }; - const registry: ResolvedRegistry = { + const registry: ResolvedGitLabRegistry = { type: "gitlab", host: "gitlab.com", project: "group/project", diff --git a/src/registry/clients/gitlab.ts b/src/registry/clients/gitlab.ts index 4481c48..8a323d8 100644 --- a/src/registry/clients/gitlab.ts +++ b/src/registry/clients/gitlab.ts @@ -13,7 +13,7 @@ import { validateTarballContents, generateSecureTempPath, type FileSystem, type HttpClient, type TarOperations } from "#/core"; import type { RegistryClient, - ResolvedRegistry, + ResolvedGitLabRegistry, DownloadResult, PublishResult, RegistryArtifactInfo, @@ -40,15 +40,11 @@ export class GitLabRegistryClient implements RegistryClient { private tar: TarOperations; constructor( - registry: ResolvedRegistry, + registry: ResolvedGitLabRegistry, http: HttpClient, fs: FileSystem, tar: TarOperations ) { - if (!registry.project) { - throw new Error("GitLab registry requires 'project' field in config"); - } - this.host = normalizeHost(registry.host); // URL-encode the project path for API calls // GitLab API accepts both numeric IDs and URL-encoded paths diff --git a/src/registry/factory.test.ts b/src/registry/factory.test.ts index df7351a..9b82dd5 100644 --- a/src/registry/factory.test.ts +++ b/src/registry/factory.test.ts @@ -53,17 +53,8 @@ describe("factory", () => { expect(client).toBeInstanceOf(DefaultRegistryClient); }); - test("throws when gitlab registry missing project", () => { - const registry: ResolvedRegistry = { - type: "gitlab", - host: "gitlab.com", - // project is missing - }; - - expect(() => createRegistryClient(registry, http, fs, shell, tar)).toThrow( - "GitLab registry requires 'project' field in config" - ); - }); + // project is now required by the type system for gitlab/github types, + // so missing-project tests are no longer needed test("passes token to gitlab client", () => { const registry: ResolvedRegistry = { diff --git a/src/registry/registry.types.ts b/src/registry/registry.types.ts index 10f0759..05db5c9 100644 --- a/src/registry/registry.types.ts +++ b/src/registry/registry.types.ts @@ -12,18 +12,31 @@ export type { LocalConfig, RegistryEntry } from "#/schemas"; export type RegistryType = "gitlab" | "github" | "default"; /** - * Normalized registry configuration. - * Created by resolver from raw config, used by factory to create clients. + * Shared fields for all resolved registry configurations. */ -export interface ResolvedRegistry { - type: RegistryType; +interface ResolvedRegistryBase { host: string; - project?: string; token?: string; - prefix?: string; // Package name prefix (e.g., "frontend" → "frontend-artifact-name") + prefix?: string; // Artifact name prefix (e.g., "frontend" → "frontend-artifact-name") apiBasePath?: string; // REST API base path for default registry (e.g., "/functions/v1") } +/** + * Normalized registry configuration. + * Created by resolver from raw config, used by factory to create clients. + * + * Uses discriminated union: project is guaranteed for gitlab/github types, + * eliminating runtime null checks in registry clients. + */ +export type ResolvedGitLabRegistry = ResolvedRegistryBase & { type: "gitlab"; project: string }; +export type ResolvedGitHubRegistry = ResolvedRegistryBase & { type: "github"; project: string }; +export type ResolvedDefaultRegistry = ResolvedRegistryBase & { type: "default"; project?: string }; + +export type ResolvedRegistry = + | ResolvedGitLabRegistry + | ResolvedGitHubRegistry + | ResolvedDefaultRegistry; + /** * Result from download operation */ diff --git a/src/registry/resolver.ts b/src/registry/resolver.ts index f435bb5..3cdf124 100644 --- a/src/registry/resolver.ts +++ b/src/registry/resolver.ts @@ -105,14 +105,17 @@ export function resolveRegistry( } } - return { - type: entry.type, - host: entry.host || getDefaultHost(entry.type), - project: entry.project, - token, - prefix: entry.prefix, - apiBasePath: entry.type === "default" ? DEFAULT_REGISTRY_API_PATH : undefined, - }; + const host = entry.host || getDefaultHost(entry.type); + const base = { host, token, prefix: entry.prefix }; + + switch (entry.type) { + case "gitlab": + return { ...base, type: "gitlab" as const, project: entry.project }; + case "github": + return { ...base, type: "github" as const, project: entry.project }; + case "default": + return { ...base, type: "default" as const, project: entry.project, apiBasePath: DEFAULT_REGISTRY_API_PATH }; + } } /** diff --git a/src/schemas/index.test.ts b/src/schemas/index.test.ts index fae1a53..5a81e49 100644 --- a/src/schemas/index.test.ts +++ b/src/schemas/index.test.ts @@ -607,7 +607,7 @@ describe("schemas", () => { }); describe("RegistryEntrySchema", () => { - test("parses gitlab registry", () => { + test("parses gitlab registry with required project", () => { const entry = { type: "gitlab" as const, project: "myteam/artifacts", @@ -632,13 +632,37 @@ describe("schemas", () => { expect(result.host).toBe("gitlab.company.com"); }); - test("accepts all valid types", () => { - const types = ["gitlab", "github", "default"] as const; + test("parses github registry with required project", () => { + const entry = { + type: "github" as const, + project: "myorg", + }; - for (const type of types) { - const result = RegistryEntrySchema.parse({ type }); - expect(result.type).toBe(type); - } + const result = RegistryEntrySchema.parse(entry); + + expect(result.type).toBe("github"); + expect(result.project).toBe("myorg"); + }); + + test("parses default registry without project", () => { + const entry = { type: "default" as const }; + + const result = RegistryEntrySchema.parse(entry); + + expect(result.type).toBe("default"); + expect(result.project).toBeUndefined(); + }); + + test("rejects gitlab without project", () => { + const entry = { type: "gitlab" as const }; + + expect(() => RegistryEntrySchema.parse(entry)).toThrow(); + }); + + test("rejects github without project", () => { + const entry = { type: "github" as const }; + + expect(() => RegistryEntrySchema.parse(entry)).toThrow(); }); }); diff --git a/src/schemas/index.ts b/src/schemas/index.ts index 4cbcbeb..58dcf7f 100644 --- a/src/schemas/index.ts +++ b/src/schemas/index.ts @@ -208,14 +208,32 @@ export const ArtifactMetadataSchema = z.object({ }); export type ArtifactMetadata = z.infer; -// Registry entry for local config (.grekt/config.yaml) -export const RegistryEntrySchema = z.object({ - type: z.enum(["gitlab", "github", "default"]), - project: z.string().optional(), // Required for gitlab/github, validated at runtime +// Shared optional fields for all registry types +const registrySharedFields = { host: z.string().optional(), // Optional, has defaults (gitlab.com, github.com) token: z.string().optional(), // Can also be set via env vars - prefix: z.string().optional(), // Package name prefix (e.g., "frontend" → "frontend-artifact-name") -}); + prefix: z.string().optional(), // Artifact name prefix (e.g., "frontend" → "frontend-artifact-name") +}; + +// Registry entry for local config (.grekt/config.yaml) +// Uses discriminated union: project is required for gitlab/github, optional for default +export const RegistryEntrySchema = z.discriminatedUnion("type", [ + z.object({ + type: z.literal("gitlab"), + project: z.string(), // Required: GitLab project path (e.g., "myteam/artifacts") + ...registrySharedFields, + }), + z.object({ + type: z.literal("github"), + project: z.string(), // Required: GHCR namespace (e.g., "myorg") + ...registrySharedFields, + }), + z.object({ + type: z.literal("default"), + project: z.string().optional(), + ...registrySharedFields, + }), +]); export type RegistryEntry = z.infer; // Session stored in local config (generated by grekt login)