Skip to content

Commit b350a46

Browse files
atilafassinaatilafassina
authored
fix: always output valid types (#152)
* fix: always output valid types * Update packages/appkit/src/type-generator/query-registry.ts * chore: remove duplication by creating a `cacheFiledQuery` helper
1 parent c1a41dc commit b350a46

4 files changed

Lines changed: 213 additions & 77 deletions

File tree

‎packages/appkit/src/type-generator/cache.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
import crypto from "node:crypto";
22
import fs from "node:fs";
33
import path from "node:path";
4+
import { createLogger } from "../logging/logger";
5+
6+
const logger = createLogger("type-generator:cache");
47

58
/**
69
* Cache types
@@ -10,6 +13,7 @@ import path from "node:path";
1013
interface CacheEntry {
1114
hash: string;
1215
type: string;
16+
retry: boolean;
1317
}
1418

1519
/**
@@ -22,7 +26,7 @@ interface Cache {
2226
queries: Record<string, CacheEntry>;
2327
}
2428

25-
export const CACHE_VERSION = "1";
29+
export const CACHE_VERSION = "2";
2630
const CACHE_FILE = ".appkit-types-cache.json";
2731
const CACHE_DIR = path.join(
2832
process.cwd(),
@@ -60,7 +64,7 @@ export function loadCache(): Cache {
6064
}
6165
}
6266
} catch {
63-
// ignore cache errors
67+
logger.warn("Cache file is corrupted, flushing cache completely.");
6468
}
6569
return { version: CACHE_VERSION, queries: {} };
6670
}

‎packages/appkit/src/type-generator/query-registry.ts‎

Lines changed: 76 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -30,39 +30,43 @@ export function extractParameters(sql: string): string[] {
3030
// parameters that are injected by the server
3131
export const SERVER_INJECTED_PARAMS = ["workspaceId"];
3232

33+
/**
34+
* Generates the TypeScript type literal for query parameters from SQL.
35+
* Shared by both the success and failure paths.
36+
*/
37+
function formatParametersType(sql: string): string {
38+
const params = extractParameters(sql).filter(
39+
(p) => !SERVER_INJECTED_PARAMS.includes(p),
40+
);
41+
const paramTypes = extractParameterTypes(sql);
42+
43+
return params.length > 0
44+
? `{\n ${params
45+
.map((p) => {
46+
const sqlType = paramTypes[p];
47+
const markerType = sqlType
48+
? sqlTypeToMarker[sqlType]
49+
: "SQLTypeMarker";
50+
const helper = sqlType ? sqlTypeToHelper[sqlType] : "sql.*()";
51+
return `/** ${sqlType || "any"} - use ${helper} */\n ${p}: ${markerType}`;
52+
})
53+
.join(";\n ")};\n }`
54+
: "Record<string, never>";
55+
}
56+
3357
export function convertToQueryType(
3458
result: DatabricksStatementExecutionResponse,
3559
sql: string,
3660
queryName: string,
37-
): string {
61+
): { type: string; hasResults: boolean } {
3862
const dataRows = result.result?.data_array || [];
3963
const columns = dataRows.map((row) => ({
4064
name: row[0] || "",
4165
type_name: row[1]?.toUpperCase() || "STRING",
4266
comment: row[2] || undefined,
4367
}));
4468

45-
const params = extractParameters(sql).filter(
46-
(p) => !SERVER_INJECTED_PARAMS.includes(p),
47-
);
48-
49-
const paramTypes = extractParameterTypes(sql);
50-
51-
// generate parameters types with JSDoc hints
52-
const paramsType =
53-
params.length > 0
54-
? `{\n ${params
55-
.map((p) => {
56-
const sqlType = paramTypes[p];
57-
// if no type annotation, use SQLTypeMarker (union type)
58-
const markerType = sqlType
59-
? sqlTypeToMarker[sqlType]
60-
: "SQLTypeMarker";
61-
const helper = sqlType ? sqlTypeToHelper[sqlType] : "sql.*()";
62-
return `/** ${sqlType || "any"} - use ${helper} */\n ${p}: ${markerType}`;
63-
})
64-
.join(";\n ")};\n }`
65-
: "Record<string, never>";
69+
const paramsType = formatParametersType(sql);
6670

6771
// generate result fields with JSDoc
6872
const resultFields = columns.map((column) => {
@@ -81,15 +85,50 @@ export function convertToQueryType(
8185
return `${comment}${name}: ${mappedType}`;
8286
});
8387

84-
return `{
88+
const hasResults = resultFields.length > 0;
89+
90+
const type = `{
8591
name: "${queryName}";
8692
parameters: ${paramsType};
87-
result: Array<{
93+
result: ${
94+
hasResults
95+
? `Array<{
8896
${resultFields.join(";\n ")};
89-
}>;
97+
}>`
98+
: "unknown"
99+
};
100+
}`;
101+
102+
return { type, hasResults };
103+
}
104+
105+
/**
106+
* Used when DESCRIBE QUERY fails so the query still appears in QueryRegistry.
107+
* Generates a type with unknown result from SQL alone (no warehouse call).
108+
*/
109+
function generateUnknownResultQuery(sql: string, queryName: string): string {
110+
const paramsType = formatParametersType(sql);
111+
112+
return `{
113+
name: "${queryName}";
114+
parameters: ${paramsType};
115+
result: unknown;
90116
}`;
91117
}
92118

119+
function cacheFailedQuery(
120+
cache: ReturnType<typeof loadCache>,
121+
querySchemas: QuerySchema[],
122+
sql: string,
123+
queryName: string,
124+
sqlHash: string,
125+
): void {
126+
const type = generateUnknownResultQuery(sql, queryName);
127+
querySchemas.push({ name: queryName, type });
128+
cache.queries[queryName] = { hash: sqlHash, type, retry: true };
129+
saveCache(cache);
130+
}
131+
93132
export function extractParameterTypes(sql: string): Record<string, string> {
94133
const paramTypes: Record<string, string> = {};
95134
const regex =
@@ -131,7 +170,6 @@ export async function generateQueriesFromDescribe(
131170

132171
const client = new WorkspaceClient({});
133172
const querySchemas: QuerySchema[] = [];
134-
const failedQueries: { name: string; error: string }[] = [];
135173
const spinner = new Spinner();
136174

137175
// process each query file
@@ -144,9 +182,9 @@ export async function generateQueriesFromDescribe(
144182
const sql = fs.readFileSync(path.join(queryFolder, file), "utf8");
145183
const sqlHash = hashSQL(sql);
146184

147-
// check cache
185+
// check cache (skip if marked for retry after a failed DESCRIBE)
148186
const cached = cache.queries[queryName];
149-
if (cached && cached.hash === sqlHash) {
187+
if (cached && cached.hash === sqlHash && !cached.retry) {
150188
querySchemas.push({ name: queryName, type: cached.type });
151189
spinner.start(`Processing ${queryName} (${i + 1}/${queryFiles.length})`);
152190
spinner.stop(`✓ ${queryName} (cached)`);
@@ -170,40 +208,33 @@ export async function generateQueriesFromDescribe(
170208
if (result.status.state === "FAILED") {
171209
const sqlError =
172210
result.status.error?.message || "Query execution failed";
211+
cacheFailedQuery(cache, querySchemas, sql, queryName, sqlHash);
173212
spinner.stop(`✗ ${queryName} - failed`);
174213
spinner.printDetail(`SQL Error: ${sqlError}`);
175214
spinner.printDetail(`Query: ${cleanedSql.slice(0, 200)}`);
176-
failedQueries.push({
177-
name: queryName,
178-
error: sqlError,
179-
});
180215
continue;
181216
}
182217

183218
// convert result to query schema
184-
const type = convertToQueryType(result, sql, queryName);
219+
const { type, hasResults } = convertToQueryType(result, sql, queryName);
185220
querySchemas.push({ name: queryName, type });
186221

187-
// update cache
188-
cache.queries[queryName] = { hash: sqlHash, type };
222+
// update cache immediately so successful results survive partial failures
223+
// retry if DESCRIBE returned no columns (result: unknown)
224+
const retry = !hasResults;
225+
cache.queries[queryName] = { hash: sqlHash, type, retry };
226+
saveCache(cache);
189227

190228
spinner.stop(`✓ ${queryName}`);
191229
} catch (error) {
192230
const errorMessage =
193231
error instanceof Error ? error.message : "Unknown error";
194-
spinner.stop(`✗ ${queryName} - ${errorMessage}`);
195-
failedQueries.push({ name: queryName, error: errorMessage });
232+
spinner.stop(`✗ ${queryName}`);
233+
spinner.printDetail(errorMessage);
234+
cacheFailedQuery(cache, querySchemas, sql, queryName, sqlHash);
196235
}
197236
}
198237

199-
// save cache
200-
saveCache(cache);
201-
202-
// log warning if there are failed queries
203-
if (failedQueries.length > 0) {
204-
logger.debug("Warning: %d queries failed", failedQueries.length);
205-
}
206-
207238
return querySchemas;
208239
}
209240

‎packages/appkit/src/type-generator/tests/generate-queries.test.ts‎

Lines changed: 83 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ const mocks = vi.hoisted(() => ({
66
executeStatement: vi.fn(),
77
spinnerStop: vi.fn(),
88
spinnerPrintDetail: vi.fn(),
9-
loadCache: vi.fn(() => ({ version: "1", queries: {} })),
9+
loadCache: vi.fn(() => ({ version: "2", queries: {} })),
1010
saveCache: vi.fn(),
1111
}));
1212

@@ -70,9 +70,10 @@ describe("generateQueriesFromDescribe", () => {
7070
expect(schemas[0].type).toContain("id: number");
7171
expect(schemas[0].type).toContain("name: string");
7272
expect(mocks.spinnerStop).toHaveBeenCalledWith("✓ users");
73+
expect(mocks.saveCache).toHaveBeenCalledTimes(1);
7374
});
7475

75-
test("FAILED status with error message — reports SQL error via spinner", async () => {
76+
test("FAILED status with error message — reports SQL error and produces unknown result type", async () => {
7677
mocks.readdirSync.mockReturnValue(["bad_table.sql"]);
7778
mocks.readFileSync.mockReturnValue("SELECT * FROM bad_table");
7879
mocks.executeStatement.mockResolvedValue({
@@ -85,17 +86,20 @@ describe("generateQueriesFromDescribe", () => {
8586

8687
const schemas = await generateQueriesFromDescribe("/queries", "wh-123");
8788

88-
expect(schemas).toHaveLength(0);
89+
expect(schemas).toHaveLength(1);
90+
expect(schemas[0].name).toBe("bad_table");
91+
expect(schemas[0].type).toContain("result: unknown");
8992
expect(mocks.spinnerStop).toHaveBeenCalledWith("✗ bad_table - failed");
9093
expect(mocks.spinnerPrintDetail).toHaveBeenCalledWith(
9194
"SQL Error: Table or view not found: bad_table",
9295
);
9396
expect(mocks.spinnerPrintDetail).toHaveBeenCalledWith(
9497
expect.stringContaining("Query:"),
9598
);
99+
expect(mocks.saveCache).toHaveBeenCalledTimes(1);
96100
});
97101

98-
test("FAILED status without error message — uses fallback message", async () => {
102+
test("FAILED status without error message — uses fallback message and produces unknown result type", async () => {
99103
mocks.readdirSync.mockReturnValue(["query.sql"]);
100104
mocks.readFileSync.mockReturnValue("SELECT 1");
101105
mocks.executeStatement.mockResolvedValue({
@@ -105,10 +109,84 @@ describe("generateQueriesFromDescribe", () => {
105109

106110
const schemas = await generateQueriesFromDescribe("/queries", "wh-123");
107111

108-
expect(schemas).toHaveLength(0);
112+
expect(schemas).toHaveLength(1);
113+
expect(schemas[0].name).toBe("query");
114+
expect(schemas[0].type).toContain("result: unknown");
109115
expect(mocks.spinnerStop).toHaveBeenCalledWith("✗ query - failed");
110116
expect(mocks.spinnerPrintDetail).toHaveBeenCalledWith(
111117
"SQL Error: Query execution failed",
112118
);
119+
expect(mocks.saveCache).toHaveBeenCalledTimes(1);
120+
});
121+
122+
test("partial failure — caches success, unknown result for failure, output includes both", async () => {
123+
mocks.readdirSync.mockReturnValue(["good.sql", "bad.sql"]);
124+
mocks.readFileSync
125+
.mockReturnValueOnce("SELECT id FROM good_table WHERE status = :status")
126+
.mockReturnValueOnce("SELECT * FROM missing_table");
127+
128+
mocks.executeStatement
129+
.mockResolvedValueOnce(succeededResult([["id", "INT", null]]))
130+
.mockResolvedValueOnce({
131+
statement_id: "stmt-fail",
132+
status: {
133+
state: "FAILED",
134+
error: { message: "Table not found" },
135+
},
136+
});
137+
138+
const schemas = await generateQueriesFromDescribe("/queries", "wh-123");
139+
140+
expect(schemas).toHaveLength(2);
141+
142+
// success entry is fully typed
143+
expect(schemas[0].name).toBe("good");
144+
expect(schemas[0].type).toContain("id: number");
145+
146+
// failure entry is unknown result with unknown result
147+
expect(schemas[1].name).toBe("bad");
148+
expect(schemas[1].type).toContain("result: unknown");
149+
150+
// saveCache called for both queries (success + failure with retry: true)
151+
expect(mocks.saveCache).toHaveBeenCalledTimes(2);
152+
});
153+
154+
test("all queries fail — caches with retry flag, all unknown result types", async () => {
155+
mocks.readdirSync.mockReturnValue(["a.sql", "b.sql"]);
156+
mocks.readFileSync
157+
.mockReturnValueOnce("SELECT * FROM table_a")
158+
.mockReturnValueOnce("SELECT * FROM table_b");
159+
160+
mocks.executeStatement
161+
.mockRejectedValueOnce(new Error("Connection refused"))
162+
.mockResolvedValueOnce({
163+
statement_id: "stmt-fail-2",
164+
status: { state: "FAILED", error: { message: "Table not found" } },
165+
});
166+
167+
const schemas = await generateQueriesFromDescribe("/queries", "wh-123");
168+
169+
expect(schemas).toHaveLength(2);
170+
expect(schemas[0].name).toBe("a");
171+
expect(schemas[0].type).toContain("result: unknown");
172+
expect(schemas[1].name).toBe("b");
173+
expect(schemas[1].type).toContain("result: unknown");
174+
175+
expect(mocks.saveCache).toHaveBeenCalledTimes(2);
176+
});
177+
178+
test("unknown result type includes parameters from SQL", async () => {
179+
mocks.readdirSync.mockReturnValue(["parameterized.sql"]);
180+
mocks.readFileSync.mockReturnValue(
181+
"-- @param status STRING\nSELECT * FROM t WHERE status = :status AND org = :org",
182+
);
183+
mocks.executeStatement.mockRejectedValueOnce(new Error("timeout"));
184+
185+
const schemas = await generateQueriesFromDescribe("/queries", "wh-123");
186+
187+
expect(schemas).toHaveLength(1);
188+
expect(schemas[0].type).toContain("status: SQLStringMarker");
189+
expect(schemas[0].type).toContain("org: SQLTypeMarker");
190+
expect(schemas[0].type).toContain("result: unknown");
113191
});
114192
});

0 commit comments

Comments
 (0)