From 176eea3b0654b2dfcd7ffa61a49cf708aa432c4d Mon Sep 17 00:00:00 2001 From: Volodymyr Zotov Date: Tue, 2 Apr 2024 21:36:35 -0500 Subject: [PATCH 1/5] Don't throw error when OP_CONNECT_CLIENT_REQ_TIMEOUT is not set --- src/onepasswordconnectsdk/async_client.py | 1 - src/onepasswordconnectsdk/utils.py | 10 ++++------ src/tests/test_client_items.py | 8 +++++++- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/onepasswordconnectsdk/async_client.py b/src/onepasswordconnectsdk/async_client.py index 23c1bfe..2adf379 100644 --- a/src/onepasswordconnectsdk/async_client.py +++ b/src/onepasswordconnectsdk/async_client.py @@ -24,7 +24,6 @@ def __init__(self, url: str, token: str) -> None: self.serializer = Serializer() def create_session(self, url: str, token: str) -> httpx.AsyncClient: - # import here to avoid circular import return httpx.AsyncClient(base_url=url, headers=self.build_headers(token), timeout=get_timeout()) def build_headers(self, token: str) -> Dict[str, str]: diff --git a/src/onepasswordconnectsdk/utils.py b/src/onepasswordconnectsdk/utils.py index 6699e42..4ce4a26 100644 --- a/src/onepasswordconnectsdk/utils.py +++ b/src/onepasswordconnectsdk/utils.py @@ -1,8 +1,6 @@ import os -from typing import Union -from httpx import USE_CLIENT_DEFAULT -from httpx._client import UseClientDefault +from httpx._client import DEFAULT_TIMEOUT_CONFIG, Timeout UUIDLength = 26 ENV_CLIENT_REQUEST_TIMEOUT = "OP_CONNECT_CLIENT_REQ_TIMEOUT" @@ -68,7 +66,7 @@ def _append_path(self, path_chunk: str = None, query: str = None) -> 'PathBuilde self.path += f"?{query}" -def get_timeout() -> Union[int, UseClientDefault]: +def get_timeout() -> Timeout: """Get the timeout to be used in the HTTP Client""" - timeout = int(os.getenv(ENV_CLIENT_REQUEST_TIMEOUT, 0)) - return timeout if timeout else USE_CLIENT_DEFAULT + timeout = float(os.getenv(ENV_CLIENT_REQUEST_TIMEOUT, 0)) + return timeout if timeout else DEFAULT_TIMEOUT_CONFIG diff --git a/src/tests/test_client_items.py b/src/tests/test_client_items.py index d9e23c9..b9b1ca4 100644 --- a/src/tests/test_client_items.py +++ b/src/tests/test_client_items.py @@ -3,6 +3,7 @@ from unittest import mock from httpx import Response +from httpx._client import DEFAULT_TIMEOUT_CONFIG from onepasswordconnectsdk import client, models from onepasswordconnectsdk.utils import ENV_CLIENT_REQUEST_TIMEOUT @@ -446,6 +447,11 @@ def generate_full_item(): return item +def test_default_timeout(): + client_instance = client.new_client(HOST, TOKEN) + assert client_instance.session.timeout.read == DEFAULT_TIMEOUT_CONFIG.read + + def test_set_timeout_using_env_variable(): with mock.patch.dict(os.environ, {ENV_CLIENT_REQUEST_TIMEOUT: '120'}): client_instance = client.new_client(HOST, TOKEN) @@ -456,4 +462,4 @@ def test_set_timeout_using_env_variable(): def test_set_timeout_using_env_variable_async(): with mock.patch.dict(os.environ, {ENV_CLIENT_REQUEST_TIMEOUT: '120'}): client_instance = client.new_client(HOST, TOKEN, is_async=True) - assert client_instance.session.timeout.read == 120 \ No newline at end of file + assert client_instance.session.timeout.read == 120 From dad43fb8a639c78e97c031825ac97652bca23135 Mon Sep 17 00:00:00 2001 From: Volodymyr Zotov Date: Tue, 2 Apr 2024 22:00:09 -0500 Subject: [PATCH 2/5] Cover the case when timeout is None of empty string --- src/onepasswordconnectsdk/utils.py | 11 +++++++++-- src/tests/test_client_items.py | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/onepasswordconnectsdk/utils.py b/src/onepasswordconnectsdk/utils.py index 4ce4a26..1de4678 100644 --- a/src/onepasswordconnectsdk/utils.py +++ b/src/onepasswordconnectsdk/utils.py @@ -68,5 +68,12 @@ def _append_path(self, path_chunk: str = None, query: str = None) -> 'PathBuilde def get_timeout() -> Timeout: """Get the timeout to be used in the HTTP Client""" - timeout = float(os.getenv(ENV_CLIENT_REQUEST_TIMEOUT, 0)) - return timeout if timeout else DEFAULT_TIMEOUT_CONFIG + raw_timeout = os.getenv(ENV_CLIENT_REQUEST_TIMEOUT, 0.0) + if raw_timeout == 'None': + return Timeout(None) # disable all timeouts + elif raw_timeout == '': + return DEFAULT_TIMEOUT_CONFIG + else: + timeout = float(raw_timeout) + t = timeout if timeout else DEFAULT_TIMEOUT_CONFIG + return t diff --git a/src/tests/test_client_items.py b/src/tests/test_client_items.py index b9b1ca4..6767c03 100644 --- a/src/tests/test_client_items.py +++ b/src/tests/test_client_items.py @@ -463,3 +463,21 @@ def test_set_timeout_using_env_variable_async(): with mock.patch.dict(os.environ, {ENV_CLIENT_REQUEST_TIMEOUT: '120'}): client_instance = client.new_client(HOST, TOKEN, is_async=True) assert client_instance.session.timeout.read == 120 + + +def test_disable_all_timeouts(): + with mock.patch.dict(os.environ, {ENV_CLIENT_REQUEST_TIMEOUT: 'None'}): + client_instance = client.new_client(HOST, TOKEN) + assert client_instance.session.timeout.read is None + + +def test_env_client_request_timeout_env_var_is_empty_string(): + with mock.patch.dict(os.environ, {ENV_CLIENT_REQUEST_TIMEOUT: ''}): + client_instance = client.new_client(HOST, TOKEN) + assert client_instance.session.timeout.read == DEFAULT_TIMEOUT_CONFIG.read + + +def test_env_client_request_timeout_env_var_is_zero(): + with mock.patch.dict(os.environ, {ENV_CLIENT_REQUEST_TIMEOUT: '0'}): + client_instance = client.new_client(HOST, TOKEN) + assert client_instance.session.timeout.read == DEFAULT_TIMEOUT_CONFIG.read From 074c2391dd4ca5a7ece95c07c2effb88f94a1748 Mon Sep 17 00:00:00 2001 From: Volodymyr Zotov Date: Tue, 2 Apr 2024 22:04:40 -0500 Subject: [PATCH 3/5] Inline the return statement --- src/onepasswordconnectsdk/utils.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/onepasswordconnectsdk/utils.py b/src/onepasswordconnectsdk/utils.py index 1de4678..6c120b1 100644 --- a/src/onepasswordconnectsdk/utils.py +++ b/src/onepasswordconnectsdk/utils.py @@ -75,5 +75,4 @@ def get_timeout() -> Timeout: return DEFAULT_TIMEOUT_CONFIG else: timeout = float(raw_timeout) - t = timeout if timeout else DEFAULT_TIMEOUT_CONFIG - return t + return timeout if timeout else DEFAULT_TIMEOUT_CONFIG From 32366611468d6b048aa3be1fe50e7be343aa8883 Mon Sep 17 00:00:00 2001 From: Volodymyr Zotov Date: Wed, 3 Apr 2024 08:11:21 -0500 Subject: [PATCH 4/5] Improve get_timeout function to cover all non numeric strings --- src/onepasswordconnectsdk/utils.py | 8 ++++---- src/tests/test_client_items.py | 12 ++++++++++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/onepasswordconnectsdk/utils.py b/src/onepasswordconnectsdk/utils.py index 6c120b1..b287477 100644 --- a/src/onepasswordconnectsdk/utils.py +++ b/src/onepasswordconnectsdk/utils.py @@ -68,11 +68,11 @@ def _append_path(self, path_chunk: str = None, query: str = None) -> 'PathBuilde def get_timeout() -> Timeout: """Get the timeout to be used in the HTTP Client""" - raw_timeout = os.getenv(ENV_CLIENT_REQUEST_TIMEOUT, 0.0) + raw_timeout = os.getenv(ENV_CLIENT_REQUEST_TIMEOUT, '0.0') if raw_timeout == 'None': return Timeout(None) # disable all timeouts - elif raw_timeout == '': - return DEFAULT_TIMEOUT_CONFIG - else: + elif raw_timeout.isnumeric(): timeout = float(raw_timeout) return timeout if timeout else DEFAULT_TIMEOUT_CONFIG + else: + return DEFAULT_TIMEOUT_CONFIG diff --git a/src/tests/test_client_items.py b/src/tests/test_client_items.py index 6767c03..9e471c4 100644 --- a/src/tests/test_client_items.py +++ b/src/tests/test_client_items.py @@ -477,6 +477,18 @@ def test_env_client_request_timeout_env_var_is_empty_string(): assert client_instance.session.timeout.read == DEFAULT_TIMEOUT_CONFIG.read +def test_env_client_request_timeout_env_var_is_single_space_string(): + with mock.patch.dict(os.environ, {ENV_CLIENT_REQUEST_TIMEOUT: ' '}): + client_instance = client.new_client(HOST, TOKEN) + assert client_instance.session.timeout.read == DEFAULT_TIMEOUT_CONFIG.read + + +def test_env_client_request_timeout_env_var_is_not_numeric_string(): + with mock.patch.dict(os.environ, {ENV_CLIENT_REQUEST_TIMEOUT: 'abc'}): + client_instance = client.new_client(HOST, TOKEN) + assert client_instance.session.timeout.read == DEFAULT_TIMEOUT_CONFIG.read + + def test_env_client_request_timeout_env_var_is_zero(): with mock.patch.dict(os.environ, {ENV_CLIENT_REQUEST_TIMEOUT: '0'}): client_instance = client.new_client(HOST, TOKEN) From 4bcf4d79089484c7dbc034123be24612c7dbe557 Mon Sep 17 00:00:00 2001 From: Volodymyr Zotov Date: Wed, 3 Apr 2024 10:12:12 -0500 Subject: [PATCH 5/5] Add additional unit test to check that it uses default timeout if env var has negative number --- src/tests/test_client_items.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/tests/test_client_items.py b/src/tests/test_client_items.py index 9e471c4..b670f7d 100644 --- a/src/tests/test_client_items.py +++ b/src/tests/test_client_items.py @@ -493,3 +493,9 @@ def test_env_client_request_timeout_env_var_is_zero(): with mock.patch.dict(os.environ, {ENV_CLIENT_REQUEST_TIMEOUT: '0'}): client_instance = client.new_client(HOST, TOKEN) assert client_instance.session.timeout.read == DEFAULT_TIMEOUT_CONFIG.read + + +def test_env_client_request_timeout_env_var_is_negative_number(): + with mock.patch.dict(os.environ, {ENV_CLIENT_REQUEST_TIMEOUT: '-10'}): + client_instance = client.new_client(HOST, TOKEN) + assert client_instance.session.timeout.read == DEFAULT_TIMEOUT_CONFIG.read