Opened 7 years ago
Closed 6 years ago
#29500 closed Bug (fixed)
SQLite functions crashes on NULL values
Reported by: | Sergey Fedoseev | Owned by: | Nick Pope |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 2.0 |
Severity: | Normal | Keywords: | |
Cc: | Carlton Gibson | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
In [14]: TestModel2.objects.annotate(null=models.Value(None, output_field=models.IntegerField())).values(pow=models.F('null') ** models.F('null')).first() --------------------------------------------------------------------------- OperationalError Traceback (most recent call last) ~/dev/django/django/db/backends/utils.py in _execute(self, sql, params, *ignored_wrapper_args) 84 else: ---> 85 return self.cursor.execute(sql, params) 86 ~/dev/django/django/db/backends/sqlite3/base.py in execute(self, query, params) 295 query = self.convert_query(query) --> 296 return Database.Cursor.execute(self, query, params) 297 OperationalError: user-defined function raised exception
Change History (11)
comment:2 by , 7 years ago
Cc: | added |
---|
comment:3 by , 7 years ago
Replying to Carlton Gibson:
I'll guess it'll be this:
>>> None ** None Traceback (most recent call last): File "<console>", line 1, in <module> TypeError: unsupported operand type(s) for ** or pow(): 'NoneType' and 'NoneType'
It is.
By SQLite functions
I meant user-defined function created here: https://github.com/django/django/blob/6dd4edb1b4f5441c5f543e29395039839c50d10b/django/db/backends/sqlite3/base.py#L158-L175
The list (incomplete?) of functions that crash on NULL values:
comment:4 by , 7 years ago
Triage Stage: | Unreviewed → Accepted |
---|
OK, thanks for the clarification. We could certainly consider a PR adding test cases and a bulletproofing to the functions we're shipping.
comment:5 by , 6 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:6 by , 6 years ago
We need to be careful how we handle this to ensure that the behaviour mirrors other backends.
I've checked PostgreSQL and when any one of the arguments to POWER()
. LPAD()
or RPAD()
is NULL
they return NULL
.
We should ensure that we check whether any one of the arguments is None
and, if so, return None
.
We must not catch exceptions such as TypeError
or ValueError
to do this as has been done in the initial version of the PR.
If we were to pass a string to _sqlite_power()
we would expect a TypeError
which should blow up, not return None
. Compare to PostgreSQL:
postgres=# select power(2, 'abc'); ERROR: invalid input syntax for type double precision: "abc" LINE 1: select power(2, 'abc'); ^
The second part of the problem here is that the sqlite backend suppresses the error message and returns a different exception:
OperationalError: user-defined function raised exception
Obviously this is not particularly helpful, but a quick search and I found the following on Stack Overflow: https://stackoverflow.com/a/45834923
It points to the documentation for sqlite3.enable_callback_tracebacks().
I would recommend the following:
- Creation of a decorator to check for
None
passed into any of the arguments which returnsNone
or calls the function as appropriate. - Enabling callbacks on tracebacks for sqlite3 (always / when debug enabled / documentation change to give instruction).
Note that the outcome of this pull request will affect PR/9622 which I am reviewing, particular with respect to my comment.
comment:7 by , 6 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:11 by , 6 years ago
Patch needs improvement: | set |
---|
comment:12 by , 6 years ago
Patch needs improvement: | unset |
---|
Hmmm. Not sure we'll be able to do anything about this. (Postgres certainly behaves better.)
Could you enable callback trackbacks on the client? We can then see the error.
I'll guess it'll be this:
`
Traceback (most recent call last):
TypeError: unsupported operand type(s) for or pow(): 'NoneType' and 'NoneType'
`
If so we may just have to workaround it by using a function for
pow
which checks forNone
.This works:
`
{'pow': None}
`
So it's just the
**
operation.