Let CollectionElement fail early

Fixes #2835
Fixes #2904

openslides/utils/autoupdate.py

@@ -7,6 +7,7 @@ from channels import Channel, Group
 from channels.asgi import get_channel_layer
 from channels.auth import channel_session_user, channel_session_user_from_http
 from django.apps import apps
+from django.core.exceptions import ObjectDoesNotExist
 from django.db import transaction
 
 from ..core.config import config
@@ -150,7 +151,11 @@ def send_data(message):
             # Anonymous user
             user = None
         else:
+            try:
                 user = CollectionElement.from_values('users/user', user_id)
+            except ObjectDoesNotExist:
+                # The user does not exist. Skip him/her.
+                continue
         output = collection_elements.as_autoupdate_for_user(user)
         for channel_name in channel_names:
             send_or_wait(Channel(channel_name).send, {'text': json.dumps(output)})

openslides/utils/collection.py

@@ -60,10 +60,13 @@ class CollectionElement:
         if self.is_deleted():
             # Delete the element from the cache, if self.is_deleted() is True:
             self.delete_from_cache()
-        elif instance is not None:
+        else:
-            # If this element is created via instance and the instance is not deleted
+            # The call to get_full_data() has some sideeffects. When the object
-            # then update the cache.
+            # was created with from_instance() or the object is not in the cache
-            self.save_to_cache()
+            # then get_full_data() will save the object into the cache.
+            # This will also raise a DoesNotExist error, if the object does
+            # neither exist in the cache nor in the database.
+            self.get_full_data()
 
     def __eq__(self, collection_element):
         """
@@ -184,11 +187,14 @@ class CollectionElement:
         """
         Returns the full_data of this collection_element from with all other
         dics can be generated.
+
+        Raises a DoesNotExist error on the requested the coresponding model, if
+        the object does neither exist in the cache nor in the database.
         """
         # If the full_data is already loaded, return it
         # If there is a db_instance, use it to get the full_data
         # else: try to use the cache.
-        # If there is no value in the cach, get the content from the db and save
+        # If there is no value in the cache, get the content from the db and save
         # it to the cache.
         if self.full_data is None and self.instance is None:
             # Use the cache version if self.instance is not set.

tests/integration/utils/test_collection.py

@@ -27,11 +27,12 @@ class TestCollectionElementCache(TestCase):
         Tests that the data is retrieved from the database.
         """
         topic = Topic.objects.create(title='test topic')
-        collection_element = collection.CollectionElement.from_values('topics/topic', 1)
         caches['locmem'].clear()
 
         with self.assertNumQueries(3):
+            collection_element = collection.CollectionElement.from_values('topics/topic', 1)
             instance = collection_element.get_full_data()
+
         self.assertEqual(topic.title, instance['title'])
 
     def test_with_cache(self):
@@ -49,20 +50,32 @@ class TestCollectionElementCache(TestCase):
             instance = collection_element.get_full_data()
         self.assertEqual(topic.title, instance['title'])
 
-    def test_non_existing_instance(self):
+    @patch('openslides.utils.collection.cache')
-        collection_element = collection.CollectionElement.from_values('topics/topic', 1)
+    def test_save_to_cache_called_once(self, mock_cache):
+        """
+        Makes sure, that save_to_cache ins called (only) once, if CollectionElement
+        is created with "from_instance()".
+        """
+        topic = Topic.objects.create(title='test topic')
+        mock_cache.set.reset_mock()
+        collection.CollectionElement.from_instance(topic)
 
+        # cache.set() is called two times. Once for the object and once for the collection.
+        self.assertEqual(mock_cache.set.call_count, 2)
+
+    def test_fail_early(self):
+        """
+        Tests that a CollectionElement.from_values fails, if the object does
+        not exist.
+        """
         with self.assertRaises(Topic.DoesNotExist):
-            collection_element.get_full_data()
+            collection.CollectionElement.from_values('topics/topic', 999)
 
 
 class TestCollectionCache(TestCase):
     def test_clean_cache(self):
         """
         Tests that the instances are retrieved from the database.
-
-        Currently there are 10 queries needed. This can change in the future,
-        but it has to be more then zero.
         """
         Topic.objects.create(title='test topic1')
         Topic.objects.create(title='test topic2')

tests/unit/utils/test_collection.py

@@ -64,6 +64,7 @@ class TestGetModelFromCollectionString(TestCase):
 
 class TestCollectionElement(TestCase):
     def test_from_values(self):
+        with patch.object(collection.CollectionElement, 'get_full_data'):
             collection_element = collection.CollectionElement.from_values('testmodule/model', 42)
 
         self.assertEqual(collection_element.collection_string, 'testmodule/model')
@@ -84,6 +85,7 @@ class TestCollectionElement(TestCase):
         mock_collection().delete_id_from_cache.assert_called_with(42)
 
     def test_as_channel_message(self):
+        with patch.object(collection.CollectionElement, 'get_full_data'):
             collection_element = collection.CollectionElement.from_values('testmodule/model', 42)
 
         self.assertEqual(
@@ -113,6 +115,7 @@ class TestCollectionElement(TestCase):
         self.assertEqual(created_collection_element.information, {'some': 'information'})
 
     def test_as_autoupdate_for_user(self):
+        with patch.object(collection.CollectionElement, 'get_full_data'):
             collection_element = collection.CollectionElement.from_values('testmodule/model', 42)
         fake_user = MagicMock()
         collection_element.get_access_permissions = MagicMock()
@@ -128,6 +131,7 @@ class TestCollectionElement(TestCase):
         collection_element.get_full_data.assert_called_once_with()
 
     def test_as_autoupdate_for_user_no_permission(self):
+        with patch.object(collection.CollectionElement, 'get_full_data'):
             collection_element = collection.CollectionElement.from_values('testmodule/model', 42)
         fake_user = MagicMock()
         collection_element.get_access_permissions = MagicMock()
@@ -171,6 +175,7 @@ class TestCollectionElement(TestCase):
              'value': 'config_value'})
 
     def test_get_instance(self):
+        with patch.object(collection.CollectionElement, 'get_full_data'):
             collection_element = collection.CollectionElement.from_values('testmodule/model', 42)
         collection_element.get_model = MagicMock()
 
@@ -184,6 +189,7 @@ class TestCollectionElement(TestCase):
         Test that the cache and the self.get_instance() is not hit, when the
         instance is already loaded.
         """
+        with patch.object(collection.CollectionElement, 'get_full_data'):
             collection_element = collection.CollectionElement.from_values('testmodule/model', 42)
         collection_element.full_data = 'my_full_data'
         collection_element.get_instance = MagicMock()
@@ -199,6 +205,7 @@ class TestCollectionElement(TestCase):
         Test that the value from the cache is used not get_instance is not
         called.
         """
+        with patch.object(collection.CollectionElement, 'get_full_data'):
             collection_element = collection.CollectionElement.from_values('testmodule/model', 42)
         collection_element.get_instance = MagicMock()
         mock_cache.get.return_value = 'cache_value'
@@ -215,6 +222,7 @@ class TestCollectionElement(TestCase):
         """
         Test that the value from get_instance is used and saved to the cache
         """
+        with patch.object(collection.CollectionElement, 'get_full_data'):
             collection_element = collection.CollectionElement.from_values('testmodule/model', 42)
         collection_element.get_instance = MagicMock()
         collection_element.get_access_permissions = MagicMock()
@@ -230,7 +238,8 @@ class TestCollectionElement(TestCase):
         mock_Collection.assert_called_once_with('testmodule/model')
         mock_Collection().add_id_to_cache.assert_called_once_with(42)
 
-    def test_equal(self):
+    @patch.object(collection.CollectionElement, 'get_full_data')
+    def test_equal(self, mock_get_full_data):
         self.assertEqual(
             collection.CollectionElement.from_values('testmodule/model', 1),
             collection.CollectionElement.from_values('testmodule/model', 1))
@@ -244,7 +253,8 @@ class TestCollectionElement(TestCase):
             collection.CollectionElement.from_values('testmodule/model', 1),
             collection.CollectionElement.from_values('testmodule/other_model', 1))
 
-    def test_config_cache_key(self):
+    @patch.object(collection.CollectionElement, 'get_full_data')
+    def test_config_cache_key(self, mock_get_full_data):
         """
         Test that collection elements for config values do always use the
         config key as cache key.
@@ -262,7 +272,8 @@ class TestCollectionElement(TestCase):
 
 
 class TestcollectionElementList(TestCase):
-    def test_channel_message(self):
+    @patch.object(collection.CollectionElement, 'get_full_data')
+    def test_channel_message(self, mock_get_full_data):
         """
         Test that a channel message from three collection elements can crate
         the same collection element list.
@@ -276,7 +287,8 @@ class TestcollectionElementList(TestCase):
             collection_elements,
             collection.CollectionElementList.from_channels_message(collection_elements.as_channels_message()))
 
-    def test_as_autoupdate_for_user(self):
+    @patch.object(collection.CollectionElement, 'get_full_data')
+    def test_as_autoupdate_for_user(self, mock_get_full_data):
         """
         Test that as_autoupdate_for_user is a list of as_autoupdate_for_user
         for each individual element in the list.