fix(auth): improving cache handling and error handling#52
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces graceful handling of cache failures in the remote authorization logic by wrapping cache operations in try...except blocks. It also includes a version bump to 0.4.3 and adds unit tests to verify system behavior when cache retrieval or storage fails. Feedback was provided to restore explicit checks for the existence of the cache object to avoid unnecessary AttributeError exceptions and misleading log warnings when no cache is configured.
| cached = None | ||
| try: | ||
| cached = await cache.get(key) | ||
| except Exception as cache_error: | ||
| logger.warning("Cache get failed, proceeding without cache: %s", cache_error) |
There was a problem hiding this comment.
The explicit check for the existence of the cache object was removed in this change. Since cache is optional and defaults to None, calling cache.get(key) will raise an AttributeError when no cache is configured. While the try...except block catches this, it results in misleading 'Cache get failed' warnings in the logs for a normal configuration. It is recommended to restore the if cache: check.
| cached = None | |
| try: | |
| cached = await cache.get(key) | |
| except Exception as cache_error: | |
| logger.warning("Cache get failed, proceeding without cache: %s", cache_error) | |
| cached = None | |
| if cache: | |
| try: | |
| cached = await cache.get(key) | |
| except Exception as cache_error: | |
| logger.warning("Cache get failed, proceeding without cache: %s", cache_error) |
| try: | ||
| value = json.dumps({"authorized": authorized, "context": context}) | ||
| await cache.set(key, value, cache_ttl) | ||
| except Exception as cache_error: | ||
| logger.warning("Cache set failed, proceeding without cache: %s", cache_error) |
There was a problem hiding this comment.
Similar to the cache retrieval, the check if cache: was removed here. If cache is None, calling cache.set(...) will raise an AttributeError. This should be avoided by checking for the cache object's existence explicitly to prevent unnecessary exceptions and misleading log messages.
| try: | |
| value = json.dumps({"authorized": authorized, "context": context}) | |
| await cache.set(key, value, cache_ttl) | |
| except Exception as cache_error: | |
| logger.warning("Cache set failed, proceeding without cache: %s", cache_error) | |
| if cache: | |
| try: | |
| value = json.dumps({"authorized": authorized, "context": context}) | |
| await cache.set(key, value, cache_ttl) | |
| except Exception as cache_error: | |
| logger.warning("Cache set failed, proceeding without cache: %s", cache_error) |
Contain
Details