[Django] #35459: Case.extra is undocumented, untested, and can hide potential issues

[Django]

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

[Django]

#35459: Case.extra is undocumented, untested, and can hide potential issues
-------------------------------------+-------------------------------------

Reporter: Baptiste Mispelon | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* cc: Tim Graham (added)
* stage: Unreviewed => Accepted
* type: Bug => Cleanup/optimization

Comment:

So I think it's related to #25759 and `extra` is
[https://docs.djangoproject.com/en/5.0/ref/models/expressions/#django.db.models.Func
documented in Func].
We might want a similar note for `Subquery` and `Case` and tests are
always welcome 👍
--
Ticket URL: <https://code.djangoproject.com/ticket/35459#comment:1>

[Django]

#35459: Case.extra is undocumented, untested, and can hide potential issues
-------------------------------------+-------------------------------------

Reporter: Baptiste Mispelon | Owner: amns13
Type: | Status: assigned

Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by amns13):

* owner: nobody => amns13
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/35459#comment:2>

[Django]

#35459: Case.extra is undocumented, untested, and can hide potential issues
-------------------------------------+-------------------------------------

Reporter: Baptiste Mispelon | Owner: Priyank
Type: | Panchal
Cleanup/optimization | Status: assigned

Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Priyank Panchal):

* has_patch: 0 => 1
* owner: amns13 => Priyank Panchal

--
Ticket URL: <https://code.djangoproject.com/ticket/35459#comment:3>

[Django]

#35459: Case.extra is undocumented, untested, and can hide potential issues
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Priyank
Type: | Panchal
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Priyank Panchal):

I'm happy to receive any guidance on what needs to be improved.
--
Ticket URL: <https://code.djangoproject.com/ticket/35459#comment:4>

[Django]

#35459: Case.extra is undocumented, untested, and can hide potential issues
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Priyank
Type: | Panchal
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Tim Graham):

I'm not sure that removing the functionality is the correct solution, but
I have to look at this more closely.
--
Ticket URL: <https://code.djangoproject.com/ticket/35459#comment:5>

[Django]

#35459: Case.extra is undocumented, untested, and can hide potential issues
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Priyank
Type: | Panchal
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Priyank Panchal):

Replying to [comment:5 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35459#comment:6>

[Django]

#35459: Case.extra is undocumented, untested, and can hide potential issues
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Priyank
Type: | Panchal
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35459#comment:7>

[Django]

#35459: Case.extra is undocumented, untested, and can hide potential issues
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Priyank
Type: | Panchal
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* has_patch: 1 => 0

Comment:

> 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
[https://forum.djangoproject.com/t/deprecate-unmatched-keyword-args-to-
func-case-and-subquery/33534 forum post] first.
--
Ticket URL: <https://code.djangoproject.com/ticket/35459#comment:8>

[Django]

#35459: Case.extra is undocumented, untested, and can hide potential issues
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Priyank
Type: | Panchal
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Priyank Panchal):

[https://github.com/django/django/pull/18419/files]
I have updated the PR. I would appreciate any suggestions you might have.
--
Ticket URL: <https://code.djangoproject.com/ticket/35459#comment:9>

[Django]

#35459: Case.extra is undocumented, untested, and can hide potential issues
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Priyank
Type: | Panchal
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/35459#comment:10>

[Django]

#35459: Case.extra is undocumented, untested, and can hide potential issues
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Priyank
Type: | Panchal
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Jacob Walls):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/35459#comment:11>

[Django]

#35459: Case.extra is undocumented, untested, and can hide potential issues
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Priyank
Type: | Panchal
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Jacob Walls):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/35459#comment:12>

[Django]

#35459: Case.extra is undocumented, untested, and can hide potential issues
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Priyank
Type: | Panchal
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Tim Graham):

The proposed test doesn't seem to be a compelling use case:
{{{#!python
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/35459#comment:13>

[Django]

#35459: Case.extra is undocumented, untested, and can hide potential issues
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Priyank
Type: | Panchal
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35459#comment:14>

[Django]

#35459: Case.extra is undocumented, untested, and can hide potential issues
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Priyank
Type: | Panchal
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* needs_better_patch: 0 => 1

Comment:

Marking "Patch needs improvement" until a decision is made on whether to
deprecate `**extra` in `Case` and `Subquery`
--
Ticket URL: <https://code.djangoproject.com/ticket/35459#comment:15>

[Django]

#35459: Case.extra is undocumented, untested, and can hide potential issues
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: Priyank
Type: | Panchal
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35459#comment:16>