Opened 16 years ago

Closed 16 years ago

Last modified 13 years ago

#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)

update_fix.diff (6.3 KB ) - added by jbronn 16 years ago.
A patch of last resort. Need something more elegant.
update-fix.diff (16.4 KB ) - added by Malcolm Tredinnick 16 years ago.
Proposed fix
update-fix-v2.diff (16.3 KB ) - added by jbronn 16 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 by jbronn, 16 years ago

Also discussed in this GeoDjango user groups thread: http://groups.google.com/group/geodjango/browse_thread/thread/55ad6ff9e2e28b4a.

comment:2 by Malcolm Tredinnick, 16 years ago

Triage Stage: UnreviewedAccepted

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 jbronn, 16 years ago

Attachment: update_fix.diff added

A patch of last resort. Need something more elegant.

in reply to:  2 comment:3 by jbronn, 16 years ago

Status: newassigned

Replying to mtredinnick:

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. :-)

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 Malcolm Tredinnick, 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 Malcolm Tredinnick, 16 years ago

Owner: changed from jbronn to Malcolm Tredinnick
Status: assignednew

by Malcolm Tredinnick, 16 years ago

Attachment: update-fix.diff added

Proposed fix

comment:6 by Malcolm Tredinnick, 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 jbronn, 16 years ago

Attachment: update-fix-v2.diff added

comment:7 by jbronn, 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 Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: newclosed

(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.

comment:9 by Jacob, 13 years ago

milestone: 1.1

Milestone 1.1 deleted

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