Support for queries with no results (fix for #12856) #12888
Conversation
homeassistant
added
platform: sensor.sql
small-pr
| @@ -129,15 +129,17 @@ def update(self): | |||
| finally: | |||
| sess.close() | |||
|
|
|||
| if not result.returns_rows: | |||
| _LOGGER.error("%s returned no results", self._query) | |||
There was a problem hiding this comment.
I think that should be a warning
tests/components/sensor/test_sql.py
Outdated
| @@ -35,3 +36,22 @@ def test_query(self): | |||
|
|
|||
| state = self.hass.states.get('sensor.count_tables') | |||
| self.assertEqual(state.state, '0') | |||
|
|
|||
| for res in result: | ||
| _LOGGER.debug(res.items()) | ||
| _LOGGER.debug("result = %s", res.items()) | ||
| data = res[self._column_name] | ||
| self._attributes = {k: str(v) for k, v in res.items()} |
There was a problem hiding this comment.
Why would we convert this to strings? What about numbers?
There was a problem hiding this comment.
res.items() will return various columns, so it's not a scalar to convert to a number.
This call will resort to __str___ of sqlalchemy to get something human readable.
There was a problem hiding this comment.
It would be better for number types to remain numbers. Otherwise you can't do checks with it in templates without converting it to a number.
There was a problem hiding this comment.
Now I understand you initial comment.
Well casting to str() assured me nothing strange would show up in attributes.
| @@ -129,15 +129,17 @@ def update(self): | |||
| finally: | |||
| sess.close() | |||
|
|
|||
| if not result.returns_rows or result.rowcount == 0: | |||
There was a problem hiding this comment.
returns_rows -> I would expect this to be validated in the PLATFORM_SCHEMA by checking if the query starts with SELECT ?
There was a problem hiding this comment.
this is me being extra careful not to raise any exception :)
How would I validade the query string using voluptuous ?
- Should I create a new "ensure_sql_query" in home-assistant/homeassistant/helpers/config_validation.py ?
There was a problem hiding this comment.
No need to add things to config validation if only used once. You can add a one off validator to this file and use that.
def validate_sql_select(value):
if not value.lstrip().lower().startswith('select'):
raise vol.Invalid('Only select queries allowed')
return value
vol.Schema({
'query': vol.All(cv.string, validate_sql_select)
})
There was a problem hiding this comment.
So now we can remove the returns_rows check ?
There was a problem hiding this comment.
whats is wrong with checking returns_rows ?
tests/components/sensor/test_sql.py
Outdated
| @@ -35,3 +36,22 @@ def test_query(self): | |||
|
|
|||
| state = self.hass.states.get('sensor.count_tables') | |||
| self.assertEqual(state.state, '0') | |||
|
|
|||
| def test_no_results_query(self): | |||
There was a problem hiding this comment.
You should test the opposite. This case is already covered by all the other tests.
There was a problem hiding this comment.
This is the test I missed in the first place (which led to the issue)... don't understand you 😕
dgomes
changed the title
ghost
added
Description:
Tests if there are 0 results and clears the state and attributes.
Related issue (if applicable): fixes #12856
Checklist:
If the code does not interact with devices:
toxrun successfully. Your PR cannot be merged unless tests pass