[Django] #33047: str decode error when applying migration with a CheckConstraint using a GEOS geometry and PostGISAdapter
Django
GEOS geometry and PostGISAdapter
-------------------------------------+-------------------------------------
Reporter: Daniel | Owner: (none)
Swain |
Type: Bug | Status: new
Component: | Version: 3.2
contrib.postgres | Keywords: constraint,
Severity: Normal | postgres, postgis, geos,
| attributeerror, migration,
| schema_editor, getquoted,
Triage Stage: | quote_value
Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Trying to apply a migration adding a `CheckConstraint` with a spatial
lookup (within) fails with: `AttributeError: 'str' object has no attribute
'decode'.` when creating the SQL statement.
== Traceback:
{{{
File "/usr/local/lib/python3.9/site-
packages/django/db/migrations/operations/models.py", line 858, in
database_forwards
schema_editor.add_constraint(model, self.constraint)
File "/usr/local/lib/python3.9/site-
packages/django/db/backends/base/schema.py", line 379, in add_constraint
sql = constraint.create_sql(model, self)
File "/usr/local/lib/python3.9/site-
packages/django/db/models/constraints.py", line 54, in create_sql
check = self._get_check_sql(model, schema_editor)
File "/usr/local/lib/python3.9/site-
packages/django/db/models/constraints.py", line 47, in _get_check_sql
return sql % tuple(schema_editor.quote_value(p) for p in params)
File "/usr/local/lib/python3.9/site-
packages/django/db/models/constraints.py", line 47, in <genexpr>
return sql % tuple(schema_editor.quote_value(p) for p in params)
File "/usr/local/lib/python3.9/site-
packages/django/db/backends/postgresql/schema.py", line 45, in quote_value
return adapted.getquoted().decode()
}}}
== Investigation
Looking at the `DatabaseSchemaEditor` class in the `postgresql` backend,
it looks like `quote_value` expects `adapted.getquoted()` to return a
`bytestring`:
{{{#!python
def quote_value(self, value):
...
# getquoted() returns a quoted bytestring of the adapted value.
return adapted.getquoted().decode()
}}}
The adaper used in this migration, `PostGISAdapter` returns a `str` from
`getquoted`, not a `bytestring`:
{{{#!python
class PostGISAdapter:
def getquoted(self):
"""
Return a properly quoted string for use in PostgreSQL/PostGIS.
"""
if self.is_geometry:
# Psycopg will figure out whether to use E'\\000' or '\000'.
return '%s(%s)' % (
'ST_GeogFromWKB' if self.geography else 'ST_GeomFromEWKB',
self._adapter.getquoted().decode()
)
else:
# For rasters, add explicit type cast to WKB string.
return "'%s'::raster" % self.ewkb
}}}
== Example model.
We have a model definition similar to:
{{{#!python
from django.contrib.gis.db import models
class Example(models.Model):
location = models.models.PointField(null=True, blank=True)
}}}
We are trying to add a `CheckConstraint` that checks that the location is
within a bounding box: `(x0, y0, x1, y1)` (The rest of these examples will
use a bounding box for Australia).
{{{#!python
from django.contrib.gis.geos import Polygon
class Example(models.Model):
location = models.models.PointField(null=True, blank=True)
class Meta:
constraints = [
models.CheckConstraint(
check=models.Q(location__within=Polygon.from_bbox((96.816941, -43.740510,
167.998035, -9.142176)),
name="location_in_australia",
),
]
}}}
A migration is successfully created:
{{{#!python
class Migration(migration.Migration):
operations = [
migrations.AddConstraint(
model_name='example',
constraint=models.CheckConstraint(check=models.Q(('location__within',
django.contrib.gis.geos.polygon.Polygon(((96.816941, -43.74051),
(96.816941, -9.142176), (167.998035, -9.142176), (167.998035, -43.74051),
(96.816941, -43.74051))))), name='location_in_australia'),
),
]
}}}
However, trying to run that migration gives the traceback shown at the
top.
== Potential fix/workaround.
We were able to run the migration by editing the file
`django/contrib/gis/db/backends/postgis/adapter.py` to make
`PostGISAdapter.getquoted()` return a `bytestring`, but we don't know what
effect this will have elsewhere.
Example:
{{{#!python
def getquoted(self):
if self.is_geometry:
# Psycopg will figure out whether to use E'\\000' or '\000'.
quoted = '%s(%s)' % (
'ST_GeogFromWKB' if self.geography else 'ST_GeomFromEWKB',
self._adapter.getquoted().decode()
)
else:
# For rasters, add explicit type cast to WKB string.
quoted = "'%s'::raster" % self.ewkb
return quoted.encode()
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33047>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Daniel Swain | Owner: nobody
Type: Bug | Status: new
Component: GIS | Version: 3.2
Severity: Normal | Resolution:
Keywords: constraint, | Triage Stage: Accepted
postgres, postgis, geos, |
attributeerror, migration, |
schema_editor, getquoted, |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: (none) => nobody
* component: contrib.postgres => GIS
* stage: Unreviewed => Accepted
Comment:
Thanks for the report.
Reproduced at 022d29c934107c515dd6d3181945146a2077bdf0.
--
Ticket URL: <https://code.djangoproject.com/ticket/33047#comment:1>
Django
-------------------------------------+-------------------------------------
| Ghassemi
Type: Bug | Status: assigned
Component: GIS | Version: 3.2
Severity: Normal | Resolution:
Keywords: constraint, | Triage Stage: Accepted
attributeerror, migration, |
schema_editor, getquoted, |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Arsalan Ghassemi
* status: new => assigned
Comment:
Hi,
I'm working on a fix, the topic branch :
https://github.com/ArsaCode/django/tree/ticket_33047
--
Ticket URL: <https://code.djangoproject.com/ticket/33047#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Daniel Swain | Owner: Arsalan
| Ghassemi
Type: Bug | Status: assigned
Component: GIS | Version: 3.2
Severity: Normal | Resolution:
Keywords: constraint, | Triage Stage: Accepted
attributeerror, migration, |
schema_editor, getquoted, |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Arsalan Ghassemi):
PR (work in progress) : https://github.com/django/django/pull/15130
With current changes the build fails on mysql_gis :
database=mysql_gis,label=mariadb,python=python3.10
Error :
MySQLdb._exceptions.OperationalError: (1583, "Incorrect parameters in the
call to native function 'ST_GeomFromText'")
database=mysql_gis,label=bionic-pr,python=python3.10
database=mysql_gis,label=bionic-pr,python=python3.8
database=mysql_gis,label=bionic-pr,python=python3.9
Error :
MySQLdb._exceptions.OperationalError: (1583, "Incorrect parameters in the
call to native function 'st_geometryfromtext'")
It seems like the parameter given to the function
"ST_GeomFromText"/"st_geometryfromtext" is invalid. I looked at the
Polygon assertions in the MySQL documentation and the values I've put in
the regression test migration seems not to be a valid Polygon for MySQL :
Polygon Assertions
The boundary of a Polygon consists of a set of LinearRing objects
(that is, LineString objects that are both simple and closed) that make up
its exterior and interior boundaries.
A Polygon has no rings that cross. The rings in the boundary of a
Polygon may intersect at a Point, but only as a tangent.
A Polygon has no lines, spikes, or punctures.
A Polygon has an interior that is a connected point set.
A Polygon may have holes. The exterior of a Polygon with holes is not
connected. Each hole defines a connected component of the exterior.
I think I'm not testing the patch correctly but I'm not sure how to do a
regression test for it (since adding the new test migration was making the
test fail before the patch). Any help would be appreciated :)
--
Ticket URL: <https://code.djangoproject.com/ticket/33047#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Daniel Swain | Owner: Arsalan
| Ghassemi
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: constraint, | Triage Stage: Accepted
attributeerror, migration, |
schema_editor, getquoted, |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
* version: 3.2 => dev
Comment:
My [https://github.com/django/django/pull/15134 PR] based on Arsalan
version.
--
Ticket URL: <https://code.djangoproject.com/ticket/33047#comment:4>
Django
backends.
-------------------------------------+-------------------------------------
Reporter: Daniel Swain | Owner: Arsalan
| Ghassemi
Type: Bug | Status: assigned
Component: GIS | Version: dev
Severity: Normal | Resolution:
Keywords: constraint, | Triage Stage: Accepted
attributeerror, migration, |
schema_editor, getquoted, |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33047#comment:5>
Django
backends.
-------------------------------------+-------------------------------------
| Paroz
Type: Bug | Status: assigned
Component: GIS | Version: dev
Severity: Normal | Resolution:
Keywords: constraint, | Triage Stage: Accepted
attributeerror, migration, |
schema_editor, getquoted, |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: Arsalan Ghassemi => Claude Paroz
--
Ticket URL: <https://code.djangoproject.com/ticket/33047#comment:6>
Django
backends.
-------------------------------------+-------------------------------------
Reporter: Daniel Swain | Owner: Claude
| Paroz
Type: Bug | Status: assigned
Component: GIS | Version: dev
Severity: Normal | Resolution:
postgres, postgis, geos, | checkin
attributeerror, migration, |
schema_editor, getquoted, |
quote_value |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33047#comment:7>
Django
backends.
-------------------------------------+-------------------------------------
Reporter: Daniel Swain | Owner: Claude
| Paroz
Component: GIS | Version: dev
Severity: Normal | Resolution: fixed
Keywords: constraint, | Triage Stage: Ready for
postgres, postgis, geos, | checkin
attributeerror, migration, |
schema_editor, getquoted, |
quote_value |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"64c3f049ea3bcb1c82f35ae09f1dd5349a826a5c" 64c3f049]:
{{{
#!CommitTicketReference repository=""
revision="64c3f049ea3bcb1c82f35ae09f1dd5349a826a5c"
Fixed #33047 -- Fixed CheckConstraint crash with GIS lookups on PostGIS
and MySQL GIS backends.
Thanks Daniel Swain for the report and Arsalan Ghassemi for the initial
patch.
Co-authored-by: Mariusz Felisiak <felisiak...@gmail.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33047#comment:8>