From 61f684947f70b948bb8eb32560f644135235d1c2 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Tue, 4 Aug 2026 17:06:50 +0200 Subject: [PATCH] fix(appkit): gate typegen fallback per surface, not across surfaces The `--wait` committed-types fallback tracked a single `hadEnvironmentalFailure` flag for both analytics queries and metric views, and satisfied the gate if *either* committed artifact existed. When only one surface failed, an unrelated committed artifact could stand in for the missing one: queries failing environmentally with no committed `analytics.d.ts` still exited 0 as long as a `metric-views.d.ts` happened to be present, leaving CI green with types that were never generated. Track the failure per surface and require each failing surface to have its own committed artifact. Metric-view degradation now participates in the gate the same way degraded queries already did, and the fatal error names the missing artifacts so the remedy points at the right surface. Signed-off-by: Atila Fassina --- packages/appkit/src/type-generator/index.ts | 60 +++++-------- .../src/type-generator/tests/index.test.ts | 86 +++++++++++++------ .../tests/unreachable-warehouse-gate.test.ts | 21 +++++ 3 files changed, 105 insertions(+), 62 deletions(-) diff --git a/packages/appkit/src/type-generator/index.ts b/packages/appkit/src/type-generator/index.ts index 286874852..e8d47feac 100644 --- a/packages/appkit/src/type-generator/index.ts +++ b/packages/appkit/src/type-generator/index.ts @@ -84,21 +84,6 @@ function plural(count: number, singular: string, pluralForm = `${singular}s`) { return count === 1 ? singular : pluralForm; } -/** - * Check if committed type artifacts exist (at least one of the requested surfaces). - * Serving types are excluded (gitignored, never part of the gate). - * Returns true if either the analytics or metric-views committed .d.ts file exists. - */ -function hasCommittedTypes( - analyticsOutFile: string, - metricViewsOutFile: string | undefined, -): boolean { - const hasAnalytics = existsSync(analyticsOutFile); - const hasMetrics = - metricViewsOutFile !== undefined && existsSync(metricViewsOutFile); - return hasAnalytics || hasMetrics; -} - function isQueryDegraded(schema: QuerySchema): boolean { return schema.degraded === true; } @@ -361,6 +346,8 @@ export async function generateFromEntryPoint(options: { const metricViewsFolder = options.metricViewsFolder ?? (queryFolder ? path.resolve(queryFolder, "..", "metric-views") : undefined); + const resolvedMvFile = + mvOutFile ?? path.join(path.dirname(outFile), METRIC_TYPES_FILE); const projectRoot = resolveProjectRoot(outFile); @@ -370,8 +357,8 @@ export async function generateFromEntryPoint(options: { let syntaxErrors: QuerySyntaxError[] = []; // Deterministic fatal errors only (404/400). let fatalErrors: QueryFatalError[] = []; - // Track whether an environmental failure occurred in blocking mode. - let hadEnvironmentalFailure = false; + let queryHadEnvironmentalFailure = false; + let metricsHadEnvironmentalFailure = false; // Track the coarse cause of the environmental failure for the warning message. let environmentalCause: "auth" | "unreachable" | "unavailable" | undefined; @@ -383,8 +370,7 @@ export async function generateFromEntryPoint(options: { queryRegistry = result.schemas; syntaxErrors = result.syntaxErrors ?? []; fatalErrors = result.fatalErrors ?? []; - hadEnvironmentalFailure = - hadEnvironmentalFailure || (result.hadEnvironmentalFailure ?? false); + queryHadEnvironmentalFailure = result.hadEnvironmentalFailure ?? false; environmentalCause = environmentalCause ?? result.environmentalCause ?? undefined; } @@ -399,7 +385,7 @@ export async function generateFromEntryPoint(options: { // A degraded schema always participates in the committed-types gate. Keep // this invariant next to write suppression so a new producer cannot update // one decision without the other. - hadEnvironmentalFailure = true; + queryHadEnvironmentalFailure = true; environmentalCause = environmentalCause ?? "unavailable"; } const shouldWriteQueries = mode !== "blocking" || !hasAnyDegradedQuery; @@ -414,15 +400,12 @@ export async function generateFromEntryPoint(options: { // metric views without any `.sql` queries). `syncMetricViewsTypes` still // returns `noConfig` when the folder holds no `definitions.json`. if (metricViewsFolder) { - const mvFile = - mvOutFile ?? path.join(path.dirname(outFile), METRIC_TYPES_FILE); - let mvResult: SyncMetricViewsTypesResult; try { mvResult = await syncMetricViewsTypes({ metricViewsFolder, warehouseId, - metricOutFile: mvFile, + metricOutFile: resolvedMvFile, cache: !noCache, metricFetcher, mode, @@ -451,9 +434,9 @@ export async function generateFromEntryPoint(options: { fatalErrors.push(fe); } - // Thread through the environmental failure flag and cause. - hadEnvironmentalFailure = - hadEnvironmentalFailure || (mvResult.hadEnvironmentalFailure ?? false); + metricsHadEnvironmentalFailure = + (mvResult.hadEnvironmentalFailure ?? false) || + (mode === "blocking" && hasAnyDegradedMetrics(mvResult.schemas)); environmentalCause = environmentalCause ?? mvResult.environmentalCause ?? undefined; @@ -483,15 +466,19 @@ export async function generateFromEntryPoint(options: { throw new TypegenFatalError(fatalErrors, warehouseId); } - // Environmental failures (in blocking mode) trigger the has-types gate. - if (mode === "blocking" && hadEnvironmentalFailure) { - // Determine resolved metric-views file for the has-types check. - const resolvedMvFile = - options.mvOutFile ?? path.join(path.dirname(outFile), METRIC_TYPES_FILE); - - const hasTypes = hasCommittedTypes(outFile, resolvedMvFile); + if ( + mode === "blocking" && + (queryHadEnvironmentalFailure || metricsHadEnvironmentalFailure) + ) { + const missingCommittedTypes: string[] = []; + if (queryHadEnvironmentalFailure && !existsSync(outFile)) { + missingCommittedTypes.push(path.basename(outFile)); + } + if (metricsHadEnvironmentalFailure && !existsSync(resolvedMvFile)) { + missingCommittedTypes.push(path.basename(resolvedMvFile)); + } - if (hasTypes) { + if (missingCommittedTypes.length === 0) { // Committed types present: emit loud warning and exit 0. const warningMessage = determineWarningMessage( environmentalCause ?? "unavailable", @@ -499,12 +486,11 @@ export async function generateFromEntryPoint(options: { ); logger.warn(warningMessage); } else { - // No committed types: crash with a generic message. throw new TypegenFatalError( [ { name: "type-generator", - message: `Warehouse ${warehouseId} could not be reached and no committed types exist. Run 'npx @databricks/appkit generate-types --wait' locally and commit the generated .d.ts files.`, + message: `Warehouse ${warehouseId} could not provide schemas and the required committed type ${plural(missingCommittedTypes.length, "artifact is", "artifacts are")} missing: ${missingCommittedTypes.join(", ")}. Run 'npx @databricks/appkit generate-types --wait' locally and commit the generated .d.ts files.`, }, ], warehouseId, diff --git a/packages/appkit/src/type-generator/tests/index.test.ts b/packages/appkit/src/type-generator/tests/index.test.ts index b14bc85f6..830752d61 100644 --- a/packages/appkit/src/type-generator/tests/index.test.ts +++ b/packages/appkit/src/type-generator/tests/index.test.ts @@ -325,6 +325,13 @@ describe("generateFromEntryPoint — metric-view emission", () => { ); }; + const writeCommittedMetricTypes = () => { + const committed = "// committed metric types\n"; + fs.mkdirSync(path.dirname(metricFile), { recursive: true }); + fs.writeFileSync(metricFile, committed, "utf-8"); + return committed; + }; + beforeEach(() => { vi.clearAllMocks(); mocks.cacheFile.contents = undefined; @@ -605,9 +612,7 @@ describe("generateFromEntryPoint — metric-view emission", () => { test("blocking + transient metric DESCRIBE failure: warns and preserves committed metric types", async () => { writeMetricConfig(); - fs.mkdirSync(path.dirname(metricFile), { recursive: true }); - const committed = "// committed metric types\n"; - fs.writeFileSync(metricFile, committed, "utf-8"); + const committed = writeCommittedMetricTypes(); const unreachable = Object.assign( new Error("connect ECONNREFUSED 10.0.0.1:443"), @@ -636,8 +641,33 @@ describe("generateFromEntryPoint — metric-view emission", () => { } }); + test("blocking + transient metric failure: generated analytics types do not satisfy a missing metric fallback", async () => { + writeMetricConfig(); + const unreachable = Object.assign( + new Error("connect ECONNREFUSED 10.0.0.1:443"), + { code: "ECONNREFUSED" }, + ); + mocks.getWarehouseState.mockRejectedValue(unreachable); + + const error = await generateFromEntryPoint({ + outFile, + queryFolder, + warehouseId: "wh-1", + mode: "blocking", + }).then( + () => undefined, + (reason: unknown) => reason, + ); + + expect(error).toBeInstanceOf(TypegenFatalError); + expect((error as Error).message).toContain("metric-views.d.ts"); + expect(fs.existsSync(outFile)).toBe(true); + expect(fs.existsSync(metricFile)).toBe(false); + }); + test("blocking + a non-terminal DESCRIBE (warehouse not ready): degrades, does NOT escalate", async () => { writeMetricConfig(); + const committed = writeCommittedMetricTypes(); const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); @@ -663,8 +693,9 @@ describe("generateFromEntryPoint — metric-view emission", () => { const warned = warnSpy.mock.calls.flat().map(String).join("\n"); expect(warned).not.toContain("metric sync failed"); - // Degraded artifacts are suppressed, not written (to preserve committed types). - expect(fs.existsSync(metricFile)).toBe(false); + // Degraded artifacts are suppressed, preserving the surface-specific + // committed fallback byte-for-byte. + expect(fs.readFileSync(metricFile, "utf-8")).toBe(committed); } finally { warnSpy.mockRestore(); logSpy.mockRestore(); @@ -744,9 +775,10 @@ describe("generateFromEntryPoint — metric-view emission", () => { }); test("blocking + DELETED: environmental failure with committed types → no throw, warning emitted", async () => { - // DELETED is environmental. Since the query path writes analytics.d.ts - // (even with empty registry), committed types exist, so emit warning + return 0. + // DELETED is environmental. A committed metric-view fallback lets the + // generator emit a warning and return 0. writeMetricConfig(); + const committed = writeCommittedMetricTypes(); mocks.getWarehouseState.mockResolvedValue("DELETED"); const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); @@ -769,8 +801,8 @@ describe("generateFromEntryPoint — metric-view emission", () => { expect(mocks.waitUntilRunning).not.toHaveBeenCalled(); expect(mocks.executeStatement).not.toHaveBeenCalled(); - // Degraded metric artifacts are NOT written in blocking mode (committed types preserved). - expect(fs.existsSync(metricFile)).toBe(false); + // Degraded metric artifacts are NOT written in blocking mode. + expect(fs.readFileSync(metricFile, "utf-8")).toBe(committed); // The degraded outcome is NEVER cached (mirrors the query path): the key is // left uncached so a later pass re-probes, and no stale/sticky entry can be @@ -780,9 +812,10 @@ describe("generateFromEntryPoint — metric-view emission", () => { }); test("blocking + preflight wait rejects with a timeout: environmental failure with committed types → no throw, warning emitted", async () => { - // Timeout is environmental. Since the query path writes analytics.d.ts, - // committed types exist, so emit warning + return 0. + // Timeout is environmental. A committed metric-view fallback lets the + // generator emit a warning and return 0. writeMetricConfig(); + const committed = writeCommittedMetricTypes(); mocks.getWarehouseState.mockResolvedValue("STARTING"); mocks.waitUntilRunning.mockRejectedValue( new Error( @@ -815,8 +848,8 @@ describe("generateFromEntryPoint — metric-view emission", () => { expect.objectContaining({ maxMs: 300_000 }), ); expect(mocks.executeStatement).not.toHaveBeenCalled(); - // Degraded metric artifacts are NOT written in blocking mode (committed types preserved). - expect(fs.existsSync(metricFile)).toBe(false); + // Degraded metric artifacts are NOT written in blocking mode. + expect(fs.readFileSync(metricFile, "utf-8")).toBe(committed); // The degraded outcome is not cached — the key stays uncached for the next // pass to re-probe. @@ -828,9 +861,10 @@ describe("generateFromEntryPoint — metric-view emission", () => { // A non-RUNNING *resolve* (not a throw) for a startable state is soft: fall // through to DESCRIBE, which degrades on the still-cold warehouse. Only a // DELETED/DELETING resolve (or a thrown deterministic error) is fatal. - // Degraded artifacts are NOT written in blocking mode when there are no failures - // (to preserve committed good types). + // Degraded artifacts are NOT written in blocking mode when there are no + // failures (to preserve committed good types). writeMetricConfig(); + const committed = writeCommittedMetricTypes(); mocks.getWarehouseState.mockResolvedValue("STARTING"); mocks.waitUntilRunning.mockResolvedValue("STOPPED"); // The fall-through DESCRIBE hits a still-cold warehouse: non-terminal @@ -872,8 +906,8 @@ describe("generateFromEntryPoint — metric-view emission", () => { // The DESCRIBE batch still ran (fall-through), and its non-terminal answer // degraded the key. expect(mocks.executeStatement).toHaveBeenCalledTimes(1); - // Degraded artifacts are suppressed, not written (to preserve committed types). - expect(fs.existsSync(metricFile)).toBe(false); + // Degraded artifacts are suppressed, preserving committed types. + expect(fs.readFileSync(metricFile, "utf-8")).toBe(committed); // The degraded outcome is not cached; the key stays uncached and the next // describe-capable pass re-probes it (convergence via re-describe, not via a @@ -891,9 +925,10 @@ describe("generateFromEntryPoint — metric-view emission", () => { ])( "blocking + warehouse deleted mid-wait (probe read %s): environmental failure with committed types → no throw, warning emitted", async (probedState, startsWarehouse) => { - // DELETED mid-wait is environmental. Since the query path writes - // analytics.d.ts, committed types exist, so emit warning + return 0. + // DELETED mid-wait is environmental. A committed metric-view fallback + // lets the generator emit a warning and return 0. writeMetricConfig(); + const committed = writeCommittedMetricTypes(); mocks.getWarehouseState.mockResolvedValue(probedState); mocks.startWarehouse.mockResolvedValue(undefined); // The warehouse was deleted while the preflight waited: the wait @@ -915,8 +950,8 @@ describe("generateFromEntryPoint — metric-view emission", () => { // The DESCRIBE batch is skipped — nothing can answer it. expect(mocks.executeStatement).not.toHaveBeenCalled(); - // Degraded metric artifacts are NOT written in blocking mode (committed types preserved). - expect(fs.existsSync(metricFile)).toBe(false); + // Degraded metric artifacts are NOT written in blocking mode. + expect(fs.readFileSync(metricFile, "utf-8")).toBe(committed); // The degraded outcome is not cached — no sticky entry to serve later. const metrics = @@ -2143,7 +2178,7 @@ describe("generateFromEntryPoint — warning message with cause labels", () => { fs.rmSync(warningTestDir, { recursive: true, force: true }); fs.mkdirSync(queryFolder, { recursive: true }); fs.mkdirSync(metricViewsFolder, { recursive: true }); - // Pre-create committed types files so the gate triggers + // Pre-create the committed query types required by these query-only cases. fs.mkdirSync(path.dirname(outFile), { recursive: true }); fs.writeFileSync(outFile, "// committed types\n", "utf-8"); mocks.generateQueriesFromDescribe.mockResolvedValue({ @@ -2315,8 +2350,9 @@ describe("generateFromEntryPoint — warning message with cause labels", () => { } }); - test("partial presence: only analytics.d.ts exists (metric absent) + environmental → warning emitted (partial presence counts)", async () => { - // Keep analytics.d.ts but remove metric file + test("query degradation only: analytics.d.ts exists and metric types are absent → warning emitted", async () => { + // This surface has no metric-view configuration, so only query types are + // required as a committed fallback. expect(fs.existsSync(outFile)).toBe(true); fs.rmSync(metricFile, { force: true }); @@ -2339,7 +2375,7 @@ describe("generateFromEntryPoint — warning message with cause labels", () => { mode: "blocking", }); - // Warning emitted because at least one committed type exists (analytics.d.ts) + // Warning emitted because the affected query surface has its artifact. const warnCalls = warnSpy.mock.calls .flat() .map(String) diff --git a/packages/appkit/src/type-generator/tests/unreachable-warehouse-gate.test.ts b/packages/appkit/src/type-generator/tests/unreachable-warehouse-gate.test.ts index 0a4dde781..e96233b6b 100644 --- a/packages/appkit/src/type-generator/tests/unreachable-warehouse-gate.test.ts +++ b/packages/appkit/src/type-generator/tests/unreachable-warehouse-gate.test.ts @@ -57,6 +57,7 @@ const { generateFromEntryPoint, TypegenFatalError } = await import("../index"); const testDir = path.join(__dirname, "__output_unreachable_gate__"); const queryFolder = path.join(testDir, "queries"); const outFile = path.join(testDir, "generated", "analytics.d.ts"); +const metricFile = path.join(testDir, "generated", "metric-views.d.ts"); /** DNS-style transport failure: what a CI runner without warehouse egress sees. */ function unreachableError() { @@ -134,6 +135,26 @@ describe("--wait gate: environmental query failures (real query path)", () => { } }); + test("committed metric types do not satisfy a missing query fallback", async () => { + fs.mkdirSync(path.dirname(metricFile), { recursive: true }); + fs.writeFileSync(metricFile, "// committed metric types\n", "utf-8"); + + const error = await generateFromEntryPoint({ + outFile, + queryFolder, + warehouseId: "wh-unreachable", + mode: "blocking", + }).then( + () => undefined, + (reason: unknown) => reason, + ); + + expect(error).toBeInstanceOf(TypegenFatalError); + expect((error as Error).message).toContain("analytics.d.ts"); + expect(fs.existsSync(outFile)).toBe(false); + expect(fs.existsSync(metricFile)).toBe(true); + }); + test("non-terminal DESCRIBE + no committed types → crashes instead of silently exiting 0", async () => { mocks.getWarehouse.mockResolvedValue({ state: "RUNNING" }); mocks.executeStatement.mockResolvedValue({