#35459: Case.extra is undocumented, untested, and can hide potential issues
-------------------------------------+-------------------------------------
Reporter: Baptiste | Owner: nobody
Mispelon |
Type: Bug | Status: new
Component: Database | Version: 5.0
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
I came across this today while accidentally doing something like the
following:
{{{#!python
MyModel.objects.annotate(x=Case(When(somefield=123), then=456,
default=789))
}}}
The query ran without errors and produced results but they were slightly
wrong. It took me longer than I'd like to admit to realize that I should
have written:
{{{#!python
MyModel.objects.annotate(x=Case(When(somefield=123, then=456),
default=789))
}}}
(in case you hadn't seen the difference, the `then` kwarg was passed to
`Case` instead of `When`).
Once I figured out what was going on, I was surprised that Django had
accepted the `then` argument for `Case`. I would have expected a
`TypeError`.
The documentation [1] does mention that the signature is `class
Case(*cases, **extra)`, but doesn't explain what happens to `**extra`, and
even later refers to "the default keyword argument" which seems
inconsistent.
To top it all of, it seems that the feature is untested. I removed
`**extra` from `Case.__init__` and `Case.as_sql()`, ran the full test
suite (under sqlite), and got zero errors:
{{{#!diff
diff --git a/django/db/models/expressions.py
b/django/db/models/expressions.py
index 4ee22420d9..34308fad9c 100644
--- a/django/db/models/expressions.py
+++ b/django/db/models/expressions.py
@@ -1562,13 +1562,12 @@ class Case(SQLiteNumericMixin, Expression):
template = "CASE %(cases)s ELSE %(default)s END"
case_joiner = " "
- def __init__(self, *cases, default=None, output_field=None, **extra):
+ def __init__(self, *cases, default=None, output_field=None):
if not all(isinstance(case, When) for case in cases):
raise TypeError("Positional arguments must all be When
objects.")
super().__init__(output_field)
self.cases = list(cases)
self.default = self._parse_expressions(default)[0]
- self.extra = extra
def __str__(self):
return "CASE %s, ELSE %r" % (
@@ -1610,7 +1609,7 @@ class Case(SQLiteNumericMixin, Expression):
connection.ops.check_expression_support(self)
if not self.cases:
return compiler.compile(self.default)
- template_params = {**self.extra, **extra_context}
+ template_params = {**extra_context}
case_parts = []
sql_params = []
default_sql, default_params = compiler.compile(self.default)
}}}
As far as I can tell, this feature has been present since `Case` was
introduced in #24031 (65246de7b1d70d25831ab394c4f4a75813f629fe).
I would be tempted to drop it considering the way it silently swallows
unknown kwargs. Am I missing something?
[1] https://docs.djangoproject.com/en/dev/ref/models/conditional-
expressions/#case
--
Ticket URL: <https://code.djangoproject.com/ticket/35459>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/0107018f81ac2ace-326215f8-cc94-4b1a-b93f-b2e97b88bdc6-000000%40eu-central-1.amazonses.com.
