From ee4597d53f331c557f4b54795d9c9fe0f181c139 Mon Sep 17 00:00:00 2001 From: Sujai Kumar Gupta Date: Fri, 26 Jan 2024 22:52:19 +0530 Subject: [PATCH 1/3] Refactor error handling in CustomDjangoCache --- .../deployment/default/custom_django_cache.py | 177 ++++++++++-------- 1 file changed, 95 insertions(+), 82 deletions(-) diff --git a/kolibri/deployment/default/custom_django_cache.py b/kolibri/deployment/default/custom_django_cache.py index ee7dc661c43..065f618b43f 100644 --- a/kolibri/deployment/default/custom_django_cache.py +++ b/kolibri/deployment/default/custom_django_cache.py @@ -13,6 +13,32 @@ class CustomDjangoCache(DjangoCache): https://github.com/grantjenks/python-diskcache/blob/v4.1.0/diskcache/djangocache.py """ + ERRORS_TO_HANDLE = (sqlite3.OperationalError, AssertionError) + + def try_execute_return_none_on_error(self, method, *args, **kwargs): + try: + return method(*args, **kwargs) + except self.ERRORS_TO_HANDLE: + return None + + def try_execute_return_false_on_error(self, method, *args, **kwargs): + try: + return method(*args, **kwargs) + except self.ERRORS_TO_HANDLE: + return False + + def try_execute_no_return_on_error(self, method, *args, **kwargs): + try: + method(*args, **kwargs) + except self.ERRORS_TO_HANDLE: + pass + + def try_execute_return_zero_on_error(self, method, *args, **kwargs): + try: + return method(*args, **kwargs) + except self.ERRORS_TO_HANDLE: + return 0 + def add( self, key, @@ -23,12 +49,16 @@ def add( tag=None, retry=True, ): - try: - return super(CustomDjangoCache, self).add( - key, value, timeout, version, read, tag, retry - ) - except sqlite3.OperationalError: - return False + return self.try_execute_return_false_on_error( + super(CustomDjangoCache, self).add, + key, + value, + timeout, + version, + read, + tag, + retry, + ) def has_key(self, key, version=None): """Returns True if the key is in the cache and has not expired. @@ -38,12 +68,9 @@ def has_key(self, key, version=None): :return: True if key is found """ - try: - return super(CustomDjangoCache, self).has_key( # noqa: W601 - key, version=version - ) - except sqlite3.OperationalError: - return False + return self.try_execute_return_false_on_error( + super(CustomDjangoCache, self).has_key, key, version=version + ) def get( self, @@ -55,12 +82,16 @@ def get( tag=False, retry=False, ): - try: - return super(CustomDjangoCache, self).get( - key, default, version, read, expire_time, tag, retry - ) - except sqlite3.OperationalError: - return None + return self.try_execute_return_none_on_error( + super(CustomDjangoCache, self).get, + key, + default, + version, + read, + expire_time, + tag, + retry, + ) def set( self, @@ -72,95 +103,77 @@ def set( tag=None, retry=True, ): - try: - return super(CustomDjangoCache, self).set( - key, value, timeout, version, read, tag, retry - ) - except sqlite3.OperationalError: - return False + return self.try_execute_return_false_on_error( + super(CustomDjangoCache, self).set, + key, + value, + timeout, + version, + read, + tag, + retry, + ) def touch(self, key, timeout=DEFAULT_TIMEOUT, version=None, retry=True): - try: - return super(CustomDjangoCache, self).touch(key, timeout, version, retry) - except sqlite3.OperationalError: - return False + return self.try_execute_return_false_on_error( + super(CustomDjangoCache, self).touch, key, timeout, version, retry + ) def pop( self, key, default=None, version=None, expire_time=False, tag=False, retry=True ): - try: - return super(CustomDjangoCache, self).pop( - key, default, version, expire_time, tag, retry - ) - except sqlite3.OperationalError: - return None + return self.try_execute_return_none_on_error( + super(CustomDjangoCache, self).pop, + key, + default, + version, + expire_time, + tag, + retry, + ) def delete(self, key, version=None, retry=True): - try: - super(CustomDjangoCache, self).delete(key, version, retry) - except sqlite3.OperationalError: - pass + self.try_execute_no_return_on_error( + super(CustomDjangoCache, self).delete, key, version, retry + ) def incr(self, key, delta=1, version=None, default=None, retry=True): - try: - return super(CustomDjangoCache, self).incr( - key, delta, version, default, retry - ) - except sqlite3.OperationalError: - return None + return self.try_execute_return_none_on_error( + super(CustomDjangoCache, self).incr, key, delta, version, default, retry + ) def decr(self, key, delta=1, version=None, default=None, retry=True): - try: - return super(CustomDjangoCache, self).decr( - key, delta, version, default, retry - ) - except sqlite3.OperationalError: - return None + return self.try_execute_return_none_on_error( + super(CustomDjangoCache, self).decr, key, delta, version, default, retry + ) def expire(self, retry=False): - try: - return super(CustomDjangoCache, self).expire(retry) - except sqlite3.OperationalError: - return 0 + return self.try_execute_return_zero_on_error(super().expire, retry) def stats(self, enable=True, reset=False): - try: - return super(CustomDjangoCache, self).stats(enable, reset) - except sqlite3.OperationalError: - return 0, 0 + result = self.try_execute_return_none_on_error(super().stats, enable, reset) + return result if isinstance(result, tuple) else (0, 0) def create_tag_index(self): - try: - super(CustomDjangoCache, self).create_tag_index() - except sqlite3.OperationalError: - pass + self.try_execute_no_return_on_error( + super(CustomDjangoCache, self).create_tag_index + ) def drop_tag_index(self): - try: - super(CustomDjangoCache, self).drop_tag_index() - except sqlite3.OperationalError: - pass + self.try_execute_no_return_on_error( + super(CustomDjangoCache, self).drop_tag_index + ) def evict(self, tag): - try: - return super(CustomDjangoCache, self).evict(tag) - except sqlite3.OperationalError: - return 0 + return self.try_execute_return_zero_on_error(super().evict, tag) def cull(self): - try: - return super(CustomDjangoCache, self).cull() - except sqlite3.OperationalError: - return 0 + return self.try_execute_return_zero_on_error(super().cull) def clear(self): - try: - return super(CustomDjangoCache, self).clear() - except sqlite3.OperationalError: - return 0 + return self.try_execute_return_zero_on_error(super().clear) def close(self, **kwargs): - try: - super(CustomDjangoCache, self).close(**kwargs) - except sqlite3.OperationalError: - pass + self.try_execute_no_return_on_error( + super(CustomDjangoCache, self).close, **kwargs + ) From e7f2ff571839ebd9f9b50d901599f59d12ef8394 Mon Sep 17 00:00:00 2001 From: Sujai Kumar Gupta Date: Sat, 27 Jan 2024 12:31:10 +0530 Subject: [PATCH 2/3] refactor: use single method for error_handling in CustomDjangoCache --- .../deployment/default/custom_django_cache.py | 178 ++++++++++-------- 1 file changed, 98 insertions(+), 80 deletions(-) diff --git a/kolibri/deployment/default/custom_django_cache.py b/kolibri/deployment/default/custom_django_cache.py index 065f618b43f..c36b25f949a 100644 --- a/kolibri/deployment/default/custom_django_cache.py +++ b/kolibri/deployment/default/custom_django_cache.py @@ -15,29 +15,27 @@ class CustomDjangoCache(DjangoCache): ERRORS_TO_HANDLE = (sqlite3.OperationalError, AssertionError) - def try_execute_return_none_on_error(self, method, *args, **kwargs): - try: - return method(*args, **kwargs) - except self.ERRORS_TO_HANDLE: - return None - - def try_execute_return_false_on_error(self, method, *args, **kwargs): - try: - return method(*args, **kwargs) - except self.ERRORS_TO_HANDLE: - return False - - def try_execute_no_return_on_error(self, method, *args, **kwargs): - try: - method(*args, **kwargs) - except self.ERRORS_TO_HANDLE: - pass - - def try_execute_return_zero_on_error(self, method, *args, **kwargs): + def try_execute(self, method_name, error_return_value, *args, **kwargs): + """ + Safely executes a method with error handling. + :param method_name --> (str): name of method to execute + :param error_return_value --> (any): value to return if error occur + :param *args: positional arguments for method + :param *kwargs: keyword arguments for method + :return: The return value of the executed method if successful, + otherwise returns error_return_value + """ try: + method = getattr(super(CustomDjangoCache, self), method_name) + if method is None: + raise ValueError( + "{method_name} is not a valid method".format( + method_name=method_name + ) + ) return method(*args, **kwargs) except self.ERRORS_TO_HANDLE: - return 0 + return error_return_value def add( self, @@ -49,15 +47,16 @@ def add( tag=None, retry=True, ): - return self.try_execute_return_false_on_error( - super(CustomDjangoCache, self).add, - key, - value, - timeout, - version, - read, - tag, - retry, + return self.try_execute( + method_name="add", + error_return_value=False, + key=key, + value=value, + timeout=timeout, + version=version, + read=read, + tag=tag, + retry=retry, ) def has_key(self, key, version=None): @@ -68,8 +67,8 @@ def has_key(self, key, version=None): :return: True if key is found """ - return self.try_execute_return_false_on_error( - super(CustomDjangoCache, self).has_key, key, version=version + return self.try_execute( + method_name="has_key", error_return_value=False, key=key, version=version ) def get( @@ -82,15 +81,16 @@ def get( tag=False, retry=False, ): - return self.try_execute_return_none_on_error( - super(CustomDjangoCache, self).get, - key, - default, - version, - read, - expire_time, - tag, - retry, + return self.try_execute( + method_name="get", + error_return_value=None, + key=key, + default=default, + version=version, + read=read, + expire_time=expire_time, + tag=tag, + retry=retry, ) def set( @@ -103,77 +103,95 @@ def set( tag=None, retry=True, ): - return self.try_execute_return_false_on_error( - super(CustomDjangoCache, self).set, - key, - value, - timeout, - version, - read, - tag, - retry, + return self.try_execute( + method_name="set", + error_return_value=False, + key=key, + value=value, + timeout=timeout, + version=version, + read=read, + tag=tag, + retry=retry, ) def touch(self, key, timeout=DEFAULT_TIMEOUT, version=None, retry=True): - return self.try_execute_return_false_on_error( - super(CustomDjangoCache, self).touch, key, timeout, version, retry + return self.try_execute( + method_name="touch", + error_return_value=False, + key=key, + timeout=timeout, + version=version, + retry=retry, ) def pop( self, key, default=None, version=None, expire_time=False, tag=False, retry=True ): - return self.try_execute_return_none_on_error( - super(CustomDjangoCache, self).pop, - key, - default, - version, - expire_time, - tag, - retry, + return self.try_execute( + method_name="pop", + error_return_value=None, + key=key, + default=default, + version=version, + expire_time=expire_time, + tag=tag, + retry=retry, ) def delete(self, key, version=None, retry=True): - self.try_execute_no_return_on_error( - super(CustomDjangoCache, self).delete, key, version, retry + self.try_execute( + method_name="delete", + error_return_value=None, + key=key, + version=version, + retry=retry, ) def incr(self, key, delta=1, version=None, default=None, retry=True): - return self.try_execute_return_none_on_error( - super(CustomDjangoCache, self).incr, key, delta, version, default, retry + return self.try_execute( + method_name="incr", + error_return_value=None, + key=key, + delta=delta, + version=version, + default=default, + retry=retry, ) def decr(self, key, delta=1, version=None, default=None, retry=True): - return self.try_execute_return_none_on_error( - super(CustomDjangoCache, self).decr, key, delta, version, default, retry + return self.try_execute( + method_name="decr", + key=key, + error_return_value=None, + delta=delta, + version=version, + default=default, + retry=retry, ) def expire(self, retry=False): - return self.try_execute_return_zero_on_error(super().expire, retry) + return self.try_execute(method_name="expire", error_return_value=0, retry=retry) def stats(self, enable=True, reset=False): - result = self.try_execute_return_none_on_error(super().stats, enable, reset) - return result if isinstance(result, tuple) else (0, 0) + return self.try_execute( + method_name="stats", error_return_value=(0, 0), enable=enable, reset=reset + ) def create_tag_index(self): - self.try_execute_no_return_on_error( - super(CustomDjangoCache, self).create_tag_index - ) + return self.try_execute(method_name="create_tag_index", error_return_value=None) def drop_tag_index(self): - self.try_execute_no_return_on_error( - super(CustomDjangoCache, self).drop_tag_index - ) + return self.try_execute(method_name="drop_tag_index", error_return_value=None) def evict(self, tag): - return self.try_execute_return_zero_on_error(super().evict, tag) + return self.try_execute(method_name="evict", error_return_value=0, tag=tag) def cull(self): - return self.try_execute_return_zero_on_error(super().cull) + return self.try_execute(method_name="cull", error_return_value=0) def clear(self): - return self.try_execute_return_zero_on_error(super().clear) + return self.try_execute(method_name="clear", error_return_value=0) def close(self, **kwargs): - self.try_execute_no_return_on_error( - super(CustomDjangoCache, self).close, **kwargs - ) + return self.try_execute(method_name="close", error_return_value=None, **kwargs) From 49575e6726b5259376fe9b2356c95e6b06eb3943 Mon Sep 17 00:00:00 2001 From: Sujai Kumar Gupta Date: Thu, 1 Feb 2024 11:26:57 +0530 Subject: [PATCH 3/3] pass method and error_return_value as positional args --- .../deployment/default/custom_django_cache.py | 54 +++++++++---------- 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/kolibri/deployment/default/custom_django_cache.py b/kolibri/deployment/default/custom_django_cache.py index c36b25f949a..a81d95392fd 100644 --- a/kolibri/deployment/default/custom_django_cache.py +++ b/kolibri/deployment/default/custom_django_cache.py @@ -48,8 +48,8 @@ def add( retry=True, ): return self.try_execute( - method_name="add", - error_return_value=False, + "add", + False, key=key, value=value, timeout=timeout, @@ -67,9 +67,7 @@ def has_key(self, key, version=None): :return: True if key is found """ - return self.try_execute( - method_name="has_key", error_return_value=False, key=key, version=version - ) + return self.try_execute("has_key", False, key=key, version=version) def get( self, @@ -82,8 +80,8 @@ def get( retry=False, ): return self.try_execute( - method_name="get", - error_return_value=None, + "get", + None, key=key, default=default, version=version, @@ -104,8 +102,8 @@ def set( retry=True, ): return self.try_execute( - method_name="set", - error_return_value=False, + "set", + False, key=key, value=value, timeout=timeout, @@ -117,8 +115,8 @@ def set( def touch(self, key, timeout=DEFAULT_TIMEOUT, version=None, retry=True): return self.try_execute( - method_name="touch", - error_return_value=False, + "touch", + False, key=key, timeout=timeout, version=version, @@ -129,8 +127,8 @@ def pop( self, key, default=None, version=None, expire_time=False, tag=False, retry=True ): return self.try_execute( - method_name="pop", - error_return_value=None, + "pop", + None, key=key, default=default, version=version, @@ -141,8 +139,8 @@ def pop( def delete(self, key, version=None, retry=True): self.try_execute( - method_name="delete", - error_return_value=None, + "delete", + None, key=key, version=version, retry=retry, @@ -150,8 +148,8 @@ def delete(self, key, version=None, retry=True): def incr(self, key, delta=1, version=None, default=None, retry=True): return self.try_execute( - method_name="incr", - error_return_value=None, + "incr", + None, key=key, delta=delta, version=version, @@ -161,9 +159,9 @@ def incr(self, key, delta=1, version=None, default=None, retry=True): def decr(self, key, delta=1, version=None, default=None, retry=True): return self.try_execute( - method_name="decr", + "decr", + None, key=key, - error_return_value=None, delta=delta, version=version, default=default, @@ -171,27 +169,25 @@ def decr(self, key, delta=1, version=None, default=None, retry=True): ) def expire(self, retry=False): - return self.try_execute(method_name="expire", error_return_value=0, retry=retry) + return self.try_execute("expire", 0, retry=retry) def stats(self, enable=True, reset=False): - return self.try_execute( - method_name="stats", error_return_value=(0, 0), enable=enable, reset=reset - ) + return self.try_execute("stats", (0, 0), enable=enable, reset=reset) def create_tag_index(self): - return self.try_execute(method_name="create_tag_index", error_return_value=None) + return self.try_execute("create_tag_index", None) def drop_tag_index(self): - return self.try_execute(method_name="drop_tag_index", error_return_value=None) + return self.try_execute("drop_tag_index", None) def evict(self, tag): - return self.try_execute(method_name="evict", error_return_value=0, tag=tag) + return self.try_execute("evict", 0, tag=tag) def cull(self): - return self.try_execute(method_name="cull", error_return_value=0) + return self.try_execute("cull", 0) def clear(self): - return self.try_execute(method_name="clear", error_return_value=0) + return self.try_execute("clear", 0) def close(self, **kwargs): - return self.try_execute(method_name="close", error_return_value=None, **kwargs) + return self.try_execute("close", None, **kwargs)