Case.extra is undocumented, untested, and can hide potential issues

[Baptiste Mispelon]

I came across this today while accidentally doing something like the following:

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:

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:

django/db/models/expressions.py

@@ -1562 +1562 @@
     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 +1609 @@
         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

[Sarah Boyce]

So I think it's related to #25759 and extra is ​documented in Func.
We might want a similar note for Subquery and Case and tests are always welcome 👍

[Priyank Panchal]

I'm happy to receive any guidance on what needs to be improved.

[Tim Graham]

I'm not sure that removing the functionality is the correct solution, but I have to look at this more closely.

[Priyank Panchal]

Replying to Tim Graham:

I'm not sure that removing the functionality is the correct solution, but I have to look at this more closely.

Thank you for your response. It would be helpful if you could suggest an approach for these tickets.

[Jacob Walls]

I think this ticket was accepted to add test coverage and improve the documentation. **extra is documented in Func, but in Case the ticket author reports the documentation being unclear, and in Subquery it's not documented at all, so those two need doc edits and tests.

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.

The key/value pairs get interpolated into the SQL template, which by default includes the template param default (hence why the doc'd example provides default), but the SQL template is a class variable and could have arbitrary params if subclassed. This could be clarified.

[Jacob Walls]

I would be tempted to drop it considering the way it silently swallows unknown kwargs. Am I missing something?

We could consider deprecating the current behavior of accepting unused keyword args, but I think that should go through a ​forum post first.

[Priyank Panchal]

​https://github.com/django/django/pull/18419/files
I have updated the PR. I would appreciate any suggestions you might have.

[Tim Graham]

The proposed test doesn't seem to be a compelling use case:

Subquery(
    Employee.objects.all().values("salary"),
    salary=20,
    template="(SELECT salary FROM (%(subquery)s) _subquery "
    "WHERE salary = %(salary)s)",
)

If the programmer provides a custom template, it seems they can just as easily interpolate parameters into that template beforehand without relying on Subquery to do it.

I'm not sure if it was correct to include **extra in the original patch for Case considering it was undocumented and untested.

Sarah's comment 1 references #25759 but that's about adding **extra_context to as_sql(), not **extra in __init__(). While Func.__init__() accepts **extra, its subclasses do so inconsistently (and when it does, the use case seems to be setting output_field). Further, Case and Subquery inherit from BaseExpression (which doesn't accept **extra), not from Func.

[Jacob Walls]

Thanks, Tim. Before I saw the test case I wasn't aware that template was an allowed kwarg to Case and Subquery. I thought it had to be a class attribute, which is why I thought there was a use case for interpolating params at query time. But if you can provide the template dynamically, I agree the use case isn't compelling. Given that, deprecating **extra here makes sense to me.

Regardless, should we accept a unit test for providing a template kwarg to Case and Subquery? I don't see one in the suite.

[Sarah Boyce]

Marking "Patch needs improvement" until a decision is made on whether to deprecate **extra in Case and Subquery

[Tim Graham]

I think **extra could be removed without a deprecation since it's undocumented and doesn't appear to be useful, however, __init__() would need template=None adding to its signature if we want to continue allowing that. (Currently, template is pulled from extra: template_params.get("template", self.template) in as_sql().) I'm not sure if it's useful. As Jacob pointed out, it's undocumented and untested.