[Django] #34838: GeoDjango database functions incompatible with GeneratedField
Django
-------------------------------------+-------------------------------------
Reporter: Paolo | Owner: nobody
Melchiorre |
Type: Bug | Status: new
Component: GIS | Version: dev
Severity: Release | Keywords: field, database,
blocker | generated, gis, feodjango
Triage Stage: | Has patch: 1
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34838>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
| Melchiorre
Type: Bug | Status: assigned
Component: GIS | Version: dev
Severity: Release blocker | Resolution:
Keywords: field, database, | Triage Stage: Accepted
generated, gis, feodjango |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Paolo Melchiorre
* status: new => assigned
* needs_tests: 0 => 1
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/34838#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Paolo Melchiorre | Owner: Paolo
| Melchiorre
Type: Bug | Status: assigned
Component: GIS | Version: dev
Severity: Release blocker | Resolution:
Keywords: field, database, | Triage Stage: Accepted
generated, gis, feodjango |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/34838#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Paolo Melchiorre | Owner: Paolo
| Melchiorre
Type: Bug | Status: assigned
Component: GIS | Version: dev
Severity: Release blocker | Resolution:
Keywords: field, database, | Triage Stage: Accepted
generated, gis, feodjango |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Paolo Melchiorre):
Replying to [comment:2 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`.
>
> {{{#!diff
> diff --git a/django/db/models/fields/generated.py
b/django/db/models/fields/generated.py
> index 0980be98af..948d11d003 100644
> --- a/django/db/models/fields/generated.py
> +++ b/django/db/models/fields/generated.py
> @@ -48,6 +48,11 @@ def contribute_to_class(self, *args, **kwargs):
> for lookup_name, lookup in
self.output_field.get_class_lookups().items():
> self.register_lookup(lookup, lookup_name=lookup_name)
>
> + def get_col(self, alias, output_field=None):
> + if output_field is None:
> + output_field = self.output_field
> + return super().get_col(alias, output_field)
> +
> def generated_sql(self, connection):
> return self._resolved_expression.as_sql(
> 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34838#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Paolo Melchiorre | Owner: Paolo
| Melchiorre
Type: Bug | Status: assigned
Component: GIS | Version: dev
Severity: Release blocker | Resolution:
Keywords: field, database, | Triage Stage: Accepted
generated, gis, feodjango |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_tests: 1 => 0
Comment:
PR https://github.com/django/django/pull/17259
--
Ticket URL: <https://code.djangoproject.com/ticket/34838#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Paolo Melchiorre | Owner: Paolo
| Melchiorre
Type: Bug | Status: assigned
Component: GIS | Version: dev
Severity: Release blocker | Resolution:
Keywords: field, database, | Triage Stage: Accepted
generated, gis, feodjango |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Paolo Melchiorre):
I think the piece is ready. Thanks to Simon for the great help.
--
Ticket URL: <https://code.djangoproject.com/ticket/34838#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Paolo Melchiorre | Owner: Paolo
| Melchiorre
Type: Bug | Status: assigned
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: field, database, | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* keywords: field, database, generated, gis, feodjango => field, database,
generated, output_field
* component: GIS => Database layer (models, ORM)
--
Ticket URL: <https://code.djangoproject.com/ticket/34838#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Paolo Melchiorre | Owner: Paolo
| Melchiorre
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
generated, output_field | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/34838#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Paolo Melchiorre | Owner: Paolo
| Melchiorre
Component: Database layer | Version: dev
(models, ORM) |
Keywords: field, database, | Triage Stage: Ready for
generated, output_field | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"68d769e691eb0d765228defddb3ba982eabdc761" 68d769e6]:
{{{
#!CommitTicketReference repository=""
revision="68d769e691eb0d765228defddb3ba982eabdc761"
Fixed #34838 -- Corrected output_field of resolved columns for
GeneratedFields.
Thanks Simon Charette for the implementation idea.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34838#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Paolo Melchiorre | Owner: Paolo
| Melchiorre
Type: Bug | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: field, database, | Triage Stage: Ready for
generated, output_field | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
In [changeset:"5f180216409d75290478c71ddb0ff8a68c91dc16" 5f18021]:
{{{#!CommitTicketReference repository=""
revision="5f180216409d75290478c71ddb0ff8a68c91dc16"
Fixed #35344, Refs #34838 -- Corrected output_field of resolved columns
for GeneratedFields in aliased tables.
Thanks Simon Charette for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34838#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Paolo Melchiorre | Owner: Paolo
| Melchiorre
Type: Bug | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: field, database, | Triage Stage: Ready for
generated, output_field | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
In [changeset:"14ab15d69ab2171ef26b295dcbd561b723b7eb7e" 14ab15d6]:
{{{#!CommitTicketReference repository=""
revision="14ab15d69ab2171ef26b295dcbd561b723b7eb7e"
[5.0.x] Fixed #35344, Refs #34838 -- Corrected output_field of resolved
Thanks Simon Charette for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34838#comment:10>