[GitHub] [airflow] davlum commented on a change in pull request #17387: Invalidate Vault cached prop when not authenticated

[GitBox <gi...@apache.org>]

davlum commented on a change in pull request #17387:
URL: https://github.com/apache/airflow/pull/17387#discussion_r681851621



##########
File path: airflow/providers/hashicorp/_internal_client/vault_client.py
##########
@@ -191,8 +191,21 @@ def __init__(
         self.radius_secret = radius_secret
         self.radius_port = radius_port
 
+    @property
+    def client(self):
+        """
+        Wrapper function to invalidate cache
+
+        :rtype: hvac.Client
+        :return: Vault Client
+
+        """
+        if '_client' in self.__dict__ and not self._client.is_authenticated():
+            del self.__dict__['_client']
+        return self._client

Review comment:
       I'm not sure it is easier to understand. The assignment here is implicitly making sure that `_client` is inside `__dict__`, whereas the `if '_client' in self.__dict__` is explicit. I agree that modifying `__dict__` is hacky, but as mentioned this is just how `cached_property` works and is documented. 
   
   I'm not sure that we want to use the `lru_cache`, as it could be semantically confusing here. There is no benefit to using a lru except for the convenient `.cache_clear()`. I think adding a comment is definitely a good idea though.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org