-
Notifications
You must be signed in to change notification settings - Fork 207
refactor(cli): replace deprecated vm2 with native node:vm runner #2310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
4101a6e
d81abda
6c6cd2a
9147749
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,152 @@ | ||
| import { expect } from "chai"; | ||
| import * as fs from "fs-extra"; | ||
| import * as path from "path"; | ||
|
|
||
| import { compile } from "df/cli/vm/compile"; | ||
| import { handleJitRequest } from "df/cli/vm/jit_worker"; | ||
| import { decode64 } from "df/common/protos"; | ||
| import { dataform } from "df/protos/ts"; | ||
| import { suite, test } from "df/testing"; | ||
| import { TmpDirFixture } from "df/testing/fixtures"; | ||
|
|
||
| suite("cli/vm", ({ afterEach }) => { | ||
| const tmpDirFixture = new TmpDirFixture(afterEach); | ||
|
|
||
| // Allow require("@dataform/core") to resolve to the prebuilt core bundle in the test environment | ||
| // tslint:disable-next-line: no-require-imports | ||
| const Module = require("module"); | ||
| const origResolve = Module._resolveFilename; | ||
| Module._resolveFilename = function (request: string, parent: any, isMain: boolean, options: any) { | ||
| if (request === "@dataform/core") { | ||
| return path.join(process.cwd(), "core", "node_modules", "@dataform", "core", "bundle.js"); | ||
| } | ||
| return origResolve.apply(this, arguments); | ||
| }; | ||
|
|
||
| test("compile() runs end-to-end against prebuilt @dataform/core bundle", () => { | ||
| const projectDir = tmpDirFixture.createNewTmpDir(); | ||
|
|
||
| // Copy built @dataform/core from Bazel runfiles into the project's node_modules. | ||
| fs.copySync( | ||
| path.join(process.cwd(), "core", "node_modules"), | ||
| path.join(projectDir, "node_modules"), | ||
| ); | ||
|
|
||
| fs.writeFileSync( | ||
| path.join(projectDir, "workflow_settings.yaml"), | ||
| ` | ||
| defaultProject: test-project | ||
| defaultDataset: test-dataset | ||
| defaultLocation: US | ||
| `, | ||
| ); | ||
|
|
||
| fs.mkdirSync(path.join(projectDir, "definitions")); | ||
| fs.writeFileSync( | ||
| path.join(projectDir, "definitions", "example.sqlx"), | ||
| ` | ||
| config { | ||
| type: "table", | ||
| name: "example" | ||
| } | ||
| SELECT 1 AS col | ||
| `, | ||
| ); | ||
| fs.writeFileSync( | ||
| path.join(projectDir, "definitions", "actions.yaml"), | ||
| ` | ||
| actions: | ||
| - notebook: | ||
| filename: test_notebook.ipynb | ||
| `, | ||
| ); | ||
| fs.writeFileSync( | ||
| path.join(projectDir, "definitions", "test_notebook.ipynb"), | ||
| JSON.stringify({ cells: [] }), | ||
| ); | ||
|
|
||
| const encodedResponse = compile({ projectDir }); | ||
| const response = decode64(dataform.CoreExecutionResponse, encodedResponse); | ||
|
|
||
| expect(response.compile).to.be.an("object"); | ||
| expect(response.compile.compiledGraph).to.be.an("object"); | ||
| const tables = response.compile.compiledGraph.tables; | ||
| expect(tables).to.have.lengthOf(1); | ||
| expect(tables[0].target.name).to.equal("example"); | ||
| expect(tables[0].target.schema).to.equal("test-dataset"); | ||
| expect(tables[0].target.database).to.equal("test-project"); | ||
|
|
||
| const notebooks = response.compile.compiledGraph.notebooks; | ||
| expect(notebooks).to.have.lengthOf(1); | ||
| expect(notebooks[0].target.name).to.equal("test_notebook"); | ||
| }); | ||
|
|
||
| test("handleJitRequest compiles request with local core (hasProjectLocalCore = true)", async () => { | ||
| const projectDir = tmpDirFixture.createNewTmpDir(); | ||
| fs.copySync( | ||
| path.join(process.cwd(), "core", "node_modules"), | ||
| path.join(projectDir, "node_modules"), | ||
| ); | ||
|
|
||
| const request = dataform.JitCompilationRequest.create({ | ||
| jitCode: `async (ctx) => "SELECT 1"`, | ||
| target: { | ||
| database: "db", | ||
| schema: "schema", | ||
| name: "test_op", | ||
| }, | ||
| compilationTargetType: | ||
| dataform.JitCompilationTargetType.JIT_COMPILATION_TARGET_TYPE_OPERATION, | ||
| }); | ||
|
|
||
| const messages: any[] = []; | ||
| const origSend = process.send; | ||
| (process as any).send = (msg: any) => messages.push(msg); | ||
|
|
||
| try { | ||
| await handleJitRequest({ request, projectDir }); | ||
| } finally { | ||
| (process as any).send = origSend; | ||
| } | ||
|
|
||
| expect(messages).to.have.lengthOf(1); | ||
| expect(messages[0].type).to.equal("jit_response"); | ||
| expect(messages[0].response.operation.queries).to.deep.equal(["SELECT 1"]); | ||
| }); | ||
|
|
||
| test("handleJitRequest compiles request with fallback core (hasProjectLocalCore = false)", async () => { | ||
| const projectDir = tmpDirFixture.createNewTmpDir(); | ||
| // Provide only package.json for @dataform/core without bundle.js so hasProjectLocalCore is false | ||
| const coreDir = path.join(projectDir, "node_modules", "@dataform", "core"); | ||
| fs.mkdirSync(coreDir, { recursive: true }); | ||
| fs.writeFileSync( | ||
| path.join(coreDir, "package.json"), | ||
| JSON.stringify({ name: "@dataform/core", version: "3.0.0" }), | ||
| ); | ||
|
|
||
| const request = dataform.JitCompilationRequest.create({ | ||
| jitCode: `async (ctx) => "SELECT 42"`, | ||
| target: { | ||
| database: "db", | ||
| schema: "schema", | ||
| name: "test_op2", | ||
| }, | ||
| compilationTargetType: | ||
| dataform.JitCompilationTargetType.JIT_COMPILATION_TARGET_TYPE_OPERATION, | ||
| }); | ||
|
|
||
| const messages: any[] = []; | ||
| const origSend = process.send; | ||
| (process as any).send = (msg: any) => messages.push(msg); | ||
|
|
||
| try { | ||
| await handleJitRequest({ request, projectDir }); | ||
| } finally { | ||
| (process as any).send = origSend; | ||
| } | ||
|
|
||
| expect(messages).to.have.lengthOf(1); | ||
| expect(messages[0].type).to.equal("jit_response"); | ||
| expect(messages[0].response.operation.queries).to.deep.equal(["SELECT 42"]); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import * as fs from "fs"; | ||
| import * as path from "path"; | ||
| import { NodeVM } from "vm2"; | ||
|
|
||
| import { VmRunner } from "df/common/vm/vm_runner"; | ||
| import { dataform } from "df/protos/ts"; | ||
|
|
||
| const pendingRpcCallbacks = new Map< | ||
|
|
@@ -85,18 +85,15 @@ export async function handleJitRequest(message: { request: any; projectDir: stri | |
|
|
||
| const vmFileName = path.resolve(projectDir, "index.js"); | ||
|
|
||
| const vm = new NodeVM({ | ||
| require: { | ||
| builtin: [], | ||
| context: "sandbox", | ||
| external: { modules: ["@dataform/*"], transitive: false }, | ||
| root: projectDir, | ||
| mock: hasProjectLocalCore | ||
| ? {} | ||
| : { | ||
| "@dataform/core": require("@dataform/core"), | ||
| }, | ||
| }, | ||
| const vm = new VmRunner({ | ||
| projectDir, | ||
| builtinModules: [], | ||
| allowedModules: ["@dataform/*"], | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. vm2 had external: { modules: ["@dataform/"], transitive: false }. The new allowedModules: ["@dataform/"] drops the transitive-blocking semantics and means that if @dataform/core does ANY dynamic require("some-runtime-dep") (not a static bundled import), it'll be rejected. Confirm the core bundle has no dynamic requires at runtime, or widen the allowlist. Add a test covering the hasProjectLocalCore = true path (currently only a unit test of the VmRunner primitive exists - no test wires jit_worker end-to-end).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| mockModules: hasProjectLocalCore | ||
| ? {} | ||
| : { | ||
| "@dataform/core": require("@dataform/core"), | ||
| }, | ||
| sourceExtensions: ["js", "json", "yaml", "yml"], | ||
| }); | ||
|
apilaskowski marked this conversation as resolved.
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this intended that you don't pass these external modules in new version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restored. Added
allowedModulesoption to VmRunner and passed["@dataform/*"]here.