Conversation
home-assistant
bot
added
cla-signed
code-quality
integration: sql
small-pr
|
Hey there @gjohansson-ST, @dougiteixeira, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull Request Overview
This PR aims to remove template processing from the SQL service schema validation by changing the query parameter from cv.template to cv.string. However, the implementation has a critical bug that breaks template rendering in SQL queries.
Key changes:
- Service schema now accepts plain strings instead of Template objects for queries
- Validation function updated to handle both Template and str types via
async_get_hass() - Query rendering step removed from service execution path
Critical issue: The query is validated (with templates rendered) but then the unrendered string is passed to SQLAlchemy for execution, breaking template support in queries.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| homeassistant/components/sql/util.py | Modified validate_sql_select to accept both Template and str types, using async_get_hass() to obtain HomeAssistant instance for string inputs; adds type ignore comment for Template.hass access |
| homeassistant/components/sql/services.py | Changed service schema from template-based to string-based validation; removed template rendering before query execution (bug: queries with templates will now fail) |
| if isinstance(value, str): | ||
| hass = async_get_hass() |
There was a problem hiding this comment.
Using async_get_hass() in a schema validator is problematic. Schema validation can happen in various contexts where the HomeAssistant instance may not be available in thread-local storage, which will cause async_get_hass() to raise a HomeAssistantError.
According to the Home Assistant developer documentation, async_get_hass() "should be used where it's very cumbersome or downright impossible to pass hass to the code which needs it." In this case, since the validator is called during service schema validation before the service handler receives call.hass, there's no way to pass hass directly.
However, this creates a fragile dependency on thread-local state. Consider these alternatives:
- Move the validation logic to the service handler where
call.hassis available - Accept that validation during schema processing will be limited and defer full validation to the service handler
Note: While this may work in the current service call path, it creates a tight coupling to the execution context that could break if the validation is triggered from other contexts.
| if isinstance(value, str): | ||
| hass = async_get_hass() | ||
| else: | ||
| hass = value.hass # type: ignore[assignment] |
There was a problem hiding this comment.
The # type: ignore[assignment] comment suppresses a legitimate type error. The issue is that value.hass (when value is a Template) has type HomeAssistant | None, but the type annotation declares hass: HomeAssistant without the optional.
Instead of suppressing the error, consider handling the None case explicitly:
if isinstance(value, str):
hass = async_get_hass()
else:
if value.hass is None:
raise vol.Invalid("Template does not have hass instance")
hass = value.hassThis makes the code more robust and aligns with Home Assistant's linting guidelines that discourage using # type: ignore when the underlying issue can be properly addressed.
| hass = value.hass # type: ignore[assignment] | |
| if value.hass is None: | |
| raise vol.Invalid("Template does not have hass instance") | |
| hass = value.hass |
| rendered_query = check_and_render_sql_query(call.hass, query_str) | ||
| try: | ||
| result: Result = sess.execute(generate_lambda_stmt(rendered_query)) | ||
| result: Result = sess.execute(generate_lambda_stmt(query_str)) |
There was a problem hiding this comment.
The query string is being passed directly to generate_lambda_stmt() without rendering any Jinja2 templates it may contain. Previously, check_and_render_sql_query() was called to render templates before execution.
If the query contains template syntax like {{ states('sensor.example') }}, it will be passed as-is to SQLAlchemy, which will fail or produce incorrect results.
The query needs to be rendered before execution. You should call check_and_render_sql_query(call.hass, query_str) and use the returned rendered query:
from .util import (
async_create_sessionmaker,
check_and_render_sql_query, # Re-add this import
convert_value,
generate_lambda_stmt,
# ...
)
# In _execute_and_convert_query:
def _execute_and_convert_query() -> list[JsonValueType]:
"""Execute the query and return the results with converted types."""
sess: Session = sessmaker()
rendered_query = check_and_render_sql_query(call.hass, query_str)
try:
result: Result = sess.execute(generate_lambda_stmt(rendered_query))Note that check_and_render_sql_query is defined in util.py and can handle both Template and str inputs, so this will work with the new string-based schema.
| hass: HomeAssistant | ||
| if isinstance(value, str): | ||
| hass = async_get_hass() | ||
| else: | ||
| hass = value.hass # type: ignore[assignment] | ||
| try: | ||
| assert value.hass | ||
| check_and_render_sql_query(value.hass, value) | ||
| check_and_render_sql_query(hass, value) |
There was a problem hiding this comment.
I'm not sure this is correct. If we get a string, I don't think we should be trying to render it.
There was a problem hiding this comment.
It's at least unnecessary, I'll rewrite so we don't do that.
|
Marked the PR a draft, until the above has been done/resolved. |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
4 participants
Proposed change
Remove template from sql service schema, fixes late review comment #150287 (comment)
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: