From 74f0ff50d19eea0f50f84990a66a19fab496b067 Mon Sep 17 00:00:00 2001 From: Josh Sacks Date: Fri, 10 Apr 2026 11:35:45 -0600 Subject: [PATCH 01/11] feat: add source parameter support to query execution endpoints Allow callers to pass source parameters (key-value string maps) to query execution and notebook cell execution endpoints. Parameters are validated against declared Malloy source parameter types, converted to the correct Malloy literal syntax, and injected into query strings. - New `source_params.ts` utility with type-aware parsing, validation, and Malloy clause building - REST endpoints accept `sourceParameters` in request body/query params - MCP `malloy_executeQuery` tool accepts `sourceParameters` field - Notebook cells with compilation errors (e.g. missing required params) are deferred to execution time and recompiled with injected params - OpenAPI spec and agent docs updated - Comprehensive unit tests (59 new tests across 2 files) Made-with: Cursor --- api-doc.yaml | 20 + docs/ai-agents.md | 37 ++ .../server/src/controller/model.controller.ts | 3 +- .../server/src/controller/query.controller.ts | 2 + .../src/mcp/tools/execute_query_tool.ts | 12 + packages/server/src/server.ts | 13 + packages/server/src/service/model.spec.ts | 627 +++++++++++++++++- packages/server/src/service/model.ts | 337 +++++++--- .../server/src/utils/source_params.spec.ts | 332 ++++++++++ packages/server/src/utils/source_params.ts | 115 ++++ 10 files changed, 1413 insertions(+), 85 deletions(-) create mode 100644 packages/server/src/utils/source_params.spec.ts create mode 100644 packages/server/src/utils/source_params.ts diff --git a/api-doc.yaml b/api-doc.yaml index 55782be9..e70fda71 100644 --- a/api-doc.yaml +++ b/api-doc.yaml @@ -1471,6 +1471,15 @@ paths: required: false schema: $ref: "#/components/schemas/VersionIdPattern" + - name: sourceParameters + in: query + description: >- + JSON-encoded object of source parameter key-value string pairs. + Keys must match parameter names declared on sources in the notebook's + Malloy model. Values are parsed according to declared parameter types. + required: false + schema: + type: string responses: "200": description: Cell execution result @@ -1916,6 +1925,17 @@ components: type: boolean default: false description: 'If true, returns a simple JSON array of row objects in the form {"columnName": value}. If false (default), returns the full Malloy result with type metadata for rendering.' + sourceParameters: + type: object + additionalProperties: + type: string + description: | + Source parameter values as key-value string pairs. Keys must match parameter + names declared on the source in the Malloy model. Values are strings that are + parsed according to the parameter's declared type (string, number, boolean, + date, timestamp, or filter expression). Required parameters (those without a + default value in the model) must be provided or the request will fail with a + 400 error. versionId: type: string description: Version ID diff --git a/docs/ai-agents.md b/docs/ai-agents.md index 619d31ae..b1e7e582 100644 --- a/docs/ai-agents.md +++ b/docs/ai-agents.md @@ -28,6 +28,43 @@ The primary way clients interact with the server is through tool calls. These ar * **Query Execution Tool**: Used by the AI to get data. * `malloy_executeQuery`: Executes a Malloy query and returns the results in JSON format. +#### Source Parameters + +Malloy sources can declare **parameters** that must (or may) be provided at query time. The `malloy_executeQuery` tool accepts an optional `sourceParameters` field — a JSON object of key-value string pairs — that supplies values for these parameters. + +```json +{ + "projectName": "my_project", + "packageName": "my_package", + "modelPath": "model.malloy", + "sourceName": "flights", + "queryName": "by_carrier", + "sourceParameters": { + "min_distance": "2000", + "carrier_filter": "UA" + } +} +``` + +**Type parsing:** Values are always passed as strings and the publisher parses them according to the parameter's declared type in the Malloy model: + +| Malloy Type | Example Value | Notes | +|-------------|---------------|-------| +| `string` | `"UA"` | | +| `number` | `"2000"` | Integer or decimal | +| `boolean` | `"true"` | Accepts `true`/`false` or `1`/`0` | +| `date` | `"2024-01-15"` | ISO date format | +| `timestamp` | `"2024-01-15 10:30:00"` | ISO datetime format | +| `filter` | `"last week for two days"` | Malloy filter expression | + +**Required parameters:** If a source declares a parameter without a default value, it must be provided in `sourceParameters` or the request will fail with a `400` error listing the missing parameter(s). + +The same `sourceParameters` mechanism is available on the REST API's query execution endpoint (`POST .../models/{path}/query`) as a field in the JSON request body. + +##### Implicit Parameters + +Some models use parameters that are set by the system rather than by users or agents. By convention, these parameters are named with an `implicit_` prefix (e.g., `implicit_dataset_id`, `implicit_acl_token`). Agents should **not** set parameters whose names start with `implicit_` — they are managed by the deployment environment and injected by infrastructure (e.g., derived from authentication tokens or deployment configuration). + #### Prompts & Resources The MCP server can also provide clients with **prompts** (e.g., suggested questions to start a conversation) and **resources** (e.g., links to documentation or data dictionaries). However, these are nascent capabilities of the MCP standard, and many current MCP clients do not yet utilize them. diff --git a/packages/server/src/controller/model.controller.ts b/packages/server/src/controller/model.controller.ts index 228e3dba..d83ffc55 100644 --- a/packages/server/src/controller/model.controller.ts +++ b/packages/server/src/controller/model.controller.ts @@ -84,6 +84,7 @@ export class ModelController { packageName: string, notebookPath: string, cellIndex: number, + sourceParameters?: Record, ): Promise<{ type: "code" | "markdown"; text: string; @@ -101,6 +102,6 @@ export class ModelController { throw new ModelNotFoundError(`${notebookPath} is a model`); } - return model.executeNotebookCell(cellIndex); + return model.executeNotebookCell(cellIndex, sourceParameters); } } diff --git a/packages/server/src/controller/query.controller.ts b/packages/server/src/controller/query.controller.ts index 08c0ee13..bb3011bb 100644 --- a/packages/server/src/controller/query.controller.ts +++ b/packages/server/src/controller/query.controller.ts @@ -29,6 +29,7 @@ export class QueryController { queryName: string, query: string, compactJson: boolean = false, + sourceParameters?: Record, ): Promise { const project = await this.projectStore.getProject(projectName, false); const p = await project.getPackage(packageName, false); @@ -41,6 +42,7 @@ export class QueryController { sourceName, queryName, query, + sourceParameters, ); const renderLogs = validateRenderTags(result); return { diff --git a/packages/server/src/mcp/tools/execute_query_tool.ts b/packages/server/src/mcp/tools/execute_query_tool.ts index 905e4288..c19ea7b4 100644 --- a/packages/server/src/mcp/tools/execute_query_tool.ts +++ b/packages/server/src/mcp/tools/execute_query_tool.ts @@ -24,6 +24,15 @@ const executeQueryShape = { query: z.string().optional().describe("Ad-hoc Malloy query code"), sourceName: z.string().optional().describe("Source name for a view"), queryName: z.string().optional().describe("Named query or view"), + sourceParameters: z + .record(z.string()) + .optional() + .describe( + "Source parameter values as key-value string pairs. " + + "Values are parsed according to the parameter's declared type in the model " + + "(e.g. numbers, booleans, dates). " + + "Required parameters that lack a default value must be provided.", + ), }; // Type inference is handled automatically by the MCP server based on the executeQueryShape @@ -49,6 +58,7 @@ export function registerExecuteQueryTool( query, sourceName, queryName, + sourceParameters, } = params; logger.info("[MCP Tool executeQuery] Received params:", { params }); @@ -120,6 +130,7 @@ export function registerExecuteQueryTool( undefined, undefined, query, + sourceParameters, ); const { validateRenderTags } = await import( "@malloydata/render-validator" @@ -165,6 +176,7 @@ export function registerExecuteQueryTool( sourceName, queryName, undefined, + sourceParameters, ); const { validateRenderTags } = await import( "@malloydata/render-validator" diff --git a/packages/server/src/server.ts b/packages/server/src/server.ts index feacaefb..ad78e907 100644 --- a/packages/server/src/server.ts +++ b/packages/server/src/server.ts @@ -817,12 +817,21 @@ app.get( // Express stores wildcard matches in params['0'] const notebookPath = (req.params as Record)["0"]; + const sourceParameters = + typeof req.query.sourceParameters === "string" + ? (JSON.parse(req.query.sourceParameters) as Record< + string, + string + >) + : undefined; + res.status(200).json( await modelController.executeNotebookCell( req.params.projectName, req.params.packageName, notebookPath, cellIndex, + sourceParameters, ), ); } catch (error) { @@ -870,6 +879,9 @@ app.post( try { // Express stores wildcard matches in params['0'] const modelPath = (req.params as Record)["0"]; + const sourceParameters = req.body.sourceParameters as + | Record + | undefined; res.status(200).json( await queryController.getQuery( req.params.projectName, @@ -879,6 +891,7 @@ app.post( req.body.queryName as string, req.body.query as string, req.body.compactJson === true, + sourceParameters, ), ); } catch (error) { diff --git a/packages/server/src/service/model.spec.ts b/packages/server/src/service/model.spec.ts index 6b17eb04..81d4cf7b 100644 --- a/packages/server/src/service/model.spec.ts +++ b/packages/server/src/service/model.spec.ts @@ -1,4 +1,10 @@ -import { MalloyError, Runtime } from "@malloydata/malloy"; +import { + MalloyError, + ModelDef, + ModelMaterializer, + Runtime, +} from "@malloydata/malloy"; +import type * as Malloy from "@malloydata/malloy-interfaces"; import { describe, expect, it } from "bun:test"; import fs from "fs/promises"; import sinon from "sinon"; @@ -254,4 +260,623 @@ describe("service/model", () => { }); }); }); + + // ------------------------------------------------------------------ + // Source Parameters integration with getQueryResults + // ------------------------------------------------------------------ + describe("getQueryResults with sourceParameters", () => { + const minimalModelDef = { + type: "model", + name: "test", + exports: [], + contents: {}, + queryList: [], + } as unknown as ModelDef; + + function makeSourceInfos( + params: Malloy.ParameterInfo[], + sourceName = "flights", + ): Malloy.SourceInfo[] { + return [ + { + kind: "source", + name: sourceName, + schema: { fields: [] }, + parameters: params, + } as unknown as Malloy.SourceInfo, + ]; + } + + function param( + name: string, + kind: string, + hasDefault = false, + ): Malloy.ParameterInfo { + return { + name, + type: { kind } as Malloy.ParameterType, + default_value: hasDefault + ? ({ + kind: "string_literal", + string_value: "x", + } as Malloy.LiteralValue) + : undefined, + }; + } + + it("should throw BadRequestError when a required source parameter is missing", async () => { + const loadQueryStub = sinon.stub(); + const model = new Model( + packageName, + mockModelPath, + {}, + "model", + { loadQuery: loadQueryStub } as unknown as ModelMaterializer, + minimalModelDef, + undefined, + undefined, + makeSourceInfos([param("carrier", "string_type")]), + undefined, + undefined, + ); + + await expect( + model.getQueryResults("flights", "by_carrier", undefined, {}), + ).rejects.toThrow(/Parameter "carrier" is required/); + + expect(loadQueryStub.called).toBe(false); + sinon.restore(); + }); + + it("should throw BadRequestError listing all missing required params", async () => { + const loadQueryStub = sinon.stub(); + const model = new Model( + packageName, + mockModelPath, + {}, + "model", + { loadQuery: loadQueryStub } as unknown as ModelMaterializer, + minimalModelDef, + undefined, + undefined, + makeSourceInfos([ + param("carrier", "string_type"), + param("year", "number_type"), + ]), + undefined, + undefined, + ); + + await expect( + model.getQueryResults("flights", "by_carrier", undefined, {}), + ).rejects.toThrow(/Parameters "carrier", "year" are required/); + + sinon.restore(); + }); + + it("should throw BadRequestError for unknown source parameter", async () => { + const loadQueryStub = sinon.stub(); + const model = new Model( + packageName, + mockModelPath, + {}, + "model", + { loadQuery: loadQueryStub } as unknown as ModelMaterializer, + minimalModelDef, + undefined, + undefined, + makeSourceInfos([param("carrier", "string_type", true)]), + undefined, + undefined, + ); + + await expect( + model.getQueryResults("flights", "by_carrier", undefined, { + nonexistent: "val", + }), + ).rejects.toThrow(/Unknown source parameter "nonexistent"/); + + sinon.restore(); + }); + + it("should inject param clause into loadQuery for named query", async () => { + const loadQueryStub = sinon.stub().returns({ + getPreparedResult: sinon.stub().rejects(new Error("stop")), + }); + const model = new Model( + packageName, + mockModelPath, + {}, + "model", + { loadQuery: loadQueryStub } as unknown as ModelMaterializer, + minimalModelDef, + undefined, + undefined, + makeSourceInfos([param("carrier", "string_type")]), + undefined, + undefined, + ); + + try { + await model.getQueryResults("flights", "by_carrier", undefined, { + carrier: "AA", + }); + } catch { + // expected — getPreparedResult throws + } + + expect(loadQueryStub.calledOnce).toBe(true); + expect(loadQueryStub.firstCall.args[0]).toBe( + '\nrun: flights(carrier is "AA")->by_carrier', + ); + sinon.restore(); + }); + + it("should not inject params when sourceParameters is empty", async () => { + const loadQueryStub = sinon.stub().returns({ + getPreparedResult: sinon.stub().rejects(new Error("stop")), + }); + const model = new Model( + packageName, + mockModelPath, + {}, + "model", + { loadQuery: loadQueryStub } as unknown as ModelMaterializer, + minimalModelDef, + undefined, + undefined, + makeSourceInfos([param("carrier", "string_type", true)]), + undefined, + undefined, + ); + + try { + await model.getQueryResults("flights", "by_carrier", undefined, {}); + } catch { + // expected + } + + expect(loadQueryStub.calledOnce).toBe(true); + expect(loadQueryStub.firstCall.args[0]).toBe( + "\nrun: flights->by_carrier", + ); + sinon.restore(); + }); + + it("should skip param validation when source has no declared params", async () => { + const loadQueryStub = sinon.stub().returns({ + getPreparedResult: sinon.stub().rejects(new Error("stop")), + }); + const model = new Model( + packageName, + mockModelPath, + {}, + "model", + { loadQuery: loadQueryStub } as unknown as ModelMaterializer, + minimalModelDef, + undefined, + undefined, + makeSourceInfos([]), + undefined, + undefined, + ); + + try { + await model.getQueryResults( + "flights", + "by_carrier", + undefined, + undefined, + ); + } catch { + // expected + } + + expect(loadQueryStub.calledOnce).toBe(true); + expect(loadQueryStub.firstCall.args[0]).toBe( + "\nrun: flights->by_carrier", + ); + sinon.restore(); + }); + + it("should convert number params to unquoted literals", async () => { + const loadQueryStub = sinon.stub().returns({ + getPreparedResult: sinon.stub().rejects(new Error("stop")), + }); + const model = new Model( + packageName, + mockModelPath, + {}, + "model", + { loadQuery: loadQueryStub } as unknown as ModelMaterializer, + minimalModelDef, + undefined, + undefined, + makeSourceInfos([param("min_distance", "number_type")]), + undefined, + undefined, + ); + + try { + await model.getQueryResults("flights", "query1", undefined, { + min_distance: "500", + }); + } catch { + // expected + } + + expect(loadQueryStub.firstCall.args[0]).toBe( + "\nrun: flights(min_distance is 500)->query1", + ); + sinon.restore(); + }); + + it("should convert boolean params correctly", async () => { + const loadQueryStub = sinon.stub().returns({ + getPreparedResult: sinon.stub().rejects(new Error("stop")), + }); + const model = new Model( + packageName, + mockModelPath, + {}, + "model", + { loadQuery: loadQueryStub } as unknown as ModelMaterializer, + minimalModelDef, + undefined, + undefined, + makeSourceInfos([param("active", "boolean_type")]), + undefined, + undefined, + ); + + try { + await model.getQueryResults("flights", "query1", undefined, { + active: "true", + }); + } catch { + // expected + } + + expect(loadQueryStub.firstCall.args[0]).toBe( + "\nrun: flights(active is true)->query1", + ); + sinon.restore(); + }); + }); + + // ------------------------------------------------------------------ + // executeNotebookCell with sourceParameters + // ------------------------------------------------------------------ + describe("executeNotebookCell with sourceParameters", () => { + it("should return markdown cell without execution", async () => { + const model = new Model( + packageName, + "test.malloynb", + {}, + "notebook", + undefined, + undefined, + undefined, + undefined, + undefined, + [{ type: "markdown", text: "# Hello" }], + undefined, + ); + + const result = await model.executeNotebookCell(0); + expect(result.type).toBe("markdown"); + expect(result.text).toBe("# Hello"); + expect(result.result).toBeUndefined(); + + sinon.restore(); + }); + + it("should throw BadRequestError for out-of-range cell index", async () => { + const model = new Model( + packageName, + "test.malloynb", + {}, + "notebook", + undefined, + undefined, + undefined, + undefined, + undefined, + [{ type: "code", text: "run: x -> { }" }], + undefined, + ); + + await expect(model.executeNotebookCell(5)).rejects.toThrow( + /out of range/, + ); + await expect(model.executeNotebookCell(-1)).rejects.toThrow( + /out of range/, + ); + + sinon.restore(); + }); + + it("should throw compilation error message when no sourceParameters provided", async () => { + const model = new Model( + packageName, + "test.malloynb", + {}, + "notebook", + undefined, + undefined, + undefined, + undefined, + undefined, + [ + { + type: "code", + text: "run: flights -> { aggregate: c is count() }", + cellCompilationError: new Error( + 'Parameter "carrier" is required', + ), + }, + ], + undefined, + ); + + await expect(model.executeNotebookCell(0)).rejects.toThrow( + /Cell 0 failed to compile.*carrier.*is required/, + ); + + sinon.restore(); + }); + + it("should suggest providing sourceParameters when cell has compilation error", async () => { + const model = new Model( + packageName, + "test.malloynb", + {}, + "notebook", + undefined, + undefined, + undefined, + undefined, + undefined, + [ + { + type: "code", + text: "run: flights -> { }", + cellCompilationError: new Error("some error"), + }, + ], + undefined, + ); + + await expect(model.executeNotebookCell(0)).rejects.toThrow( + /provide sourceParameters/, + ); + + sinon.restore(); + }); + + it("should throw when runtime context is missing for recompilation", async () => { + const model = new Model( + packageName, + "test.malloynb", + {}, + "notebook", + undefined, + undefined, + undefined, + undefined, + undefined, + [ + { + type: "code", + text: "run: flights -> { }", + cellCompilationError: new Error("missing param"), + }, + ], + undefined, + // no runtime, no importBaseURL + ); + + await expect( + model.executeNotebookCell(0, { carrier: "AA" }), + ).rejects.toThrow(/missing runtime context/); + + sinon.restore(); + }); + + it("should inject param clause into cell text during recompilation", async () => { + const extendModelStub = sinon.stub(); + const loadFinalQueryStub = sinon.stub(); + const getPreparedResultStub = sinon + .stub() + .resolves({ resultExplore: {} }); + const runStub = sinon.stub().resolves({}); + const getPreparedQueryStub = sinon + .stub() + .resolves({ _query: { name: "test_q" } }); + + extendModelStub.returns({ loadFinalQuery: loadFinalQueryStub }); + loadFinalQueryStub.returns({ + getPreparedResult: getPreparedResultStub, + run: runStub, + getPreparedQuery: getPreparedQueryStub, + }); + + const sourceInfos: Malloy.SourceInfo[] = [ + { + kind: "source", + name: "flights", + schema: { fields: [] }, + parameters: [ + { + name: "carrier", + type: { kind: "string_type" } as Malloy.ParameterType, + }, + ], + } as unknown as Malloy.SourceInfo, + ]; + + const model = new Model( + packageName, + "test.malloynb", + {}, + "notebook", + undefined, + undefined, + undefined, + undefined, + sourceInfos, + [ + { + type: "code", + text: "run: flights -> { aggregate: c is count() }", + cellCompilationError: new Error("missing param"), + priorModelMaterializer: { + extendModel: extendModelStub, + } as unknown as ModelMaterializer, + }, + ], + undefined, + sinon.createStubInstance(Runtime), + new URL("file:///test/"), + ); + + const result = await model.executeNotebookCell(0, { carrier: "AA" }); + + expect(extendModelStub.calledOnce).toBe(true); + const modifiedText = extendModelStub.firstCall.args[0] as string; + expect(modifiedText).toContain('flights(carrier is "AA")'); + expect(modifiedText).toContain("->"); + + expect(result.type).toBe("code"); + expect(result.text).toBe( + "run: flights -> { aggregate: c is count() }", + ); + expect(result.queryName).toBe("test_q"); + + sinon.restore(); + }); + + it("should use runtime.loadModel when priorModelMaterializer is undefined (first cell)", async () => { + const loadModelStub = sinon.stub(); + const loadFinalQueryStub = sinon.stub(); + const getPreparedResultStub = sinon + .stub() + .resolves({ resultExplore: {} }); + const runStub = sinon.stub().resolves({}); + const getPreparedQueryStub = sinon + .stub() + .resolves({ _query: { name: "q1" } }); + + loadModelStub.returns({ loadFinalQuery: loadFinalQueryStub }); + loadFinalQueryStub.returns({ + getPreparedResult: getPreparedResultStub, + run: runStub, + getPreparedQuery: getPreparedQueryStub, + }); + + const mockRuntime = sinon.createStubInstance(Runtime); + mockRuntime.loadModel.callsFake(loadModelStub); + + const sourceInfos: Malloy.SourceInfo[] = [ + { + kind: "source", + name: "flights", + schema: { fields: [] }, + parameters: [ + { + name: "carrier", + type: { kind: "string_type" } as Malloy.ParameterType, + }, + ], + } as unknown as Malloy.SourceInfo, + ]; + + const model = new Model( + packageName, + "test.malloynb", + {}, + "notebook", + undefined, + undefined, + undefined, + undefined, + sourceInfos, + [ + { + type: "code", + text: "run: flights -> { aggregate: c is count() }", + cellCompilationError: new Error("missing param"), + // no priorModelMaterializer — first cell + }, + ], + undefined, + mockRuntime, + new URL("file:///test/"), + ); + + const result = await model.executeNotebookCell(0, { carrier: "AA" }); + + expect(loadModelStub.calledOnce).toBe(true); + const modifiedText = loadModelStub.firstCall.args[0] as string; + expect(modifiedText).toContain('flights(carrier is "AA")'); + expect(result.type).toBe("code"); + + sinon.restore(); + }); + + it("should not modify cell text for sources without parameters", async () => { + const extendModelStub = sinon.stub(); + const loadFinalQueryStub = sinon.stub(); + extendModelStub.returns({ loadFinalQuery: loadFinalQueryStub }); + loadFinalQueryStub.returns({ + getPreparedResult: sinon.stub().resolves({ resultExplore: {} }), + run: sinon.stub().resolves({}), + getPreparedQuery: sinon.stub().resolves({ _query: { name: "q" } }), + }); + + const sourceInfos: Malloy.SourceInfo[] = [ + { + kind: "source", + name: "airports", + schema: { fields: [] }, + // no parameters + } as unknown as Malloy.SourceInfo, + ]; + + const cellText = "run: airports -> { aggregate: c is count() }"; + + const model = new Model( + packageName, + "test.malloynb", + {}, + "notebook", + undefined, + undefined, + undefined, + undefined, + sourceInfos, + [ + { + type: "code", + text: cellText, + cellCompilationError: new Error("some error"), + priorModelMaterializer: { + extendModel: extendModelStub, + } as unknown as ModelMaterializer, + }, + ], + undefined, + sinon.createStubInstance(Runtime), + new URL("file:///test/"), + ); + + await model.executeNotebookCell(0, { carrier: "AA" }); + + const modifiedText = extendModelStub.firstCall.args[0] as string; + // Text should be unchanged since airports has no parameters + expect(modifiedText).toBe(cellText); + + sinon.restore(); + }); + }); }); diff --git a/packages/server/src/service/model.ts b/packages/server/src/service/model.ts index 992265a1..6c5e6c15 100644 --- a/packages/server/src/service/model.ts +++ b/packages/server/src/service/model.ts @@ -42,6 +42,11 @@ import { } from "../errors"; import { logger } from "../logger"; import { URL_READER } from "../utils"; +import { + buildMalloyParamClause, + getSourceParams, + validateRequiredParams, +} from "../utils/source_params"; type ApiCompiledModel = components["schemas"]["CompiledModel"]; type ApiNotebookCell = components["schemas"]["NotebookCell"]; @@ -66,6 +71,9 @@ interface RunnableNotebookCell { runnable?: QueryMaterializer; newSources?: Malloy.SourceInfo[]; queryInfo?: Malloy.QueryInfo; + /** MM from *before* this cell — used to recompile with params */ + priorModelMaterializer?: ModelMaterializer; + cellCompilationError?: Error; } export class Model { @@ -81,6 +89,8 @@ export class Model { private sourceInfos: Malloy.SourceInfo[] | undefined; private runnableNotebookCells: RunnableNotebookCell[] | undefined; private compilationError: MalloyError | Error | undefined; + private runtime: Runtime | undefined; + private importBaseURL: URL | undefined; private meter = metrics.getMeter("publisher"); private queryExecutionHistogram = this.meter.createHistogram( "malloy_model_query_duration", @@ -103,6 +113,8 @@ export class Model { sourceInfos: Malloy.SourceInfo[] | undefined, runnableNotebookCells: RunnableNotebookCell[] | undefined, compilationError: MalloyError | Error | undefined, + runtime?: Runtime, + importBaseURL?: URL, ) { this.packageName = packageName; this.modelPath = modelPath; @@ -115,6 +127,8 @@ export class Model { this.sourceInfos = sourceInfos; this.runnableNotebookCells = runnableNotebookCells; this.compilationError = compilationError; + this.runtime = runtime; + this.importBaseURL = importBaseURL; this.modelInfo = this.modelDef ? modelDefToModelInfo(this.modelDef) : undefined; @@ -207,6 +221,8 @@ export class Model { sourceInfos.length > 0 ? sourceInfos : undefined, runnableNotebookCells, undefined, + runtime, + importBaseURL, ); } catch (error) { let computedError = error; @@ -292,6 +308,7 @@ export class Model { sourceName?: string, queryName?: string, query?: string, + sourceParameters?: Record, ): Promise<{ result: Malloy.Result; compactResult: QueryData; @@ -316,13 +333,21 @@ export class Model { if (!this.modelMaterializer || !this.modelDef || !this.modelInfo) throw new BadRequestError("Model has no queryable entities."); + // Validate and build source parameter clause if params are provided + const params = sourceParameters ?? {}; + const declaredParams = getSourceParams(this.sourceInfos, sourceName); + if (declaredParams.length > 0 || Object.keys(params).length > 0) { + validateRequiredParams(declaredParams, params); + } + const paramClause = buildMalloyParamClause(params, declaredParams); + // Wrap loadQuery calls in try-catch to handle query parsing errors try { if (!sourceName && !queryName && query) { runnable = this.modelMaterializer.loadQuery("\n" + query); } else if (queryName && !query) { runnable = this.modelMaterializer.loadQuery( - `\nrun: ${sourceName ? sourceName + "->" : ""}${queryName}`, + `\nrun: ${sourceName ? sourceName + paramClause + "->" : ""}${queryName}`, ); } else { const endTime = performance.now(); @@ -491,7 +516,10 @@ export class Model { } as ApiRawNotebook; } - public async executeNotebookCell(cellIndex: number): Promise<{ + public async executeNotebookCell( + cellIndex: number, + sourceParameters?: Record, + ): Promise<{ type: "code" | "markdown"; text: string; queryName?: string; @@ -521,6 +549,19 @@ export class Model { }; } + // If the cell failed initial compilation (e.g. missing required source + // params) and sourceParameters were provided, try to recompile with + // the params injected into the cell text. + if (cell.cellCompilationError) { + if (sourceParameters && Object.keys(sourceParameters).length > 0) { + return this.executeNotebookCellWithParams(cell, sourceParameters); + } + throw new BadRequestError( + `Cell ${cellIndex} failed to compile: ${cell.cellCompilationError.message}. ` + + `If this cell uses a parameterized source, provide sourceParameters.`, + ); + } + // For code cells, execute the runnable if available let queryName: string | undefined = undefined; let queryResult: string | undefined = undefined; @@ -566,6 +607,101 @@ export class Model { }; } + /** + * Recompile and execute a notebook cell that failed initial compilation, + * injecting source parameter values into source invocations in the cell + * text. Uses the stored ModelMaterializer to rebuild the query in context. + */ + /** + * Recompile and execute a notebook cell that failed initial compilation, + * injecting source parameter values into source invocations in the cell + * text. Uses the stored prior ModelMaterializer to rebuild the query + * from the correct model context (before this cell was added). + */ + private async executeNotebookCellWithParams( + cell: RunnableNotebookCell, + sourceParameters: Record, + ): Promise<{ + type: "code" | "markdown"; + text: string; + queryName?: string; + result?: string; + newSources?: string[]; + }> { + if (!this.runtime || !this.importBaseURL) { + throw new BadRequestError( + "Cannot recompile notebook cell: missing runtime context", + ); + } + + // Inject parameter values into source invocations in the cell text. + // For each known parameterized source, replace bare source references + // in `run:` statements with parameterized invocations. + const allSourceInfos = this.sourceInfos ?? []; + const sourcesByName = new Map(allSourceInfos.map((s) => [s.name, s])); + + let modifiedText = cell.text; + for (const [sourceName, sourceInfo] of sourcesByName) { + if (!sourceInfo.parameters || sourceInfo.parameters.length === 0) + continue; + + const paramClause = buildMalloyParamClause( + sourceParameters, + sourceInfo.parameters, + ); + if (!paramClause) continue; + + // Match `run: sourceName ->` or `run: sourceName` at end of line, + // and inject the param clause after the source name. + const pattern = new RegExp(`(run:\\s+${sourceName})\\s*(->|$)`, "gm"); + modifiedText = modifiedText.replace(pattern, `$1${paramClause} $2`); + } + + // Rebuild: extend the prior model (before this cell) with modified text + let recompiledMM: ModelMaterializer; + if (cell.priorModelMaterializer) { + recompiledMM = cell.priorModelMaterializer.extendModel(modifiedText, { + importBaseURL: this.importBaseURL, + }); + } else { + recompiledMM = this.runtime.loadModel(modifiedText, { + importBaseURL: this.importBaseURL, + }); + } + + const runnable = recompiledMM.loadFinalQuery(); + + try { + const rowLimit = + (await runnable.getPreparedResult()).resultExplore.limit || + ROW_LIMIT; + const result = await runnable.run({ rowLimit }); + const query = (await runnable.getPreparedQuery())._query; + const queryName = (query as NamedQueryDef).as || query.name; + const queryResult = + result?._queryResult && + this.modelInfo && + JSON.stringify(API.util.wrapResult(result)); + + return { + type: cell.type, + text: cell.text, + queryName, + result: queryResult || undefined, + newSources: cell.newSources?.map((source) => + JSON.stringify(source), + ), + }; + } catch (error) { + if (error instanceof MalloyError) throw error; + const errorMessage = + error instanceof Error ? error.message : String(error); + throw new BadRequestError( + `Cell execution with parameters failed: ${errorMessage}`, + ); + } + } + static async getModelRuntime( packagePath: string, modelPath: string, @@ -757,10 +893,12 @@ export class Model { let mm: ModelMaterializer | undefined = undefined; const oldImports: string[] = []; const oldSources: Record = {}; - // First generate the sequence of ModelMaterializers. - // This has to happen sync, since mm.getModel() is async and - // may execute out-of-order. + // First generate the sequence of ModelMaterializers (lazy — no + // compilation happens here). Track the MM *before* each statement + // so failed cells can be recompiled with different text later. + const priorMMs: (ModelMaterializer | undefined)[] = []; const mms = parse.statements.map((stmt) => { + priorMMs.push(mm); if (stmt.type === MalloySQLStatementType.MALLOY) { if (!mm) { mm = runtime.loadModel(stmt.text, { importBaseURL }); @@ -777,89 +915,36 @@ export class Model { // Get the Materializer for the current cell/statement. const localMM = mms[index]; if (!localMM) { - // This can't happen because the to be in this branch there stmt must be - // MalloySQLStatementType.MALLOY and we must have a model materializer. throw new Error("Model materializer is undefined"); } - // Pull available sources from the current model. - // Add any of then that are new into newSources and then add them to oldSources. - const currentModelDef = (await localMM.getModel())._modelDef; - let newSources: Malloy.SourceInfo[] = []; - const newImports = currentModelDef.imports?.slice( - oldImports.length, - ); - if (newImports) { - await Promise.all( - newImports.map(async (importLocation) => { - const modelString = await runtime.urlReader.readURL( - new URL(importLocation.importURL), - ); - const importModel = ( - await runtime - .loadModel(modelString as string, { - importBaseURL, - }) - .getModel() - )._modelDef; - const importModelInfo = - modelDefToModelInfo(importModel); - newSources = importModelInfo.entries - .filter((entry) => entry.kind === "source") - .filter( - (source) => !(source.name in oldSources), - ) as Malloy.SourceInfo[]; - oldImports.push(importLocation.importURL.toString()); - }), - ); - } - const currentModelInfo = modelDefToModelInfo(currentModelDef); - newSources = newSources.concat( - currentModelInfo.entries - .filter((entry) => entry.kind === "source") - .filter( - (source) => !(source.name in oldSources), - ) as Malloy.SourceInfo[], - ); - - for (const source of newSources) { - oldSources[source.name] = source; - } - const runnable = localMM.loadFinalQuery(); - - // Extract QueryInfo from the runnable - let queryInfo: Malloy.QueryInfo | undefined = undefined; + // Compilation is wrapped per-cell so that a single cell + // with missing source parameters (or other errors) does not + // prevent the rest of the notebook from loading. try { - const preparedQuery = await runnable.getPreparedQuery(); - const query = preparedQuery._query as NamedQueryDef; - const queryName = query.as || query.name; - const anonymousQuery = - currentModelInfo.anonymous_queries[ - currentModelInfo.anonymous_queries.length - 1 - ]; - - if (anonymousQuery) { - queryInfo = { - name: queryName, - schema: anonymousQuery.schema, - annotations: anonymousQuery.annotations, - definition: anonymousQuery.definition, - code: anonymousQuery.code, - location: anonymousQuery.location, - } as Malloy.QueryInfo; - } - } catch (_error) { - // If we can't extract query info (e.g., no query in cell), that's okay - // This can happen for cells that only define sources + return await Model.compileNotebookCell( + stmt.text, + localMM, + runtime, + importBaseURL, + oldImports, + oldSources, + ); + } catch (cellError) { + logger.warn( + `Notebook cell ${index} compilation failed, deferring to execution time`, + { error: cellError }, + ); + return { + type: "code", + text: stmt.text, + priorModelMaterializer: priorMMs[index], + cellCompilationError: + cellError instanceof Error + ? cellError + : new Error(String(cellError)), + } as RunnableNotebookCell; } - - return { - type: "code", - text: stmt.text, - runnable: runnable, - newSources, - queryInfo, - } as RunnableNotebookCell; } else if (stmt.type === MalloySQLStatementType.MARKDOWN) { return { type: "markdown", @@ -878,6 +963,92 @@ export class Model { }; } + /** + * Compiles a single notebook cell: resolves imports, extracts new sources, + * builds the runnable, and extracts query info. Throws on compilation + * failure (caller decides how to handle). + */ + private static async compileNotebookCell( + cellText: string, + localMM: ModelMaterializer, + runtime: Runtime, + importBaseURL: URL, + oldImports: string[], + oldSources: Record, + ): Promise { + const currentModelDef = (await localMM.getModel())._modelDef; + let newSources: Malloy.SourceInfo[] = []; + const newImports = currentModelDef.imports?.slice(oldImports.length); + if (newImports) { + await Promise.all( + newImports.map(async (importLocation) => { + const modelString = await runtime.urlReader.readURL( + new URL(importLocation.importURL), + ); + const importModel = ( + await runtime + .loadModel(modelString as string, { importBaseURL }) + .getModel() + )._modelDef; + const importModelInfo = modelDefToModelInfo(importModel); + newSources = importModelInfo.entries + .filter((entry) => entry.kind === "source") + .filter( + (source) => !(source.name in oldSources), + ) as Malloy.SourceInfo[]; + oldImports.push(importLocation.importURL.toString()); + }), + ); + } + const currentModelInfo = modelDefToModelInfo(currentModelDef); + newSources = newSources.concat( + currentModelInfo.entries + .filter((entry) => entry.kind === "source") + .filter( + (source) => !(source.name in oldSources), + ) as Malloy.SourceInfo[], + ); + + for (const source of newSources) { + oldSources[source.name] = source; + } + + const runnable = localMM.loadFinalQuery(); + + let queryInfo: Malloy.QueryInfo | undefined = undefined; + try { + const preparedQuery = await runnable.getPreparedQuery(); + const query = preparedQuery._query as NamedQueryDef; + const queryName = query.as || query.name; + const anonymousQuery = + currentModelInfo.anonymous_queries[ + currentModelInfo.anonymous_queries.length - 1 + ]; + + if (anonymousQuery) { + queryInfo = { + name: queryName, + schema: anonymousQuery.schema, + annotations: anonymousQuery.annotations, + definition: anonymousQuery.definition, + code: anonymousQuery.code, + location: anonymousQuery.location, + } as Malloy.QueryInfo; + } + } catch (_error) { + // No query in this cell — that's fine for source-definition-only cells + } + + return { + type: "code", + text: cellText, + runnable, + modelMaterializer: localMM, + newSources, + queryInfo, + } as RunnableNotebookCell; + } + public getModelType(): ModelType { return this.modelType; } diff --git a/packages/server/src/utils/source_params.spec.ts b/packages/server/src/utils/source_params.spec.ts new file mode 100644 index 00000000..984a2ef2 --- /dev/null +++ b/packages/server/src/utils/source_params.spec.ts @@ -0,0 +1,332 @@ +import { describe, expect, it } from "bun:test"; +import type * as Malloy from "@malloydata/malloy-interfaces"; +import { BadRequestError } from "../errors"; +import { + buildMalloyParamClause, + getSourceParams, + paramValueToMalloyLiteral, + validateRequiredParams, +} from "./source_params"; + +// --------------------------------------------------------------------------- +// Helpers to build ParameterInfo fixtures +// --------------------------------------------------------------------------- + +function makeParam( + name: string, + kind: Malloy.ParameterTypeType, + defaultValue?: Malloy.LiteralValue, +): Malloy.ParameterInfo { + return { + name, + type: { kind } as Malloy.ParameterType, + default_value: defaultValue, + }; +} + +function makeStringParam( + name: string, + defaultValue?: string, +): Malloy.ParameterInfo { + return makeParam( + name, + "string_type", + defaultValue !== undefined + ? { kind: "string_literal", string_value: defaultValue } + : undefined, + ); +} + +function makeNumberParam( + name: string, + defaultValue?: number, +): Malloy.ParameterInfo { + return makeParam( + name, + "number_type", + defaultValue !== undefined + ? { kind: "number_literal", number_value: defaultValue } + : undefined, + ); +} + +function makeBooleanParam( + name: string, + defaultValue?: boolean, +): Malloy.ParameterInfo { + return makeParam( + name, + "boolean_type", + defaultValue !== undefined + ? { kind: "boolean_literal", boolean_value: defaultValue } + : undefined, + ); +} + +// --------------------------------------------------------------------------- +// paramValueToMalloyLiteral +// --------------------------------------------------------------------------- + +describe("paramValueToMalloyLiteral", () => { + it("should convert a string value to a quoted Malloy literal", () => { + const param = makeStringParam("s"); + expect(paramValueToMalloyLiteral("hello", param)).toBe('"hello"'); + }); + + it("should escape quotes and backslashes in string values", () => { + const param = makeStringParam("s"); + expect(paramValueToMalloyLiteral('say "hi"', param)).toBe( + '"say \\"hi\\""', + ); + expect(paramValueToMalloyLiteral("back\\slash", param)).toBe( + '"back\\\\slash"', + ); + }); + + it("should convert an integer string to an unquoted number literal", () => { + const param = makeNumberParam("n"); + expect(paramValueToMalloyLiteral("42", param)).toBe("42"); + }); + + it("should convert a decimal string to an unquoted number literal", () => { + const param = makeNumberParam("n"); + expect(paramValueToMalloyLiteral("3.14", param)).toBe("3.14"); + }); + + it("should convert negative numbers", () => { + const param = makeNumberParam("n"); + expect(paramValueToMalloyLiteral("-100", param)).toBe("-100"); + }); + + it("should throw for non-numeric value on a number parameter", () => { + const param = makeNumberParam("n"); + expect(() => paramValueToMalloyLiteral("abc", param)).toThrow( + BadRequestError, + ); + expect(() => paramValueToMalloyLiteral("abc", param)).toThrow( + /expects a number/, + ); + }); + + it('should convert "true"/"false" to boolean literals', () => { + const param = makeBooleanParam("b"); + expect(paramValueToMalloyLiteral("true", param)).toBe("true"); + expect(paramValueToMalloyLiteral("false", param)).toBe("false"); + }); + + it('should convert "1"/"0" to boolean literals', () => { + const param = makeBooleanParam("b"); + expect(paramValueToMalloyLiteral("1", param)).toBe("true"); + expect(paramValueToMalloyLiteral("0", param)).toBe("false"); + }); + + it("should be case-insensitive for boolean values", () => { + const param = makeBooleanParam("b"); + expect(paramValueToMalloyLiteral("TRUE", param)).toBe("true"); + expect(paramValueToMalloyLiteral("False", param)).toBe("false"); + }); + + it("should throw for invalid boolean value", () => { + const param = makeBooleanParam("b"); + expect(() => paramValueToMalloyLiteral("yes", param)).toThrow( + BadRequestError, + ); + expect(() => paramValueToMalloyLiteral("yes", param)).toThrow( + /expects a boolean/, + ); + }); + + it("should convert date values to @-prefixed literals", () => { + const param = makeParam("d", "date_type"); + expect(paramValueToMalloyLiteral("2024-01-15", param)).toBe( + "@2024-01-15", + ); + }); + + it("should convert timestamp values to @-prefixed literals", () => { + const param = makeParam("t", "timestamp_type"); + expect(paramValueToMalloyLiteral("2024-01-15 10:30:00", param)).toBe( + "@2024-01-15 10:30:00", + ); + }); + + it("should convert timestamptz values to @-prefixed literals", () => { + const param = makeParam("t", "timestamptz_type"); + expect(paramValueToMalloyLiteral("2024-01-15 10:30:00Z", param)).toBe( + "@2024-01-15 10:30:00Z", + ); + }); + + it("should convert filter expression values to f'' literals", () => { + const param = makeParam("f", "filter_expression_type"); + expect( + paramValueToMalloyLiteral("last week for two days", param), + ).toBe("f'last week for two days'"); + }); + + it("should default to quoted string for unknown type kinds", () => { + const param = makeParam("x", "json_type"); + expect(paramValueToMalloyLiteral('{"a":1}', param)).toBe( + '"{\\\"a\\\":1}"', + ); + }); +}); + +// --------------------------------------------------------------------------- +// validateRequiredParams +// --------------------------------------------------------------------------- + +describe("validateRequiredParams", () => { + it("should not throw when all required params are provided", () => { + const declared = [makeStringParam("a"), makeNumberParam("b")]; + expect(() => + validateRequiredParams(declared, { a: "val", b: "42" }), + ).not.toThrow(); + }); + + it("should not throw when params with defaults are missing", () => { + const declared = [ + makeStringParam("a"), + makeNumberParam("b", 10), + ]; + // Only 'a' is required (no default); 'b' has a default + expect(() => validateRequiredParams(declared, { a: "val" })).not.toThrow(); + }); + + it("should not throw when there are no declared params", () => { + expect(() => validateRequiredParams([], {})).not.toThrow(); + }); + + it("should throw for a single missing required param", () => { + const declared = [makeStringParam("required_param")]; + expect(() => validateRequiredParams(declared, {})).toThrow( + BadRequestError, + ); + expect(() => validateRequiredParams(declared, {})).toThrow( + 'Parameter "required_param" is required', + ); + }); + + it("should throw listing all missing required params", () => { + const declared = [ + makeStringParam("a"), + makeNumberParam("b"), + makeBooleanParam("c", false), // has default — not required + ]; + expect(() => validateRequiredParams(declared, {})).toThrow( + BadRequestError, + ); + expect(() => validateRequiredParams(declared, {})).toThrow( + /Parameters "a", "b" are required/, + ); + }); + + it("should throw only for the params not provided", () => { + const declared = [ + makeStringParam("a"), + makeNumberParam("b"), + ]; + expect(() => validateRequiredParams(declared, { a: "val" })).toThrow( + 'Parameter "b" is required', + ); + }); +}); + +// --------------------------------------------------------------------------- +// buildMalloyParamClause +// --------------------------------------------------------------------------- + +describe("buildMalloyParamClause", () => { + it("should return empty string when no params are provided", () => { + const declared = [makeStringParam("a", "default")]; + expect(buildMalloyParamClause({}, declared)).toBe(""); + }); + + it("should build a single-param clause", () => { + const declared = [makeStringParam("name")]; + expect(buildMalloyParamClause({ name: "Alice" }, declared)).toBe( + '(name is "Alice")', + ); + }); + + it("should build a multi-param clause with correct types", () => { + const declared = [ + makeStringParam("label"), + makeNumberParam("count"), + makeBooleanParam("active"), + ]; + const result = buildMalloyParamClause( + { label: "test", count: "5", active: "true" }, + declared, + ); + expect(result).toBe('(label is "test", count is 5, active is true)'); + }); + + it("should throw for an unknown parameter name", () => { + const declared = [makeStringParam("known")]; + expect(() => + buildMalloyParamClause({ unknown: "val" }, declared), + ).toThrow(BadRequestError); + expect(() => + buildMalloyParamClause({ unknown: "val" }, declared), + ).toThrow(/Unknown source parameter "unknown"/); + }); + + it("should list available params in unknown-param error", () => { + const declared = [makeStringParam("alpha"), makeNumberParam("beta")]; + expect(() => + buildMalloyParamClause({ gamma: "val" }, declared), + ).toThrow(/Available parameters: alpha, beta/); + }); + + it("should throw for unknown param when declared list is empty", () => { + expect(() => + buildMalloyParamClause({ x: "1" }, []), + ).toThrow(/Available parameters: \(none\)/); + }); +}); + +// --------------------------------------------------------------------------- +// getSourceParams +// --------------------------------------------------------------------------- + +describe("getSourceParams", () => { + const sources: Malloy.SourceInfo[] = [ + { + name: "flights", + schema: { fields: [] }, + parameters: [ + makeStringParam("carrier"), + makeNumberParam("min_distance", 0), + ], + }, + { + name: "airports", + schema: { fields: [] }, + // no parameters + }, + ]; + + it("should return parameters for a known source", () => { + const params = getSourceParams(sources, "flights"); + expect(params).toHaveLength(2); + expect(params[0].name).toBe("carrier"); + expect(params[1].name).toBe("min_distance"); + }); + + it("should return empty array for a source with no parameters", () => { + expect(getSourceParams(sources, "airports")).toEqual([]); + }); + + it("should return empty array for an unknown source name", () => { + expect(getSourceParams(sources, "nonexistent")).toEqual([]); + }); + + it("should return empty array when sourceInfos is undefined", () => { + expect(getSourceParams(undefined, "flights")).toEqual([]); + }); + + it("should return empty array when sourceName is undefined", () => { + expect(getSourceParams(sources, undefined)).toEqual([]); + }); +}); diff --git a/packages/server/src/utils/source_params.ts b/packages/server/src/utils/source_params.ts new file mode 100644 index 00000000..b8a1b43f --- /dev/null +++ b/packages/server/src/utils/source_params.ts @@ -0,0 +1,115 @@ +import type * as Malloy from "@malloydata/malloy-interfaces"; +import { BadRequestError } from "../errors"; + +/** + * Parses a raw string value into a Malloy literal expression based on + * the declared parameter type from the SourceInfo. + */ +export function paramValueToMalloyLiteral( + rawValue: string, + paramInfo: Malloy.ParameterInfo, +): string { + const kind = paramInfo.type.kind; + + switch (kind) { + case "number_type": { + const num = Number(rawValue); + if (isNaN(num)) { + throw new BadRequestError( + `Parameter "${paramInfo.name}" expects a number, got "${rawValue}"`, + ); + } + return rawValue; + } + + case "boolean_type": { + const lower = rawValue.toLowerCase(); + if (lower === "true" || lower === "1") return "true"; + if (lower === "false" || lower === "0") return "false"; + throw new BadRequestError( + `Parameter "${paramInfo.name}" expects a boolean (true/false), got "${rawValue}"`, + ); + } + + case "date_type": + return `@${rawValue}`; + + case "timestamp_type": + case "timestamptz_type": + return `@${rawValue}`; + + case "filter_expression_type": + return `f'${rawValue}'`; + + case "string_type": + default: + return `"${rawValue.replace(/\\/g, "\\\\").replace(/"/g, '\\"')}"`; + } +} + +/** + * Validates that all required source parameters (those without a default + * value) have been provided. Throws BadRequestError listing any missing + * required parameters. + */ +export function validateRequiredParams( + declaredParams: Malloy.ParameterInfo[], + providedParams: Record, +): void { + const missing: string[] = []; + for (const param of declaredParams) { + if (!param.default_value && !(param.name in providedParams)) { + missing.push(param.name); + } + } + if (missing.length === 1) { + throw new BadRequestError( + `Parameter "${missing[0]}" is required`, + ); + } + if (missing.length > 1) { + throw new BadRequestError( + `Parameters ${missing.map((n) => `"${n}"`).join(", ")} are required`, + ); + } +} + +/** + * Builds the Malloy parameter-passing syntax for a source invocation. + * Returns a string like `(param1 is 42, param2 is "hello")` or empty + * string if no params are provided. + */ +export function buildMalloyParamClause( + providedParams: Record, + declaredParams: Malloy.ParameterInfo[], +): string { + const paramsByName = new Map(declaredParams.map((p) => [p.name, p])); + const assignments: string[] = []; + + for (const [name, rawValue] of Object.entries(providedParams)) { + const paramInfo = paramsByName.get(name); + if (!paramInfo) { + throw new BadRequestError( + `Unknown source parameter "${name}". Available parameters: ${declaredParams.map((p) => p.name).join(", ") || "(none)"}`, + ); + } + const literal = paramValueToMalloyLiteral(rawValue, paramInfo); + assignments.push(`${name} is ${literal}`); + } + + if (assignments.length === 0) return ""; + return `(${assignments.join(", ")})`; +} + +/** + * Finds the ParameterInfo array for a source by name. Returns an empty + * array when the source has no parameters or is not found. + */ +export function getSourceParams( + sourceInfos: Malloy.SourceInfo[] | undefined, + sourceName: string | undefined, +): Malloy.ParameterInfo[] { + if (!sourceInfos || !sourceName) return []; + const source = sourceInfos.find((s) => s.name === sourceName); + return source?.parameters ?? []; +} From 7171a62aa9f8c1e1d26f1b6da3f16303d4d610fe Mon Sep 17 00:00:00 2001 From: Josh Sacks Date: Fri, 10 Apr 2026 11:52:29 -0600 Subject: [PATCH 02/11] fix: change sourceParameters type from Record to Record Allow callers to pass native numbers, booleans, etc. directly instead of requiring everything to be string-encoded. The paramValueToMalloyLiteral function now handles both native types and string representations. Made-with: Cursor --- api-doc.yaml | 20 +++++----- .../server/src/controller/model.controller.ts | 2 +- .../server/src/controller/query.controller.ts | 2 +- .../src/mcp/tools/execute_query_tool.ts | 8 ++-- packages/server/src/server.ts | 4 +- packages/server/src/service/model.ts | 6 +-- .../server/src/utils/source_params.spec.ts | 35 +++++++++++++++++ packages/server/src/utils/source_params.ts | 38 ++++++++++++------- 8 files changed, 80 insertions(+), 35 deletions(-) diff --git a/api-doc.yaml b/api-doc.yaml index e70fda71..b625a832 100644 --- a/api-doc.yaml +++ b/api-doc.yaml @@ -1474,9 +1474,10 @@ paths: - name: sourceParameters in: query description: >- - JSON-encoded object of source parameter key-value string pairs. + JSON-encoded object of source parameter key-value pairs. Keys must match parameter names declared on sources in the notebook's - Malloy model. Values are parsed according to declared parameter types. + Malloy model. Values can be strings, numbers, or booleans and are + converted to the correct Malloy literal based on declared parameter types. required: false schema: type: string @@ -1927,15 +1928,14 @@ components: description: 'If true, returns a simple JSON array of row objects in the form {"columnName": value}. If false (default), returns the full Malloy result with type metadata for rendering.' sourceParameters: type: object - additionalProperties: - type: string + additionalProperties: true description: | - Source parameter values as key-value string pairs. Keys must match parameter - names declared on the source in the Malloy model. Values are strings that are - parsed according to the parameter's declared type (string, number, boolean, - date, timestamp, or filter expression). Required parameters (those without a - default value in the model) must be provided or the request will fail with a - 400 error. + Source parameter values as key-value pairs. Keys must match parameter + names declared on the source in the Malloy model. Values can be strings, + numbers, or booleans — they are converted to the correct Malloy literal + based on the parameter's declared type. Required parameters (those without + a default value in the model) must be provided or the request will fail + with a 400 error. versionId: type: string description: Version ID diff --git a/packages/server/src/controller/model.controller.ts b/packages/server/src/controller/model.controller.ts index d83ffc55..ce7e4afd 100644 --- a/packages/server/src/controller/model.controller.ts +++ b/packages/server/src/controller/model.controller.ts @@ -84,7 +84,7 @@ export class ModelController { packageName: string, notebookPath: string, cellIndex: number, - sourceParameters?: Record, + sourceParameters?: Record, ): Promise<{ type: "code" | "markdown"; text: string; diff --git a/packages/server/src/controller/query.controller.ts b/packages/server/src/controller/query.controller.ts index bb3011bb..463d11fc 100644 --- a/packages/server/src/controller/query.controller.ts +++ b/packages/server/src/controller/query.controller.ts @@ -29,7 +29,7 @@ export class QueryController { queryName: string, query: string, compactJson: boolean = false, - sourceParameters?: Record, + sourceParameters?: Record, ): Promise { const project = await this.projectStore.getProject(projectName, false); const p = await project.getPackage(packageName, false); diff --git a/packages/server/src/mcp/tools/execute_query_tool.ts b/packages/server/src/mcp/tools/execute_query_tool.ts index c19ea7b4..2b168a5d 100644 --- a/packages/server/src/mcp/tools/execute_query_tool.ts +++ b/packages/server/src/mcp/tools/execute_query_tool.ts @@ -25,12 +25,12 @@ const executeQueryShape = { sourceName: z.string().optional().describe("Source name for a view"), queryName: z.string().optional().describe("Named query or view"), sourceParameters: z - .record(z.string()) + .record(z.unknown()) .optional() .describe( - "Source parameter values as key-value string pairs. " + - "Values are parsed according to the parameter's declared type in the model " + - "(e.g. numbers, booleans, dates). " + + "Source parameter values as key-value pairs. " + + "Values can be strings, numbers, or booleans — they are converted " + + "to the correct Malloy literal based on the parameter's declared type. " + "Required parameters that lack a default value must be provided.", ), }; diff --git a/packages/server/src/server.ts b/packages/server/src/server.ts index ad78e907..7175f607 100644 --- a/packages/server/src/server.ts +++ b/packages/server/src/server.ts @@ -821,7 +821,7 @@ app.get( typeof req.query.sourceParameters === "string" ? (JSON.parse(req.query.sourceParameters) as Record< string, - string + unknown >) : undefined; @@ -880,7 +880,7 @@ app.post( // Express stores wildcard matches in params['0'] const modelPath = (req.params as Record)["0"]; const sourceParameters = req.body.sourceParameters as - | Record + | Record | undefined; res.status(200).json( await queryController.getQuery( diff --git a/packages/server/src/service/model.ts b/packages/server/src/service/model.ts index 6c5e6c15..55ab5161 100644 --- a/packages/server/src/service/model.ts +++ b/packages/server/src/service/model.ts @@ -308,7 +308,7 @@ export class Model { sourceName?: string, queryName?: string, query?: string, - sourceParameters?: Record, + sourceParameters?: Record, ): Promise<{ result: Malloy.Result; compactResult: QueryData; @@ -518,7 +518,7 @@ export class Model { public async executeNotebookCell( cellIndex: number, - sourceParameters?: Record, + sourceParameters?: Record, ): Promise<{ type: "code" | "markdown"; text: string; @@ -620,7 +620,7 @@ export class Model { */ private async executeNotebookCellWithParams( cell: RunnableNotebookCell, - sourceParameters: Record, + sourceParameters: Record, ): Promise<{ type: "code" | "markdown"; text: string; diff --git a/packages/server/src/utils/source_params.spec.ts b/packages/server/src/utils/source_params.spec.ts index 984a2ef2..3cb44343 100644 --- a/packages/server/src/utils/source_params.spec.ts +++ b/packages/server/src/utils/source_params.spec.ts @@ -136,6 +136,28 @@ describe("paramValueToMalloyLiteral", () => { ); }); + // --- Native (non-string) value tests --- + + it("should accept native number values directly", () => { + const param = makeNumberParam("n"); + expect(paramValueToMalloyLiteral(42, param)).toBe("42"); + expect(paramValueToMalloyLiteral(3.14, param)).toBe("3.14"); + expect(paramValueToMalloyLiteral(-100, param)).toBe("-100"); + }); + + it("should throw for NaN native number", () => { + const param = makeNumberParam("n"); + expect(() => paramValueToMalloyLiteral(NaN, param)).toThrow( + /expects a number, got NaN/, + ); + }); + + it("should accept native boolean values directly", () => { + const param = makeBooleanParam("b"); + expect(paramValueToMalloyLiteral(true, param)).toBe("true"); + expect(paramValueToMalloyLiteral(false, param)).toBe("false"); + }); + it("should convert date values to @-prefixed literals", () => { const param = makeParam("d", "date_type"); expect(paramValueToMalloyLiteral("2024-01-15", param)).toBe( @@ -284,6 +306,19 @@ describe("buildMalloyParamClause", () => { buildMalloyParamClause({ x: "1" }, []), ).toThrow(/Available parameters: \(none\)/); }); + + it("should handle native number and boolean values", () => { + const declared = [ + makeNumberParam("limit"), + makeBooleanParam("active"), + makeStringParam("label"), + ]; + const result = buildMalloyParamClause( + { limit: 100, active: true, label: "test" }, + declared, + ); + expect(result).toBe('(limit is 100, active is true, label is "test")'); + }); }); // --------------------------------------------------------------------------- diff --git a/packages/server/src/utils/source_params.ts b/packages/server/src/utils/source_params.ts index b8a1b43f..650eb7fc 100644 --- a/packages/server/src/utils/source_params.ts +++ b/packages/server/src/utils/source_params.ts @@ -2,48 +2,58 @@ import type * as Malloy from "@malloydata/malloy-interfaces"; import { BadRequestError } from "../errors"; /** - * Parses a raw string value into a Malloy literal expression based on - * the declared parameter type from the SourceInfo. + * Converts a raw value (string, number, boolean, etc.) into a Malloy + * literal expression based on the declared parameter type. */ export function paramValueToMalloyLiteral( - rawValue: string, + rawValue: unknown, paramInfo: Malloy.ParameterInfo, ): string { const kind = paramInfo.type.kind; + const str = String(rawValue); switch (kind) { case "number_type": { - const num = Number(rawValue); + if (typeof rawValue === "number") { + if (isNaN(rawValue)) { + throw new BadRequestError( + `Parameter "${paramInfo.name}" expects a number, got NaN`, + ); + } + return String(rawValue); + } + const num = Number(str); if (isNaN(num)) { throw new BadRequestError( - `Parameter "${paramInfo.name}" expects a number, got "${rawValue}"`, + `Parameter "${paramInfo.name}" expects a number, got "${str}"`, ); } - return rawValue; + return str; } case "boolean_type": { - const lower = rawValue.toLowerCase(); + if (typeof rawValue === "boolean") return String(rawValue); + const lower = str.toLowerCase(); if (lower === "true" || lower === "1") return "true"; if (lower === "false" || lower === "0") return "false"; throw new BadRequestError( - `Parameter "${paramInfo.name}" expects a boolean (true/false), got "${rawValue}"`, + `Parameter "${paramInfo.name}" expects a boolean (true/false), got "${str}"`, ); } case "date_type": - return `@${rawValue}`; + return `@${str}`; case "timestamp_type": case "timestamptz_type": - return `@${rawValue}`; + return `@${str}`; case "filter_expression_type": - return `f'${rawValue}'`; + return `f'${str}'`; case "string_type": default: - return `"${rawValue.replace(/\\/g, "\\\\").replace(/"/g, '\\"')}"`; + return `"${str.replace(/\\/g, "\\\\").replace(/"/g, '\\"')}"`; } } @@ -54,7 +64,7 @@ export function paramValueToMalloyLiteral( */ export function validateRequiredParams( declaredParams: Malloy.ParameterInfo[], - providedParams: Record, + providedParams: Record, ): void { const missing: string[] = []; for (const param of declaredParams) { @@ -80,7 +90,7 @@ export function validateRequiredParams( * string if no params are provided. */ export function buildMalloyParamClause( - providedParams: Record, + providedParams: Record, declaredParams: Malloy.ParameterInfo[], ): string { const paramsByName = new Map(declaredParams.map((p) => [p.name, p])); From 6586e76bc8c4b70a9718bd0201e043e758d99fab Mon Sep 17 00:00:00 2001 From: Josh Sacks Date: Fri, 10 Apr 2026 12:19:32 -0600 Subject: [PATCH 03/11] style: fix eslint/prettier formatting in source_params files Made-with: Cursor --- .../server/src/utils/source_params.spec.ts | 36 ++++++++----------- packages/server/src/utils/source_params.ts | 4 +-- 2 files changed, 16 insertions(+), 24 deletions(-) diff --git a/packages/server/src/utils/source_params.spec.ts b/packages/server/src/utils/source_params.spec.ts index 3cb44343..f74849cc 100644 --- a/packages/server/src/utils/source_params.spec.ts +++ b/packages/server/src/utils/source_params.spec.ts @@ -181,16 +181,14 @@ describe("paramValueToMalloyLiteral", () => { it("should convert filter expression values to f'' literals", () => { const param = makeParam("f", "filter_expression_type"); - expect( - paramValueToMalloyLiteral("last week for two days", param), - ).toBe("f'last week for two days'"); + expect(paramValueToMalloyLiteral("last week for two days", param)).toBe( + "f'last week for two days'", + ); }); it("should default to quoted string for unknown type kinds", () => { const param = makeParam("x", "json_type"); - expect(paramValueToMalloyLiteral('{"a":1}', param)).toBe( - '"{\\\"a\\\":1}"', - ); + expect(paramValueToMalloyLiteral('{"a":1}', param)).toBe('"{\\"a\\":1}"'); }); }); @@ -207,12 +205,11 @@ describe("validateRequiredParams", () => { }); it("should not throw when params with defaults are missing", () => { - const declared = [ - makeStringParam("a"), - makeNumberParam("b", 10), - ]; + const declared = [makeStringParam("a"), makeNumberParam("b", 10)]; // Only 'a' is required (no default); 'b' has a default - expect(() => validateRequiredParams(declared, { a: "val" })).not.toThrow(); + expect(() => + validateRequiredParams(declared, { a: "val" }), + ).not.toThrow(); }); it("should not throw when there are no declared params", () => { @@ -244,10 +241,7 @@ describe("validateRequiredParams", () => { }); it("should throw only for the params not provided", () => { - const declared = [ - makeStringParam("a"), - makeNumberParam("b"), - ]; + const declared = [makeStringParam("a"), makeNumberParam("b")]; expect(() => validateRequiredParams(declared, { a: "val" })).toThrow( 'Parameter "b" is required', ); @@ -296,15 +290,15 @@ describe("buildMalloyParamClause", () => { it("should list available params in unknown-param error", () => { const declared = [makeStringParam("alpha"), makeNumberParam("beta")]; - expect(() => - buildMalloyParamClause({ gamma: "val" }, declared), - ).toThrow(/Available parameters: alpha, beta/); + expect(() => buildMalloyParamClause({ gamma: "val" }, declared)).toThrow( + /Available parameters: alpha, beta/, + ); }); it("should throw for unknown param when declared list is empty", () => { - expect(() => - buildMalloyParamClause({ x: "1" }, []), - ).toThrow(/Available parameters: \(none\)/); + expect(() => buildMalloyParamClause({ x: "1" }, [])).toThrow( + /Available parameters: \(none\)/, + ); }); it("should handle native number and boolean values", () => { diff --git a/packages/server/src/utils/source_params.ts b/packages/server/src/utils/source_params.ts index 650eb7fc..7a82ad4e 100644 --- a/packages/server/src/utils/source_params.ts +++ b/packages/server/src/utils/source_params.ts @@ -73,9 +73,7 @@ export function validateRequiredParams( } } if (missing.length === 1) { - throw new BadRequestError( - `Parameter "${missing[0]}" is required`, - ); + throw new BadRequestError(`Parameter "${missing[0]}" is required`); } if (missing.length > 1) { throw new BadRequestError( From 615380dce07c50dac6e6e820a464d3c868647a9c Mon Sep 17 00:00:00 2001 From: Josh Sacks Date: Fri, 10 Apr 2026 12:37:44 -0600 Subject: [PATCH 04/11] fix: silently ignore unknown source parameters instead of throwing Extra parameters not declared on the source are now skipped rather than rejected, so callers can pass a superset of params without error. Made-with: Cursor --- packages/server/src/service/model.spec.ts | 20 ++++++++++----- .../server/src/utils/source_params.spec.ts | 25 +++++++------------ packages/server/src/utils/source_params.ts | 6 +---- 3 files changed, 24 insertions(+), 27 deletions(-) diff --git a/packages/server/src/service/model.spec.ts b/packages/server/src/service/model.spec.ts index 81d4cf7b..c59a9856 100644 --- a/packages/server/src/service/model.spec.ts +++ b/packages/server/src/service/model.spec.ts @@ -354,8 +354,10 @@ describe("service/model", () => { sinon.restore(); }); - it("should throw BadRequestError for unknown source parameter", async () => { - const loadQueryStub = sinon.stub(); + it("should silently ignore unknown source parameters", async () => { + const loadQueryStub = sinon.stub().returns({ + getPreparedResult: sinon.stub().rejects(new Error("stop")), + }); const model = new Model( packageName, mockModelPath, @@ -370,12 +372,18 @@ describe("service/model", () => { undefined, ); - await expect( - model.getQueryResults("flights", "by_carrier", undefined, { + try { + await model.getQueryResults("flights", "by_carrier", undefined, { nonexistent: "val", - }), - ).rejects.toThrow(/Unknown source parameter "nonexistent"/); + }); + } catch { + // expected — getPreparedResult throws + } + expect(loadQueryStub.calledOnce).toBe(true); + expect(loadQueryStub.firstCall.args[0]).toBe( + "\nrun: flights->by_carrier", + ); sinon.restore(); }); diff --git a/packages/server/src/utils/source_params.spec.ts b/packages/server/src/utils/source_params.spec.ts index f74849cc..66313a07 100644 --- a/packages/server/src/utils/source_params.spec.ts +++ b/packages/server/src/utils/source_params.spec.ts @@ -278,27 +278,20 @@ describe("buildMalloyParamClause", () => { expect(result).toBe('(label is "test", count is 5, active is true)'); }); - it("should throw for an unknown parameter name", () => { + it("should silently ignore unknown parameter names", () => { const declared = [makeStringParam("known")]; - expect(() => - buildMalloyParamClause({ unknown: "val" }, declared), - ).toThrow(BadRequestError); - expect(() => - buildMalloyParamClause({ unknown: "val" }, declared), - ).toThrow(/Unknown source parameter "unknown"/); + expect(buildMalloyParamClause({ unknown: "val" }, declared)).toBe(""); }); - it("should list available params in unknown-param error", () => { - const declared = [makeStringParam("alpha"), makeNumberParam("beta")]; - expect(() => buildMalloyParamClause({ gamma: "val" }, declared)).toThrow( - /Available parameters: alpha, beta/, - ); + it("should include only matching params and skip unknown ones", () => { + const declared = [makeStringParam("name")]; + expect( + buildMalloyParamClause({ name: "Alice", extra: "ignored" }, declared), + ).toBe('(name is "Alice")'); }); - it("should throw for unknown param when declared list is empty", () => { - expect(() => buildMalloyParamClause({ x: "1" }, [])).toThrow( - /Available parameters: \(none\)/, - ); + it("should return empty string when no declared params exist", () => { + expect(buildMalloyParamClause({ x: "1" }, [])).toBe(""); }); it("should handle native number and boolean values", () => { diff --git a/packages/server/src/utils/source_params.ts b/packages/server/src/utils/source_params.ts index 7a82ad4e..6fab659a 100644 --- a/packages/server/src/utils/source_params.ts +++ b/packages/server/src/utils/source_params.ts @@ -96,11 +96,7 @@ export function buildMalloyParamClause( for (const [name, rawValue] of Object.entries(providedParams)) { const paramInfo = paramsByName.get(name); - if (!paramInfo) { - throw new BadRequestError( - `Unknown source parameter "${name}". Available parameters: ${declaredParams.map((p) => p.name).join(", ") || "(none)"}`, - ); - } + if (!paramInfo) continue; const literal = paramValueToMalloyLiteral(rawValue, paramInfo); assignments.push(`${name} is ${literal}`); } From fe83a741f13385116c7417e04618e2516a4cefbc Mon Sep 17 00:00:00 2001 From: Josh Sacks Date: Mon, 13 Apr 2026 10:13:29 -0600 Subject: [PATCH 05/11] DCO Remediation Commit for Josh Sacks I, Josh Sacks , hereby add my Signed-off-by to this commit: 74f0ff50d19eea0f50f84990a66a19fab496b067 I, Josh Sacks , hereby add my Signed-off-by to this commit: 7171a62aa9f8c1e1d26f1b6da3f16303d4d610fe I, Josh Sacks , hereby add my Signed-off-by to this commit: 6586e76bc8c4b70a9718bd0201e043e758d99fab I, Josh Sacks , hereby add my Signed-off-by to this commit: 615380dce07c50dac6e6e820a464d3c868647a9c Signed-off-by: Josh Sacks --- packages/server/src/utils/source_params.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/server/src/utils/source_params.spec.ts b/packages/server/src/utils/source_params.spec.ts index 66313a07..d0d2c24b 100644 --- a/packages/server/src/utils/source_params.spec.ts +++ b/packages/server/src/utils/source_params.spec.ts @@ -1,5 +1,5 @@ -import { describe, expect, it } from "bun:test"; import type * as Malloy from "@malloydata/malloy-interfaces"; +import { describe, expect, it } from "bun:test"; import { BadRequestError } from "../errors"; import { buildMalloyParamClause, @@ -70,7 +70,7 @@ function makeBooleanParam( describe("paramValueToMalloyLiteral", () => { it("should convert a string value to a quoted Malloy literal", () => { const param = makeStringParam("s"); - expect(paramValueToMalloyLiteral("hello", param)).toBe('"hello"'); + expect(paramValueToMalloyLiteral("hello2", param)).toBe('"hello2"'); }); it("should escape quotes and backslashes in string values", () => { From a4818b9839c349c31cb7d0f4031210103db58b1c Mon Sep 17 00:00:00 2001 From: Josh Sacks Date: Mon, 13 Apr 2026 10:44:56 -0600 Subject: [PATCH 06/11] fix: allow notebooks with parameterized sources to load without errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a notebook cell fails compilation (e.g. missing required source parameters), the model-level getModel() also fails because the materializer includes the broken cell text. Previously this caused the entire notebook to be stored with a compilationError, making getNotebook() throw. Now Model.create() catches notebook model-level compilation failures gracefully: it collects sourceInfos from successfully compiled cells and returns a usable Model with per-cell error info intact. This lets the publisher load packages containing parameterized notebooks without crashing. Adds integration test with real DuckDB + Malloy experimental parameters that verifies the full load → getNotebook → executeCell flow. Made-with: Cursor --- packages/server/src/service/model.ts | 116 ++++++++++----- .../src/service/model_notebook_params.spec.ts | 137 ++++++++++++++++++ 2 files changed, 213 insertions(+), 40 deletions(-) create mode 100644 packages/server/src/service/model_notebook_params.spec.ts diff --git a/packages/server/src/service/model.ts b/packages/server/src/service/model.ts index 55ab5161..7e668bd5 100644 --- a/packages/server/src/service/model.ts +++ b/packages/server/src/service/model.ts @@ -159,52 +159,88 @@ export class Model { let queries = undefined; const sourceInfos: Malloy.SourceInfo[] = []; if (modelMaterializer) { - modelDef = (await modelMaterializer.getModel())._modelDef; - sources = Model.getSources(modelPath, modelDef); - queries = Model.getQueries(modelPath, modelDef); - - // Collect sourceInfos from imported models first - // This follows the same pattern as notebook imports handling - const imports = modelDef.imports || []; - const importedSourceNames = new Set(); - for (const importLocation of imports) { - try { - const modelString = await runtime.urlReader.readURL( - new URL(importLocation.importURL), + try { + modelDef = (await modelMaterializer.getModel())._modelDef; + } catch (modelError) { + // For notebooks, some cells may have failed compilation + // (e.g. missing required source parameters). The + // materializer includes those broken cells so getModel() + // fails, but we still have per-cell results and can + // collect sourceInfos from successfully compiled cells. + if (runnableNotebookCells) { + logger.warn( + "Notebook model-level compilation failed; " + + "using per-cell results", + { error: modelError }, ); - const importedModelDef = ( - await runtime - .loadModel(modelString as string, { importBaseURL }) - .getModel() - )._modelDef; - const importedModelInfo = - modelDefToModelInfo(importedModelDef); - const importedSources = importedModelInfo.entries.filter( - (entry) => entry.kind === "source", - ) as Malloy.SourceInfo[]; - for (const source of importedSources) { - if (!importedSourceNames.has(source.name)) { - sourceInfos.push(source); - importedSourceNames.add(source.name); + } else { + throw modelError; + } + } + + if (modelDef) { + sources = Model.getSources(modelPath, modelDef); + queries = Model.getQueries(modelPath, modelDef); + + // Collect sourceInfos from imported models first + const imports = modelDef.imports || []; + const importedSourceNames = new Set(); + for (const importLocation of imports) { + try { + const modelString = await runtime.urlReader.readURL( + new URL(importLocation.importURL), + ); + const importedModelDef = ( + await runtime + .loadModel(modelString as string, { + importBaseURL, + }) + .getModel() + )._modelDef; + const importedModelInfo = + modelDefToModelInfo(importedModelDef); + const importedSources = importedModelInfo.entries.filter( + (entry) => entry.kind === "source", + ) as Malloy.SourceInfo[]; + for (const source of importedSources) { + if (!importedSourceNames.has(source.name)) { + sourceInfos.push(source); + importedSourceNames.add(source.name); + } } + } catch (importError) { + logger.warn("Failed to load sourceInfo from import", { + importURL: importLocation.importURL, + error: importError, + }); + } + } + + // Add locally-defined sources (not already added from imports) + const localModelInfo = modelDefToModelInfo(modelDef); + const localSources = localModelInfo.entries.filter( + (entry) => entry.kind === "source", + ) as Malloy.SourceInfo[]; + for (const source of localSources) { + if (!importedSourceNames.has(source.name)) { + sourceInfos.push(source); } - } catch (importError) { - // Log but don't fail if we can't load an import's sourceInfo - logger.warn("Failed to load sourceInfo from import", { - importURL: importLocation.importURL, - error: importError, - }); } } - // Add locally-defined sources (not already added from imports) - const localModelInfo = modelDefToModelInfo(modelDef); - const localSources = localModelInfo.entries.filter( - (entry) => entry.kind === "source", - ) as Malloy.SourceInfo[]; - for (const source of localSources) { - if (!importedSourceNames.has(source.name)) { - sourceInfos.push(source); + // For notebooks without a modelDef (model-level compilation + // failed), collect sourceInfos from successfully compiled cells. + if (!modelDef && runnableNotebookCells) { + const seenNames = new Set(); + for (const cell of runnableNotebookCells) { + if (cell.newSources) { + for (const source of cell.newSources) { + if (!seenNames.has(source.name)) { + sourceInfos.push(source); + seenNames.add(source.name); + } + } + } } } } diff --git a/packages/server/src/service/model_notebook_params.spec.ts b/packages/server/src/service/model_notebook_params.spec.ts new file mode 100644 index 00000000..47c75f2c --- /dev/null +++ b/packages/server/src/service/model_notebook_params.spec.ts @@ -0,0 +1,137 @@ +import { DuckDBConnection } from "@malloydata/db-duckdb"; +import type { Connection } from "@malloydata/malloy"; +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import fs from "fs/promises"; +import path from "path"; +import { Model } from "./model"; + +/** + * Integration test: loading a notebook that uses a parameterized source + * without providing the required parameter values. The publisher should + * load the notebook successfully (deferring the compilation error to + * execution time) rather than failing the entire load. + */ +describe("notebook with parameterized source (integration)", () => { + const tmpDir = path.join( + import.meta.dir, + "__test_notebook_params_" + process.pid, + ); + let connections: Map; + + beforeEach(async () => { + await fs.mkdir(tmpDir, { recursive: true }); + + // Model file: declares a parameterized source with a required param + await fs.writeFile( + path.join(tmpDir, "model.malloy"), + [ + "##! experimental.parameters", + "", + "source: param_source(min_val::number) is duckdb.sql(", + ' """SELECT 1 as id, 100 as value"""', + ") extend {", + " where: value >= min_val", + " measure: row_count is count()", + "}", + ].join("\n"), + ); + + // Notebook file: imports the model and runs a query WITHOUT + // providing the required parameter — this cell should fail to + // compile, but the notebook should still load. + await fs.writeFile( + path.join(tmpDir, "notebook.malloynb"), + [ + ">>>malloy", + 'import "model.malloy"', + "", + ">>>markdown", + "# Test Notebook", + "", + ">>>malloy", + "run: param_source -> { aggregate: row_count }", + ].join("\n"), + ); + + const duckdb = new DuckDBConnection("duckdb", ":memory:", tmpDir); + connections = new Map([["duckdb", duckdb]]); + }); + + afterEach(async () => { + await fs.rm(tmpDir, { recursive: true, force: true }).catch(() => {}); + }); + + it( + "should load the notebook without throwing", + async () => { + const model = await Model.create( + "test-pkg", + tmpDir, + "notebook.malloynb", + connections, + ); + + expect(model).toBeInstanceOf(Model); + expect(model.getType()).toBe("notebook"); + }, + { timeout: 30000 }, + ); + + it( + "should expose notebook cells from the loaded model", + async () => { + const model = await Model.create( + "test-pkg", + tmpDir, + "notebook.malloynb", + connections, + ); + + const notebook = await model.getNotebook(); + expect(notebook.notebookCells).toBeDefined(); + expect(notebook.notebookCells!.length).toBeGreaterThanOrEqual(2); + }, + { timeout: 30000 }, + ); + + it( + "should report an error when executing the parameterized cell without sourceParameters", + async () => { + const model = await Model.create( + "test-pkg", + tmpDir, + "notebook.malloynb", + connections, + ); + + const notebook = await model.getNotebook(); + // Find the code cell that runs the parameterized query + const codeCellIndex = notebook.notebookCells!.findIndex( + (c) => c.type === "code" && c.text.includes("param_source"), + ); + expect(codeCellIndex).toBeGreaterThanOrEqual(0); + + await expect( + model.executeNotebookCell(codeCellIndex), + ).rejects.toThrow(); + }, + { timeout: 30000 }, + ); + + it( + "should also load the standalone model file successfully", + async () => { + const model = await Model.create( + "test-pkg", + tmpDir, + "model.malloy", + connections, + ); + + expect(model).toBeInstanceOf(Model); + expect(model.getType()).toBe("model"); + expect(model.getSourceInfos()).toBeDefined(); + }, + { timeout: 30000 }, + ); +}); From 55c7464a3fa97471311c46562f35eb7e80472e38 Mon Sep 17 00:00:00 2001 From: Josh Sacks Date: Mon, 13 Apr 2026 10:59:00 -0600 Subject: [PATCH 07/11] feat: compile parameterized notebook cells with stub values at load time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of deferring compilation errors for notebook cells that reference parameterized sources without providing values, the loader now injects type-appropriate stub values (e.g. 0 for number, "" for string) during compilation. This validates notebook structure at load time — real errors like typos are caught immediately, while parameter values are injected at execution time. Key changes: - Add stubValueForParam, buildStubParamClause, injectParamClauseIntoText utilities in source_params.ts - Rewrite getNotebookModelMaterializer to process cells sequentially, discovering parameterized sources incrementally and injecting stubs into subsequent cells - Cells compiled with stubs are marked requiresSourceParameters=true; executeNotebookCell enforces real params at execution time - Collect notebook sourceInfos from cell newSources (model-level modelDef.imports doesn't carry forward across extendModel stages) - Fix regex bug in injectParamClauseIntoText (global regex lastIndex advancing after test() caused replace() to miss) Made-with: Cursor --- CLAUDE.md | 5 + packages/server/src/service/model.spec.ts | 32 ++- packages/server/src/service/model.ts | 213 +++++++++++------- .../src/service/model_notebook_params.spec.ts | 76 ++++++- .../server/src/utils/source_params.spec.ts | 132 +++++++++++ packages/server/src/utils/source_params.ts | 66 ++++++ 6 files changed, 424 insertions(+), 100 deletions(-) create mode 100644 CLAUDE.md diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 00000000..ac2029a6 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,5 @@ +# Repository Notes + +## DCO (Developer Certificate of Origin) + +This repository requires DCO sign-off on all commits. Do **not** create commits unless you can sign them (i.e. use `git commit -s` or `--signoff`). Unsigned commits will be rejected. diff --git a/packages/server/src/service/model.spec.ts b/packages/server/src/service/model.spec.ts index c59a9856..bfb0595e 100644 --- a/packages/server/src/service/model.spec.ts +++ b/packages/server/src/service/model.spec.ts @@ -634,7 +634,7 @@ describe("service/model", () => { sinon.restore(); }); - it("should suggest providing sourceParameters when cell has compilation error", async () => { + it("should throw compilation error message when cell has compilation error (not parameter-related)", async () => { const model = new Model( packageName, "test.malloynb", @@ -656,7 +656,35 @@ describe("service/model", () => { ); await expect(model.executeNotebookCell(0)).rejects.toThrow( - /provide sourceParameters/, + /failed to compile.*some error/, + ); + + sinon.restore(); + }); + + it("should require sourceParameters when cell was compiled with stubs", async () => { + const model = new Model( + packageName, + "test.malloynb", + {}, + "notebook", + undefined, + undefined, + undefined, + undefined, + undefined, + [ + { + type: "code", + text: "run: flights -> { }", + requiresSourceParameters: true, + }, + ], + undefined, + ); + + await expect(model.executeNotebookCell(0)).rejects.toThrow( + /parameterized source/, ); sinon.restore(); diff --git a/packages/server/src/service/model.ts b/packages/server/src/service/model.ts index 7e668bd5..8af5c4e8 100644 --- a/packages/server/src/service/model.ts +++ b/packages/server/src/service/model.ts @@ -44,7 +44,9 @@ import { logger } from "../logger"; import { URL_READER } from "../utils"; import { buildMalloyParamClause, + buildStubParamClause, getSourceParams, + injectParamClauseIntoText, validateRequiredParams, } from "../utils/source_params"; @@ -74,6 +76,9 @@ interface RunnableNotebookCell { /** MM from *before* this cell — used to recompile with params */ priorModelMaterializer?: ModelMaterializer; cellCompilationError?: Error; + /** True when stub values were injected during compilation — execution + * must supply real sourceParameters. */ + requiresSourceParameters?: boolean; } export class Model { @@ -228,10 +233,13 @@ export class Model { } } - // For notebooks without a modelDef (model-level compilation - // failed), collect sourceInfos from successfully compiled cells. - if (!modelDef && runnableNotebookCells) { - const seenNames = new Set(); + // For notebooks, always collect sourceInfos from compiled cells. + // The model-level modelDef.imports doesn't carry forward + // imports from earlier extendModel stages, so the import-based + // collection above may miss sources. Cell newSources are the + // authoritative source of truth for notebooks. + if (runnableNotebookCells) { + const seenNames = new Set(sourceInfos.map((s) => s.name)); for (const cell of runnableNotebookCells) { if (cell.newSources) { for (const source of cell.newSources) { @@ -585,16 +593,24 @@ export class Model { }; } - // If the cell failed initial compilation (e.g. missing required source - // params) and sourceParameters were provided, try to recompile with - // the params injected into the cell text. + // Cell was compiled with stub values — real params must be provided. + if (cell.requiresSourceParameters) { + if (sourceParameters && Object.keys(sourceParameters).length > 0) { + return this.executeNotebookCellWithParams(cell, sourceParameters); + } + throw new BadRequestError( + `Cell ${cellIndex} uses a parameterized source. ` + + `Provide sourceParameters to execute it.`, + ); + } + + // Cell failed compilation for a non-parameter reason. if (cell.cellCompilationError) { if (sourceParameters && Object.keys(sourceParameters).length > 0) { return this.executeNotebookCellWithParams(cell, sourceParameters); } throw new BadRequestError( - `Cell ${cellIndex} failed to compile: ${cell.cellCompilationError.message}. ` + - `If this cell uses a parameterized source, provide sourceParameters.`, + `Cell ${cellIndex} failed to compile: ${cell.cellCompilationError.message}`, ); } @@ -670,27 +686,28 @@ export class Model { ); } - // Inject parameter values into source invocations in the cell text. - // For each known parameterized source, replace bare source references - // in `run:` statements with parameterized invocations. + // Build real parameter clauses for each parameterized source and + // inject them into the cell text. const allSourceInfos = this.sourceInfos ?? []; - const sourcesByName = new Map(allSourceInfos.map((s) => [s.name, s])); - - let modifiedText = cell.text; - for (const [sourceName, sourceInfo] of sourcesByName) { - if (!sourceInfo.parameters || sourceInfo.parameters.length === 0) - continue; - - const paramClause = buildMalloyParamClause( + const realClauses = new Map(); + for (const source of allSourceInfos) { + if (!source.parameters || source.parameters.length === 0) continue; + const clause = buildMalloyParamClause( sourceParameters, - sourceInfo.parameters, + source.parameters, ); - if (!paramClause) continue; + if (clause) realClauses.set(source.name, clause); + } + + let { text: modifiedText } = injectParamClauseIntoText( + cell.text, + realClauses, + ); - // Match `run: sourceName ->` or `run: sourceName` at end of line, - // and inject the param clause after the source name. - const pattern = new RegExp(`(run:\\s+${sourceName})\\s*(->|$)`, "gm"); - modifiedText = modifiedText.replace(pattern, `$1${paramClause} $2`); + // The (param is value) invocation syntax requires the + // experimental.parameters flag in the compiling context. + if (modifiedText !== cell.text) { + modifiedText = "##! experimental.parameters\n" + modifiedText; } // Rebuild: extend the prior model (before this cell) with modified text @@ -926,76 +943,100 @@ export class Model { throw new Error("Could not parse model: " + modelPath); } + // Process cells sequentially so we can discover parameterized + // sources from earlier cells and inject stub values into later + // cells that reference them. This validates the notebook's + // structure at load time without requiring real parameter values. let mm: ModelMaterializer | undefined = undefined; const oldImports: string[] = []; const oldSources: Record = {}; - // First generate the sequence of ModelMaterializers (lazy — no - // compilation happens here). Track the MM *before* each statement - // so failed cells can be recompiled with different text later. - const priorMMs: (ModelMaterializer | undefined)[] = []; - const mms = parse.statements.map((stmt) => { - priorMMs.push(mm); - if (stmt.type === MalloySQLStatementType.MALLOY) { - if (!mm) { - mm = runtime.loadModel(stmt.text, { importBaseURL }); - } else { - mm = mm.extendModel(stmt.text, { importBaseURL }); - } + + // source name → stub param clause for sources with required params + const stubClauses = new Map(); + const runnableNotebookCells: RunnableNotebookCell[] = []; + + for (let index = 0; index < parse.statements.length; index++) { + const stmt = parse.statements[index]; + + if (stmt.type === MalloySQLStatementType.MARKDOWN) { + runnableNotebookCells.push({ + type: "markdown", + text: stmt.text, + }); + continue; } - return mm; - }); - const runnableNotebookCells: RunnableNotebookCell[] = ( - await Promise.all( - parse.statements.map(async (stmt, index) => { - if (stmt.type === MalloySQLStatementType.MALLOY) { - // Get the Materializer for the current cell/statement. - const localMM = mms[index]; - if (!localMM) { - throw new Error("Model materializer is undefined"); - } - // Compilation is wrapped per-cell so that a single cell - // with missing source parameters (or other errors) does not - // prevent the rest of the notebook from loading. - try { - return await Model.compileNotebookCell( - stmt.text, - localMM, - runtime, - importBaseURL, - oldImports, - oldSources, - ); - } catch (cellError) { - logger.warn( - `Notebook cell ${index} compilation failed, deferring to execution time`, - { error: cellError }, - ); - return { - type: "code", - text: stmt.text, - priorModelMaterializer: priorMMs[index], - cellCompilationError: - cellError instanceof Error - ? cellError - : new Error(String(cellError)), - } as RunnableNotebookCell; + if (stmt.type !== MalloySQLStatementType.MALLOY) continue; + + // Check if this cell references known parameterized sources + // and inject stub values for required parameters. + const injected = injectParamClauseIntoText(stmt.text, stubClauses); + const needsRealParams = injected.modified; + + // The (param is value) invocation syntax requires the + // experimental.parameters flag in the compiling context. + const compilationText = needsRealParams + ? "##! experimental.parameters\n" + injected.text + : injected.text; + + const priorMM = mm; + + // Build materializer with (possibly stub-injected) text + if (!mm) { + mm = runtime.loadModel(compilationText, { importBaseURL }); + } else { + mm = mm.extendModel(compilationText, { importBaseURL }); + } + + try { + const cell = await Model.compileNotebookCell( + stmt.text, + mm, + runtime, + importBaseURL, + oldImports, + oldSources, + ); + + if (needsRealParams) { + cell.requiresSourceParameters = true; + cell.priorModelMaterializer = priorMM; + } + + runnableNotebookCells.push(cell); + + // Discover newly compiled parameterized sources so we can + // inject stubs in subsequent cells that reference them. + if (cell.newSources) { + for (const source of cell.newSources) { + if (source.parameters && source.parameters.length > 0) { + const clause = buildStubParamClause(source.parameters); + if (clause) { + stubClauses.set(source.name, clause); + } } - } else if (stmt.type === MalloySQLStatementType.MARKDOWN) { - return { - type: "markdown", - text: stmt.text, - } as RunnableNotebookCell; - } else { - return undefined; } - }), - ) - ).filter((cell) => cell !== undefined); + } + } catch (cellError) { + logger.warn( + `Notebook cell ${index} compilation failed, deferring to execution time`, + { error: cellError }, + ); + runnableNotebookCells.push({ + type: "code", + text: stmt.text, + priorModelMaterializer: priorMM, + cellCompilationError: + cellError instanceof Error + ? cellError + : new Error(String(cellError)), + }); + } + } return { modelMaterializer: mm, - runnableNotebookCells: runnableNotebookCells, + runnableNotebookCells, }; } diff --git a/packages/server/src/service/model_notebook_params.spec.ts b/packages/server/src/service/model_notebook_params.spec.ts index 47c75f2c..87541365 100644 --- a/packages/server/src/service/model_notebook_params.spec.ts +++ b/packages/server/src/service/model_notebook_params.spec.ts @@ -8,8 +8,8 @@ import { Model } from "./model"; /** * Integration test: loading a notebook that uses a parameterized source * without providing the required parameter values. The publisher should - * load the notebook successfully (deferring the compilation error to - * execution time) rather than failing the entire load. + * compile the notebook with stub values at load time (validating + * structure) and require real sourceParameters at execution time. */ describe("notebook with parameterized source (integration)", () => { const tmpDir = path.join( @@ -21,7 +21,6 @@ describe("notebook with parameterized source (integration)", () => { beforeEach(async () => { await fs.mkdir(tmpDir, { recursive: true }); - // Model file: declares a parameterized source with a required param await fs.writeFile( path.join(tmpDir, "model.malloy"), [ @@ -36,9 +35,6 @@ describe("notebook with parameterized source (integration)", () => { ].join("\n"), ); - // Notebook file: imports the model and runs a query WITHOUT - // providing the required parameter — this cell should fail to - // compile, but the notebook should still load. await fs.writeFile( path.join(tmpDir, "notebook.malloynb"), [ @@ -78,7 +74,7 @@ describe("notebook with parameterized source (integration)", () => { ); it( - "should expose notebook cells from the loaded model", + "should expose notebook cells with queryInfo from stub compilation", async () => { const model = await Model.create( "test-pkg", @@ -90,12 +86,44 @@ describe("notebook with parameterized source (integration)", () => { const notebook = await model.getNotebook(); expect(notebook.notebookCells).toBeDefined(); expect(notebook.notebookCells!.length).toBeGreaterThanOrEqual(2); + + // The parameterized cell should have queryInfo from the + // stub-compiled query — proves structural validation worked. + const paramCell = notebook.notebookCells!.find( + (c) => c.type === "code" && c.text.includes("param_source"), + ); + expect(paramCell).toBeDefined(); + expect(paramCell!.queryInfo).toBeDefined(); }, { timeout: 30000 }, ); it( - "should report an error when executing the parameterized cell without sourceParameters", + "should expose sourceInfos with parameter metadata", + async () => { + const model = await Model.create( + "test-pkg", + tmpDir, + "notebook.malloynb", + connections, + ); + + const sourceInfos = model.getSourceInfos(); + expect(sourceInfos).toBeDefined(); + + const paramSource = sourceInfos!.find( + (s) => s.name === "param_source", + ); + expect(paramSource).toBeDefined(); + expect(paramSource!.parameters).toBeDefined(); + expect(paramSource!.parameters!.length).toBe(1); + expect(paramSource!.parameters![0].name).toBe("min_val"); + }, + { timeout: 30000 }, + ); + + it( + "should throw when executing the parameterized cell without sourceParameters", async () => { const model = await Model.create( "test-pkg", @@ -105,15 +133,39 @@ describe("notebook with parameterized source (integration)", () => { ); const notebook = await model.getNotebook(); - // Find the code cell that runs the parameterized query const codeCellIndex = notebook.notebookCells!.findIndex( (c) => c.type === "code" && c.text.includes("param_source"), ); expect(codeCellIndex).toBeGreaterThanOrEqual(0); - await expect( - model.executeNotebookCell(codeCellIndex), - ).rejects.toThrow(); + await expect(model.executeNotebookCell(codeCellIndex)).rejects.toThrow( + /parameterized source/, + ); + }, + { timeout: 30000 }, + ); + + it( + "should execute the parameterized cell when sourceParameters are provided", + async () => { + const model = await Model.create( + "test-pkg", + tmpDir, + "notebook.malloynb", + connections, + ); + + const notebook = await model.getNotebook(); + const codeCellIndex = notebook.notebookCells!.findIndex( + (c) => c.type === "code" && c.text.includes("param_source"), + ); + + const result = await model.executeNotebookCell(codeCellIndex, { + min_val: 50, + }); + + expect(result.type).toBe("code"); + expect(result.result).toBeDefined(); }, { timeout: 30000 }, ); diff --git a/packages/server/src/utils/source_params.spec.ts b/packages/server/src/utils/source_params.spec.ts index d0d2c24b..64675161 100644 --- a/packages/server/src/utils/source_params.spec.ts +++ b/packages/server/src/utils/source_params.spec.ts @@ -3,8 +3,11 @@ import { describe, expect, it } from "bun:test"; import { BadRequestError } from "../errors"; import { buildMalloyParamClause, + buildStubParamClause, getSourceParams, + injectParamClauseIntoText, paramValueToMalloyLiteral, + stubValueForParam, validateRequiredParams, } from "./source_params"; @@ -352,3 +355,132 @@ describe("getSourceParams", () => { expect(getSourceParams(sources, undefined)).toEqual([]); }); }); + +// --------------------------------------------------------------------------- +// stubValueForParam +// --------------------------------------------------------------------------- + +describe("stubValueForParam", () => { + it("should return 0 for number_type", () => { + expect(stubValueForParam(makeNumberParam("x"))).toBe("0"); + }); + + it("should return true for boolean_type", () => { + expect(stubValueForParam(makeBooleanParam("x"))).toBe("true"); + }); + + it('should return "" for string_type', () => { + expect(stubValueForParam(makeStringParam("x"))).toBe('""'); + }); + + it("should return @2000-01-01 for date_type", () => { + expect(stubValueForParam(makeParam("x", "date_type"))).toBe( + "@2000-01-01", + ); + }); + + it("should return @2000-01-01 00:00:00 for timestamp_type", () => { + expect(stubValueForParam(makeParam("x", "timestamp_type"))).toBe( + "@2000-01-01 00:00:00", + ); + }); + + it("should return f'true' for filter_expression_type", () => { + expect(stubValueForParam(makeParam("x", "filter_expression_type"))).toBe( + "f'true'", + ); + }); +}); + +// --------------------------------------------------------------------------- +// buildStubParamClause +// --------------------------------------------------------------------------- + +describe("buildStubParamClause", () => { + it("should generate stubs only for required params (no default)", () => { + const declared = [ + makeNumberParam("limit"), + makeStringParam("label", "default_label"), + ]; + expect(buildStubParamClause(declared)).toBe("(limit is 0)"); + }); + + it("should return empty string when all params have defaults", () => { + const declared = [ + makeNumberParam("limit", 10), + makeStringParam("label", "x"), + ]; + expect(buildStubParamClause(declared)).toBe(""); + }); + + it("should generate stubs for multiple required params", () => { + const declared = [ + makeNumberParam("a"), + makeBooleanParam("b"), + makeStringParam("c"), + ]; + expect(buildStubParamClause(declared)).toBe( + '(a is 0, b is true, c is "")', + ); + }); + + it("should return empty string for empty param list", () => { + expect(buildStubParamClause([])).toBe(""); + }); +}); + +// --------------------------------------------------------------------------- +// injectParamClauseIntoText +// --------------------------------------------------------------------------- + +describe("injectParamClauseIntoText", () => { + it("should inject clause into run: source ->", () => { + const clauses = new Map([["flights", "(carrier is 42)"]]); + const result = injectParamClauseIntoText( + "run: flights -> { aggregate: c is count() }", + clauses, + ); + expect(result.modified).toBe(true); + expect(result.text).toBe( + "run: flights(carrier is 42) -> { aggregate: c is count() }", + ); + }); + + it("should not modify text when source is not referenced", () => { + const clauses = new Map([["airports", "(code is 0)"]]); + const result = injectParamClauseIntoText( + "run: flights -> { aggregate: c is count() }", + clauses, + ); + expect(result.modified).toBe(false); + expect(result.text).toBe("run: flights -> { aggregate: c is count() }"); + }); + + it("should handle run: source at end of line (no ->)", () => { + const clauses = new Map([["flights", "(x is 1)"]]); + const result = injectParamClauseIntoText("run: flights", clauses); + expect(result.modified).toBe(true); + expect(result.text).toBe("run: flights(x is 1) "); + }); + + it("should handle multiple sources in one text", () => { + const clauses = new Map([ + ["src_a", "(p is 1)"], + ["src_b", '(q is "x")'], + ]); + const text = "run: src_a -> { select: * }\nrun: src_b -> { select: * }"; + const result = injectParamClauseIntoText(text, clauses); + expect(result.modified).toBe(true); + expect(result.text).toContain("src_a(p is 1)"); + expect(result.text).toContain('src_b(q is "x")'); + }); + + it("should skip empty clauses", () => { + const clauses = new Map([["flights", ""]]); + const result = injectParamClauseIntoText( + "run: flights -> { select: * }", + clauses, + ); + expect(result.modified).toBe(false); + }); +}); diff --git a/packages/server/src/utils/source_params.ts b/packages/server/src/utils/source_params.ts index 6fab659a..efac3762 100644 --- a/packages/server/src/utils/source_params.ts +++ b/packages/server/src/utils/source_params.ts @@ -105,6 +105,72 @@ export function buildMalloyParamClause( return `(${assignments.join(", ")})`; } +/** + * Returns a Malloy literal stub value for a parameter based on its type. + * Used during notebook loading to fill in required parameters so the + * notebook compiles (validating structure) without real values. + */ +export function stubValueForParam(paramInfo: Malloy.ParameterInfo): string { + switch (paramInfo.type.kind) { + case "number_type": + return "0"; + case "boolean_type": + return "true"; + case "date_type": + return "@2000-01-01"; + case "timestamp_type": + case "timestamptz_type": + return "@2000-01-01 00:00:00"; + case "filter_expression_type": + return "f'true'"; + case "string_type": + default: + return '""'; + } +} + +/** + * Builds a Malloy parameter clause containing stub values for every + * *required* parameter (those without a default value). Returns an + * empty string when no stubs are needed. + */ +export function buildStubParamClause( + declaredParams: Malloy.ParameterInfo[], +): string { + const assignments: string[] = []; + for (const param of declaredParams) { + if (!param.default_value) { + assignments.push(`${param.name} is ${stubValueForParam(param)}`); + } + } + if (assignments.length === 0) return ""; + return `(${assignments.join(", ")})`; +} + +/** + * Scans cell text for `run: ` patterns that reference known + * parameterized sources and injects the given parameter clause after the + * source name. Returns the modified text and whether any injection + * occurred. + */ +export function injectParamClauseIntoText( + cellText: string, + parameterizedSources: Map, +): { text: string; modified: boolean } { + let text = cellText; + let modified = false; + for (const [sourceName, clause] of parameterizedSources) { + if (!clause) continue; + const pattern = new RegExp(`(run:\\s+${sourceName})\\s*(->|$)`, "gm"); + const replaced = text.replace(pattern, `$1${clause} $2`); + if (replaced !== text) { + text = replaced; + modified = true; + } + } + return { text, modified }; +} + /** * Finds the ParameterInfo array for a source by name. Returns an empty * array when the source has no parameters or is not found. From a4ccf421fe9296c8b32081deb5cf031728912e60 Mon Sep 17 00:00:00 2001 From: Josh Sacks Date: Mon, 13 Apr 2026 11:07:26 -0600 Subject: [PATCH 08/11] fix: resolve typecheck errors in notebook params test - Replace import.meta.dir with __dirname (CommonJS module mode) - Add optional chaining for c.text?.includes (text may be undefined) - Fix prettier formatting Made-with: Cursor --- .../server/src/service/model_notebook_params.spec.ts | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/packages/server/src/service/model_notebook_params.spec.ts b/packages/server/src/service/model_notebook_params.spec.ts index 87541365..cbc09b16 100644 --- a/packages/server/src/service/model_notebook_params.spec.ts +++ b/packages/server/src/service/model_notebook_params.spec.ts @@ -12,10 +12,7 @@ import { Model } from "./model"; * structure) and require real sourceParameters at execution time. */ describe("notebook with parameterized source (integration)", () => { - const tmpDir = path.join( - import.meta.dir, - "__test_notebook_params_" + process.pid, - ); + const tmpDir = path.join(__dirname, "__test_notebook_params_" + process.pid); let connections: Map; beforeEach(async () => { @@ -90,7 +87,7 @@ describe("notebook with parameterized source (integration)", () => { // The parameterized cell should have queryInfo from the // stub-compiled query — proves structural validation worked. const paramCell = notebook.notebookCells!.find( - (c) => c.type === "code" && c.text.includes("param_source"), + (c) => c.type === "code" && c.text?.includes("param_source"), ); expect(paramCell).toBeDefined(); expect(paramCell!.queryInfo).toBeDefined(); @@ -134,7 +131,7 @@ describe("notebook with parameterized source (integration)", () => { const notebook = await model.getNotebook(); const codeCellIndex = notebook.notebookCells!.findIndex( - (c) => c.type === "code" && c.text.includes("param_source"), + (c) => c.type === "code" && c.text?.includes("param_source"), ); expect(codeCellIndex).toBeGreaterThanOrEqual(0); @@ -157,7 +154,7 @@ describe("notebook with parameterized source (integration)", () => { const notebook = await model.getNotebook(); const codeCellIndex = notebook.notebookCells!.findIndex( - (c) => c.type === "code" && c.text.includes("param_source"), + (c) => c.type === "code" && c.text?.includes("param_source"), ); const result = await model.executeNotebookCell(codeCellIndex, { From ce347c7eff16419cba1abbdc9940d9ed97cef688 Mon Sep 17 00:00:00 2001 From: Josh Sacks Date: Mon, 13 Apr 2026 11:25:59 -0600 Subject: [PATCH 09/11] fix: inject source parameters into raw query path too The raw query execution path (query without sourceName/queryName) was validating sourceParameters but never injecting them into the query text. Now uses injectParamClauseIntoText to scan for `run: source ->` patterns and inject params, same as the notebook path. Also adds ##! experimental.parameters flag to the named-query path when params are present. Made-with: Cursor --- packages/server/src/service/model.spec.ts | 6 ++--- packages/server/src/service/model.ts | 31 +++++++++++++++++++++-- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/packages/server/src/service/model.spec.ts b/packages/server/src/service/model.spec.ts index bfb0595e..f9723c6f 100644 --- a/packages/server/src/service/model.spec.ts +++ b/packages/server/src/service/model.spec.ts @@ -415,7 +415,7 @@ describe("service/model", () => { expect(loadQueryStub.calledOnce).toBe(true); expect(loadQueryStub.firstCall.args[0]).toBe( - '\nrun: flights(carrier is "AA")->by_carrier', + '##! experimental.parameters\n\nrun: flights(carrier is "AA")->by_carrier', ); sinon.restore(); }); @@ -514,7 +514,7 @@ describe("service/model", () => { } expect(loadQueryStub.firstCall.args[0]).toBe( - "\nrun: flights(min_distance is 500)->query1", + "##! experimental.parameters\n\nrun: flights(min_distance is 500)->query1", ); sinon.restore(); }); @@ -546,7 +546,7 @@ describe("service/model", () => { } expect(loadQueryStub.firstCall.args[0]).toBe( - "\nrun: flights(active is true)->query1", + "##! experimental.parameters\n\nrun: flights(active is true)->query1", ); sinon.restore(); }); diff --git a/packages/server/src/service/model.ts b/packages/server/src/service/model.ts index 8af5c4e8..c3d81fd7 100644 --- a/packages/server/src/service/model.ts +++ b/packages/server/src/service/model.ts @@ -388,10 +388,37 @@ export class Model { // Wrap loadQuery calls in try-catch to handle query parsing errors try { if (!sourceName && !queryName && query) { - runnable = this.modelMaterializer.loadQuery("\n" + query); + // For raw queries, inject source parameters into any + // `run: source ->` references found in the query text. + let queryText = query; + if (Object.keys(params).length > 0 && this.sourceInfos) { + const clauses = new Map(); + for (const source of this.sourceInfos) { + if (!source.parameters || source.parameters.length === 0) + continue; + const clause = buildMalloyParamClause( + params, + source.parameters, + ); + if (clause) clauses.set(source.name, clause); + } + if (clauses.size > 0) { + const injected = injectParamClauseIntoText( + queryText, + clauses, + ); + if (injected.modified) { + queryText = + "##! experimental.parameters\n" + injected.text; + } + } + } + runnable = this.modelMaterializer.loadQuery("\n" + queryText); } else if (queryName && !query) { + const prefix = paramClause ? "##! experimental.parameters\n" : ""; + const arrow = sourceName ? "->" : ""; runnable = this.modelMaterializer.loadQuery( - `\nrun: ${sourceName ? sourceName + paramClause + "->" : ""}${queryName}`, + `${prefix}\nrun: ${sourceName ? sourceName + paramClause : ""}${arrow}${queryName}`, ); } else { const endTime = performance.now(); From 636fdc5b142a7e447d8d874e9214400e326affab Mon Sep 17 00:00:00 2001 From: Josh Sacks Date: Mon, 13 Apr 2026 11:27:52 -0600 Subject: [PATCH 10/11] test: add integration tests for getQueryResults with source parameters Tests use a parameterized source over inline DuckDB data (4 rows with values 10, 50, 100, 200) filtered by min_val. Verifies both named query and ad-hoc query paths return correct row counts and aggregates for different parameter values, and that missing required params throw. Made-with: Cursor --- .../src/service/model_notebook_params.spec.ts | 226 ++++++++++++++---- 1 file changed, 181 insertions(+), 45 deletions(-) diff --git a/packages/server/src/service/model_notebook_params.spec.ts b/packages/server/src/service/model_notebook_params.spec.ts index cbc09b16..a2a6f62a 100644 --- a/packages/server/src/service/model_notebook_params.spec.ts +++ b/packages/server/src/service/model_notebook_params.spec.ts @@ -5,49 +5,58 @@ import fs from "fs/promises"; import path from "path"; import { Model } from "./model"; -/** - * Integration test: loading a notebook that uses a parameterized source - * without providing the required parameter values. The publisher should - * compile the notebook with stub values at load time (validating - * structure) and require real sourceParameters at execution time. - */ +// Shared fixture: a model with a parameterized source over inline data. +// The source filters rows where value >= min_val, so the number of +// matching rows depends on the parameter value passed at query time. +const MALLOY_MODEL = [ + "##! experimental.parameters", + "", + "source: items(min_val::number) is duckdb.sql(", + ' """SELECT * FROM (VALUES (1,10),(2,50),(3,100),(4,200)) AS t(id,value)"""', + ") extend {", + " where: value >= min_val", + " measure: row_count is count()", + " measure: total_value is sum(value)", + " view: summary is { aggregate: row_count, total_value }", + "}", +].join("\n"); + +const MALLOY_NOTEBOOK = [ + ">>>malloy", + 'import "model.malloy"', + "", + ">>>markdown", + "# Test Notebook", + "", + ">>>malloy", + "run: items -> { aggregate: row_count }", +].join("\n"); + +function makeTmpDir() { + return path.join( + __dirname, + "__test_params_" + process.pid + "_" + Date.now(), + ); +} + +async function setupFixture(tmpDir: string) { + await fs.mkdir(tmpDir, { recursive: true }); + await fs.writeFile(path.join(tmpDir, "model.malloy"), MALLOY_MODEL); + await fs.writeFile(path.join(tmpDir, "notebook.malloynb"), MALLOY_NOTEBOOK); + const duckdb = new DuckDBConnection("duckdb", ":memory:", tmpDir); + return new Map([["duckdb", duckdb]]); +} + +// ----------------------------------------------------------------------- +// Notebook loading with parameterized sources +// ----------------------------------------------------------------------- + describe("notebook with parameterized source (integration)", () => { - const tmpDir = path.join(__dirname, "__test_notebook_params_" + process.pid); + const tmpDir = makeTmpDir(); let connections: Map; beforeEach(async () => { - await fs.mkdir(tmpDir, { recursive: true }); - - await fs.writeFile( - path.join(tmpDir, "model.malloy"), - [ - "##! experimental.parameters", - "", - "source: param_source(min_val::number) is duckdb.sql(", - ' """SELECT 1 as id, 100 as value"""', - ") extend {", - " where: value >= min_val", - " measure: row_count is count()", - "}", - ].join("\n"), - ); - - await fs.writeFile( - path.join(tmpDir, "notebook.malloynb"), - [ - ">>>malloy", - 'import "model.malloy"', - "", - ">>>markdown", - "# Test Notebook", - "", - ">>>malloy", - "run: param_source -> { aggregate: row_count }", - ].join("\n"), - ); - - const duckdb = new DuckDBConnection("duckdb", ":memory:", tmpDir); - connections = new Map([["duckdb", duckdb]]); + connections = await setupFixture(tmpDir); }); afterEach(async () => { @@ -87,7 +96,7 @@ describe("notebook with parameterized source (integration)", () => { // The parameterized cell should have queryInfo from the // stub-compiled query — proves structural validation worked. const paramCell = notebook.notebookCells!.find( - (c) => c.type === "code" && c.text?.includes("param_source"), + (c) => c.type === "code" && c.text?.includes("items"), ); expect(paramCell).toBeDefined(); expect(paramCell!.queryInfo).toBeDefined(); @@ -108,9 +117,7 @@ describe("notebook with parameterized source (integration)", () => { const sourceInfos = model.getSourceInfos(); expect(sourceInfos).toBeDefined(); - const paramSource = sourceInfos!.find( - (s) => s.name === "param_source", - ); + const paramSource = sourceInfos!.find((s) => s.name === "items"); expect(paramSource).toBeDefined(); expect(paramSource!.parameters).toBeDefined(); expect(paramSource!.parameters!.length).toBe(1); @@ -131,7 +138,7 @@ describe("notebook with parameterized source (integration)", () => { const notebook = await model.getNotebook(); const codeCellIndex = notebook.notebookCells!.findIndex( - (c) => c.type === "code" && c.text?.includes("param_source"), + (c) => c.type === "code" && c.text?.includes("items"), ); expect(codeCellIndex).toBeGreaterThanOrEqual(0); @@ -154,7 +161,7 @@ describe("notebook with parameterized source (integration)", () => { const notebook = await model.getNotebook(); const codeCellIndex = notebook.notebookCells!.findIndex( - (c) => c.type === "code" && c.text?.includes("param_source"), + (c) => c.type === "code" && c.text?.includes("items"), ); const result = await model.executeNotebookCell(codeCellIndex, { @@ -184,3 +191,132 @@ describe("notebook with parameterized source (integration)", () => { { timeout: 30000 }, ); }); + +// ----------------------------------------------------------------------- +// getQueryResults with source parameters (integration) +// ----------------------------------------------------------------------- + +describe("getQueryResults with source parameters (integration)", () => { + const tmpDir = makeTmpDir(); + let connections: Map; + let model: Model; + + beforeEach(async () => { + connections = await setupFixture(tmpDir); + model = await Model.create( + "test-pkg", + tmpDir, + "model.malloy", + connections, + ); + }); + + afterEach(async () => { + await fs.rm(tmpDir, { recursive: true, force: true }).catch(() => {}); + }); + + // -- Named query (sourceName + queryName) path ------------------------- + + it( + "named query: min_val=0 returns all 4 rows", + async () => { + const { compactResult } = await model.getQueryResults( + "items", + "summary", + undefined, + { min_val: 0 }, + ); + expect(compactResult).toHaveLength(1); + expect(compactResult[0]["row_count"]).toBe(4); + expect(compactResult[0]["total_value"]).toBe(360); + }, + { timeout: 30000 }, + ); + + it( + "named query: min_val=100 returns only rows with value >= 100", + async () => { + const { compactResult } = await model.getQueryResults( + "items", + "summary", + undefined, + { min_val: 100 }, + ); + expect(compactResult).toHaveLength(1); + expect(compactResult[0]["row_count"]).toBe(2); + expect(compactResult[0]["total_value"]).toBe(300); + }, + { timeout: 30000 }, + ); + + it( + "named query: min_val=999 returns 0 rows", + async () => { + const { compactResult } = await model.getQueryResults( + "items", + "summary", + undefined, + { min_val: 999 }, + ); + expect(compactResult).toHaveLength(1); + expect(compactResult[0]["row_count"]).toBe(0); + }, + { timeout: 30000 }, + ); + + // -- Ad-hoc query (raw query string) path ------------------------------ + + it( + "ad-hoc query: min_val=50 filters correctly", + async () => { + const { compactResult } = await model.getQueryResults( + undefined, + undefined, + "run: items -> { aggregate: row_count, total_value }", + { min_val: 50 }, + ); + expect(compactResult).toHaveLength(1); + expect(compactResult[0]["row_count"]).toBe(3); + expect(compactResult[0]["total_value"]).toBe(350); + }, + { timeout: 30000 }, + ); + + it( + "ad-hoc query: min_val=200 returns single matching row", + async () => { + const { compactResult } = await model.getQueryResults( + undefined, + undefined, + "run: items -> { aggregate: row_count, total_value }", + { min_val: 200 }, + ); + expect(compactResult).toHaveLength(1); + expect(compactResult[0]["row_count"]).toBe(1); + expect(compactResult[0]["total_value"]).toBe(200); + }, + { timeout: 30000 }, + ); + + // -- Error cases ------------------------------------------------------- + + it( + "should throw when required param is missing", + async () => { + await expect( + model.getQueryResults("items", "summary", undefined, {}), + ).rejects.toThrow(/required/); + }, + { timeout: 30000 }, + ); + + it( + "should throw when no sourceParameters provided at all", + async () => { + await expect( + model.getQueryResults("items", "summary"), + ).rejects.toThrow(/required/); + }, + { timeout: 30000 }, + ); +}); From ba8655f2cb5ccc829a99d78ab351e4b0348590c7 Mon Sep 17 00:00:00 2001 From: Josh Sacks Date: Mon, 13 Apr 2026 11:34:51 -0600 Subject: [PATCH 11/11] DCO Remediation Commit for Josh Sacks I, Josh Sacks , hereby add my Signed-off-by to this commit: a4818b9839c349c31cb7d0f4031210103db58b1c I, Josh Sacks , hereby add my Signed-off-by to this commit: 55c7464a3fa97471311c46562f35eb7e80472e38 I, Josh Sacks , hereby add my Signed-off-by to this commit: a4ccf421fe9296c8b32081deb5cf031728912e60 I, Josh Sacks , hereby add my Signed-off-by to this commit: ce347c7eff16419cba1abbdc9940d9ed97cef688 I, Josh Sacks , hereby add my Signed-off-by to this commit: 636fdc5b142a7e447d8d874e9214400e326affab Signed-off-by: Josh Sacks --- packages/server/src/utils/source_params.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/server/src/utils/source_params.spec.ts b/packages/server/src/utils/source_params.spec.ts index 64675161..14ead277 100644 --- a/packages/server/src/utils/source_params.spec.ts +++ b/packages/server/src/utils/source_params.spec.ts @@ -73,7 +73,7 @@ function makeBooleanParam( describe("paramValueToMalloyLiteral", () => { it("should convert a string value to a quoted Malloy literal", () => { const param = makeStringParam("s"); - expect(paramValueToMalloyLiteral("hello2", param)).toBe('"hello2"'); + expect(paramValueToMalloyLiteral("hello", param)).toBe('"hello"'); }); it("should escape quotes and backslashes in string values", () => {