#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 django-updates+unsubscr...@googlegroups.com.
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.
