Opened 4 years ago

Closed 4 years ago

Last modified 15 months ago

#31967 closed Bug (fixed)

Resolving an output_field for mixed types crashes in database functions.

Reported by: Matt Hegarty Owned by: Hasan Ramezani
Component: Documentation Version: dev
Severity: Normal Keywords:
Cc: Matt Hegarty, Simon Charette Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This is related to #31919. Please see attached for full stack trace.

This is a regression because it does not occur in 3.1.

To reproduce:

git clone https://github.com/matthewhegarty/rest-framework-tutorial
cd rest-framework-tutorial
git checkout bug-expression-contains-mixed-types
python3 -m venv env
source env/bin/activate
pip install -r requirements.txt
./manage.py migrate
./manage.py runserver

browse to http://localhost:8000/snippets/

Attachments (1)

stack_trace.txt (5.5 KB ) - added by Matt Hegarty 4 years ago.
stack trace

Download all attachments as: .zip

Change History (16)

by Matt Hegarty, 4 years ago

Attachment: stack_trace.txt added

stack trace

comment:1 by Matt Hegarty, 4 years ago

Cc: Matt Hegarty added

comment:2 by Mariusz Felisiak, 4 years ago

Cc: Simon Charette added
Component: UncategorizedDatabase layer (models, ORM)
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

Thanks for this report. It's a regression in 1e38f1191de21b6e96736f58df57dfb851a28c1f but it's not related with fields subclasses. I was able to reproduce it with a builtin DecimalField(), e.g.

>>> list(MyModel.objects.annotate(x=Coalesce(F('field_decimal'), 0))
...
  File "django/django/db/models/expressions.py", line 306, in _resolve_output_field
    source.__class__.__name__,
django.core.exceptions.FieldError: Expression contains mixed types: DecimalField, IntegerField. You must set output_field.

comment:3 by Mariusz Felisiak, 4 years ago

Summary: Django fails with FieldError: Expression contains mixed types: MoneyField, IntegerField. You must set output_field.Resolving an output_field for mixed types crashes in database functions.

comment:4 by Mariusz Felisiak, 4 years ago

Component: Database layer (models, ORM)Documentation
Severity: Release blockerNormal

We didn't resolve output fields for Value() expressions before 1e38f1191de21b6e96736f58df57dfb851a28c1f, so they were ignored when resolving an output field for functions. Now, we take them into account, that's why an error is raised. IMO it's an expected behavior. It should be enough to mention this in release notes.

You can fix this by using Decimal('0'), e.g. Coalesce(F('field_decimal'), Decimal('0')))

Last edited 4 years ago by Mariusz Felisiak (previous) (diff)

comment:5 by Matt Hegarty, 4 years ago

When I try to set the second arg to Decimal('0'), I get:

django.db.utils.ProgrammingError: can't adapt type 'DecimalField'

Also, I think this workaround will be confusing, because the error message tells you to set the output_field. I did try this when I first encountered the error and it didn't work. So it's very likely that others will do the same rather than checking the release notes.

Version 0, edited 4 years ago by Matt Hegarty (next)

in reply to:  5 comment:6 by Mariusz Felisiak, 4 years ago

Replying to Matt Hegarty:

When I try to set the second arg to Decimal('0'), I get:

django.db.utils.ProgrammingError: can't adapt type 'DecimalField'

True, sorry, setting output_field is the best way.

comment:7 by Matt Hegarty, 4 years ago

I confirm that this call does not crash:

Coalesce(
    SubquerySum(
        "price__price"
    ),
    0,
    output_field=DecimalField()
),

Thank you for your help.

comment:8 by Simon Charette, 4 years ago

So we've got two approach we could take here.

Determine that it was never meant to work and document it or use a clemency based approach where we deprecate this behaviour by ignoring the output_field of Value source expressions in Expression._resolve_output_field when it fails with FieldError.

I'm surprised Coalesce(F('field_decimal'), Decimal('0'))) doesn't actually work.

comment:9 by Mariusz Felisiak, 4 years ago

I think it's enough to document this change in release notes, because it's easy to fix on the users side, it's explicit, and it was never meant to work.

comment:10 by Hasan Ramezani, 4 years ago

Has patch: set
Owner: changed from nobody to Hasan Ramezani
Status: newassigned

comment:11 by Mariusz Felisiak, 4 years ago

Triage Stage: AcceptedReady for checkin

comment:12 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In 9369f0ce:

Fixed #31967 -- Doc'd consequences of resolving an output_field for Value().

comment:13 by Anne "Anya" Macedo, 15 months ago

Hello! I see that this is fixed, but I still don't understand how to workaround this issue.

I'm working on updating a Django app using this library: https://github.com/annemacedo-tw/django-spillway

I get this error when using it:

django.core.exceptions.FieldError: Expression contains mixed types: MultiPolygonField, FloatField. You must set output_field.

However I don't get the same error on Django 3.1

As far as I debugged, this line here yields these two fields https://github.com/annemacedo-tw/django-spillway/blob/fa008ef0db5f3434c1ef438f66231f35a90b1e6d/spillway/query.py#L153

SimplifyPreserveTopology(F(geometry), Value(0.0439453125))

The first one being a MultiPolygonField and the other one being a FloatField. I tried setting the output_field and recreating the source_expressions but it didn't really work.

        sql = SimplifyPreserveTopology(sql, tilew)
        sql_field = sql.source_expressions[0]
        sql_value = sql.source_expressions[1]
        sql_value.output_field = models.MultiPolygonField()
        sql.set_source_expressions([sql_field, sql_value])

(I understand that I'm setting an output field for MultiPolygonField here instead of FloatField as should be expected, I set it up to avoid the mixed types error)

  File "/usr/local/lib/python3.9/site-packages/django/contrib/gis/db/models/fields.py", line 194, in get_prep_value
    raise ValueError('Cannot use object with type %s for a spatial lookup parameter.' % type(obj).__name__)
ValueError: Cannot use object with type float for a spatial lookup parameter.

TL;DR how can I work around this issue when using GeoDjango?

Another thing I noticed is that ever since GeoManager was deprecated way back in 1.10, django.contrib.gis.db.models.FloatField resolves to django.db.models.fields.FloatField. I wonder if the gis FloatField would play nicely with MultiPolygonField.

(Pdb) from django.contrib.gis.db import models
(Pdb) models.FloatField
<class 'django.db.models.fields.FloatField'>

I wish I could open a separate ticket for this, but I'm working on legacy code that I don't understand so well, so I don't know how to do a minimal version out of it.

comment:14 by Simon Charette, 15 months ago

I suggest posting your question in the GeoDjango category of the forum instead.

I suspect you only have to pass output_field=models.MultiPolygonField() when initializing your SimplifyPreserveTopology as hinted by the error message: You must set output_field.

SimplifyPreserveTopology(
    F(geometry), 0.0439453125, output_field=models.MultiPolygonField()
)
Last edited 15 months ago by Simon Charette (previous) (diff)

in reply to:  14 comment:15 by Anne "Anya" Macedo, 15 months ago

Replying to Simon Charette:

I suggest posting your question in the GeoDjango category of the forum instead.

I suspect you only have to pass output_field=models.MultiPolygonField() when initializing your SimplifyPreserveTopology as hinted by the error message: You must set output_field.

SimplifyPreserveTopology(
    F(geometry), 0.0439453125, output_field=models.MultiPolygonField()
)

I believe it worked! Been trying to fix this bug for a whole week :)

Note: See TracTickets for help on using tickets.
Back to Top