Opened 11 months ago

Closed 11 months ago

Last modified 4 months ago

#34838 closed Bug (fixed)

GeoDjango database functions incompatible with GeneratedField

Reported by: Paolo Melchiorre Owned by: Paolo Melchiorre
Component: Database layer (models, ORM) Version: dev
Severity: Release blocker Keywords: field, database, generated, output_field
Cc: 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

GeoDjango model functions raise an incompatibility error when invoked on generated fields.

Steps

Steps to reproduce the error.

Model

from django.contrib.gis.db import models

class Area(models.Model):
    polygon = models.PolygonField()
    centroid = models.GeneratedField(
        db_persist=True,
        expression=models.functions.Centroid("polygon"),
        output_field=models.PointField(),
    )

Query

>>> from django.contrib.gis.geos import Polygon
>>> Area.objects.create(polygon=Polygon(((0,0), (2,0), (2,2), (0,2), (0,0))))
>>> Area.objects.values_list(models.functions.AsWKT("centroid"), models.functions.AsWKT("polygon"))

Traceback

Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/home/paulox/Projects/django/django/db/models/manager.py", line 87, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/paulox/Projects/django/django/db/models/query.py", line 1629, in annotate
    return self._annotate(args, kwargs, select=True)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/paulox/Projects/django/django/db/models/query.py", line 1677, in _annotate
    clone.query.add_annotation(
  File "/home/paulox/Projects/django/django/db/models/sql/query.py", line 1185, in add_annotation
    annotation = annotation.resolve_expression(self, allow_joins=True, reuse=None)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/paulox/Projects/django/django/contrib/gis/db/models/functions.py", line 80, in resolve_expression
    raise TypeError(
TypeError: AsWKT function requires a GeometryField in position 1, got GeneratedField.

Patch

diff --git a/django/contrib/gis/db/models/functions.py b/django/contrib/gis/db/models/functions.py
index 19da355d28..90ca87a051 100644
--- a/django/contrib/gis/db/models/functions.py
+++ b/django/contrib/gis/db/models/functions.py
@@ -76,6 +76,8 @@ class GeoFuncMixin:
         source_fields = res.get_source_fields()
         for pos in self.geom_param_pos:
             field = source_fields[pos]
+            if field.generated:
+                field = field.output_field
             if not isinstance(field, GeometryField):
                 raise TypeError(
                     "%s function requires a GeometryField in position %s, got %s."
@@ -86,7 +88,7 @@ class GeoFuncMixin:
                     )
                 )
 
-        base_srid = res.geo_field.srid
+        base_srid = res.geo_field.srid if not res.geo_field.generated else res.geo_field.output_field.srid
         for pos in self.geom_param_pos[1:]:
             expr = res.source_expressions[pos]
             expr_srid = expr.output_field.srid

Change History (10)

comment:1 by Mariusz Felisiak, 11 months ago

Needs tests: set
Owner: changed from nobody to Paolo Melchiorre
Status: newassigned
Triage Stage: UnreviewedAccepted

comment:2 by Simon Charette, 11 months ago

I suspect we'll want to avoid adding many output_field = field; if field.generated: field.output_field all over the codebase and that we should favour an approach where expression resolving (see sql.query.Query.resolve_ref) returns a Col with the proper output_field.

Maybe this can be done at the GeneratedField.get_col level where the returned Col instance defaults to output_field=self.output_field instead of self.

  • django/db/models/fields/generated.py

    diff --git a/django/db/models/fields/generated.py b/django/db/models/fields/generated.py
    index 0980be98af..948d11d003 100644
    a b def contribute_to_class(self, *args, **kwargs):  
    4848        for lookup_name, lookup in self.output_field.get_class_lookups().items():
    4949            self.register_lookup(lookup, lookup_name=lookup_name)
    5050
     51    def get_col(self, alias, output_field=None):
     52        if output_field is None:
     53            output_field = self.output_field
     54        return super().get_col(alias, output_field)
     55
    5156    def generated_sql(self, connection):
    5257        return self._resolved_expression.as_sql(
    5358            compiler=connection.ops.compiler("SQLCompiler")(
Last edited 11 months ago by Simon Charette (previous) (diff)

in reply to:  2 comment:3 by Paolo Melchiorre, 11 months ago

Replying to Simon Charette:

Maybe this can be done at the GeneratedField.get_col level where the returned Col instance defaults to output_field=self.output_field instead of self.

  • django/db/models/fields/generated.py

    diff --git a/django/db/models/fields/generated.py b/django/db/models/fields/generated.py
    index 0980be98af..948d11d003 100644
    a b def contribute_to_class(self, *args, **kwargs):  
    4848        for lookup_name, lookup in self.output_field.get_class_lookups().items():
    4949            self.register_lookup(lookup, lookup_name=lookup_name)
    5050
     51    def get_col(self, alias, output_field=None):
     52        if output_field is None:
     53            output_field = self.output_field
     54        return super().get_col(alias, output_field)
     55
    5156    def generated_sql(self, connection):
    5257        return self._resolved_expression.as_sql(
    5358            compiler=connection.ops.compiler("SQLCompiler")(

Thanks for the suggestion Simon. After reading your comment I had defined the get_col function exactly as you then sent it. I think I also wrote a couple of correct tests. I'm going to open the PR.

comment:4 by Paolo Melchiorre, 11 months ago

Needs tests: unset

comment:5 by Paolo Melchiorre, 11 months ago

I think the pathc is ready.
Thanks to Simon for the great help.

Last edited 11 months ago by Paolo Melchiorre (previous) (diff)

comment:6 by Simon Charette, 11 months ago

Component: GISDatabase layer (models, ORM)
Keywords: output_field added; gis feodjango removed

comment:7 by Mariusz Felisiak, 11 months ago

Triage Stage: AcceptedReady for checkin

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 11 months ago

Resolution: fixed
Status: assignedclosed

In 68d769e6:

Fixed #34838 -- Corrected output_field of resolved columns for GeneratedFields.

Thanks Simon Charette for the implementation idea.

comment:9 by Johannes Westphal <jojo@…>, 4 months ago

In 5f18021:

Fixed #35344, Refs #34838 -- Corrected output_field of resolved columns for GeneratedFields in aliased tables.

Thanks Simon Charette for the review.

comment:10 by Natalia <124304+nessita@…>, 4 months ago

In 14ab15d6:

[5.0.x] Fixed #35344, Refs #34838 -- Corrected output_field of resolved columns for GeneratedFields in aliased tables.

Thanks Simon Charette for the review.

Backport of 5f180216409d75290478c71ddb0ff8a68c91dc16 from main

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