Opened 8 years ago
Closed 8 years ago
#26789 closed Bug (fixed)
ORM produces query with NULL instead of empty geometry
Reported by: | Sergey Fedoseev | Owned by: | Sergey Fedoseev |
---|---|---|---|
Component: | GIS | Version: | 1.9 |
Severity: | Normal | Keywords: | |
Cc: | robert.coup@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
from test_app.models import City from django.contrib.gis.geos.geometry import GEOSGeometry City.objects.create(point=GEOSGeometry('POINT EMPTY', srid=4326))
raises error
/home/sergey/dev/django/django/db/models/manager.pyc in manager_method(self, *args, **kwargs) 83 def create_method(name, method): 84 def manager_method(self, *args, **kwargs): ---> 85 return getattr(self.get_queryset(), name)(*args, **kwargs) 86 manager_method.__name__ = method.__name__ 87 manager_method.__doc__ = method.__doc__ /home/sergey/dev/django/django/db/models/query.pyc in create(self, **kwargs) 397 obj = self.model(**kwargs) 398 self._for_write = True --> 399 obj.save(force_insert=True, using=self.db) 400 return obj 401 /home/sergey/dev/django/django/db/models/base.pyc in save(self, force_insert, force_update, using, update_fields) 794 795 self.save_base(using=using, force_insert=force_insert, --> 796 force_update=force_update, update_fields=update_fields) 797 save.alters_data = True 798 /home/sergey/dev/django/django/db/models/base.pyc in save_base(self, raw, force_insert, force_update, using, update_fields) 822 if not raw: 823 self._save_parents(cls, using, update_fields) --> 824 updated = self._save_table(raw, cls, force_insert, force_update, using, update_fields) 825 # Store the database on which the object was saved 826 self._state.db = using /home/sergey/dev/django/django/db/models/base.pyc in _save_table(self, raw, cls, force_insert, force_update, using, update_fields) 906 907 update_pk = bool(meta.has_auto_field and not pk_set) --> 908 result = self._do_insert(cls._base_manager, using, fields, update_pk, raw) 909 if update_pk: 910 setattr(self, meta.pk.attname, result) /home/sergey/dev/django/django/db/models/base.pyc in _do_insert(self, manager, using, fields, update_pk, raw) 945 """ 946 return manager._insert([self], fields=fields, return_id=update_pk, --> 947 using=using, raw=raw) 948 949 def delete(self, using=None, keep_parents=False): /home/sergey/dev/django/django/db/models/manager.pyc in manager_method(self, *args, **kwargs) 83 def create_method(name, method): 84 def manager_method(self, *args, **kwargs): ---> 85 return getattr(self.get_queryset(), name)(*args, **kwargs) 86 manager_method.__name__ = method.__name__ 87 manager_method.__doc__ = method.__doc__ /home/sergey/dev/django/django/db/models/query.pyc in _insert(self, objs, fields, return_id, raw, using) 1042 query = sql.InsertQuery(self.model) 1043 query.insert_values(fields, objs, raw=raw) -> 1044 return query.get_compiler(using=using).execute_sql(return_id) 1045 _insert.alters_data = True 1046 _insert.queryset_only = False /home/sergey/dev/django/django/db/models/sql/compiler.pyc in execute_sql(self, return_id) 1052 with self.connection.cursor() as cursor: 1053 for sql, params in self.as_sql(): -> 1054 cursor.execute(sql, params) 1055 if not (return_id and cursor): 1056 return /home/sergey/dev/django/django/db/backends/utils.pyc in execute(self, sql, params) 77 start = time() 78 try: ---> 79 return super(CursorDebugWrapper, self).execute(sql, params) 80 finally: 81 stop = time() /home/sergey/dev/django/django/db/backends/utils.pyc in execute(self, sql, params) 62 return self.cursor.execute(sql) 63 else: ---> 64 return self.cursor.execute(sql, params) 65 66 def executemany(self, sql, param_list): /home/sergey/dev/django/django/db/utils.pyc in __exit__(self, exc_type, exc_value, traceback) 92 if dj_exc_type not in (DataError, IntegrityError): 93 self.wrapper.errors_occurred = True ---> 94 six.reraise(dj_exc_type, dj_exc_value, traceback) 95 96 def __call__(self, func): /home/sergey/dev/django/django/db/backends/utils.pyc in execute(self, sql, params) 62 return self.cursor.execute(sql) 63 else: ---> 64 return self.cursor.execute(sql, params) 65 66 def executemany(self, sql, param_list): IntegrityError: ОШИБКА: нулевое значение в столбце "point" нарушает ограничение NOT NULL DETAIL: Ошибочная строка содержит (3, null).
The last query
from django.db import connection connection.queries[-1]['sql']
is
u'INSERT INTO "test_app_city" ("point") VALUES (NULL) RETURNING "test_app_city"."id"'
Change History (11)
comment:1 by , 8 years ago
Component: | Uncategorized → GIS |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:2 by , 8 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Uncategorized → Bug |
comment:4 by , 8 years ago
Cc: | added |
---|
comment:5 by , 8 years ago
Simplistic fix could be something like (BaseSpatialField
):
def get_db_prep_save(self, value, connection): """ Prepare the value for saving in the database. """ if value or isinstance(value, Geometry): return connection.ops.Adapter(self.get_prep_value(value)) else: return None
A test case I used to track down the issue (gis_tests/geoapp/test_regress.py
):
def test_empty_assignment(self): c = Country.objects.get(name='Texas') c.mpoly = 'SRID=4326;MULTIPOLYGON EMPTY' self.assertIsInstance(c.mpoly, geos.MultiPolygon) self.assertTrue(c.mpoly.empty) c.save() c.refresh_from_db() self.assertIsInstance(c.mpoly, geos.MultiPolygon) self.assertTrue(c.mpoly.empty)
comment:6 by , 8 years ago
Has patch: | set |
---|
WIP pull request at https://github.com/django/django/pull/6934
Current approach passes empty to the DB and lets it decide whether to accept it.
Issues for discussion here:
POINT EMPTY
has a GEOS error on save for me w/ GEOS3.5.0 on OSX. We could workaround that.POLYGON EMPTY
has a PostGIS error on save for me w/ Postgis 2.2.2 (Postgres 9.5) on OSX- Should we delegate to backends to "check" whether a geometry is save-able, or pass it through to the DB?
- Need to test on the other DB backends, @jitai is that possible for you to do Oracle? Might need a
@no_oracle
on the test.
I considered an alternative approach of "fixing" falsey/truthy evaluation of geometries and use that as a proxy to save. Though IMO an empty geom could quite logically evaluate to False though it should be saved.
comment:7 by , 8 years ago
Patch needs improvement: | set |
---|
comment:9 by , 8 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Root cause it that geometry value begin nothing is tested with clause
if not value
. Which in turn calls__len__
on a geometry, having a following responses in different geometries:For an empty point number of coordinates is returned. In case of empty 0 (zero) is returned (false).
For a linestring empty tuple is returned
()
(false)For an polygon empty tuple of tuple is returned
((),)
(true)For a geometry collection length of 0 (zero) is returned (false)
NOTE:
In my tests at least spatialite raises NOT NULL violation with empty point.
NOTE2: Oracle (11g tested) doesn't seem to support empty geometries at all (using WKT at least)