Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor error handling in CustomDjangoCache #11808

Merged
merged 3 commits into from
Feb 23, 2024
Merged

Refactor error handling in CustomDjangoCache #11808

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ee4597d
Refactor error handling in CustomDjangoCache
thesujai Jan 26, 2024
e7f2ff5
refactor: use single method for error_handling in CustomDjangoCache
thesujai Jan 27, 2024
49575e6
pass method and error_return_value as positional args
thesujai Feb 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
177 changes: 95 additions & 82 deletions kolibri/deployment/default/custom_django_cache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could simplify this all somewhat by having a single method like this:

def try_execute(self, method_name, error_return_value, *args, **kwargs):
    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 error_return_value

That way you can guarantee you're using the Py2.7 compatible form of super, and you don't need to define 4 different functions that are doing the same thing.

If a function doesn't return a value explicitly in Python, it returns None, so you can pass None as the error_return_value in those cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the code snippet, indeed this thought crossed my mind, but i was confused about error_return_value having types. Now it makes more sense to me

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,
Expand All @@ -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.
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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
)
Loading