Code

Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#10411 closed (fixed)

Updates do not work with GeometryFields

Reported by: jbronn Owned by: mtredinnick
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: UI/UX:

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 5 years ago.
A patch of last resort. Need something more elegant.
update-fix.diff (16.4 KB) - added by mtredinnick 5 years ago.
Proposed fix
update-fix-v2.diff (16.3 KB) - added by jbronn 5 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 5 years ago by jbronn

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 follow-up: Changed 5 years ago by mtredinnick

  • Triage Stage changed from Unreviewed to 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.)

Changed 5 years ago by jbronn

A patch of last resort. Need something more elegant.

comment:3 in reply to: ↑ 2 Changed 5 years ago by jbronn

  • Status changed from new to assigned

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 Changed 5 years ago by mtredinnick

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

I'll get to it soonest.

comment:5 Changed 5 years ago by mtredinnick

  • Owner changed from jbronn to mtredinnick
  • Status changed from assigned to new

Changed 5 years ago by mtredinnick

Proposed fix

comment:6 Changed 5 years ago by mtredinnick

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

Changed 5 years ago by jbronn

comment:7 Changed 5 years ago by 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.

comment:8 Changed 5 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to 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.

comment:9 Changed 3 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.