[Django] #21496: Django 1.6 django.contrib.gis GeometryField crashes on alternate widget
Django
----------------------------+--------------------
Reporter: rhettg@… | Owner: nobody
Type: Bug | Status: new
Component: GIS | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+--------------------
If I use a TextInput widget to edit a PointField, form validation crashes
because the widget doesn't have a `map_srid`.
The string representation of a Point using the default srid (4326) doesn't
define the SRID, which is unfortunate.
>> p.srid
4326
>> str(p)
'POINT (-122.3996132001327055 37.7942941983347467)'
>> p2 = GEOSGeometry('POINT (-122.3996132001327055
37.7942941983347467)')
>> p2.srid
None
I think the best solution would be to allow the field to transform the
value into whatever the default is set as for the field, unless otherwise
specified by the widget.
So something like the following PR would be required:
https://github.com/django/django/pull/1966
--
Ticket URL: <https://code.djangoproject.com/ticket/21496>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: rhettg@… | Owner: nobody
Type: Bug | Status: new
Component: GIS | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* needs_better_patch: => 0
* stage: Unreviewed => Accepted
* needs_tests: => 0
* needs_docs: => 0
Old description:
> If I use a TextInput widget to edit a PointField, form validation crashes
> because the widget doesn't have a `map_srid`.
>
> The string representation of a Point using the default srid (4326)
> doesn't define the SRID, which is unfortunate.
> >> p.srid
> 4326
> >> str(p)
> 'POINT (-122.3996132001327055 37.7942941983347467)'
>
> >> p2 = GEOSGeometry('POINT (-122.3996132001327055
> 37.7942941983347467)')
> >> p2.srid
> None
>
> I think the best solution would be to allow the field to transform the
> value into whatever the default is set as for the field, unless otherwise
> specified by the widget.
>
> So something like the following PR would be required:
> https://github.com/django/django/pull/1966
New description:
If I use a TextInput widget to edit a PointField, form validation crashes
because the widget doesn't have a `map_srid`.
The string representation of a Point using the default srid (4326) doesn't
define the SRID, which is unfortunate.
{{{
>> p.srid
4326
>> str(p)
'POINT (-122.3996132001327055 37.7942941983347467)'
>> p2 = GEOSGeometry('POINT (-122.3996132001327055
37.7942941983347467)')
>> p2.srid
None
}}}
I think the best solution would be to allow the field to transform the
value into whatever the default is set as for the field, unless otherwise
specified by the widget.
So something like the following PR would be required:
https://github.com/django/django/pull/1966
--
--
Ticket URL: <https://code.djangoproject.com/ticket/21496#comment:1>
Django
Reporter: rhettg@… | Owner: nobody
Type: Bug | Status: new
Component: GIS | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
There are two parts in your proposed pull request. First of all, the crash
concerning the missing map_srid is bad and should be fixed, clearly. But a
simple `hasattr(self.widget, 'map_srid')` would be enough, at least at
first sight.
Then there is the part about where to convert the value into the srid of
the field. This is debatable, but I'm currently not entirely convinced by
your patch. I would probably move in `to_python` the srid attribution to
the field srid when the value has no srid, but not the `transform`
conversion. Anyway, this is a different issue, which we could discuss in
another ticket.
So I suggest to only fix something like this (which I would also backport
to 1.6):
{{{
--- a/django/contrib/gis/forms/fields.py
+++ b/django/contrib/gis/forms/fields.py
@@ -44,7 +44,7 @@ class GeometryField(forms.Field):
if not isinstance(value, GEOSGeometry):
try:
value = GEOSGeometry(value)
- if not value.srid:
+ if not value.srid and hasattr(self.widget, 'map_srid'):
value.srid = self.widget.map_srid
except (GEOSException, ValueError, TypeError):
raise
forms.ValidationError(self.error_messages['invalid_geom'],
code='invalid_geom')
}}}
... with an appropriate test (I have the patch almost ready).
--
Ticket URL: <https://code.djangoproject.com/ticket/21496#comment:2>
Django
Reporter: rhettg@… | Owner: nobody
Type: Bug | Status: new
Component: GIS | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by rhettg):
I didn't mention this, but the other reason for moving the transform out
of clean was because of another crash.
in _has_changed, this line fails:
{{{
data.transform(initial.srid)
}}}
Since to_python would then allow geom objects without srids... the srid
from the field itself doesn't get applied unless you call clean(), which
_has_changed does not.
My use of try...except was mostly habit from EAFP
(http://docs.python.org/2/glossary.html)
--
Ticket URL: <https://code.djangoproject.com/ticket/21496#comment:3>
Django
Reporter: rhettg@… | Owner: nobody
Type: Bug | Status: new
Component: GIS | Version: 1.6
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* has_patch: 0 => 1
Comment:
So what about this PR: https://github.com/django/django/pull/1971
--
Ticket URL: <https://code.djangoproject.com/ticket/21496#comment:4>
Django
Reporter: rhettg@… | Owner: nobody
Type: Bug | Status: closed
Component: GIS | Version: 1.6
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz <claude@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"34b8a385588a051814f77481c875047c8bd23b37"]:
{{{
#!CommitTicketReference repository=""
revision="34b8a385588a051814f77481c875047c8bd23b37"
Fixed #21496 -- Fixed crash when GeometryField uses TextInput
Thanks Rhett Garber for the report and initial patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21496#comment:5>
Django
Reporter: rhettg@… | Owner: nobody
Type: Bug | Status: closed
Component: GIS | Version: 1.6
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz <claude@…>):
In [changeset:"14ddc1b517266916d9e729bc20234edec1d7bd4c"]:
{{{
#!CommitTicketReference repository=""
revision="14ddc1b517266916d9e729bc20234edec1d7bd4c"
[1.6.x] Fixed #21496 -- Fixed crash when GeometryField uses TextInput
Thanks Rhett Garber for the report and initial patch.
Backport of 34b8a3855 from master.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21496#comment:6>