feat: add support for contextual vars in SQL

[hugocasa]

Important

Add support for contextual variables in SQL queries across multiple executors and update the frontend to support these changes.

• Behavior:
  • Add support for contextual variables in SQL queries across multiple executors (bigquery_executor.rs, duckdb_executor.rs, mssql_executor.rs, mysql_executor.rs, oracledb_executor.rs, pg_executor.rs, snowflake_executor.rs).
  • Update sanitize_and_interpolate_unsafe_sql_args in sanitized_sql_params.rs to handle contextual variables.
• Functions:
  • Add replace_contextual_variables() in sanitized_sql_params.rs to replace contextual variables in SQL queries.
• Frontend:
  • Update EditorBar.svelte to support contextual variable insertion for SQL languages (e.g., postgresql, mysql, bigquery).

^{This description was created by for da64ecc. You can customize this summary. It will automatically update as commits are pushed.}

[claude]

Claude finished @hugocasa's task —— View job

---

Comprehensive PR Review

I've completed my review of PR #6791 that adds support for contextual variables in SQL. This is a well-implemented feature that extends Windmill's contextual variable system to all SQL database executors.

Todo List

• Read repository documentation and setup files
• Analyze core changes in sanitized_sql_params.rs
• Review SQL executor implementations across all databases
• Check worker.rs modifications
• Review frontend changes in EditorBar.svelte
• Identify potential bugs, security issues, and performance concerns
• Provide specific feedback with inline comments
• Create testing instructions for verification

---

Review Summary

Overall Assessment: ✅ APPROVED - This is a clean, well-implemented feature that follows established patterns and maintains security best practices.

Code Quality & Implementation

Strengths:

1. Consistent Implementation: The feature is implemented consistently across all 7 SQL database executors (PostgreSQL, MySQL, BigQuery, MSSQL, Oracle, Snowflake, DuckDB)
2. Follows Established Patterns: Uses the existing get_reserved_variables function and integrates seamlessly with the existing SQL parameter sanitization system
3. Clean Architecture: The core logic is centralized in sanitized_sql_params.rs with a dedicated replace_contextual_variables function
4. Security-First Design: Uses regex matching with strict patterns (%%WM_[A-Z_]+%%) preventing injection attacks

Implementation Details:

• Core Logic (sanitized_sql_params.rs:36-56): New function replace_contextual_variables uses regex %%WM_[A-Z_]+%% to safely replace contextual variables
• SQL Executors: Each executor now passes parent_runnable_path parameter and calls get_reserved_variables to obtain contextual variables
• Frontend Integration (EditorBar.svelte:453-459): SQL languages now support contextual variable insertion using %%${name}%% syntax
• Worker Changes (worker.rs): All SQL executor calls updated to pass the parent_runnable_path parameter

Security Analysis

✅ No Security Issues Found

1. Injection Prevention: The regex pattern %%WM_[A-Z_]+%% is strict and prevents injection attacks
2. Safe Replacement: Uses simple string replacement after regex validation
3. Existing Security: Leverages the already-secure get_reserved_variables function
4. Variable Scoping: Contextual variables are properly scoped to job execution context

Performance Considerations

✅ Minimal Performance Impact

1. Efficient Regex: The regex compilation is cached using lazy_static
2. Minimal Overhead: Variable replacement only occurs when contextual variables are present
3. No Database Impact: Changes happen at the query preparation stage, not execution
4. Consistent Pattern: Follows the same performance characteristics as existing parameter sanitization

Potential Issues & Suggestions

Minor Issues:

1. Code Style (sanitized_sql_params.rs:39): Function returns () - consider Result<(), Error> for consistency

   // Current
   fn replace_contextual_variables(code: &mut String, contextual_variables: &HashMap<String, String>) -> ()
   
   // Suggested  
   fn replace_contextual_variables(code: &mut String, contextual_variables: &HashMap<String, String>) -> Result<(), Error>

2. Error Handling: Missing variable substitution is silent - consider adding logging for missing contextual variables

3. Frontend Consistency (EditorBar.svelte:123-148): The showContextVarPicker condition includes all SQL languages - ensure this aligns with backend support

Suggestions for Future Enhancement:

• Add warning logs when contextual variables in SQL are not found
• Consider adding validation to ensure contextual variable names match expected patterns
• Document the new %%VAR_NAME%% syntax in SQL comments

Testing Instructions

To verify the contextual variable functionality in SQL scripts:

Setup: Navigate to the Windmill script editor and create a new SQL script (PostgreSQL, MySQL, BigQuery, MSSQL, Oracle, Snowflake, or DuckDB).

Testing Process:

1. Click the "+Context var" button in the editor toolbar to open the contextual variables picker
2. Select any available contextual variable (such as WM_JOB_ID, WM_WORKSPACE, etc.) - this will insert the variable in the format %%WM_VARIABLE_NAME%% into your SQL query
3. Create a simple query using the inserted contextual variable, for example: SELECT '%%WM_JOB_ID%%' as job_id, '%%WM_WORKSPACE%%' as workspace
4. Run the script and verify that the contextual variables are properly replaced with their actual runtime values in the query results

Expected Behavior: The %%WM_*%% placeholders should be replaced with actual values during execution, and the query should return the resolved contextual data rather than the placeholder strings.

---

This implementation successfully extends Windmill's contextual variable system to SQL scripts while maintaining security, performance, and code quality standards. The feature follows established patterns and provides a consistent user experience across all supported SQL database types.

---

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	da64ecc
Status:	✅  Deploy successful!
Preview URL:	https://ec98cb0b.windmill.pages.dev
Branch Preview URL:	https://hc-sql-contextual-vars.windmill.pages.dev

View logs

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to da64ecc in 3 minutes and 12 seconds. Click for details.

• Reviewed 462 lines of code in 10 files
• Skipped 0 files when reviewing.
• Skipped posting 3 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. backend/windmill-worker/src/sanitized_sql_params.rs:36

• Draft comment:
  In the replace_contextual_variables function, you loop over each unique token and use code.replace to update the SQL string. While this works, note that repeatedly replacing the entire string may be inefficient for large SQL statements. Also, consider logging a warning or handling the case where a contextual variable token appears but no value is provided in the contextual_variables map.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The performance concern about string.replace() is speculative - we don't know if SQL statements will be large or if this will cause performance issues. The suggestion about logging missing variables is a feature request rather than a clear bug. Neither point identifies a definite issue that needs fixing. The performance suggestion could be valid if we knew more about typical SQL statement sizes. Missing variable logging could help with debugging. Without evidence that the performance is actually problematic or that missing variable logging is a requirement, these suggestions are speculative nice-to-haves rather than clear issues. The comment should be removed as it makes speculative suggestions without clear evidence of problems that need fixing.

2. frontend/src/lib/components/EditorBar.svelte:452

• Draft comment:
  For SQL languages (e.g. postgresql, mysql, etc.), the code inserts tokens as %%${name}%% but the backend regex expects tokens beginning with %%WM_. This mismatch may prevent contextual variable replacement. Please verify if the intended format for contextual variables should include a 'WM_' prefix or update the regex accordingly.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment makes a specific claim about backend regex expectations but provides no evidence. Without seeing the backend code, we can't verify if there truly is a mismatch. The comment is speculative - "may prevent" suggests uncertainty. The suggestion to add "WM_" prefix is not clearly justified. I could be wrong if there is a well-known convention in the codebase for SQL variable tokens to use WM_ prefix. The backend could have strict requirements I'm unaware of. Even if there is a convention, the comment should provide evidence. Making assumptions about backend requirements without proof violates the rule about needing strong evidence. Delete this comment because it makes speculative claims about backend requirements without evidence. If there is a real mismatch, it should be documented with proof.

3. frontend/src/lib/components/EditorBar.svelte:418

• Draft comment:
  The conditional logic for inserting contextual variables in various languages is implemented with multiple if/else blocks. Consider refactoring this logic to use a mapping or helper function to reduce duplication and improve maintainability.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_UMXQijXHUUBDgmfn

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}