Skip to content

Commit 9e69b40

Browse files
committed
fix(appkit): escape generated JSDoc, log Arrow decode failures, tidy mv doc comments
Review cleanups — two small safety fixes plus doc tidy, no behavior change otherwise: - escape "*/" in the generated @sqlType JSDoc sinks (render-types.ts and query-registry.ts) so a SQL type/comment containing "*/" can't close the comment early and corrupt the emitted .d.ts (S3). - warn instead of silently swallowing an ARROW_STREAM decode failure in normalizeResultRows, so a Reyden user whose decode failed gets a breadcrumb rather than mysteriously-empty types (S2). Doc tidy: MAX_METRIC_VIEWS rationale (N5), fix the self-referential "legacy {@link MV_CONFIG_FILE}" comment (N2), drop the non-resolving @link in sync.ts (N3), correct the stale "metric.json" header in the shared schema + regenerate the JSON Schema (N4), un-strand a doc comment inside resolveMetricConfig (N10), and replace the non-standard @note + redundant path literals (N11). Co-authored-by: Isaac Signed-off-by: Atila Fassina <atila@fassina.eu>
1 parent e016211 commit 9e69b40

7 files changed

Lines changed: 33 additions & 25 deletions

File tree

docs/static/schemas/metric-source.schema.json

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/appkit/src/type-generator/mv-registry/config.ts

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,8 @@ import type {
2121
const MV_CONFIG_FILE = "metric-views.json";
2222

2323
/**
24-
* {@link resolveMetricConfig} enforces these caps.
24+
* Safety cap on declared metric views — a typo / DoS guard, NOT a Unity Catalog
25+
* limit. Enforced by {@link resolveMetricConfig}.
2526
*/
2627
const MAX_METRIC_VIEWS = 200;
2728
/** Per-segment cap = UC's object-name length limit (255). */
@@ -33,10 +34,10 @@ const FQN_SEGMENT_NAMES = ["catalog", "schema", "metric_view"] as const;
3334
const FQN_SEGMENT_COUNT = FQN_SEGMENT_NAMES.length;
3435

3536
/**
36-
* Locale-independent comparator (UTF-16 code-unit order)
37-
* shared by BOTH artifact key orderings.
38-
*
39-
* @note important for caching correctness.
37+
* Locale-independent comparator (UTF-16 code-unit order) shared by BOTH artifact
38+
* key orderings. Plain `sort()` is locale-sensitive, so keys could order
39+
* differently across environments and invalidate the cache hash — this keeps the
40+
* ordering stable everywhere.
4041
*/
4142
export function compareKeys(a: string, b: string): number {
4243
return a < b ? -1 : a > b ? 1 : 0;
@@ -47,7 +48,7 @@ export function compareKeys(a: string, b: string): number {
4748
*
4849
* Returns `null` if the file does not exist (the metric-view path is
4950
* additive — apps without metric-views.json must not be penalized). There is
50-
* deliberately no fallback to the legacy {@link MV_CONFIG_FILE} filename.
51+
* deliberately no fallback to the legacy `metric.json` filename.
5152
*
5253
* Throws on JSON parse errors so misconfiguration surfaces loudly.
5354
*/
@@ -85,9 +86,9 @@ export async function readMetricConfig(
8586

8687
/**
8788
* Validate a key against the JSON Schema's metricKey pattern. Kept
88-
* lightweight — the shared Zod schema ({@link metricSourceSchema} in `packages/shared/src/schemas/metric-source.ts`)
89-
* is the canonical contract for IDE/CI; this regex is identical to its
90-
* {@link metricKeySchema} in `packages/shared/src/schemas/metric-source.ts`.
89+
* lightweight — the shared Zod schema ({@link metricSourceSchema}) is the
90+
* canonical contract for IDE/CI; this regex is identical to its
91+
* {@link metricKeySchema}.
9192
*/
9293
function isValidMetricKey(key: string): boolean {
9394
return /^[a-zA-Z_][a-zA-Z0-9_]*$/.test(key);
@@ -134,11 +135,9 @@ export function resolveMetricConfig(
134135
}
135136
}
136137

137-
/**
138-
* Default ONLY a genuinely-absent {@link MetricSourceConfig.metricViews}. `null` must fall through
139-
* to the type check below and throw — the canonical Zod schema ({@link metricSourceSchema}) rejects
140-
* `null`.
141-
*/
138+
// Default to {} only when metricViews is genuinely absent. A `null` must fall
139+
// through to the type check below and throw — the canonical Zod schema rejects
140+
// null.
142141
const metricViews =
143142
config.metricViews === undefined ? {} : config.metricViews;
144143
if (

packages/appkit/src/type-generator/mv-registry/render-types.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ function renderMetricEntry(schema: MetricSchema): string {
4444
const grainComment = col.timeGrains?.length
4545
? ` @timeGrain ${col.timeGrains.join("|")}`
4646
: "";
47-
return `${indent}/** @sqlType ${col.type}${grainComment} */
47+
return `${indent}/** @sqlType ${col.type.replace(/\*\//g, "* /")}${grainComment} */
4848
${indent}${JSON.stringify(col.name)}: ${tsTypeFor(col.type)}`;
4949
})
5050
.join(";\n");

packages/appkit/src/type-generator/mv-registry/sync.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ interface MetricDescribeOutcome {
4343
}
4444

4545
/**
46-
* Run schema synchronization for every entry in {@link import("./config").MV_CONFIG_FILE}.
46+
* Run schema synchronization for every entry in `metric-views.json`.
4747
*/
4848
export async function syncMetrics(
4949
resolution: MetricConfigResolution,

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,8 +172,8 @@ export function convertToQueryType(
172172

173173
// generate comment for column
174174
const comment = column.comment
175-
? `/** ${column.comment} */\n `
176-
: `/** @sqlType ${column.type_name} */\n `;
175+
? `/** ${column.comment.replace(/\*\//g, "* /")} */\n `
176+
: `/** @sqlType ${column.type_name.replace(/\*\//g, "* /")} */\n `;
177177

178178
return `${comment}${name}: ${mappedType}`;
179179
});

packages/appkit/src/type-generator/statement-result.ts

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
11
import type { WorkspaceClient } from "@databricks/sdk-experimental";
2+
import { createLogger } from "../logging/logger";
23
import { getErrorMessage } from "./errors";
34
import type { DatabricksStatementExecutionResponse } from "./types";
45

6+
const logger = createLogger("type-generator:statement-result");
7+
58
/**
69
* Normalize a Statement Execution response so downstream parsers can always
710
* read rows from `result.data_array`, regardless of the wire format the
@@ -78,9 +81,15 @@ export async function normalizeResultRows(
7881
data_array: dataArray,
7982
},
8083
};
81-
} catch {
82-
// Best-effort: a corrupt/partial Arrow payload must not crash the pass.
83-
// Returning it unchanged routes into the deterministic "no rows" degrade.
84+
} catch (err) {
85+
// Best-effort: a corrupt/partial Arrow payload — or a missing apache-arrow
86+
// module — must not crash the pass. Warn so a Reyden user whose decode failed
87+
// gets a breadcrumb instead of mysteriously-empty types, then return the
88+
// response unchanged: it routes into the deterministic "no rows" degrade.
89+
logger.warn(
90+
"failed to decode ARROW_STREAM DESCRIBE attachment (%s); emitting no rows — metric/query types may degrade",
91+
getErrorMessage(err),
92+
);
8493
return response;
8594
}
8695
}

packages/shared/src/schemas/metric-source.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
/**
22
* AppKit metric-source schema.
33
*
4-
* Single source of truth for `metric.json`
4+
* Single source of truth for `metric-views.json`
55
* the config that activates the Analytics' metric-view path.
66
*
7-
* `metric.json` declares UC Metric Views under a single `metricViews` map.
7+
* `metric-views.json` declares UC Metric Views under a single `metricViews` map.
88
* Each entry binds a metric key to a UC metric view FQN plus the executor
99
* the query runs as:
1010
* - `executor: "app_service_principal"` (default) — queried as the app service
@@ -93,7 +93,7 @@ export const metricSourceSchema = z
9393
})
9494
.strict()
9595
.describe(
96-
"Schema for AppKit metric.json — declares Unity Catalog Metric View sources for the analytics plugin's metric-view path. Each entry under 'metricViews' binds a metric key to a UC metric view FQN and an executor ('app_service_principal' shared cache, or 'user' per-user cache). Object form (rather than bare string) at v1 enables future per-entry option growth without breaking changes.",
96+
"Schema for AppKit metric-views.json — declares Unity Catalog Metric View sources for the analytics plugin's metric-view path. Each entry under 'metricViews' binds a metric key to a UC metric view FQN and an executor ('app_service_principal' shared cache, or 'user' per-user cache). Object form (rather than bare string) at v1 enables future per-entry option growth without breaking changes.",
9797
);
9898

9999
export type MetricKey = z.infer<typeof metricKeySchema>;

0 commit comments

Comments
 (0)