mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-06 00:02:13 +00:00
fix(python): escape reserved-keyword step ids in wrapper codegen (#9460)
A flow inline step whose id is a Python keyword (e.g. `in`) crashed with a
`SyntaxError`: the wrapper emits `from {pkg} import {step_id} as inner_script`,
and `from x import in as y` is invalid Python.
The codegen already prefixes `_` to path segments that start with a digit
(`1234` → `_1234`); this extends that guard to Python hard keywords (`in` →
`_in`) in `compute_python_module_dir` and on the leaf in `compute_py_codegen`
and `prepare_wrapper`. The relative-imports write path inherits it for free.
Fixes #8893
This commit is contained in:
@@ -597,11 +597,57 @@ async fn postinstall(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Python hard keywords cannot be used as a bare name in `import <name>` /
|
||||
/// `from <pkg> import <name>`. A flow inline step whose id (or a folder on its
|
||||
/// path) is such a keyword — e.g. a step id `in` — otherwise generates
|
||||
/// `from pkg import in as inner_script`, a SyntaxError. Prefix these with `_`,
|
||||
/// mirroring the existing digit-leading guard.
|
||||
fn is_python_keyword(s: &str) -> bool {
|
||||
matches!(
|
||||
s,
|
||||
"False"
|
||||
| "None"
|
||||
| "True"
|
||||
| "and"
|
||||
| "as"
|
||||
| "assert"
|
||||
| "async"
|
||||
| "await"
|
||||
| "break"
|
||||
| "class"
|
||||
| "continue"
|
||||
| "def"
|
||||
| "del"
|
||||
| "elif"
|
||||
| "else"
|
||||
| "except"
|
||||
| "finally"
|
||||
| "for"
|
||||
| "from"
|
||||
| "global"
|
||||
| "if"
|
||||
| "import"
|
||||
| "in"
|
||||
| "is"
|
||||
| "lambda"
|
||||
| "nonlocal"
|
||||
| "not"
|
||||
| "or"
|
||||
| "pass"
|
||||
| "raise"
|
||||
| "return"
|
||||
| "try"
|
||||
| "while"
|
||||
| "with"
|
||||
| "yield"
|
||||
)
|
||||
}
|
||||
|
||||
/// Compute the directory (relative to job_dir) where Python writes the main script.
|
||||
/// Module files must be placed in this same directory for relative imports to work.
|
||||
pub fn compute_python_module_dir(script_path: &str) -> String {
|
||||
let script_path_splitted = script_path.split("/").map(|x| {
|
||||
if x.starts_with(|x: char| x.is_ascii_digit()) {
|
||||
if x.starts_with(|x: char| x.is_ascii_digit()) || is_python_keyword(x) {
|
||||
format!("_{}", x)
|
||||
} else {
|
||||
x.to_string()
|
||||
@@ -1236,6 +1282,12 @@ pub fn compute_py_codegen(content: &str, script_path: &str) -> PyScriptCodegen {
|
||||
.replace("-", "_")
|
||||
.replace(" ", "_")
|
||||
.to_lowercase();
|
||||
// `last` is lowercased above, so this catches a keyword id in any case.
|
||||
let last = if is_python_keyword(&last) {
|
||||
format!("_{last}")
|
||||
} else {
|
||||
last
|
||||
};
|
||||
|
||||
let sig = windmill_parser_py::parse_python_signature(content, None, false).unwrap_or_default();
|
||||
let pre_sig = windmill_parser_py::parse_python_signature(
|
||||
@@ -1605,6 +1657,12 @@ async fn prepare_wrapper(
|
||||
.replace("-", "_")
|
||||
.replace(" ", "_")
|
||||
.to_lowercase();
|
||||
// `last` is lowercased above, so this catches a keyword id in any case.
|
||||
let last = if is_python_keyword(&last) {
|
||||
format!("_{last}")
|
||||
} else {
|
||||
last
|
||||
};
|
||||
let module_dir = format!("{}/{}", job_dir, dirs);
|
||||
tokio::fs::create_dir_all(format!("{module_dir}/")).await?;
|
||||
|
||||
@@ -3286,6 +3344,13 @@ mod tests {
|
||||
assert_eq!(compute_python_module_dir("u/@admin/script"), "u/.admin");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_compute_python_module_dir_keyword_segment() {
|
||||
// A folder whose name is a Python keyword would otherwise produce an
|
||||
// invalid `from f.in.x import ...`; it is underscore-prefixed.
|
||||
assert_eq!(compute_python_module_dir("f/in/script"), "f/_in");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_compute_py_codegen_basic_args() {
|
||||
let code = "def main(x: str, y: int):\n return x\n";
|
||||
@@ -3297,6 +3362,22 @@ mod tests {
|
||||
assert_eq!(cg.module_name, "script");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_compute_py_codegen_keyword_step_id() {
|
||||
// Regression for a flow inline step whose auto-assigned id is a Python
|
||||
// keyword (e.g. `in`): the generated wrapper must not emit
|
||||
// `from pkg import in as inner_script` (SyntaxError). The module name is
|
||||
// underscore-prefixed, matching the digit-leading guard.
|
||||
let code = "def main():\n return 1\n";
|
||||
let cg = compute_py_codegen(code, "u/admin/myflow/in");
|
||||
assert_eq!(cg.module_name, "_in");
|
||||
assert_eq!(cg.module_dir_dot, "u.admin.myflow");
|
||||
|
||||
// Non-keyword ids are unaffected.
|
||||
let cg2 = compute_py_codegen(code, "u/admin/myflow/step");
|
||||
assert_eq!(cg2.module_name, "step");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_compute_py_codegen_with_datetime_and_bytes() {
|
||||
let code = "import datetime\n\ndef main(name: str, created_at: datetime.datetime, file: bytes):\n return name\n";
|
||||
|
||||
Reference in New Issue
Block a user