Updates do not work with GeometryFields

[jbronn]

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

[jbronn]

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

[Malcolm Tredinnick]

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

[jbronn]

A patch of last resort. Need something more elegant.

[jbronn]

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.

[Malcolm Tredinnick]

Let's fix it properly, particularly having seen what the alternative looks like. :-)

I'll get to it soonest.

[Malcolm Tredinnick]

Proposed fix

[Malcolm Tredinnick]

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

[jbronn]

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.

[Malcolm Tredinnick]

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

[Jacob]

Milestone 1.1 deleted