[Django] #32213: QuerySet .values()/.values_list() on KeyTransforms return wrong values for double-quoted strings on SQLite and Oracle
Django
double-quoted strings on SQLite and Oracle
-------------------------------------+-------------------------------------
Reporter: sage | Owner: sage
Type: Bug | Status: assigned
Component: Database | Version: 3.1
layer (models, ORM) |
Severity: Release | Keywords:
blocker |
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Suppose we have a `JSONField` with the value `{'key': '"value"'}`. If
`.values()`/`.values_list()` are called on a `jsonfield__key` lookup, they
return `'value'` on SQLite and Oracle. On other backends, they return
`'"value"'`, which is the correct behavior.
--
Ticket URL: <https://code.djangoproject.com/ticket/32213>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
double-quoted strings on SQLite and Oracle
-------------------------------------+-------------------------------------
Reporter: sage | Owner: sage
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sqlite oracle json | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Carlton Gibson (added)
* keywords: => sqlite oracle json
* severity: Release blocker => Normal
* stage: Unreviewed => Accepted
Comment:
Thanks, I'm not sure about severity because it's niche I don't believe
that many users (if anyone) are affected by this issue. We can bump it
when evaluating a patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/32213#comment:1>
Django
double-quoted strings on SQLite and Oracle
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 3.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sqlite oracle json | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: sage => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/32213#comment:2>
Django
double-quoted strings on SQLite and Oracle
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 3.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sqlite oracle json | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
I'm also not sure this is event fixable since `JSONField` is schema less
so the ORM has not way to know it should use `->>` over `->` for the
specified key.
I think this ticket is a good candidate for ''wontfix'' consideration once
`KeyTextTransform` is documented (#15956) as this issue can be solved by
doing `values_list(KeyTextTransform("key", "json_field"))`.
--
Ticket URL: <https://code.djangoproject.com/ticket/32213#comment:3>
Django
double-quoted strings on SQLite and Oracle
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 3.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sqlite oracle json | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: bcail (added)
Comment:
I'm working on a test for this case, and having trouble reproducing it.
{{{
diff --git a/tests/model_fields/test_jsonfield.py
b/tests/model_fields/test_jsonfield.py
index 2c32d8a4ea..ecc2b9d0d5 100644
--- a/tests/model_fields/test_jsonfield.py
+++ b/tests/model_fields/test_jsonfield.py
@@ -611,6 +611,11 @@ class TestQuerying(TestCase):
[obj],
)
+ def test_value(self):
+ obj = NullableJSONModel.objects.create(value={"key": '"value"'})
+
self.assertSequenceEqual(NullableJSONModel.objects.filter(value__key='"value"').values_list("value",
flat=True), [{'key': '"value"'}])
+
self.assertSequenceEqual(NullableJSONModel.objects.filter(Q(value__has_key="key")).values_list("value",
flat=True), [{'key': '"value"'}])
+
@skipUnlessDBFeature("supports_json_field_contains")
def test_contains(self):
tests = [
}}}
This test passes on both SQLite and Postgresql for me. What do I need to
change to trigger the wrong behavior?
--
Ticket URL: <https://code.djangoproject.com/ticket/32213#comment:4>
Django
double-quoted strings on SQLite and Oracle
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 3.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sqlite oracle json | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:4 bcail]:
> This test passes on both SQLite and Postgresql for me. What do I need to
change to trigger the wrong behavior?
Add the second object `obj2 =
NullableJSONModel.objects.create(value={"key": "value"})`, `obj2` should
not be returned when you filter against `value__key='"value"'`.
--
Ticket URL: <https://code.djangoproject.com/ticket/32213#comment:5>
Django
double-quoted strings on SQLite and Oracle
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 3.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sqlite oracle json | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:4 bcail]:
> This test passes on both SQLite and Postgresql for me. What do I need to
change to trigger the wrong behavior?
This ticket is about using key transforms in the `values()/values_list()`:
{{{
# SQLite
>>>
NullableJSONModel.objects.filter(value__key='"value"').values_list("value__key",
flat=True)
['value']
# PostgreSQL
>>>
NullableJSONModel.objects.filter(value__key='"value"').values_list("value__key",
flat=True)
['"value"']
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32213#comment:5>
Django
double-quoted strings on SQLite and Oracle
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 3.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sqlite oracle json | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by bcail):
Thanks! That works for me, to show the issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/32213#comment:6>
Django
double-quoted strings on SQLite and Oracle
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 3.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sqlite oracle json | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by bcail):
Here are some tests that show the issues with various strings in a
JSONField for sqlite (they pass on postgresql):
{{{
+ def test_str_with_quotes(self):
+ value = {"key": "test_value", "key2": '"value"'}
+ obj = NullableJSONModel.objects.create(value=value)
+
+
self.assertSequenceEqual(NullableJSONModel.objects.filter(value__key='test_value').values("value__key2"),
[{"value__key2": '"value"'}])
+
self.assertSequenceEqual(NullableJSONModel.objects.filter(value__key='test_value').values_list("value__key2"),
[('"value"',)])
+
+ def test_str_with_null_false_true(self):
+ value = {"key": "test_value", "key2": "null", "key3": "false",
"key4": "true"}
+ obj = NullableJSONModel.objects.create(value=value)
+
+
self.assertSequenceEqual(NullableJSONModel.objects.filter(value__key='test_value').values_list("value__key2"),
[('null',)])
+
self.assertSequenceEqual(NullableJSONModel.objects.filter(value__key='test_value').values("value__key3"),
[{"value__key3": 'false'}])
+
self.assertSequenceEqual(NullableJSONModel.objects.filter(value__key='test_value').values("value__key4"),
[{"value__key4": 'true'}])
+
+ def test_str_with_list_dict(self):
+ value = {"key": "test_value", "key2": "[]", "key3": "{}"}
+ obj = NullableJSONModel.objects.create(value=value)
+
+
self.assertSequenceEqual(NullableJSONModel.objects.filter(value__key='test_value').values_list("value__key2"),
[("[]",)])
+
self.assertSequenceEqual(NullableJSONModel.objects.filter(value__key='test_value').values("value__key3"),
[{"value__key3": "{}"}])
}}}
This code change gets the str_with_quotes test passing, but not the other
two:
{{{
diff --git a/django/db/models/fields/json.py
b/django/db/models/fields/json.py
index 7296fe42bc..6277ce9935 100644
--- a/django/db/models/fields/json.py
+++ b/django/db/models/fields/json.py
@@ -82,8 +82,11 @@ class JSONField(CheckFieldDefaultMixin, Field):
return value
# Some backends (SQLite at least) extract non-string values in
their
# SQL datatypes.
- if isinstance(expression, KeyTransform) and not isinstance(value,
str):
- return value
+ if connection.vendor == "sqlite" and isinstance(expression,
KeyTransform):
+ if not isinstance(value, str):
+ return value
+ if value not in ['true', 'false', 'null'] and not
value.startswith('[') and not value.startswith('{'):
+ return value
try:
return json.loads(value, cls=self.decoder)
except json.JSONDecodeError:
}}}
I think that once sqlite 3.38.0 is in widespread use, and ticket #33548 is
implemented (using -> and ->> in sqlite), that may take care of this
issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/32213#comment:7>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 3.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sqlite oracle json | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32213#comment:8>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 3.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sqlite oracle json | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
I have submitted a Pull Request with the fix and regression tests:
https://github.com/django/django/pull/20487
The fix addresses the issue where SQLite `KeyTransform` lookups returned
unquoted raw strings, causing incorrect `json.loads` parsing.
Verification:
1. Added a regression test `test_ticket_32213` covering quoted strings and
numeric strings on SQLite.
2. Ran the full `model_fields.test_jsonfield` suite (124 tests), which
passed successfully, ensuring no regressions in existing JSON
functionality.
--
Ticket URL: <https://code.djangoproject.com/ticket/32213#comment:9>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sqlite oracle json | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: (none) => VIZZARD-X
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32213#comment:10>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sqlite oracle json | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 1
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
Comment:
Left some comments for improvements on the PR mainly
1. The solution should use `KeyTransform.select_format` and not override
`JSONField.from_db_value` with `KeyTransform` specific logic
2. Existing testing setup (models, test classes) should be used instead of
a per-ticket model and test module and test case class
--
Ticket URL: <https://code.djangoproject.com/ticket/32213#comment:11>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sqlite oracle json | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
-------------------------------------+-------------------------------------
I've updated the pull request to use `KeyTransform.select_format` as
suggested, ensuring the fix is isolated to the SQLite backend without
modifying `JSONField.from_db_value`.
Also added regression tests in `tests/model_fields/test_jsonfield.py`
rather than using per-ticket model which was the case previously
--
Ticket URL: <https://code.djangoproject.com/ticket/32213#comment:12>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: sqlite oracle json | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
Comment:
I've focused this patch specifically on the SQLite regression. Unchecking
''Needs tests'' and ''Patch needs improvement'' as the SQLite resolution
is looked into. The Oracle aspect mentioned in the ticket likely requires
a different implementation and wondering whether it could be addressed
separately in a separate ticket?
--
Ticket URL: <https://code.djangoproject.com/ticket/32213#comment:13>