fix: Escaping in SQL templates for qmark parameter style
#27
Conversation
📝 WalkthroughWalkthroughThe Jinja SQL rendering utilities were changed so Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant render_jinja_sql_template
participant _escape_jinja_template
participant JinjaSql
participant render_result
Caller->>render_jinja_sql_template: call(template, param_style?)
alt param_style provided
render_jinja_sql_template->>render_jinja_sql_template: effective_param_style = provided
else auto-detect or default
render_jinja_sql_template->>render_jinja_sql_template: determine effective_param_style (new variable)
end
render_jinja_sql_template->>_escape_jinja_template: _escape_jinja_template(template, effective_param_style)
alt effective_param_style == "format" or "pyformat"
_escape_jinja_template-->>render_jinja_sql_template: return template with percent signs escaped (%%)
else
_escape_jinja_template-->>render_jinja_sql_template: return original template
end
render_jinja_sql_template->>JinjaSql: construct with effective_param_style
JinjaSql->>render_result: render SQL + bind_params
render_result-->>Caller: return (sql, bind_params)
sequenceDiagram
autonumber
participant Caller
participant SQLExecutor
participant DuckDBExecutor
Caller->>SQLExecutor: execute_sql(connection_info, query, param_style=None)
SQLExecutor->>SQLExecutor: detect param_style from connection if None
opt dialect == "deepnote+duckdb" or dialect == "duckdb" and param_style is None
SQLExecutor->>SQLExecutor: set param_style = "qmark" (new mapping/explicit set)
end
SQLExecutor->>DuckDBExecutor: execute_duckdb_sql(query, param_style="qmark")
DuckDBExecutor->>DuckDBExecutor: execute query unchanged (no unescape/rebuild of %)
DuckDBExecutor-->>SQLExecutor: execution result (or preview dataframe)
SQLExecutor-->>Caller: return result
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/unit/test_sql_execution.py (1)
🪛 Ruff (0.14.4)tests/unit/test_sql_execution.py74-74: Use a regular Replace (PT009) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (1)
Comment |
|
📦 Python package built successfully!
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #27 +/- ##
=======================================
Coverage 75.39% 75.40%
=======================================
Files 99 99
Lines 5625 5627 +2
Branches 784 785 +1
=======================================
+ Hits 4241 4243 +2
Misses 1384 1384 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
deepnote_toolkit/sql/jinjasql_utils.py(2 hunks)tests/unit/test_jinjasql_utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/test_jinjasql_utils.py (1)
deepnote_toolkit/sql/jinjasql_utils.py (1)
render_jinja_sql_template(9-36)
🪛 Ruff (0.14.4)
tests/unit/test_jinjasql_utils.py
97-97: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
98-98: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
106-106: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
107-107: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
115-115: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
116-116: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
deepnote_toolkit/sql/jinjasql_utils.py
43-43: Missing return type annotation for private function _escape_jinja_template
(ANN202)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Test - Python 3.13
- GitHub Check: Test - Python 3.10
- GitHub Check: Test - Python 3.11
- GitHub Check: Test - Python 3.12
- GitHub Check: Test - Python 3.9
- GitHub Check: Typecheck - 3.13
- GitHub Check: Typecheck - 3.9
- GitHub Check: Build and push artifacts for Python 3.9
- GitHub Check: Build and push artifacts for Python 3.11
- GitHub Check: Build and push artifacts for Python 3.12
- GitHub Check: Build and push artifacts for Python 3.13
- GitHub Check: Build and push artifacts for Python 3.10
🔇 Additional comments (5)
deepnote_toolkit/sql/jinjasql_utils.py (2)
25-29: Consistent param_style handling looks good.The
effective_param_stylevariable ensures the same parameter style is used for both escaping and JinjaSql initialization. Clean approach.
43-53: Conditional escaping logic is correct.Only escaping % for "format" and "pyformat" styles properly addresses the issue with date format strings in qmark style. The regex correctly avoids Jinja blocks.
tests/unit/test_jinjasql_utils.py (3)
92-99: Good test for qmark style.Validates that date format strings with % are preserved when using qmark parameter style. This directly tests the fix.
100-108: pyformat escaping verified.Confirms that % is properly escaped for pyformat style (dict-based parameters).
109-117: format escaping verified.Confirms that % is properly escaped for format style (list-based parameters).
|
🚀 Review App Deployment Started
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
deepnote_toolkit/sql/sql_execution.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Build and push artifacts for Python 3.13
- GitHub Check: Build and push artifacts for Python 3.10
- GitHub Check: Build and push artifacts for Python 3.9
- GitHub Check: Build and push artifacts for Python 3.11
- GitHub Check: Build and push artifacts for Python 3.12
- GitHub Check: Test - Python 3.12
- GitHub Check: Test - Python 3.13
- GitHub Check: Test - Python 3.11
- GitHub Check: Test - Python 3.9
- GitHub Check: Test - Python 3.10
🔇 Additional comments (1)
deepnote_toolkit/sql/sql_execution.py (1)
298-298: No issues found.DuckDB's Python API supports qmark-style (?) positional parameters via list/tuple binding, and
execute_duckdb_sqlcorrectly passes the list returned byrender_jinja_sql_templatewith qmark style directly toduckdb.execute(parameters=...). The code handles this correctly.
m1so
deleted the
michalbaumgartner/blu-5147-fix-escaping-in-sql-templating-for-qmark-parameter-style
branch
Summary by CodeRabbit