Fixed #24064 - Prevented database access in compile time in spatialite models

[coldmind]

Here is the patch that can fix problem.
Ideas about improvements are welcome.

[charettes]

It looks like the receiver is ran multiple times (on every connection creation). What about disconnecting it upon first call. I guess this might not be thread safe however.

[coldmind]

@charettes, well, I think the fact it is running every time is not good too, PR is more to discuss the idea.
And one more thing, tests failed at CI, but locally all is OK.
Can you test it locally?

[charettes]

I don't have Spatialite installed locally but what about those changes

From 9be0ec918ebf703cd0fc93bfa6a172e3f69ac97c Mon Sep 17 00:00:00 2001
From: Simon Charette <charette.s@gmail.com>
Date: Wed, 31 Dec 2014 17:26:26 -0500
Subject: [PATCH] Add spatial version related fields once.

---
 django/contrib/gis/db/backends/spatialite/models.py | 21 +++++++++++----------
 django/dispatch/dispatcher.py                       |  4 ++++
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/django/contrib/gis/db/backends/spatialite/models.py b/django/contrib/gis/db/backends/spatialite/models.py
index fc8cd11..95ee1e0 100644
--- a/django/contrib/gis/db/backends/spatialite/models.py
+++ b/django/contrib/gis/db/backends/spatialite/models.py
@@ -74,14 +74,15 @@ def add_spatial_version_related_fields(sender, **kwargs):
     Ticket #24064 - Need to establish connection first and then add fields
     to prevent database operations on compile time.
     """
-    srtext_field_added = 'srtext' in SpatialiteSpatialRefSys._meta.get_all_field_names()
-    type_field_added = 'type' in SpatialiteGeometryColumns._meta.get_all_field_names()
-    if srtext_field_added and type_field_added:
-        return
-    spatial_version = connection.ops.spatial_version[0]
-    if spatial_version >= 4:
-        SpatialiteSpatialRefSys.add_to_class('srtext', models.CharField(max_length=2048))
-        SpatialiteGeometryColumns.add_to_class('type', models.IntegerField(db_column='geometry_type'))
-    else:
-        SpatialiteGeometryColumns.add_to_class('type', models.CharField(max_length=30))
+    if connection_created.disconnect(add_spatial_version_related_fields, sender=DatabaseWrapper):
+        srtext_field_added = 'srtext' in SpatialiteSpatialRefSys._meta.get_all_field_names()
+        type_field_added = 'type' in SpatialiteGeometryColumns._meta.get_all_field_names()
+        if srtext_field_added and type_field_added:
+            return
+        spatial_version = connection.ops.spatial_version[0]
+        if spatial_version >= 4:
+            SpatialiteSpatialRefSys.add_to_class('srtext', models.CharField(max_length=2048))
+            SpatialiteGeometryColumns.add_to_class('type', models.IntegerField(db_column='geometry_type'))
+        else:
+            SpatialiteGeometryColumns.add_to_class('type', models.CharField(max_length=30))
 connection_created.connect(add_spatial_version_related_fields, sender=DatabaseWrapper)
diff --git a/django/dispatch/dispatcher.py b/django/dispatch/dispatcher.py
index 907f7fb..3a7b678 100644
--- a/django/dispatch/dispatcher.py
+++ b/django/dispatch/dispatcher.py
@@ -165,9 +165,13 @@ class Signal(object):
             for index in range(len(self.receivers)):
                 (r_key, _) = self.receivers[index]
                 if r_key == lookup_key:
+                    disconnected = True
                     del self.receivers[index]
                     break
+            else:
+                disconnected = False
             self.sender_receivers_cache.clear()
+        return disconnected

     def has_listeners(self, sender=None):
         return bool(self._live_receivers(sender))
-- 
1.9.1

[coldmind]

@charettes, well, it works, but I don't understand the reason to check if connection_created.disconnect inside the receiver function.

[coldmind]

buildbot, test this please.

[charettes]

@coldmind, it's to prevent two concurrent threads from creating a race condition where the signal is dispatched twice.

[timgraham]

buildbot, test on trusty.

[coldmind]

@charettes, @timgraham, due to tests, disconnection is called once, so seems no need to have check if field already added to model (see diff above). But I'm afraid there can be situation where it could be. What do you think?

[timgraham]

Simon, is this a different problem from what dispatch_uid attempts to solve?

[charettes]

due to tests, disconnection is called once, so seems no need to have check if field already added to model (see diff above). But I'm afraid there can be situation where it could be. What do you think?

Simon, is this a different problem from what dispatch_uid attempts to solve?

Yes, dispatch_uid is used to prevent multiple registration of the same receiver to a signal. This was handy when your project could be accessible from multiple locations on your PYTHONPATH. It's a feature similar to the one use in ModelBase.__new__ to prevent multiple registrations of the same model given the module containing them is imported multiple times due to PYTHONPATH misconfiguration.

This is trying to prevent a race condition in a multi-threaded environment (not reproducible during tests) such as runserver where n connections are created and dispatch the connection_created signal n times before it's disconnected. In this case you would end up with n times the fields you wanted to add once.

[charettes]

buildbot, test on trusty.

[charettes]

I re-retriggered the build since the failures seems unrelated.

[timgraham]

Could you move the django/dispatch/dispatcher.py changes to a separate commit that precedes the other and add some tests for it? Otherwise, I think this is good to go.

[coldmind]

@timgraham, sure, going to add some tests for dispatcher.

[coldmind]

buildbot, test this please.

[claudep]

Note that disconnect is documented https://docs.djangoproject.com/en/dev/topics/signals/#disconnecting-signals, an update is needed there too.

[apollo13]

You could move this line above the for loop and get rid of the else that way.

[coldmind]

buildbot, test this please.

[coldmind]

jenkins failed again with stderr: fatal: reference is not a tree

[timgraham]

No worries, this is likely fine. Will commit shortly.

[timgraham]

Merged in 23f1a8d and 839f431. Thanks very much for investigating the issue!