starred/windmill
1
0
Fork 0
mirror of https://github.com/windmill-labs/windmill.git synced 2026-09-21 00:02:30 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(datatables): honour -- role: x, and fix the DuckDB attach test

Browse Source 
Two review findings, both real.

`attach_datatable_parses_name_and_role` never compiled: `parse_attach_datatable`
returns `Result<Option<_>>` now and one call site kept a single `unwrap`. Its
`?Role=analytics` case also asserted a refusal, contradicting the parser in the
same commit, which matches the key case-insensitively. Replaced with the cases
that are genuinely malformed, and a positive one for the cased key.

`-- role: analytics` fell through to the default role — the silent fallback the
strict parser exists to remove, for the spelling most likely to be typed. The
keyword now accepts an optional colon, attached or spaced, while a word that
merely starts with it (`rolebased`) is still not an attempt.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb
This commit is contained in:
Diego Imbert
2026-09-09 18:41:35 +02:00
co-authored by Claude Opus 5
parent c84a6fc28e
commit c645537ee6
2 changed files with 54 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/windmill-common/src/worker.rs
+29 -8
View File
@@ -1105,16 +1105,27 @@ impl SqlAnnotations {
if !line.starts_with("--") {
break;
}
let mut tokens = line[2..].split_whitespace();
if !tokens
.next()
.is_some_and(|t| t.eq_ignore_ascii_case("role"))
{
// `role`, `Role`, `role:` and `role:name` all open an attempt; `rolexyz` does not.
// The colon is worth accepting rather than skipping past: `-- role: x` is the likelier
// spelling, and skipping it is exactly the silent fallback this refuses.
let body = line[2..].trim_start();
let Some(after) = body
.get(..4)
.filter(|kw| kw.eq_ignore_ascii_case("role"))
.map(|_| &body[4..])
else {
continue;
};
let colon = after.starts_with(':');
let after = after.strip_prefix(':').unwrap_or(after);
if !after.is_empty() && !colon && !after.starts_with(char::is_whitespace) {
continue;
}
// Past this point the line is an attempt to name a role, so a malformed one is an
// error rather than a miss. Falling through would run the query as the data table's
// default role — quietly, and under a login the author did not choose.
let mut tokens = after.split_whitespace();
let role = tokens
.next()
.map(|role| role.strip_suffix(';').unwrap_or(role));
@@ -2731,9 +2742,15 @@ mod tests {
assert_eq!(role("SELECT 1;\n-- role analytics").unwrap(), None);
assert_eq!(role("SELECT 1").unwrap(), None);
// Unambiguous intent is honoured: the keyword matches case-insensitively, and a trailing
// semicolon is a habit carried over from SQL rather than a different role.
for accepted in ["-- Role operator\nSELECT 1", "-- role operator;\nSELECT 1"] {
// Unambiguous intent is honoured: the keyword matches case-insensitively, a trailing
// semicolon is a habit carried over from SQL rather than a different role, and the colon
// spelling is the one most likely to be typed.
for accepted in [
"-- Role operator\nSELECT 1",
"-- role operator;\nSELECT 1",
"-- role: operator\nSELECT 1",
"-- role:operator\nSELECT 1",
] {
assert_eq!(
role(accepted).unwrap(),
Some("operator".to_string()),
@@ -2747,10 +2764,14 @@ mod tests {
"-- role operator -- why\nSELECT 1",
"-- role an;alytics\nSELECT 1",
"-- role\nSELECT 1",
"-- role:\nSELECT 1",
"-- role based access is handled below\nSELECT 1",
] {
assert!(role(near_miss).is_err(), "silently ignored: {near_miss}");
}
// A word that merely starts with the keyword is not an attempt.
assert_eq!(role("-- rolebased notes\nSELECT 1").unwrap(), None);
}
fn matcher(id: &str) -> WorkspaceMatcher {
backend/windmill-worker/src/duckdb_executor.rs
+25 -5
View File
@@ -2792,8 +2792,9 @@ mod tests {
#[test]
fn attach_datatable_parses_name_and_role() {
let named =
parse_attach_datatable("ATTACH 'datatable://sales?role=analytics' AS dt").unwrap();
let named = parse_attach_datatable("ATTACH 'datatable://sales?role=analytics' AS dt")
.unwrap()
.unwrap();
assert_eq!(
(named.name, named.role, named.alias),
("sales", Some("analytics"), "dt")
@@ -2807,11 +2808,30 @@ mod tests {
.unwrap()
.unwrap();
assert_eq!((no_role.name, no_role.role), ("sales", None));
let bare = parse_attach_datatable("ATTACH 'datatable' AS dt").unwrap().unwrap();
let bare = parse_attach_datatable("ATTACH 'datatable' AS dt")
.unwrap()
.unwrap();
assert_eq!((bare.name, bare.role), ("main", None));
assert!(parse_attach_datatable("SELECT 1").unwrap().is_none());
// A malformed role is refused rather than attached under the default one.
assert!(parse_attach_datatable("ATTACH 'datatable://sales?Role=analytics' AS dt").is_err());
// The key matches case-insensitively, as the `-- role` annotation does.
let cased = parse_attach_datatable("ATTACH 'datatable://sales?Role=analytics' AS dt")
.unwrap()
.unwrap();
assert_eq!(cased.role, Some("analytics"));
// A query string that does not parse is refused rather than attached under the default
// role: the statement asked for a specific one.
for malformed in [
"ATTACH 'datatable://sales?role=' AS dt",
"ATTACH 'datatable://sales?role=an;alytics' AS dt",
"ATTACH 'datatable://sales?x=1&role=analytics' AS dt",
] {
assert!(
parse_attach_datatable(malformed).is_err(),
"silently ignored: {malformed}"
);
}
}
#[test]