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/api-doc.yaml b/api-doc.yaml index 55782be9..b625a832 100644 --- a/api-doc.yaml +++ b/api-doc.yaml @@ -1471,6 +1471,16 @@ paths: required: false schema: $ref: "#/components/schemas/VersionIdPattern" + - name: sourceParameters + in: query + description: >- + JSON-encoded object of source parameter key-value pairs. + Keys must match parameter names declared on sources in the notebook's + 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 responses: "200": description: Cell execution result @@ -1916,6 +1926,16 @@ 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: true + description: | + 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/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..ce7e4afd 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..463d11fc 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..2b168a5d 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.unknown()) + .optional() + .describe( + "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.", + ), }; // 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..7175f607 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, + unknown + >) + : 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..f9723c6f 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,659 @@ 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 silently ignore unknown source parameters", 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, { + nonexistent: "val", + }); + } catch { + // expected — getPreparedResult throws + } + + expect(loadQueryStub.calledOnce).toBe(true); + expect(loadQueryStub.firstCall.args[0]).toBe( + "\nrun: flights->by_carrier", + ); + 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( + '##! experimental.parameters\n\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( + "##! experimental.parameters\n\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( + "##! experimental.parameters\n\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 throw compilation error message when cell has compilation error (not parameter-related)", 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( + /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(); + }); + + 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..c3d81fd7 100644 --- a/packages/server/src/service/model.ts +++ b/packages/server/src/service/model.ts @@ -42,6 +42,13 @@ import { } from "../errors"; import { logger } from "../logger"; import { URL_READER } from "../utils"; +import { + buildMalloyParamClause, + buildStubParamClause, + getSourceParams, + injectParamClauseIntoText, + validateRequiredParams, +} from "../utils/source_params"; type ApiCompiledModel = components["schemas"]["CompiledModel"]; type ApiNotebookCell = components["schemas"]["NotebookCell"]; @@ -66,6 +73,12 @@ interface RunnableNotebookCell { runnable?: QueryMaterializer; newSources?: Malloy.SourceInfo[]; queryInfo?: Malloy.QueryInfo; + /** 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 { @@ -81,6 +94,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 +118,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 +132,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; @@ -145,52 +164,91 @@ 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, 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) { + if (!seenNames.has(source.name)) { + sourceInfos.push(source); + seenNames.add(source.name); + } + } + } } } } @@ -207,6 +265,8 @@ export class Model { sourceInfos.length > 0 ? sourceInfos : undefined, runnableNotebookCells, undefined, + runtime, + importBaseURL, ); } catch (error) { let computedError = error; @@ -292,6 +352,7 @@ export class Model { sourceName?: string, queryName?: string, query?: string, + sourceParameters?: Record, ): Promise<{ result: Malloy.Result; compactResult: QueryData; @@ -316,13 +377,48 @@ 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); + // 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 + "->" : ""}${queryName}`, + `${prefix}\nrun: ${sourceName ? sourceName + paramClause : ""}${arrow}${queryName}`, ); } else { const endTime = performance.now(); @@ -491,7 +587,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 +620,27 @@ export class Model { }; } + // 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}`, + ); + } + // For code cells, execute the runnable if available let queryName: string | undefined = undefined; let queryResult: string | undefined = undefined; @@ -566,6 +686,102 @@ 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", + ); + } + + // Build real parameter clauses for each parameterized source and + // inject them into the cell text. + const allSourceInfos = this.sourceInfos ?? []; + const realClauses = new Map(); + for (const source of allSourceInfos) { + if (!source.parameters || source.parameters.length === 0) continue; + const clause = buildMalloyParamClause( + sourceParameters, + source.parameters, + ); + if (clause) realClauses.set(source.name, clause); + } + + let { text: modifiedText } = injectParamClauseIntoText( + cell.text, + realClauses, + ); + + // 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 + 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, @@ -754,130 +970,189 @@ 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. - // This has to happen sync, since mm.getModel() is async and - // may execute out-of-order. - const mms = parse.statements.map((stmt) => { - 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) { - // 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; - } + if (stmt.type !== MalloySQLStatementType.MALLOY) continue; - const runnable = localMM.loadFinalQuery(); + // 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; - // Extract QueryInfo from the runnable - 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; + // 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); } - } 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 { - type: "code", - text: stmt.text, - runnable: runnable, - newSources, - queryInfo, - } as RunnableNotebookCell; - } 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, }; } + /** + * 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/service/model_notebook_params.spec.ts b/packages/server/src/service/model_notebook_params.spec.ts new file mode 100644 index 00000000..a2a6f62a --- /dev/null +++ b/packages/server/src/service/model_notebook_params.spec.ts @@ -0,0 +1,322 @@ +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"; + +// 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 = makeTmpDir(); + let connections: Map; + + beforeEach(async () => { + connections = await setupFixture(tmpDir); + }); + + 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 with queryInfo from stub compilation", + 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); + + // 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("items"), + ); + expect(paramCell).toBeDefined(); + expect(paramCell!.queryInfo).toBeDefined(); + }, + { timeout: 30000 }, + ); + + it( + "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 === "items"); + 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", + tmpDir, + "notebook.malloynb", + connections, + ); + + const notebook = await model.getNotebook(); + const codeCellIndex = notebook.notebookCells!.findIndex( + (c) => c.type === "code" && c.text?.includes("items"), + ); + expect(codeCellIndex).toBeGreaterThanOrEqual(0); + + 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("items"), + ); + + const result = await model.executeNotebookCell(codeCellIndex, { + min_val: 50, + }); + + expect(result.type).toBe("code"); + expect(result.result).toBeDefined(); + }, + { 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 }, + ); +}); + +// ----------------------------------------------------------------------- +// 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 }, + ); +}); 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..14ead277 --- /dev/null +++ b/packages/server/src/utils/source_params.spec.ts @@ -0,0 +1,486 @@ +import type * as Malloy from "@malloydata/malloy-interfaces"; +import { describe, expect, it } from "bun:test"; +import { BadRequestError } from "../errors"; +import { + buildMalloyParamClause, + buildStubParamClause, + getSourceParams, + injectParamClauseIntoText, + paramValueToMalloyLiteral, + stubValueForParam, + 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/, + ); + }); + + // --- 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( + "@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 silently ignore unknown parameter names", () => { + const declared = [makeStringParam("known")]; + expect(buildMalloyParamClause({ unknown: "val" }, declared)).toBe(""); + }); + + 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 return empty string when no declared params exist", () => { + expect(buildMalloyParamClause({ x: "1" }, [])).toBe(""); + }); + + 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")'); + }); +}); + +// --------------------------------------------------------------------------- +// 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([]); + }); +}); + +// --------------------------------------------------------------------------- +// 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 new file mode 100644 index 00000000..efac3762 --- /dev/null +++ b/packages/server/src/utils/source_params.ts @@ -0,0 +1,185 @@ +import type * as Malloy from "@malloydata/malloy-interfaces"; +import { BadRequestError } from "../errors"; + +/** + * Converts a raw value (string, number, boolean, etc.) into a Malloy + * literal expression based on the declared parameter type. + */ +export function paramValueToMalloyLiteral( + rawValue: unknown, + paramInfo: Malloy.ParameterInfo, +): string { + const kind = paramInfo.type.kind; + const str = String(rawValue); + + switch (kind) { + case "number_type": { + 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 "${str}"`, + ); + } + return str; + } + + case "boolean_type": { + 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 "${str}"`, + ); + } + + case "date_type": + return `@${str}`; + + case "timestamp_type": + case "timestamptz_type": + return `@${str}`; + + case "filter_expression_type": + return `f'${str}'`; + + case "string_type": + default: + return `"${str.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) continue; + const literal = paramValueToMalloyLiteral(rawValue, paramInfo); + assignments.push(`${name} is ${literal}`); + } + + if (assignments.length === 0) return ""; + 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. + */ +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 ?? []; +}