#10411 closed (fixed)
Updates do not work with GeometryFields
Reported by: | jbronn | Owned by: | Malcolm Tredinnick |
---|---|---|---|
Component: | GIS | Version: | 1.0 |
Severity: | Keywords: | gis update geoqueryset | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
>>> from world.models import * >>> mpoly = WorldBorders.objects.get(name='San Marino').mpoly >>> WorldBorders.objects.filter(name='Bahamas').update(mpoly=mpoly) Traceback (most recent call last): File "<console>", line 1, in <module> File "/Users/jbronn/django/trunk/django/db/models/query.py", line 450, in update rows = query.execute_sql(None) File "/Users/jbronn/django/trunk/django/db/models/sql/subqueries.py", line 119, in execute_sql cursor = super(UpdateQuery, self).execute_sql(result_type) File "/Users/jbronn/django/trunk/django/db/models/sql/query.py", line 2034, in execute_sql cursor.execute(sql, params) File "/Users/jbronn/django/trunk/django/db/backends/util.py", line 19, in execute return self.cursor.execute(sql, params) ProgrammingError: can't adapt
Looks like GeoQuerySet.update()
doesn't work. It should be noted that save()
(which uses InsertQuery
) works because ModelBase
pre-populates geometry field values through get_db_prep_save
(which converts geom objects into database adaptor objects). This does not happen because UpdateQuery
is invoked from the manager rather than at the model level and thus get_db_prep_save
is never called and the database adaptor is left wondering why it got a GEOSGeometry
object instead of something it can handle. A solution is to probably overload GeoQuerySet.update
to properly go through and run through geometry field keywords through get_db_prep_save
.
For now the solution is to not use .update()
with geometry fields and use the .save()
model method instead.
Attachments (3)
Change History (12)
comment:1 by , 16 years ago
follow-up: 3 comment:2 by , 16 years ago
Triage Stage: | Unreviewed → Accepted |
---|
I'd prefer to fix this so that the normal update()
path runs through and applies get_db_prep_save()
. We no doubt have the same potential problem with custom fields. In fact, notice the only FIXME comment in django/db/models/sql/subqueries.py
-- it's a code smell that turns out to have a functionality impact. :-)
(Bumping to "accepted" state only because we're trying to keep the number of "unreviewed" tickets to as close to zero as possible at the moment.)
by , 16 years ago
Attachment: | update_fix.diff added |
---|
A patch of last resort. Need something more elegant.
comment:3 by , 16 years ago
Status: | new → assigned |
---|
Replying to mtredinnick:
I'd prefer to fix this so that the normal
update()
path runs through and appliesget_db_prep_save()
. We no doubt have the same potential problem with custom fields. In fact, notice the only FIXME comment indjango/db/models/sql/subqueries.py
-- it's a code smell that turns out to have a functionality impact. :-)
Unfortunately I didn't read your comment before creating the attached patch (which includes the fix for #10397 as well). Regardless, I was going to contact you anyway because it didn't pass my smell test either -- too much duplicated code. In fact, an appropriate call to get_db_prep_save
at the point marked 'FIXME' would eliminate most of the code in the patch. Would keep us from having to cross the bridge of worrying about QuerySet.UpdateQuery
attributes instead of using from sql.UpdateQuery
.
comment:4 by , 16 years ago
Let's fix it properly, particularly having seen what the alternative looks like. :-)
I'll get to it soonest.
comment:5 by , 16 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
comment:6 by , 16 years ago
Has patch: | set |
---|
Justin, this latest patch fixes the tests you've provided and looks like the right approach. I've asked Russell to sanity check it, since I'm very vaguely nervous about adding a couple of do-nothing methods (return self
), but I suspect there's not anything that is easier. Unless Russell points out any huge problems, I'll commit this.
(There's a bunch of trailing whitespace stripping in here, too, since I do that routinely when I'm editing files: whitespace is an endangered resource, after all.)
by , 16 years ago
Attachment: | update-fix-v2.diff added |
---|
comment:7 by , 16 years ago
Modified your patch in some very small ways. First, had to put regression tests last in order because UPDATE
changes default ordering and will cause make_line
test to fail. Also, had to revert the regression tests to using unittest.TestCase
(instead of django.test.TestCase
), or else tests would fail in MySQL. My hunch is that these were caused by a mixing of test transaction between the Django and unittest's TestCase
-- a separate issue from fixing update
.
Regardless, these changes amounted to just small tweaks on the original patch; v2 was generated with svn diff
.
comment:8 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
(In [10003]) Pass values through get_db_prep_save() in a QuerySet.update() call.
This removes a long-standing FIXME in the update() handling and allows for
greater flexibility in the values passed in. In particular, it brings updates
into line with saves for django.contrib.gis fields, so fixed #10411.
Thanks to Justin Bronn and Russell Keith-Magee for help with this patch.
Also discussed in this GeoDjango user groups thread: http://groups.google.com/group/geodjango/browse_thread/thread/55ad6ff9e2e28b4a.