#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.