Compare commits

...

1 Commits

Author SHA1 Message Date
claude[bot]
89b7bc7071 fix(parsers): Support s3_resource_path parameter in Python S3 functions
The Python asset parser was only detecting S3 URIs in the `s3object` parameter
but not in the `s3_resource_path` parameter. This caused named arguments using
s3_resource_path to be missed during asset parsing.

Enhanced the parser to check multiple parameter positions for S3 functions:
- load_s3_file: checks both s3object (pos 0) and s3_resource_path (pos 1)
- load_s3_file_reader: checks both s3object (pos 0) and s3_resource_path (pos 1)
- write_s3_file: checks s3object (pos 0) and s3_resource_path (pos 2)

The parser now correctly identifies both direct S3 URIs (s3://) and resource
configuration paths passed via named arguments.

Fixes #6513

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
2025-09-02 08:22:49 +00:00
2 changed files with 370 additions and 27 deletions

View File

@@ -58,36 +58,56 @@ impl AssetsFinder {
.ok_or(())?;
use AssetKind::*;
let (kind, access_type, arg) = match ident.as_str() {
"load_s3_file" => (S3Object, Some(R), Arg::Pos(0, "s3object")),
"load_s3_file_reader" => (S3Object, Some(R), Arg::Pos(0, "s3object")),
"write_s3_file" => (S3Object, Some(W), Arg::Pos(0, "s3object")),
"get_resource" => (Resource, None, Arg::Pos(0, "path")),
"set_resource" => (Resource, Some(W), Arg::Pos(0, "path")),
"get_boto3_connection_settings" => (Resource, None, Arg::Pos(0, "s3_resource_path")),
"get_polars_connection_settings" => (Resource, None, Arg::Pos(0, "s3_resource_path")),
"get_duckdb_connection_settings" => (Resource, None, Arg::Pos(0, "s3_resource_path")),
let (access_type, args) = match ident.as_str() {
"load_s3_file" => (Some(R), vec![(S3Object, Arg::Pos(0, "s3object")), (Resource, Arg::Pos(1, "s3_resource_path"))]),
"load_s3_file_reader" => (Some(R), vec![(S3Object, Arg::Pos(0, "s3object")), (Resource, Arg::Pos(1, "s3_resource_path"))]),
"write_s3_file" => (Some(W), vec![(S3Object, Arg::Pos(0, "s3object")), (Resource, Arg::Pos(2, "s3_resource_path"))]),
"get_resource" => (None, vec![(Resource, Arg::Pos(0, "path"))]),
"set_resource" => (Some(W), vec![(Resource, Arg::Pos(0, "path"))]),
"get_boto3_connection_settings" => (None, vec![(Resource, Arg::Pos(0, "s3_resource_path"))]),
"get_polars_connection_settings" => (None, vec![(Resource, Arg::Pos(0, "s3_resource_path"))]),
"get_duckdb_connection_settings" => (None, vec![(Resource, Arg::Pos(0, "s3_resource_path"))]),
_ => return Err(()),
};
let arg_val = match arg {
Arg::Pos(i, name) => node.args.get(i).or_else(|| {
// Get arg by name
node.keywords
.iter()
.find(|kw| kw.arg.as_deref() == Some(name))
.map(|kw| &kw.value)
}),
};
// Check each possible argument for assets
for (kind, arg) in args {
let arg_val = match arg {
Arg::Pos(i, name) => node.args.get(i).or_else(|| {
// Get arg by name
node.keywords
.iter()
.find(|kw| kw.arg.as_deref() == Some(name))
.map(|kw| &kw.value)
}),
};
match arg_val {
Some(Expr::Constant(ExprConstant { value: Constant::Str(value), .. })) => {
let path = parse_asset_syntax(&value).map(|(_, p)| p).unwrap_or(&value);
self.assets
.push(ParseAssetsResult { kind, path: path.to_string(), access_type });
if let Some(Expr::Constant(ExprConstant { value: Constant::Str(value), .. })) = arg_val {
// For S3 functions, check if the string contains an S3 URI
if kind == S3Object {
if let Some((asset_kind, path)) = parse_asset_syntax(&value) {
if asset_kind == AssetKind::S3Object {
self.assets
.push(ParseAssetsResult { kind, path: path.to_string(), access_type });
return Ok(());
}
}
} else if kind == Resource {
// For resource parameters, accept the value as-is (could be resource path or S3 URI)
if let Some((asset_kind, path)) = parse_asset_syntax(&value) {
// If it parses as an asset URI, use that
self.assets
.push(ParseAssetsResult { kind: asset_kind, path: path.to_string(), access_type });
return Ok(());
} else {
// Otherwise, treat it as a resource path
self.assets
.push(ParseAssetsResult { kind, path: value.to_string(), access_type });
return Ok(());
}
}
}
_ => return Err(()),
};
}
Ok(())
}
}
@@ -96,3 +116,326 @@ enum Arg {
// Positional arguments in python can also be used by their name
Pos(usize, &'static str),
}
#[cfg(test)]
mod tests {
use super::*;
use windmill_parser::asset_parser::AssetKind;
#[test]
fn test_parse_s3_positional_args() {
let code = r#"
import wmill
def main():
result = wmill.load_s3_file("s3://my-bucket/my-file.txt")
return result
"#;
let assets = parse_assets(code).unwrap();
assert_eq!(assets.len(), 1);
assert_eq!(assets[0].kind, AssetKind::S3Object);
assert_eq!(assets[0].path, "my-bucket/my-file.txt");
assert_eq!(assets[0].access_type, Some(AssetUsageAccessType::R));
}
#[test]
fn test_parse_s3_named_args() {
let code = r#"
import wmill
def main():
result = wmill.load_s3_file(s3object="s3://my-bucket/my-file.txt")
return result
"#;
let assets = parse_assets(code).unwrap();
assert_eq!(assets.len(), 1);
assert_eq!(assets[0].kind, AssetKind::S3Object);
assert_eq!(assets[0].path, "my-bucket/my-file.txt");
assert_eq!(assets[0].access_type, Some(AssetUsageAccessType::R));
}
#[test]
fn test_parse_s3_write_named_args() {
let code = r#"
import wmill
def main():
result = wmill.write_s3_file(s3object="s3://my-bucket/my-file.txt", data="test")
return result
"#;
let assets = parse_assets(code).unwrap();
assert_eq!(assets.len(), 1);
assert_eq!(assets[0].kind, AssetKind::S3Object);
assert_eq!(assets[0].path, "my-bucket/my-file.txt");
assert_eq!(assets[0].access_type, Some(AssetUsageAccessType::W));
}
#[test]
fn test_parse_resource_named_args() {
let code = r#"
import wmill
def main():
result = wmill.get_resource(path="u/user/my_resource")
return result
"#;
let assets = parse_assets(code).unwrap();
assert_eq!(assets.len(), 1);
assert_eq!(assets[0].kind, AssetKind::Resource);
assert_eq!(assets[0].path, "u/user/my_resource");
assert_eq!(assets[0].access_type, None);
}
#[test]
fn test_parse_mixed_args() {
let code = r#"
import wmill
def main():
# Positional arg
result1 = wmill.load_s3_file("s3://bucket1/file1.txt")
# Named arg
result2 = wmill.write_s3_file(s3object="s3://bucket2/file2.txt", data="test")
return result1, result2
"#;
let assets = parse_assets(code).unwrap();
assert_eq!(assets.len(), 2);
// Assets should be sorted by path
assert_eq!(assets[0].path, "bucket1/file1.txt");
assert_eq!(assets[0].kind, AssetKind::S3Object);
assert_eq!(assets[0].access_type, Some(AssetUsageAccessType::R));
assert_eq!(assets[1].path, "bucket2/file2.txt");
assert_eq!(assets[1].kind, AssetKind::S3Object);
assert_eq!(assets[1].access_type, Some(AssetUsageAccessType::W));
}
#[test]
fn test_parse_direct_import() {
let code = r#"
from wmill import load_s3_file, write_s3_file
def main():
# Direct function call with named arg
result = load_s3_file(s3object="s3://my-bucket/my-file.txt")
return result
"#;
let assets = parse_assets(code).unwrap();
assert_eq!(assets.len(), 1);
assert_eq!(assets[0].kind, AssetKind::S3Object);
assert_eq!(assets[0].path, "my-bucket/my-file.txt");
assert_eq!(assets[0].access_type, Some(AssetUsageAccessType::R));
}
#[test]
fn test_parse_all_s3_functions_with_named_args() {
let code = r#"
import wmill
def main():
# All S3-related functions with named arguments
result1 = wmill.load_s3_file(s3object="s3://bucket1/file1.txt")
result2 = wmill.load_s3_file_reader(s3object="s3://bucket2/file2.txt")
result3 = wmill.write_s3_file(s3object="s3://bucket3/file3.txt", data="test")
return result1, result2, result3
"#;
let assets = parse_assets(code).unwrap();
assert_eq!(assets.len(), 3);
// Should find all three S3 objects
assert_eq!(assets[0].path, "bucket1/file1.txt");
assert_eq!(assets[0].access_type, Some(AssetUsageAccessType::R));
assert_eq!(assets[1].path, "bucket2/file2.txt");
assert_eq!(assets[1].access_type, Some(AssetUsageAccessType::R));
assert_eq!(assets[2].path, "bucket3/file3.txt");
assert_eq!(assets[2].access_type, Some(AssetUsageAccessType::W));
}
#[test]
fn test_parse_non_s3_named_args() {
let code = r#"
import wmill
def main():
# Test other function with named args
resource = wmill.get_boto3_connection_settings(s3_resource_path="u/user/my_s3_resource")
return resource
"#;
let assets = parse_assets(code).unwrap();
assert_eq!(assets.len(), 1);
assert_eq!(assets[0].kind, AssetKind::Resource);
assert_eq!(assets[0].path, "u/user/my_s3_resource");
assert_eq!(assets[0].access_type, None);
}
#[test]
fn test_parse_wrong_named_arg() {
// Test case where named arg doesn't match expected parameter name
let code = r#"
import wmill
def main():
# Using wrong parameter name - should not be detected
result = wmill.load_s3_file(wrong_param="s3://bucket/file.txt")
return result
"#;
let assets = parse_assets(code).unwrap();
// Should not detect any assets because parameter name doesn't match
assert_eq!(assets.len(), 0);
}
#[test]
fn test_parse_aliased_import() {
let code = r#"
import wmill as w
def main():
result = w.load_s3_file(s3object="s3://my-bucket/file.txt")
return result
"#;
let assets = parse_assets(code).unwrap();
assert_eq!(assets.len(), 1);
assert_eq!(assets[0].kind, AssetKind::S3Object);
assert_eq!(assets[0].path, "my-bucket/file.txt");
assert_eq!(assets[0].access_type, Some(AssetUsageAccessType::R));
}
#[test]
fn test_parse_multiple_named_params() {
let code = r#"
import wmill
def main():
result = wmill.write_s3_file(s3object="s3://bucket/file.txt", data="content", content_type="text/plain")
return result
"#;
let assets = parse_assets(code).unwrap();
assert_eq!(assets.len(), 1);
assert_eq!(assets[0].kind, AssetKind::S3Object);
assert_eq!(assets[0].path, "bucket/file.txt");
assert_eq!(assets[0].access_type, Some(AssetUsageAccessType::W));
}
#[test]
fn test_parse_complex_real_world_example() {
// This mimics a more realistic usage pattern
let code = r#"
import wmill
import pandas as pd
def main(input_data: str):
# Load some data from S3
s3_data = wmill.load_s3_file(s3object="s3://data-bucket/input.csv")
# Process the data
df = pd.read_csv(s3_data)
processed_data = df.head()
# Save results back to S3
result = wmill.write_s3_file(
s3object="s3://data-bucket/output.csv",
data=processed_data.to_csv()
)
# Also get some configuration
config = wmill.get_resource(path="u/admin/config")
return {"status": "success", "config": config}
"#;
let assets = parse_assets(code).unwrap();
assert_eq!(assets.len(), 3);
// Should be sorted: config, input, output
assert_eq!(assets[0].path, "data-bucket/input.csv");
assert_eq!(assets[0].kind, AssetKind::S3Object);
assert_eq!(assets[0].access_type, Some(AssetUsageAccessType::R));
assert_eq!(assets[1].path, "data-bucket/output.csv");
assert_eq!(assets[1].kind, AssetKind::S3Object);
assert_eq!(assets[1].access_type, Some(AssetUsageAccessType::W));
assert_eq!(assets[2].path, "u/admin/config");
assert_eq!(assets[2].kind, AssetKind::Resource);
assert_eq!(assets[2].access_type, None);
}
#[test]
fn test_parse_s3_resource_path_param() {
// Test using s3_resource_path parameter instead of s3object
let code = r#"
import wmill
def main():
# This pattern was previously failing - now it should work!
result = wmill.load_s3_file(s3_resource_path="u/admin/s3_config")
return result
"#;
let assets = parse_assets(code).unwrap();
// This should detect the resource, not the S3 object
assert_eq!(assets.len(), 1);
assert_eq!(assets[0].kind, AssetKind::Resource);
assert_eq!(assets[0].path, "u/admin/s3_config");
assert_eq!(assets[0].access_type, Some(AssetUsageAccessType::R));
}
#[test]
fn test_parse_s3_uri_in_resource_path_param() {
// Test passing an S3 URI to s3_resource_path parameter
let code = r#"
import wmill
def main():
# User might pass S3 URI via s3_resource_path parameter
result = wmill.load_s3_file(s3_resource_path="s3://bucket/file.txt")
return result
"#;
let assets = parse_assets(code).unwrap();
// This should detect as S3Object since the string contains an S3 URI
assert_eq!(assets.len(), 1);
assert_eq!(assets[0].kind, AssetKind::S3Object);
assert_eq!(assets[0].path, "bucket/file.txt");
assert_eq!(assets[0].access_type, Some(AssetUsageAccessType::R));
}
#[test]
fn test_debug_diegoimbert_case() {
// Let me try to reproduce the exact issue that @diegoimbert might be seeing
println!("=== Testing various named argument patterns ===");
let test_cases = vec![
// Case 1: Standard named arg
(r#"
import wmill
def main():
result = wmill.load_s3_file(s3object="s3://bucket/file.txt")
return result
"#, "Standard named arg"),
// Case 2: Using s3_resource_path instead
(r#"
import wmill
def main():
result = wmill.load_s3_file(s3_resource_path="u/admin/s3_config")
return result
"#, "Using s3_resource_path"),
// Case 3: Mixed positional and named
(r#"
import wmill
def main():
result = wmill.load_s3_file("s3://bucket/file.txt", s3_resource_path="u/admin/s3_config")
return result
"#, "Mixed positional and named"),
];
for (code, description) in test_cases {
println!("Testing: {}", description);
let assets = parse_assets(code).unwrap();
println!(" Found {} assets", assets.len());
for asset in &assets {
println!(" {:?}: {} (access: {:?})", asset.kind, asset.path, asset.access_type);
}
}
}
}

View File

@@ -1,6 +1,6 @@
use serde::Serialize;
#[derive(Serialize, PartialEq, Clone, Copy)]
#[derive(Serialize, PartialEq, Clone, Copy, Debug)]
#[serde(rename_all(serialize = "lowercase"))]
pub enum AssetUsageAccessType {
R,
@@ -10,7 +10,7 @@ pub enum AssetUsageAccessType {
use AssetUsageAccessType::*;
#[derive(Serialize, PartialEq, Clone, Copy)]
#[derive(Serialize, PartialEq, Clone, Copy, Debug)]
#[serde(rename_all(serialize = "lowercase"))]
pub enum AssetKind {
S3Object,