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.

[Kundan Yadav]

is there any updates? what are the final thoughts ?

[Jacob Walls]

After looking more at the history, I'm nervous about removing **extra from Case and/or Subquery/Exists given how close it is to the documented pattern for Func. In particular, we have an ​example of rewriting Coalesce from scratch that inherits from Expression rather than Func, and it follows the Func-style dynamic provision of template. The way Case/Subquery/Exists allow dynamic templates feels like dogfooding our own example.

(Currently, template is pulled from extra: template_params.get("template", self.template) in as_sql().) I'm not sure if it's useful.

A ​similar question was asked at the time, and the answer was, at least for Func subclasses, it could be useful. I'm worried saying it's useful for Func subclasses but not Exists is a hyperfine distinction. (A Subquery subclass might also want to specialize the template per backend, but accept a generic parameter for all backends in as_sql, like Cast does.)

Given that, my preference would be to close this ticket via documentation and tests, but with improving the example (perhaps using a CustomExists expression that is factored like Cast, e.g. it has vendor-specific templates all relying on a single parameter accepted from as_sql()).

[Lily]

I think we should remove **extra (possibly with a deprecation cycle) for the builtin Func subclasses. This is a lot of complexity for a pretty rare scenario. I think if a vendor needs to require/allow extra arguments to a custom template, that's the point at which they should provide a custom subclass.

[Jacob Walls]

I'd love to reduce the complexity, but if I'm understanding your proposal, you're suggesting we should replace Cast with OracleCast etc? If not, then I think I'd want to have at least a sketch of how to rewrite Cast in a simpler way.