From 366777ebfdd12e250185b3ffde7a56662d140b5f Mon Sep 17 00:00:00 2001 From: FinnStutzenstein Date: Mon, 2 Sep 2019 08:50:22 +0200 Subject: [PATCH] Fixed bug sending always all data to users, who get restricted data Due to changes in an iterator, a RuntimeError was thrown but interpred al a too low change id. Fixed the bug and make a custom exception for this. --- openslides/utils/cache.py | 17 ++++++++++------- openslides/utils/consumers.py | 4 ++-- tests/unit/utils/test_cache.py | 6 +++--- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/openslides/utils/cache.py b/openslides/utils/cache.py index 761800c20..422fc6f28 100644 --- a/openslides/utils/cache.py +++ b/openslides/utils/cache.py @@ -22,6 +22,10 @@ from .utils import get_element_id, split_element_id logger = logging.getLogger(__name__) +class ChangeIdTooLowError(Exception): + pass + + def get_all_cachables() -> List[Cachable]: """ Returns all element of OpenSlides. @@ -274,7 +278,7 @@ class ElementCache: Only returns elements with the change_id or newer. When change_id is 0, all elements are returned. - Raises a RuntimeError when the lowest change_id in redis is higher then + Raises a ChangeIdTooLowError when the lowest change_id in redis is higher then the requested change_id. In this case the method has to be rerun with change_id=0. This is importend because there could be deleted elements that the cache does not know about. @@ -288,9 +292,8 @@ class ElementCache: if change_id < lowest_change_id: # When change_id is lower then the lowest change_id in redis, we can # not inform the user about deleted elements. - raise RuntimeError( - f"change_id {change_id} is lower then the lowest change_id in redis {lowest_change_id}. " - "Catch this exception and rerun the method with change_id=0." + raise ChangeIdTooLowError( + f"change_id {change_id} is lower then the lowest change_id in redis {lowest_change_id}." ) raw_changed_elements, deleted_elements = await self.cache_provider.get_data_since( @@ -302,7 +305,9 @@ class ElementCache: } if user_id is not None: - for collection_string, elements in changed_elements.items(): + # the list(...) is important, because `changed_elements` will be + # altered during iteration and restricting data + for collection_string, elements in list(changed_elements.items()): restricter = self.cachables[collection_string].restrict_elements restricted_elements = await restricter(user_id, elements) @@ -332,8 +337,6 @@ class ElementCache: async def get_lowest_change_id(self) -> int: """ Returns the lowest change id. - - Raises a RuntimeError if there is no change_id. """ return await self.cache_provider.get_lowest_change_id() diff --git a/openslides/utils/consumers.py b/openslides/utils/consumers.py index cf3fdd150..49e97ce20 100644 --- a/openslides/utils/consumers.py +++ b/openslides/utils/consumers.py @@ -7,7 +7,7 @@ from ..utils.websocket import WEBSOCKET_CHANGE_ID_TOO_HIGH from . import logging from .auth import async_anonymous_is_enabled from .autoupdate import AutoupdateFormat -from .cache import element_cache, split_element_id +from .cache import ChangeIdTooLowError, element_cache, split_element_id from .utils import get_worker_id from .websocket import ProtocollAsyncJsonWebsocketConsumer @@ -138,7 +138,7 @@ class SiteConsumer(ProtocollAsyncJsonWebsocketConsumer): changed_elements, deleted_element_ids = await element_cache.get_data_since( user_id, change_id, max_change_id ) - except RuntimeError: + except ChangeIdTooLowError: # The change_id is lower the the lowerst change_id in redis. Return all data changed_elements = await element_cache.get_all_data_list(user_id) all_data = True diff --git a/tests/unit/utils/test_cache.py b/tests/unit/utils/test_cache.py index da6639e2c..3da97ff4c 100644 --- a/tests/unit/utils/test_cache.py +++ b/tests/unit/utils/test_cache.py @@ -3,7 +3,7 @@ from typing import Any, Dict, List import pytest -from openslides.utils.cache import ElementCache +from openslides.utils.cache import ChangeIdTooLowError, ElementCache from .cache_provider import TTestCacheProvider, example_data, get_cachable_provider @@ -156,7 +156,7 @@ async def test_get_data_since_change_id_lower_than_in_redis(element_cache): } element_cache.cache_provider.default_change_id = 2 element_cache.cache_provider.change_id_data = {2: {"app/collection1:1"}} - with pytest.raises(RuntimeError): + with pytest.raises(ChangeIdTooLowError): await element_cache.get_data_since(None, 1) @@ -283,7 +283,7 @@ async def test_get_restricted_data_2(element_cache): async def test_get_restricted_data_change_id_lower_than_in_redis(element_cache): element_cache.cache_provider.default_change_id = 2 - with pytest.raises(RuntimeError): + with pytest.raises(ChangeIdTooLowError): await element_cache.get_data_since(0, 1)