Opened 6 years ago
Closed 6 years ago
#30548 closed Cleanup/optimization (fixed)
Improve exceptions about mixed types in Expressions.
| Reported by: | Keryn Knight | Owned by: | Shubham Bhagat |
|---|---|---|---|
| Component: | Database layer (models, ORM) | Version: | dev |
| Severity: | Normal | Keywords: | expressions orm |
| Cc: | Triage Stage: | Accepted | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | yes | UI/UX: | no |
Description
The source which raises the exception has enough information to say both what types were found, and which of those were unexpected, and probably have a useful repr()
In the test suite, the unexpected output types encountered seem to be DurationField and IntegerField, so a more thorough message might be something like:
Expression repr(self) contained mixed types: DateField, DurationField. DurationField was unexpected; you must set the output_field= for this Expression to either DurationField(), DateField() or ... (??? I dunno, some concrete explanation of what the output_field has to be/implement if you're not going to use any of the builtins)
The merit of including the repr is arguable, as the Expression may not what the user put in (eg: in the test suite it always seems to be a CombinedExpression(lhs, connector, rhs)) but it gives more of a hint in a query which contains multiple expressions (either nested or separate) as to which one is actually causing the problem vs just being told "something was wrong. Put an output_field= everywhere until it ceases, your guess is as good as mine"; at the very least the word Expression could be replaced with the class name which is actually raising it.
Change History (4)
comment:1 by , 6 years ago
| Easy pickings: | set |
|---|---|
| Summary: | Improve exception message when BaseExpression raises "'Expression contains mixed types. You must set output_field.'" → Improve exceptions about mixed types in Expressions. |
| Triage Stage: | Unreviewed → Accepted |
comment:2 by , 6 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
Agreed, we can add types to this exception but I would like to keep it simple, e.g. "Expression contains mixed types: FloatField, IntegerField. You must set output_field to IntegerField."
raise FieldError( 'Expression contains mixed types: %s, %s. You must set output_field to %s.' % (output_field.__class__.__name__, source.__class__.__name__, source.__class__.__name__) )