Prevent .sqlx files in actions.yaml configs with a clear error#2189
Open
aicayzer wants to merge 1 commit into
Open
Prevent .sqlx files in actions.yaml configs with a clear error#2189aicayzer wants to merge 1 commit into
aicayzer wants to merge 1 commit into
Conversation
Contributor
|
/gcbrun |
kolina
approved these changes
Jun 4, 2026
Comment on lines
+103
to
+106
| const subConfig = (actionConfig as any)[actionType] as | ||
| | { filename?: string } | ||
| | undefined | ||
| | null; |
Contributor
There was a problem hiding this comment.
nit: don't use dynamic casts to any and and just statically list all unsupported types following this pattern
| "notebook" | ||
| ]; | ||
|
|
||
| function actionConfigFilenameIsInvalidSqlx( |
Contributor
There was a problem hiding this comment.
actionConfigIncludesSqlxFiles?
kolina
requested changes
Jun 4, 2026
Contributor
kolina
left a comment
kolina
left a comment
There was a problem hiding this comment.
Sorry, I didn't notice tests are failing with YAML require failed run_core: VMError: Cannot find module './workflow_settings.yaml' before approving.
Can you please rebase on the latest HEAD? I think this error is not related to your changes and was fixed in the latest version
Putting a .sqlx file under the ``filename`` field of an entry in ``definitions/actions.yaml`` used to fall through to ``nativeRequire``, which then failed with a cryptic error far from the source of the mistake. ``actions.yaml`` is for plain ``.sql`` files; ``.sqlx`` files are loaded directly from the ``definitions/`` directory by the sqlx compiler. Add an explicit validation step in ``loadActionConfigs`` that runs before each action is constructed. If the action's ``filename`` ends in ``.sqlx`` (case-insensitive), emit a ``compileError`` naming the action type, the offending filename, and pointing at the fix. The action is then skipped so we don't double-up with a downstream ``nativeRequire`` error. The check covers ``table``, ``view``, ``incrementalTable``, ``assertion``, ``operation``, ``declaration`` and ``notebook``. Data preparations are intentionally excluded because they support ``.dp.sqlx`` files via their own dedicated loader. Closes dataform-co#1785
aicayzer
force-pushed
the
feat/validate-actions-yaml-sqlx
branch
from
52f7337 to
f604e1e
Compare
Author
|
No problem at all @kolina, just rebased onto the latest main. Lmk if it's still something. |
Contributor
|
/gcbrun |
kolina
requested changes
Jun 7, 2026
Contributor
kolina
left a comment
kolina
left a comment
There was a problem hiding this comment.
Hm, your new test is failing like this
Step #1: at /builder/home/.cache/bazel/_bazel_root/eab0d61a99b6696edb3d2aff87b585e8/sandbox/linux-sandbox/127/execroot/df/_tmp/e6baaed8819351a8a5cad263fdd5689f/tmp_dir_48/node_modules/@dataform/core/bundle.js:18:54579
Step #1: at new u (/builder/home/.cache/bazel/_bazel_root/eab0d61a99b6696edb3d2aff87b585e8/sandbox/linux-sandbox/127/execroot/df/_tmp/e6baaed8819351a8a5cad263fdd5689f/tmp_dir_48/node_modules/@dataform/core/bundle.js:18:139849)
Step #1: at u.configureSqlx (/builder/home/.cache/bazel/_bazel_root/eab0d61a99b6696edb3d2aff87b585e8/sandbox/linux-sandbox/127/execroot/df/_tmp/e6baaed8819351a8a5cad263fdd5689f/tmp_dir_48/node_modules/@dataform/core/bundle.js:18:144800)
Step #1: at Object.require [as nativeRequire] (node_modules/vm2/lib/setup-node-sandbox.js:171:10)
Step #1: at requireImpl (node_modules/vm2/lib/setup-node-sandbox.js:90:19)
Step #1: at VM2 Wrapper.apply (node_modules/vm2/lib/bridge.js:1579:11)
Step #1: at resolve (node_modules/vm2/lib/nodevm.js:383:21)
Step #1: at CustomResolver.resolve (node_modules/vm2/lib/resolver.js:121:15)
Step #1: at CustomResolver.resolveFull (node_modules/vm2/lib/resolver.js:316:16)
Step #1: at CustomResolver.resolveFull (node_modules/vm2/lib/resolver.js:126:9)
Step #1: VMError: Cannot find module 'prep.dp.sqlx'
Step #1:
Step #1: FAILED
Step #1: @dataform/core > action configs > .sqlx filenames in actions.yaml are rejected with a clear error > but data preparations still accept .dp.sqlx files
I can't say for sure but I suspect we may need to allowlist it here. I think it wasn't caught because GCP compilation doesn't use vm2 and data preparation is quite unlikely to be used by a lot of CLI users
Author
|
Thanks @kolina. I'll look into it tomorrow. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1785.
Problem
Referencing a
.sqlxfile fromdefinitions/actions.yaml(e.g.) is unsupported, but the existing code path doesn't say so. The
Table/View/etc. constructor reachesnativeRequire(config.filename).query, which fails with aCannot find module 'definitions/my_table.sqlx'(or equivalently obscure) error far from the actual mistake. As Ekrekr noted on the issue, "this results in strange compilation behavior, and can cause cryptic errors."Fix
Validate filenames at the
loadActionConfigsentry point, before the action is constructed. For each action config sub-message that exposes afilenamefield —table,view,incrementalTable,assertion,operation,declaration,notebook— iffilenameends in.sqlx(case-insensitive), emit asession.compileErrornaming the action type, the offending filename, and the remediation (use.sql, or rely on Dataform's automatic.sqlxdiscovery). The action is then skipped so a downstreamnativeRequireerror doesn't fire on top.dataPreparationis intentionally excluded: it supports.dp.sqlxfiles via its own loader (core/actions/data_preparation.ts).Test plan
Added one new test suite to
core/main_test.ts(".sqlx filenames in actions.yaml are rejected with a clear error") covering:filename: table.SQLX).dataPreparationwith a.dp.sqlxfilename does not trigger this validation.tslint --project .passes locally. I wasn't able to runbazel test //core/...locally (a@npm//@bazel/typescript:index.bzlloading error intools/ts_library.bzlagainst bazel 5.4.0 on my machine), so the unit tests will be exercised by CI here.Scope
No proto changes. No CLI wiring. No yargs wrapper edits. Scope mirrors PR #2154 (validation + clear error in
core/, +16/-5).Files touched
core/main.ts— newactionConfigFilenameIsInvalidSqlxhelper called fromloadActionConfigs.core/main_test.ts— new test suite.