From 95fcd3260ee801ee7659b525b2e81c7fc8208d03 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Tue, 14 Dec 2021 19:54:25 -0500 Subject: [PATCH 01/33] Added account_name to client and settings and connection, where client is called. Removed logic to retrieve account_id from manager; replaced with logic in client. --- src/firebolt/async_db/connection.py | 10 +++++++++- src/firebolt/client/client.py | 20 +++++++++++++++++--- src/firebolt/common/settings.py | 1 + src/firebolt/service/manager.py | 17 +++-------------- 4 files changed, 30 insertions(+), 18 deletions(-) diff --git a/src/firebolt/async_db/connection.py b/src/firebolt/async_db/connection.py index 62f25351b6b..60b97e7a202 100644 --- a/src/firebolt/async_db/connection.py +++ b/src/firebolt/async_db/connection.py @@ -21,11 +21,16 @@ async def _resolve_engine_url( - engine_name: str, username: str, password: str, api_endpoint: str + engine_name: str, + username: str, + password: str, + account_name: str, + api_endpoint: str, ) -> str: async with AsyncClient( auth=(username, password), base_url=api_endpoint, + account_name=account_name, api_endpoint=api_endpoint, ) as client: try: @@ -130,6 +135,7 @@ class BaseConnection: "_cursors", "database", "engine_url", + "account_name", "api_endpoint", "_is_closed", ) @@ -140,11 +146,13 @@ def __init__( database: str, # TODO: Get by engine name username: str, password: str, + account_name: str, api_endpoint: str = DEFAULT_API_URL, ): self._client = AsyncClient( auth=(username, password), base_url=engine_url, + account_name=account_name, api_endpoint=api_endpoint, timeout=Timeout(DEFAULT_TIMEOUT_SECONDS, read=None), ) diff --git a/src/firebolt/client/client.py b/src/firebolt/client/client.py index f8713f85765..40bb09bb684 100644 --- a/src/firebolt/client/client.py +++ b/src/firebolt/client/client.py @@ -9,7 +9,7 @@ from firebolt.client.auth import Auth from firebolt.client.constants import DEFAULT_API_URL -from firebolt.common.urls import ACCOUNT_URL +from firebolt.common.urls import ACCOUNT_BY_NAME_URL, ACCOUNT_URL from firebolt.common.util import cached_property, fix_url_schema, mixin_for FireboltClientMixinBase = mixin_for(HttpxClient) # type: Any @@ -19,10 +19,12 @@ class FireboltClientMixin(FireboltClientMixinBase): def __init__( self, *args: Any, + account_name: str = None, api_endpoint: str = DEFAULT_API_URL, auth: AuthTypes = None, **kwargs: Any, ): + self.account_name = account_name self._api_endpoint = fix_url_schema(api_endpoint) super().__init__(*args, auth=auth, **kwargs) @@ -55,7 +57,12 @@ class Client(FireboltClientMixin, HttpxClient): @cached_property def account_id(self) -> str: - return self.get(url=ACCOUNT_URL).json()["account"]["id"] + if self.account_name is not None: + return self.get( + url=ACCOUNT_BY_NAME_URL, params={"account_name": self.account_name} + ).json()["account_id"] + else: # account_name isn't set, use the default account. + return self.get(url=ACCOUNT_URL).json()["account"]["id"] class AsyncClient(FireboltClientMixin, HttpxAsyncClient): @@ -74,4 +81,11 @@ class AsyncClient(FireboltClientMixin, HttpxAsyncClient): @async_cached_property async def account_id(self) -> str: - return (await self.get(url=ACCOUNT_URL)).json()["account"]["id"] + if self.account_name is not None: + return ( + await self.get( + url=ACCOUNT_BY_NAME_URL, params={"account_name": self.account_name} + ) + ).json()["account_id"] + else: # account_name isn't set; use the default account. + return (await self.get(url=ACCOUNT_URL)).json()["account"]["id"] diff --git a/src/firebolt/common/settings.py b/src/firebolt/common/settings.py index 2bccc54798b..317306116ab 100644 --- a/src/firebolt/common/settings.py +++ b/src/firebolt/common/settings.py @@ -2,6 +2,7 @@ class Settings(BaseSettings): + account_name: str = Field(None, env="FIREBOLT_ACCOUNT") server: str = Field(..., env="FIREBOLT_SERVER") user: str = Field(..., env="FIREBOLT_USER") password: SecretStr = Field(..., env="FIREBOLT_PASSWORD") diff --git a/src/firebolt/service/manager.py b/src/firebolt/service/manager.py index 8e7eddc9dc4..1a5115d9092 100644 --- a/src/firebolt/service/manager.py +++ b/src/firebolt/service/manager.py @@ -4,7 +4,6 @@ from firebolt.client import Client, log_request, log_response, raise_on_4xx_5xx from firebolt.common import Settings -from firebolt.common.urls import ACCOUNT_BY_NAME_URL from firebolt.service.provider import get_provider_id DEFAULT_TIMEOUT_SECONDS: int = 60 * 2 @@ -32,6 +31,7 @@ def __init__( self.client = Client( auth=(self.settings.user, self.settings.password.get_secret_value()), base_url=f"https://{ self.settings.server}", + account_name=self.settings.account_name, api_endpoint=self.settings.server, timeout=Timeout(DEFAULT_TIMEOUT_SECONDS), ) @@ -40,7 +40,7 @@ def __init__( "response": [raise_on_4xx_5xx, log_response], } - self.account_id = self._get_account_id(account_name=account_name) + self.account_id = self.client.account_id self._init_services() def _init_services(self) -> None: @@ -64,15 +64,4 @@ def _init_services(self) -> None: self.bindings = BindingService(resource_manager=self) def _get_account_id(self, account_name: Optional[str]) -> str: - """ - Given account_name, look up account_id. If account_name is None, - get the default account_id. - - Args: - account_name: Name of the account. - """ - if account_name is None: - return self.client.account_id - return self.client.get( - url=ACCOUNT_BY_NAME_URL, params={"account_name": account_name.lower()} - ).json()["account_id"] + return self.client.account_id From a0923fac9a0118e31ddd369eaa4e947d76a600dd Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Thu, 16 Dec 2021 15:22:00 -0500 Subject: [PATCH 02/33] Added account_name where required in asynch connection.py. --- src/firebolt/async_db/connection.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/firebolt/async_db/connection.py b/src/firebolt/async_db/connection.py index 60b97e7a202..2013ab27004 100644 --- a/src/firebolt/async_db/connection.py +++ b/src/firebolt/async_db/connection.py @@ -67,6 +67,7 @@ async def connect_inner( password: str = None, engine_name: Optional[str] = None, engine_url: Optional[str] = None, + account_name: Optional[str] = None, api_endpoint: str = DEFAULT_API_URL, ) -> Connection: cleandoc( @@ -79,6 +80,7 @@ async def connect_inner( password - password to use for authentication engine_name - name of the engine to connect to engine_url - engine endpoint to use + account_name - for customers with multiple accounts; if blank uses default note: either engine_name or engine_url should be provided, but not both """ ) @@ -116,6 +118,7 @@ async def connect_inner( engine_name, username, password, + account_name, api_endpoint, ) From 24638019469b5b43d4c92a0ed5e4e79e5660440d Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Sun, 19 Dec 2021 15:37:10 -0500 Subject: [PATCH 03/33] Added account_name to unit/db/text_connection. Added clarifying notes to docstrings in asynch connections.py and connections.py. --- src/firebolt/async_db/connection.py | 2 ++ src/firebolt/db/connection.py | 5 +++-- tests/unit/db/test_connection.py | 3 ++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/firebolt/async_db/connection.py b/src/firebolt/async_db/connection.py index 2013ab27004..b4170f6d71b 100644 --- a/src/firebolt/async_db/connection.py +++ b/src/firebolt/async_db/connection.py @@ -81,6 +81,7 @@ async def connect_inner( engine_name - name of the engine to connect to engine_url - engine endpoint to use account_name - for customers with multiple accounts; if blank uses default + api_endpoint(optional) - Firebolt API endpoint. Used for authentication. note: either engine_name or engine_url should be provided, but not both """ ) @@ -218,6 +219,7 @@ class Connection(BaseConnection): database - Firebolt database name username - Firebolt account username password - Firebolt account password + account_name - for entities with more than one account api_endpoint(optional) - Firebolt API endpoint. Used for authentication Methods: diff --git a/src/firebolt/db/connection.py b/src/firebolt/db/connection.py index 6c3303a0c55..9697511650f 100644 --- a/src/firebolt/db/connection.py +++ b/src/firebolt/db/connection.py @@ -27,11 +27,12 @@ class Connection(AsyncBaseConnection): database - Firebolt database name username - Firebolt account username password - Firebolt account password - api_endpoint(optional) - Firebolt API endpoint. Used for authentication + account_name - for customers with multiple accounts; if blank uses default + api_endpoint(optional) - Firebolt API endpoint. Used for authentication. Methods: cursor - create new Cursor object - close - close the Connection and all it's cursors + close - close the Connection and all its cursors Firebolt currenly doesn't support transactions so commit and rollback methods are not implemented. diff --git a/tests/unit/db/test_connection.py b/tests/unit/db/test_connection.py index d43d091a554..a0fcb8f0632 100644 --- a/tests/unit/db/test_connection.py +++ b/tests/unit/db/test_connection.py @@ -155,13 +155,14 @@ def test_connect_engine_name( database=db_name, username="u", password="p", + account_name="", api_endpoint=settings.server, ) as connection: assert connection.cursor().execute("select*") == len(python_query_data) def test_connection_unclosed_warnings(): - c = Connection("", "", "", "", "") + c = Connection("", "", "", "", "", "") with warns(UserWarning) as winfo: del c From f7d1547bbdbc29fa66b67c18e8dae99dc3ee300d Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Sun, 19 Dec 2021 16:42:47 -0500 Subject: [PATCH 04/33] Added account_name to Settings in unit/async_db/conftest.py. --- tests/unit/async_db/conftest.py | 1 + tests/unit/async_db/test_connection.py | 1 + tests/unit/conftest.py | 1 + 3 files changed, 3 insertions(+) diff --git a/tests/unit/async_db/conftest.py b/tests/unit/async_db/conftest.py index 436d45d90f1..63749606dcd 100644 --- a/tests/unit/async_db/conftest.py +++ b/tests/unit/async_db/conftest.py @@ -193,6 +193,7 @@ async def connection(settings: Settings, db_name: str) -> Connection: database=db_name, username="u", password="p", + account_name=settings.account_name, api_endpoint=settings.server, ) ) as connection: diff --git a/tests/unit/async_db/test_connection.py b/tests/unit/async_db/test_connection.py index cae65e96efe..58db3051ac8 100644 --- a/tests/unit/async_db/test_connection.py +++ b/tests/unit/async_db/test_connection.py @@ -68,6 +68,7 @@ async def test_cursor_initialized( database=db_name, username="u", password="p", + account_name=settings.account_name, api_endpoint=settings.server, ) ) as connection: diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index a065e26ac51..483585a59cd 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -94,6 +94,7 @@ def settings(server, region_1) -> Settings: user="email@domain.com", password=SecretStr("*****"), default_region=region_1.name, + account_name="account", ) From 97dc64b2237fb45f0e5d8ad34da55800411dd607 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Sun, 19 Dec 2021 18:41:25 -0500 Subject: [PATCH 05/33] Added account_name to readme_management.md. Added url and url_callback for account_id. --- README_MANAGEMENT.md | 1 + src/firebolt/async_db/connection.py | 2 +- src/firebolt/db/connection.py | 3 ++- tests/unit/async_db/test_connection.py | 7 ++++++- tests/unit/conftest.py | 14 ++++++++++---- tests/unit/db/test_connection.py | 2 +- 6 files changed, 21 insertions(+), 8 deletions(-) diff --git a/README_MANAGEMENT.md b/README_MANAGEMENT.md index b752117e736..8c1141685f2 100644 --- a/README_MANAGEMENT.md +++ b/README_MANAGEMENT.md @@ -37,6 +37,7 @@ rm = ResourceManager(settings=Settings( server="api.app.firebolt.io", user="email@domain.com", password=SecretStr("*****"), + account_name="account", # Necessary if you have multiple accounts. default_region="us-east-1", )) print(rm.client.account_id) # see your account id diff --git a/src/firebolt/async_db/connection.py b/src/firebolt/async_db/connection.py index 553ef576e8a..84f27c251ef 100644 --- a/src/firebolt/async_db/connection.py +++ b/src/firebolt/async_db/connection.py @@ -222,7 +222,7 @@ class Connection(BaseConnection): database: Firebolt database name username: Firebolt account username password: Firebolt account password - account_name: for entities with more than one account + account_name: Necessary for entities with more than one account. api_endpoint: Optional. Firebolt API endpoint. Used for authentication. Note: diff --git a/src/firebolt/db/connection.py b/src/firebolt/db/connection.py index ebac97a65d7..b03c050e249 100644 --- a/src/firebolt/db/connection.py +++ b/src/firebolt/db/connection.py @@ -26,7 +26,8 @@ class Connection(AsyncBaseConnection): database: Firebolt database name username: Firebolt account username password: Firebolt account password - account_name: For customers with multiple accounts; if blank uses default. + account_name: Necessary for customers with multiple accounts; + if blank uses default. api_endpoint: Optional. Firebolt API endpoint. Used for authentication. Note: diff --git a/tests/unit/async_db/test_connection.py b/tests/unit/async_db/test_connection.py index 58db3051ac8..259face9088 100644 --- a/tests/unit/async_db/test_connection.py +++ b/tests/unit/async_db/test_connection.py @@ -55,11 +55,14 @@ async def test_cursor_initialized( auth_url: str, query_callback: Callable, query_url: str, + account_id_callback: Callable, + account_id_url: str, python_query_data: List[List[ColType]], ) -> None: - """Connection initialised it's cursors propperly""" + """Connection initialised its cursors properly.""" httpx_mock.add_callback(auth_callback, url=auth_url) httpx_mock.add_callback(query_callback, url=query_url) + httpx_mock.add_callback(account_id_callback, url=account_id_url) for url in (settings.server, f"https://{settings.server}"): async with ( @@ -170,6 +173,7 @@ async def test_connect_engine_name( username="username", password="password", engine_name=engine_name, + account_name=settings.account_name, api_endpoint=settings.server, ): pass @@ -188,6 +192,7 @@ async def test_connect_engine_name( database=db_name, username="u", password="p", + account_name=settings.account_name, api_endpoint=settings.server, ) as connection: assert await connection.cursor().execute("select*") == len(python_query_data) diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 483585a59cd..3376c0ac5fc 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -20,6 +20,7 @@ ) from firebolt.common.settings import Settings from firebolt.common.urls import ( + ACCOUNT_BY_NAME_URL, ACCOUNT_ENGINE_URL, ACCOUNT_URL, AUTH_URL, @@ -125,7 +126,10 @@ def db_name() -> str: @pytest.fixture def account_id_url(settings: Settings) -> str: - return f"https://{settings.server}{ACCOUNT_URL}" + if settings.account_name is None: + return f"https://{settings.server}{ACCOUNT_URL}" + else: + return f"https://{settings.server}{ACCOUNT_BY_NAME_URL}" @pytest.fixture @@ -135,9 +139,11 @@ def do_mock( **kwargs, ) -> Response: assert request.url == account_id_url - return to_response( - status_code=httpx.codes.OK, json={"account": {"id": account_id}} - ) + if account_id_url.endswith(ACCOUNT_URL): + return to_response( + status_code=httpx.codes.OK, json={"account": {"id": account_id}} + ) + return to_response(status_code=httpx.codes.OK, json={"account_id": account_id}) return do_mock diff --git a/tests/unit/db/test_connection.py b/tests/unit/db/test_connection.py index a0fcb8f0632..13cbffdb646 100644 --- a/tests/unit/db/test_connection.py +++ b/tests/unit/db/test_connection.py @@ -155,7 +155,7 @@ def test_connect_engine_name( database=db_name, username="u", password="p", - account_name="", + account_name=settings.account_name, api_endpoint=settings.server, ) as connection: assert connection.cursor().execute("select*") == len(python_query_data) From 1e8c25bd6cf0b3867822d879b4d3288719c7ba57 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Mon, 20 Dec 2021 10:22:21 -0500 Subject: [PATCH 06/33] Edited tests/unit/async_db/test_connection.py to correct account_id urls. --- tests/unit/conftest.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 3376c0ac5fc..9b00fd0983e 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -129,17 +129,21 @@ def account_id_url(settings: Settings) -> str: if settings.account_name is None: return f"https://{settings.server}{ACCOUNT_URL}" else: - return f"https://{settings.server}{ACCOUNT_BY_NAME_URL}" + return ( + f"https://{settings.server}{ACCOUNT_BY_NAME_URL}" + f"?account_name={settings.account_name}" + ) @pytest.fixture -def account_id_callback(account_id: str, account_id_url: str) -> Callable: +def account_id_callback(account_id: str, settings: Settings) -> Callable: def do_mock( request: httpx.Request = None, **kwargs, ) -> Response: - assert request.url == account_id_url - if account_id_url.endswith(ACCOUNT_URL): + this_url = account_id_url + assert request.url == this_url + if this_url.endswith(ACCOUNT_URL): return to_response( status_code=httpx.codes.OK, json={"account": {"id": account_id}} ) From 2798ed77e8b819bf211db98ffc2579dd5f0ca819 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Mon, 20 Dec 2021 11:51:46 -0500 Subject: [PATCH 07/33] Added account_name to an object instantionation in async_db/connection.py. Fixed errors in account_id_callback in unit/conftest.py. --- src/firebolt/async_db/connection.py | 4 +++- tests/unit/conftest.py | 10 ++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/firebolt/async_db/connection.py b/src/firebolt/async_db/connection.py index 84f27c251ef..c34d1d984ee 100644 --- a/src/firebolt/async_db/connection.py +++ b/src/firebolt/async_db/connection.py @@ -126,7 +126,9 @@ async def connect_inner( assert engine_url is not None engine_url = fix_url_schema(engine_url) - return connection_class(engine_url, database, username, password, api_endpoint) + return connection_class( + engine_url, database, username, password, account_name, api_endpoint + ) return connect_inner diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 9b00fd0983e..048282dcc5d 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -136,17 +136,19 @@ def account_id_url(settings: Settings) -> str: @pytest.fixture -def account_id_callback(account_id: str, settings: Settings) -> Callable: +def account_id_callback( + account_id: str, account_id_url: str, settings: Settings +) -> Callable: def do_mock( request: httpx.Request = None, **kwargs, ) -> Response: - this_url = account_id_url - assert request.url == this_url - if this_url.endswith(ACCOUNT_URL): + assert request.url == account_id_url + if account_id_url.endswith(ACCOUNT_URL): # account_name shouldn't be specified. return to_response( status_code=httpx.codes.OK, json={"account": {"id": account_id}} ) + # In this case, an account_name *should* be specified. return to_response(status_code=httpx.codes.OK, json={"account_id": account_id}) return do_mock From 2ed47df69bc603bf8ce30fb5bf54caa470b78acd Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Mon, 20 Dec 2021 13:51:47 -0500 Subject: [PATCH 08/33] Added AccountNameError to deal with instances where an incorrect account name is used. --- src/firebolt/async_db/connection.py | 8 ++--- src/firebolt/client/client.py | 19 +++++++---- src/firebolt/common/exception.py | 5 ++- .../dbapi/async/test_errors_async.py | 33 ++++++++++++++++++- tests/integration/dbapi/conftest.py | 6 ++++ tests/unit/async_db/test_connection.py | 4 +-- tests/unit/conftest.py | 2 +- 7 files changed, 61 insertions(+), 16 deletions(-) diff --git a/src/firebolt/async_db/connection.py b/src/firebolt/async_db/connection.py index c34d1d984ee..9120cc12e64 100644 --- a/src/firebolt/async_db/connection.py +++ b/src/firebolt/async_db/connection.py @@ -48,12 +48,12 @@ async def _resolve_engine_url( response.raise_for_status() return response.json()["engine"]["endpoint"] except HTTPStatusError as e: - # Engine error would be 404 + # Engine error would be 404. if e.response.status_code != 404: raise InterfaceError(f"unable to retrieve engine endpoint: {e}") # Once this is point is reached we've already authenticated with # the backend so it's safe to assume the cause of the error is - # missing engine + # missing engine. raise FireboltEngineError(f"Firebolt engine {engine_name} does not exist") except (JSONDecodeError, RequestError, RuntimeError, HTTPStatusError) as e: raise InterfaceError(f"unable to retrieve engine endpoint: {e}") @@ -98,9 +98,9 @@ async def connect_inner( ) api_endpoint = fix_url_schema(api_endpoint) - # This parameters are optional in function signature, + # These parameters are optional in function signature # but are required to connect. - # It's recommended to make them kwargs by PEP 249 + # PEP 249 recommendeds making them kwargs. for param, name in ( (database, "database"), (username, "username"), diff --git a/src/firebolt/client/client.py b/src/firebolt/client/client.py index 082a6554d94..7207029c4ba 100644 --- a/src/firebolt/client/client.py +++ b/src/firebolt/client/client.py @@ -8,6 +8,7 @@ from firebolt.client.auth import Auth from firebolt.client.constants import DEFAULT_API_URL +from firebolt.common.exception import AccountError from firebolt.common.urls import ACCOUNT_BY_NAME_URL, ACCOUNT_URL from firebolt.common.util import cached_property, fix_url_schema, mixin_for @@ -57,9 +58,12 @@ class Client(FireboltClientMixin, HttpxClient): @cached_property def account_id(self) -> str: if self.account_name is not None: - return self.get( + response = self.get( url=ACCOUNT_BY_NAME_URL, params={"account_name": self.account_name} - ).json()["account_id"] + ) + if response.status_code != "200": + raise AccountError(f"{self.account_name} does not exist.") + return response.json()["account_id"] else: # account_name isn't set, use the default account. return self.get(url=ACCOUNT_URL).json()["account"]["id"] @@ -80,10 +84,11 @@ class AsyncClient(FireboltClientMixin, HttpxAsyncClient): @async_cached_property async def account_id(self) -> str: if self.account_name is not None: - return ( - await self.get( - url=ACCOUNT_BY_NAME_URL, params={"account_name": self.account_name} - ) - ).json()["account_id"] + response = await self.get( + url=ACCOUNT_BY_NAME_URL, params={"account_name": self.account_name} + ) + if response.status_code != "200": + raise AccountError(f"{self.account_name} does not exist.") + return response.json()["account_id"] else: # account_name isn't set; use the default account. return (await self.get(url=ACCOUNT_URL)).json()["account"]["id"] diff --git a/src/firebolt/common/exception.py b/src/firebolt/common/exception.py index 86bfa548181..bc131ca83b3 100644 --- a/src/firebolt/common/exception.py +++ b/src/firebolt/common/exception.py @@ -29,6 +29,10 @@ class FireboltDatabaseError(FireboltError): pass +class AccountError(FireboltError): + pass + + class AttachedEngineInUseError(FireboltDatabaseError): def __init__(self, method_name: str): self.method_name = method_name @@ -99,7 +103,6 @@ class Warning(Exception): class InterfaceError(Error): - """ Exception raised for errors that are related to the database interface rather than the database itself. diff --git a/tests/integration/dbapi/async/test_errors_async.py b/tests/integration/dbapi/async/test_errors_async.py index 3f3e806604d..60906007326 100644 --- a/tests/integration/dbapi/async/test_errors_async.py +++ b/tests/integration/dbapi/async/test_errors_async.py @@ -32,8 +32,34 @@ async def test_invalid_credentials( @mark.asyncio -async def test_engine_url_not_exists( +async def test_invalid_account( engine_url: str, database_name: str, username: str, password: str, api_endpoint: str +) -> None: + """Connection properly reacts to invalid credentials error""" + async with await connect( + engine_url=engine_url, + database=database_name, + username=username, + password=password, + account_name="-", + api_endpoint=api_endpoint, + ) as connection: + with raises(AuthenticationError) as exc_info: + await connection.cursor().execute("show tables") + + assert str(exc_info.value).startswith( + "Failed to authenticate" + ), "Invalid authentication error message" + + +@mark.asyncio +async def test_engine_url_not_exists( + engine_url: str, + database_name: str, + username: str, + password: str, + account_name: str, + api_endpoint: str, ) -> None: """Connection properly reacts to invalid engine url error""" async with await connect( @@ -41,6 +67,7 @@ async def test_engine_url_not_exists( database=database_name, username=username, password=password, + account_name=account_name, api_endpoint=api_endpoint, ) as connection: with raises(ConnectError): @@ -53,6 +80,7 @@ async def test_engine_name_not_exists( database_name: str, username: str, password: str, + account_name: str, api_endpoint: str, ) -> None: """Connection properly reacts to invalid engine name error""" @@ -62,6 +90,7 @@ async def test_engine_name_not_exists( database=database_name, username=username, password=password, + account_name=account_name, api_endpoint=api_endpoint, ) as connection: await connection.cursor().execute("show tables") @@ -73,6 +102,7 @@ async def test_engine_stopped( database_name: str, username: str, password: str, + account_name: str, api_endpoint: str, ) -> None: """Connection properly reacts to invalid engine name error""" @@ -82,6 +112,7 @@ async def test_engine_stopped( database=database_name, username=username, password=password, + account_name=account_name, api_endpoint=api_endpoint, ) as connection: await connection.cursor().execute("show tables") diff --git a/tests/integration/dbapi/conftest.py b/tests/integration/dbapi/conftest.py index 15c9e843572..5cb8ec542a0 100644 --- a/tests/integration/dbapi/conftest.py +++ b/tests/integration/dbapi/conftest.py @@ -18,6 +18,7 @@ DATABASE_NAME_ENV = "DATABASE_NAME" USER_NAME_ENV = "USER_NAME" PASSWORD_ENV = "PASSWORD" +ACCOUNT_NAME_ENV = "ACCOUNT_NAME" API_ENDPOINT_ENV = "API_ENDPOINT" @@ -62,6 +63,11 @@ def password() -> str: return must_env(PASSWORD_ENV) +@fixture(scope="session") +def account_name() -> str: + return must_env(ACCOUNT_NAME_ENV) + + @fixture(scope="session") def api_endpoint() -> str: return must_env(API_ENDPOINT_ENV) diff --git a/tests/unit/async_db/test_connection.py b/tests/unit/async_db/test_connection.py index 259face9088..1388d27907e 100644 --- a/tests/unit/async_db/test_connection.py +++ b/tests/unit/async_db/test_connection.py @@ -62,7 +62,7 @@ async def test_cursor_initialized( """Connection initialised its cursors properly.""" httpx_mock.add_callback(auth_callback, url=auth_url) httpx_mock.add_callback(query_callback, url=query_url) - httpx_mock.add_callback(account_id_callback, url=account_id_url) + # httpx_mock.add_callback(account_id_callback, url=account_id_url) for url in (settings.server, f"https://{settings.server}"): async with ( @@ -71,7 +71,7 @@ async def test_cursor_initialized( database=db_name, username="u", password="p", - account_name=settings.account_name, + account_name="a", api_endpoint=settings.server, ) ) as connection: diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 048282dcc5d..be461edb9a1 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -95,7 +95,7 @@ def settings(server, region_1) -> Settings: user="email@domain.com", password=SecretStr("*****"), default_region=region_1.name, - account_name="account", + account_name=None, ) From 8048baa95f6f0c0095b44400022065e56b3b8725 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Mon, 20 Dec 2021 17:39:41 -0500 Subject: [PATCH 09/33] Added invalid account name exception and error test. --- src/firebolt/async_db/connection.py | 4 +- src/firebolt/client/client.py | 8 +-- src/firebolt/common/exception.py | 20 +++++--- .../dbapi/async/test_errors_async.py | 50 +++++++++++-------- 4 files changed, 46 insertions(+), 36 deletions(-) diff --git a/src/firebolt/async_db/connection.py b/src/firebolt/async_db/connection.py index 9120cc12e64..a6fe279ec03 100644 --- a/src/firebolt/async_db/connection.py +++ b/src/firebolt/async_db/connection.py @@ -33,13 +33,13 @@ async def _resolve_engine_url( api_endpoint=api_endpoint, ) as client: try: + account_id = await client.account_id response = await client.get( url=ENGINE_BY_NAME_URL, - params={"engine_name": engine_name}, + params={"engine_name": engine_name, "account_id": account_id}, ) response.raise_for_status() engine_id = response.json()["engine_id"]["engine_id"] - account_id = await client.account_id response = await client.get( url=ACCOUNT_ENGINE_URL.format( account_id=account_id, engine_id=engine_id diff --git a/src/firebolt/client/client.py b/src/firebolt/client/client.py index 7207029c4ba..28de87f85e6 100644 --- a/src/firebolt/client/client.py +++ b/src/firebolt/client/client.py @@ -61,8 +61,8 @@ def account_id(self) -> str: response = self.get( url=ACCOUNT_BY_NAME_URL, params={"account_name": self.account_name} ) - if response.status_code != "200": - raise AccountError(f"{self.account_name} does not exist.") + if response.status_code != 200: + raise AccountError(self.account_name) return response.json()["account_id"] else: # account_name isn't set, use the default account. return self.get(url=ACCOUNT_URL).json()["account"]["id"] @@ -87,8 +87,8 @@ async def account_id(self) -> str: response = await self.get( url=ACCOUNT_BY_NAME_URL, params={"account_name": self.account_name} ) - if response.status_code != "200": - raise AccountError(f"{self.account_name} does not exist.") + if response.status_code != 200: + raise AccountError(self.account_name) return response.json()["account_id"] else: # account_name isn't set; use the default account. return (await self.get(url=ACCOUNT_URL)).json()["account"]["id"] diff --git a/src/firebolt/common/exception.py b/src/firebolt/common/exception.py index bc131ca83b3..0ca437d84e5 100644 --- a/src/firebolt/common/exception.py +++ b/src/firebolt/common/exception.py @@ -16,8 +16,8 @@ def __init__(self, method_name: str): def __str__(self) -> str: return ( - f"unable to call {self.method_name}: " - f"engine must to be attached to a database first." + f"Unable to call {self.method_name}: " + f"Engine must to be attached to a database first." ) @@ -30,7 +30,11 @@ class FireboltDatabaseError(FireboltError): class AccountError(FireboltError): - pass + def __init__(self, method_name: str): + self.method_name = method_name + + def __str__(self) -> str: + return f'Account "{self.method_name}" does not exist.' class AttachedEngineInUseError(FireboltDatabaseError): @@ -39,8 +43,8 @@ def __init__(self, method_name: str): def __str__(self) -> str: return ( - f"unable to call {self.method_name}: " - f"engine must not be in starting or stopping state." + f"Unable to call {self.method_name}: " + f"Engine must not be in starting or stopping state." ) @@ -61,7 +65,7 @@ def __init__(self, method_name: str): self.method_name = method_name def __str__(self) -> str: - return f"unable to call {self.method_name}: cursor closed" + return f"Unable to call {self.method_name}: cursor closed." class QueryNotRunError(CursorError): @@ -69,7 +73,7 @@ def __init__(self, method_name: str): self.method_name = method_name def __str__(self) -> str: - return f"unable to call {self.method_name}: need to run a query first" + return f"Unable to call {self.method_name}: need to run a query first." class QueryError(CursorError): @@ -86,7 +90,7 @@ def __init__(self, cause: str, api_endpoint: str): self.api_endpoint = api_endpoint def __str__(self) -> str: - return f"Failed to authenticate at {self.api_endpoint}: {self.cause}" + return f"Failed to authenticate at {self.api_endpoint}: {self.cause}." # PEP-249 diff --git a/tests/integration/dbapi/async/test_errors_async.py b/tests/integration/dbapi/async/test_errors_async.py index 60906007326..55afdd50f37 100644 --- a/tests/integration/dbapi/async/test_errors_async.py +++ b/tests/integration/dbapi/async/test_errors_async.py @@ -3,6 +3,7 @@ from firebolt.async_db import Connection, connect from firebolt.common.exception import ( + AccountError, AuthenticationError, EngineNotRunningError, FireboltDatabaseError, @@ -15,7 +16,7 @@ async def test_invalid_credentials( engine_url: str, database_name: str, username: str, password: str, api_endpoint: str ) -> None: - """Connection properly reacts to invalid credentials error""" + """Connection properly reacts to invalid credentials error.""" async with await connect( engine_url=engine_url, database=database_name, @@ -28,28 +29,33 @@ async def test_invalid_credentials( assert str(exc_info.value).startswith( "Failed to authenticate" - ), "Invalid authentication error message" + ), "Invalid authentication error message." @mark.asyncio async def test_invalid_account( - engine_url: str, database_name: str, username: str, password: str, api_endpoint: str + database_name: str, + engine_name: str, + username: str, + password: str, + api_endpoint: str, ) -> None: - """Connection properly reacts to invalid credentials error""" - async with await connect( - engine_url=engine_url, - database=database_name, - username=username, - password=password, - account_name="-", - api_endpoint=api_endpoint, - ) as connection: - with raises(AuthenticationError) as exc_info: + """Connection properly reacts to invalid account error.""" + account_name = "--" + with raises(AccountError) as exc_info: + async with await connect( + database=database_name, + engine_name=engine_name, # Omit engine_url to force account_id lookup. + username=username, + password=password, + account_name=account_name, + api_endpoint=api_endpoint, + ) as connection: await connection.cursor().execute("show tables") assert str(exc_info.value).startswith( - "Failed to authenticate" - ), "Invalid authentication error message" + f'Account "{account_name}" does not exist' + ), "Invalid account error message." @mark.asyncio @@ -61,7 +67,7 @@ async def test_engine_url_not_exists( account_name: str, api_endpoint: str, ) -> None: - """Connection properly reacts to invalid engine url error""" + """Connection properly reacts to invalid engine url error.""" async with await connect( engine_url=engine_url + "_", database=database_name, @@ -83,7 +89,7 @@ async def test_engine_name_not_exists( account_name: str, api_endpoint: str, ) -> None: - """Connection properly reacts to invalid engine name error""" + """Connection properly reacts to invalid engine name error.""" with raises(FireboltEngineError): async with await connect( engine_name=engine_name + "_________", @@ -105,7 +111,7 @@ async def test_engine_stopped( account_name: str, api_endpoint: str, ) -> None: - """Connection properly reacts to invalid engine name error""" + """Connection properly reacts to invalid engine name error.""" with raises(EngineNotRunningError): async with await connect( engine_url=stopped_engine_url, @@ -122,7 +128,7 @@ async def test_engine_stopped( async def test_database_not_exists( engine_url: str, database_name: str, username: str, password: str, api_endpoint: str ) -> None: - """Connection properly reacts to invalid database error""" + """Connection properly reacts to invalid database error.""" new_db_name = database_name + "_" async with await connect( engine_url=engine_url, @@ -136,16 +142,16 @@ async def test_database_not_exists( assert ( str(exc_info.value) == f"Database {new_db_name} does not exist" - ), "Invalid database name error message" + ), "Invalid database name error message." @mark.asyncio async def test_sql_error(connection: Connection) -> None: - """Connection properly reacts to sql execution error""" + """Connection properly reacts to sql execution error.""" with connection.cursor() as c: with raises(OperationalError) as exc_info: await c.execute("select ]") assert str(exc_info.value).startswith( "Error executing query" - ), "Invalid SQL error message" + ), "Invalid SQL error message." From c3ffb6b032bd04ee47af5f34b8149255a356794b Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Mon, 20 Dec 2021 17:44:39 -0500 Subject: [PATCH 10/33] Added test_invalid_account() to dbapi/sync/test_errors.py. --- tests/integration/dbapi/sync/test_errors.py | 26 +++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/integration/dbapi/sync/test_errors.py b/tests/integration/dbapi/sync/test_errors.py index fd27cccd25b..0322b4657f1 100644 --- a/tests/integration/dbapi/sync/test_errors.py +++ b/tests/integration/dbapi/sync/test_errors.py @@ -2,6 +2,7 @@ from pytest import raises from firebolt.common.exception import ( + AccountError, AuthenticationError, EngineNotRunningError, FireboltDatabaseError, @@ -30,6 +31,31 @@ def test_invalid_credentials( ), "Invalid authentication error message" +def test_invalid_account( + database_name: str, + engine_name: str, + username: str, + password: str, + api_endpoint: str, +) -> None: + """Connection properly reacts to invalid account error.""" + account_name = "--" + with raises(AccountError) as exc_info: + with connect( + database=database_name, + engine_name=engine_name, # Omit engine_url to force account_id lookup. + username=username, + password=password, + account_name=account_name, + api_endpoint=api_endpoint, + ) as connection: + connection.cursor().execute("show tables") + + assert str(exc_info.value).startswith( + f'Account "{account_name}" does not exist' + ), "Invalid account error message." + + def test_engine_url_not_exists( engine_url: str, database_name: str, username: str, password: str, api_endpoint: str ) -> None: From 66682327d3c776c38a93e9fc3554b3de5f250faa Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Mon, 20 Dec 2021 17:54:50 -0500 Subject: [PATCH 11/33] Added a bunch of periods to assertion error messages. --- .../integration/dbapi/async/test_auth_async.py | 4 ++-- tests/unit/async_db/test_connection.py | 18 +++++++++--------- tests/unit/client/test_auth.py | 6 +++--- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/tests/integration/dbapi/async/test_auth_async.py b/tests/integration/dbapi/async/test_auth_async.py index 351cfc7c655..5038cb7e956 100644 --- a/tests/integration/dbapi/async/test_auth_async.py +++ b/tests/integration/dbapi/async/test_auth_async.py @@ -23,10 +23,10 @@ async def test_refresh_token(connection: Connection) -> None: old = c._client.auth.token c._client.auth._expires = int(time()) - 1 - # Still works fine + # Still works fine. await c.execute("show tables") - assert c._client.auth.token != old, "Auth didn't update token on expiration" + assert c._client.auth.token != old, "Auth didn't update token on expiration." @mark.asyncio diff --git a/tests/unit/async_db/test_connection.py b/tests/unit/async_db/test_connection.py index 1388d27907e..866a70af724 100644 --- a/tests/unit/async_db/test_connection.py +++ b/tests/unit/async_db/test_connection.py @@ -36,13 +36,13 @@ async def test_cursors_closed_on_close(connection: Connection) -> None: c1, c2 = connection.cursor(), connection.cursor() assert ( len(connection._cursors) == 2 - ), "Invalid number of cursors stored in connection" + ), "Invalid number of cursors stored in connection." await connection.aclose() - assert connection.closed, "Connection was not closed on close" - assert c1.closed, "Cursor was not closed on connection close" - assert c2.closed, "Cursor was not closed on connection close" - assert len(connection._cursors) == 0, "Cursors left in connection after close" + assert connection.closed, "Connection was not closed on close." + assert c1.closed, "Cursor was not closed on connection close." + assert c2.closed, "Cursor was not closed on connection close." + assert len(connection._cursors) == 0, "Cursors left in connection after close." await connection.aclose() @@ -78,7 +78,7 @@ async def test_cursor_initialized( cursor = connection.cursor() assert ( cursor.connection == connection - ), "Invalid cursor connection attribute" + ), "Invalid cursor connection attribute." assert ( cursor._client == connection._client ), "Invalid cursor _client attribute" @@ -88,7 +88,7 @@ async def test_cursor_initialized( cursor.close() assert ( cursor not in connection._cursors - ), "Cursor wasn't removed from connection after close" + ), "Cursor wasn't removed from connection after close." @mark.asyncio @@ -138,7 +138,7 @@ async def test_connect_engine_name( ): pass assert str(exc_info.value).startswith( - "Both engine_name and engine_url are provided" + "Both engine_name and engine_url are provided." ) with raises(InterfaceError) as exc_info: @@ -149,7 +149,7 @@ async def test_connect_engine_name( ): pass assert str(exc_info.value).startswith( - "Neither engine_name nor engine_url are provided" + "Neither engine_name nor engine_url are provided." ) httpx_mock.add_callback(auth_callback, url=auth_url) diff --git a/tests/unit/client/test_auth.py b/tests/unit/client/test_auth.py index 9b775975857..29cdf77fc37 100644 --- a/tests/unit/client/test_auth.py +++ b/tests/unit/client/test_auth.py @@ -52,7 +52,7 @@ def test_auth_refresh_on_expiration( execute_generator_requests(auth.auth_flow(Request("GET", "https://host"))) assert auth.token == test_token, "invalid access token" execute_generator_requests(auth.auth_flow(Request("GET", "https://host"))) - assert auth.token == test_token2, "expired access token was not updated" + assert auth.token == test_token2, "Expired access token was not updated." def test_auth_uses_same_token_if_valid( @@ -88,7 +88,7 @@ def test_auth_uses_same_token_if_valid( execute_generator_requests(auth.auth_flow(Request("GET", "https://host"))) assert auth.token == test_token, "invalid access token" execute_generator_requests(auth.auth_flow(Request("GET", "https://host"))) - assert auth.token == test_token, "shoud not update token until it expires" + assert auth.token == test_token, "Shoud not update token until it expires." httpx_mock.reset(False) @@ -129,7 +129,7 @@ def http_error(**kwargs): execute_generator_requests(auth.get_new_token_generator()) assert ( - str(excinfo.value) == "Failed to authenticate at https://host: firebolt" + str(excinfo.value) == "Failed to authenticate at https://host: firebolt." ), "Invalid authentication error message" httpx_mock.reset(True) From 64024737f8c8020916b8a12c89e97d71d1cc5369 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Mon, 20 Dec 2021 22:14:15 -0500 Subject: [PATCH 12/33] Changed account_name from Optional in FireboltClientMixin to satisfy mypy. Also, a couple of periods added in exception messages. --- src/firebolt/async_db/connection.py | 8 ++++---- src/firebolt/db/connection.py | 2 +- tests/unit/async_db/test_connection.py | 4 +++- tests/unit/db/test_connection.py | 4 ++-- 4 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/firebolt/async_db/connection.py b/src/firebolt/async_db/connection.py index a6fe279ec03..fde5fc28022 100644 --- a/src/firebolt/async_db/connection.py +++ b/src/firebolt/async_db/connection.py @@ -36,7 +36,7 @@ async def _resolve_engine_url( account_id = await client.account_id response = await client.get( url=ENGINE_BY_NAME_URL, - params={"engine_name": engine_name, "account_id": account_id}, + params={"engine_name": engine_name}, ) response.raise_for_status() engine_id = response.json()["engine_id"]["engine_id"] @@ -66,7 +66,7 @@ async def connect_inner( password: str = None, engine_name: Optional[str] = None, engine_url: Optional[str] = None, - account_name: Optional[str] = None, + account_name: str = None, api_endpoint: str = DEFAULT_API_URL, ) -> Connection: """ @@ -88,12 +88,12 @@ async def connect_inner( if engine_name and engine_url: raise InterfaceError( - "Both engine_name and engine_url are provided." + "Both engine_name and engine_url are provided. " "Provide only one to connect." ) if not engine_name and not engine_url: raise InterfaceError( - "Neither engine_name nor engine_url are provided." + "Neither engine_name nor engine_url is provided. " "Provide one to connect." ) diff --git a/src/firebolt/db/connection.py b/src/firebolt/db/connection.py index b03c050e249..b975b5f02b9 100644 --- a/src/firebolt/db/connection.py +++ b/src/firebolt/db/connection.py @@ -60,7 +60,7 @@ def close(self) -> None: # Context manager support def __enter__(self) -> Connection: if self.closed: - raise ConnectionClosedError("Connection is already closed") + raise ConnectionClosedError("Connection is already closed.") return self def __exit__( diff --git a/tests/unit/async_db/test_connection.py b/tests/unit/async_db/test_connection.py index 866a70af724..2d599043697 100644 --- a/tests/unit/async_db/test_connection.py +++ b/tests/unit/async_db/test_connection.py @@ -135,6 +135,7 @@ async def test_connect_engine_name( database="db", username="username", password="password", + account_name="account_name", ): pass assert str(exc_info.value).startswith( @@ -146,10 +147,11 @@ async def test_connect_engine_name( database="db", username="username", password="password", + account_name="account", ): pass assert str(exc_info.value).startswith( - "Neither engine_name nor engine_url are provided." + "Neither engine_name nor engine_url is provided." ) httpx_mock.add_callback(auth_callback, url=auth_url) diff --git a/tests/unit/db/test_connection.py b/tests/unit/db/test_connection.py index 13cbffdb646..bdb161f9132 100644 --- a/tests/unit/db/test_connection.py +++ b/tests/unit/db/test_connection.py @@ -121,7 +121,7 @@ def test_connect_engine_name( password="password", ) assert str(exc_info.value).startswith( - "Both engine_name and engine_url are provided" + "Both engine_name and engine_url are provided." ) with raises(InterfaceError) as exc_info: @@ -131,7 +131,7 @@ def test_connect_engine_name( password="password", ) assert str(exc_info.value).startswith( - "Neither engine_name nor engine_url are provided" + "Neither engine_name nor engine_url is provided." ) httpx_mock.add_callback(auth_callback, url=auth_url) From 9c3aea2fee41be2c5819ed03d8d3f2c089b840b8 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Mon, 20 Dec 2021 22:22:25 -0500 Subject: [PATCH 13/33] Try two to change account_name from Optional in FireboltClientMixin to satisfy mypy. --- src/firebolt/async_db/connection.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/firebolt/async_db/connection.py b/src/firebolt/async_db/connection.py index fde5fc28022..b47cbacff37 100644 --- a/src/firebolt/async_db/connection.py +++ b/src/firebolt/async_db/connection.py @@ -78,7 +78,7 @@ async def connect_inner( password: password to use for authentication engine_name: Optional The name of the engine to connect to engine_url: Optional. The engine endpoint to use - account_name: For customers with multiple accounts; if blank uses default. + account_name: For customers with multiple accounts; if None uses default. api_endpoint(optional): Firebolt API endpoint. Used for authentication. Note: From 65a10cd5e6b3b41ccb6cb5749d56938dd51dcab4 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Mon, 20 Dec 2021 22:37:17 -0500 Subject: [PATCH 14/33] Try three: changed account_name to Optional in async_connect_factory and FireboltClientMixin to satisfy mypy. --- src/firebolt/async_db/connection.py | 2 +- src/firebolt/client/client.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/firebolt/async_db/connection.py b/src/firebolt/async_db/connection.py index b47cbacff37..421aa492689 100644 --- a/src/firebolt/async_db/connection.py +++ b/src/firebolt/async_db/connection.py @@ -66,7 +66,7 @@ async def connect_inner( password: str = None, engine_name: Optional[str] = None, engine_url: Optional[str] = None, - account_name: str = None, + account_name: Optional[str] = None, api_endpoint: str = DEFAULT_API_URL, ) -> Connection: """ diff --git a/src/firebolt/client/client.py b/src/firebolt/client/client.py index 28de87f85e6..f011d310697 100644 --- a/src/firebolt/client/client.py +++ b/src/firebolt/client/client.py @@ -19,7 +19,7 @@ class FireboltClientMixin(FireboltClientMixinBase): def __init__( self, *args: Any, - account_name: str = None, + account_name: Optional[str] = None, api_endpoint: str = DEFAULT_API_URL, auth: AuthTypes = None, **kwargs: Any, From a21fcd45c945992f134d4aa9a5556fb2432377c2 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Mon, 20 Dec 2021 23:00:22 -0500 Subject: [PATCH 15/33] Passed mypy by setting account_name to Optional[str] in _resolve_engine_url. --- src/firebolt/async_db/connection.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/firebolt/async_db/connection.py b/src/firebolt/async_db/connection.py index 421aa492689..6796bd4684b 100644 --- a/src/firebolt/async_db/connection.py +++ b/src/firebolt/async_db/connection.py @@ -23,7 +23,7 @@ async def _resolve_engine_url( engine_name: str, username: str, password: str, - account_name: str, + account_name: Optional[str], api_endpoint: str, ) -> str: async with AsyncClient( From 596d06aa34d370faa685f171c23212a119dc17a3 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Tue, 21 Dec 2021 10:57:28 -0500 Subject: [PATCH 16/33] Removed and replaced ENGINE_BY_NAME_URL with ACCOUNT_ENGINE_BY_NAME_URL everywhere. Made account_name type to Optional[str]=None in _resolve_engine_url. Added account_name to conftests. Minor text corrections. --- src/firebolt/async_db/connection.py | 16 ++++++++-------- src/firebolt/common/urls.py | 1 - tests/integration/dbapi/async/conftest.py | 10 +++++++++- .../dbapi/async/test_queries_async.py | 2 +- tests/integration/dbapi/sync/conftest.py | 10 +++++++++- tests/integration/dbapi/sync/test_errors.py | 4 ++-- tests/unit/async_db/test_connection.py | 6 +++--- tests/unit/db/test_connection.py | 4 ++-- 8 files changed, 34 insertions(+), 19 deletions(-) diff --git a/src/firebolt/async_db/connection.py b/src/firebolt/async_db/connection.py index 6796bd4684b..f45b6e3e987 100644 --- a/src/firebolt/async_db/connection.py +++ b/src/firebolt/async_db/connection.py @@ -13,7 +13,7 @@ FireboltEngineError, InterfaceError, ) -from firebolt.common.urls import ACCOUNT_ENGINE_URL, ENGINE_BY_NAME_URL +from firebolt.common.urls import ACCOUNT_ENGINE_BY_NAME_URL, ACCOUNT_ENGINE_URL from firebolt.common.util import fix_url_schema DEFAULT_TIMEOUT_SECONDS: int = 5 @@ -23,8 +23,8 @@ async def _resolve_engine_url( engine_name: str, username: str, password: str, - account_name: Optional[str], api_endpoint: str, + account_name: Optional[str] = None, ) -> str: async with AsyncClient( auth=(username, password), @@ -35,7 +35,7 @@ async def _resolve_engine_url( try: account_id = await client.account_id response = await client.get( - url=ENGINE_BY_NAME_URL, + url=ACCOUNT_ENGINE_BY_NAME_URL.format(account_id=account_id), params={"engine_name": engine_name}, ) response.raise_for_status() @@ -116,11 +116,11 @@ async def connect_inner( if engine_name: engine_url = await _resolve_engine_url( - engine_name, - username, - password, - account_name, - api_endpoint, + engine_name=engine_name, + username=username, + password=password, + account_name=account_name, + api_endpoint=api_endpoint, ) assert engine_url is not None diff --git a/src/firebolt/common/urls.py b/src/firebolt/common/urls.py index 989ddb3a869..af64b53a5b4 100644 --- a/src/firebolt/common/urls.py +++ b/src/firebolt/common/urls.py @@ -2,7 +2,6 @@ DATABASES_URL = "/core/v1/account/databases" -ENGINE_BY_NAME_URL = "/core/v1/account/engines:getIdByName" ENGINES_URL = "/core/v1/account/engines" ENGINES_BY_IDS_URL = "/core/v1/engines:getByIds" diff --git a/tests/integration/dbapi/async/conftest.py b/tests/integration/dbapi/async/conftest.py index 8ad6be6d793..4354d03d98e 100644 --- a/tests/integration/dbapi/async/conftest.py +++ b/tests/integration/dbapi/async/conftest.py @@ -5,13 +5,19 @@ @fixture async def connection( - engine_url: str, database_name: str, username: str, password: str, api_endpoint: str + engine_url: str, + database_name: str, + username: str, + password: str, + account_name: str, + api_endpoint: str, ) -> Connection: async with await connect( engine_url=engine_url, database=database_name, username=username, password=password, + account_name=account_name, api_endpoint=api_endpoint, ) as connection: yield connection @@ -23,6 +29,7 @@ async def connection_engine_name( database_name: str, username: str, password: str, + account_name: str, api_endpoint: str, ) -> Connection: @@ -31,6 +38,7 @@ async def connection_engine_name( database=database_name, username=username, password=password, + account_name=account_name, api_endpoint=api_endpoint, ) as connection: yield connection diff --git a/tests/integration/dbapi/async/test_queries_async.py b/tests/integration/dbapi/async/test_queries_async.py index 42288bd5c4f..51db5fd3fe5 100644 --- a/tests/integration/dbapi/async/test_queries_async.py +++ b/tests/integration/dbapi/async/test_queries_async.py @@ -69,7 +69,7 @@ async def test_select( async def test_drop_create( connection: Connection, create_drop_description: List[Column] ) -> None: - """Create and drop table/index queries are handled propperly""" + """Create and drop table/index queries are handled properly.""" async def test_query(c: Cursor, query: str) -> None: assert await c.execute(query) == 1, "Invalid row count returned" diff --git a/tests/integration/dbapi/sync/conftest.py b/tests/integration/dbapi/sync/conftest.py index 5bb12677bfd..c518bd2c83e 100644 --- a/tests/integration/dbapi/sync/conftest.py +++ b/tests/integration/dbapi/sync/conftest.py @@ -5,13 +5,19 @@ @fixture def connection( - engine_url: str, database_name: str, username: str, password: str, api_endpoint: str + engine_url: str, + database_name: str, + username: str, + password: str, + account_name: str, + api_endpoint: str, ) -> Connection: connection = connect( engine_url=engine_url, database=database_name, username=username, password=password, + account_name=account_name, api_endpoint=api_endpoint, ) yield connection @@ -24,6 +30,7 @@ def connection_engine_name( database_name: str, username: str, password: str, + account_name: str, api_endpoint: str, ) -> Connection: connection = connect( @@ -31,6 +38,7 @@ def connection_engine_name( database=database_name, username=username, password=password, + account_name=account_name, api_endpoint=api_endpoint, ) yield connection diff --git a/tests/integration/dbapi/sync/test_errors.py b/tests/integration/dbapi/sync/test_errors.py index 0322b4657f1..268383afdf1 100644 --- a/tests/integration/dbapi/sync/test_errors.py +++ b/tests/integration/dbapi/sync/test_errors.py @@ -112,7 +112,7 @@ def test_engine_stopped( def test_database_not_exists( engine_url: str, database_name: str, username: str, password: str, api_endpoint: str ) -> None: - """Connection properly reacts to invalid database error""" + """Connection properly reacts to invalid database error.""" new_db_name = database_name + "_" with connect( engine_url=engine_url, @@ -130,7 +130,7 @@ def test_database_not_exists( def test_sql_error(connection: Connection) -> None: - """Connection properly reacts to sql execution error""" + """Connection properly reacts to sql execution error.""" with connection.cursor() as c: with raises(OperationalError) as exc_info: c.execute("select ]") diff --git a/tests/unit/async_db/test_connection.py b/tests/unit/async_db/test_connection.py index 2d599043697..f2f2c8e81db 100644 --- a/tests/unit/async_db/test_connection.py +++ b/tests/unit/async_db/test_connection.py @@ -12,7 +12,7 @@ InterfaceError, ) from firebolt.common.settings import Settings -from firebolt.common.urls import ENGINE_BY_NAME_URL +from firebolt.common.urls import ACCOUNT_ENGINE_BY_NAME_URL @mark.asyncio @@ -164,7 +164,7 @@ async def test_connect_engine_name( # Mock engine id lookup error httpx_mock.add_response( url=f"https://{settings.server}" - + ENGINE_BY_NAME_URL + + ACCOUNT_ENGINE_BY_NAME_URL.format(account_id=account_id) + f"?engine_name={engine_name}", status_code=codes.NOT_FOUND, ) @@ -183,7 +183,7 @@ async def test_connect_engine_name( # Mock engine id lookup by name httpx_mock.add_response( url=f"https://{settings.server}" - + ENGINE_BY_NAME_URL + + ACCOUNT_ENGINE_BY_NAME_URL.format(account_id=account_id) + f"?engine_name={engine_name}", status_code=codes.OK, json={"engine_id": {"engine_id": engine_id}}, diff --git a/tests/unit/db/test_connection.py b/tests/unit/db/test_connection.py index bdb161f9132..b9150588349 100644 --- a/tests/unit/db/test_connection.py +++ b/tests/unit/db/test_connection.py @@ -7,7 +7,7 @@ from firebolt.async_db._types import ColType from firebolt.common.exception import ConnectionClosedError, InterfaceError from firebolt.common.settings import Settings -from firebolt.common.urls import ENGINE_BY_NAME_URL +from firebolt.common.urls import ACCOUNT_ENGINE_BY_NAME_URL from firebolt.db import Connection, connect @@ -144,7 +144,7 @@ def test_connect_engine_name( # Mock engine id lookup by name httpx_mock.add_response( url=f"https://{settings.server}" - + ENGINE_BY_NAME_URL + + ACCOUNT_ENGINE_BY_NAME_URL.format(account_id=account_id) + f"?engine_name={engine_name}", status_code=codes.OK, json={"engine_id": {"engine_id": engine_id}}, From 39174b9dfafa3f59482b7d4e37562b5b0692f84d Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Tue, 21 Dec 2021 23:05:01 -0500 Subject: [PATCH 17/33] Couldn't help myself: added a bunch of periods to test_queries_async.py. --- .../dbapi/async/test_queries_async.py | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/integration/dbapi/async/test_queries_async.py b/tests/integration/dbapi/async/test_queries_async.py index 51db5fd3fe5..ee19fbde3b1 100644 --- a/tests/integration/dbapi/async/test_queries_async.py +++ b/tests/integration/dbapi/async/test_queries_async.py @@ -72,14 +72,14 @@ async def test_drop_create( """Create and drop table/index queries are handled properly.""" async def test_query(c: Cursor, query: str) -> None: - assert await c.execute(query) == 1, "Invalid row count returned" - assert c.rowcount == 1, "Invalid rowcount value" + assert await c.execute(query) == 1, "Invalid row count returned." + assert c.rowcount == 1, "Invalid rowcount value." assert_deep_eq( c.description, create_drop_description, - "Invalid create table query description", + "Invalid create table query description.", ) - assert len(await c.fetchall()) == 1, "Invalid data returned" + assert len(await c.fetchall()) == 1, "Invalid data returned." """Create table query is handled properly""" with connection.cursor() as c: @@ -131,12 +131,12 @@ async def test_query(c: Cursor, query: str) -> None: @mark.asyncio async def test_insert(connection: Connection) -> None: - """Insert and delete queries are handled propperly""" + """Insert and delete queries are handled properly.""" async def test_empty_query(c: Cursor, query: str) -> None: - assert await c.execute(query) == -1, "Invalid row count returned" - assert c.rowcount == -1, "Invalid rowcount value" - assert c.description is None, "Invalid description" + assert await c.execute(query) == -1, "Invalid row count returned." + assert c.rowcount == -1, "Invalid rowcount value." + assert c.description is None, "Invalid description." with raises(DataError): await c.fetchone() @@ -161,7 +161,7 @@ async def test_empty_query(c: Cursor, query: str) -> None: assert ( await c.execute("SELECT * FROM test_tb ORDER BY test_tb.id") == 1 - ), "Invalid data length in table after insert" + ), "Invalid data length in table after insert." assert_deep_eq( await c.fetchall(), @@ -176,5 +176,5 @@ async def test_empty_query(c: Cursor, query: str) -> None: [1, 2, 3], ], ], - "Invalid data in table after insert", + "Invalid data in table after insert.", ) From 7214c72c74be88a8f91ada47b88c3961e5caf677 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Tue, 21 Dec 2021 23:18:34 -0500 Subject: [PATCH 18/33] Removed unused _get_account_id() from service/manager.py. --- src/firebolt/service/manager.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/firebolt/service/manager.py b/src/firebolt/service/manager.py index 45844a9e6dc..d33947ec2b9 100644 --- a/src/firebolt/service/manager.py +++ b/src/firebolt/service/manager.py @@ -64,6 +64,3 @@ def _init_services(self) -> None: self.engines = EngineService(resource_manager=self) self.engine_revisions = EngineRevisionService(resource_manager=self) self.bindings = BindingService(resource_manager=self) - - def _get_account_id(self, account_name: Optional[str]) -> str: - return self.client.account_id From 15b80fefedffbbad7e75ed4b6cf8be32f55030df Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Tue, 21 Dec 2021 23:35:24 -0500 Subject: [PATCH 19/33] Removed all edits to comments and documentation. Will be added in separate branch. --- src/firebolt/async_db/connection.py | 14 ++++++------ src/firebolt/common/exception.py | 14 ++++++------ src/firebolt/db/connection.py | 2 +- .../dbapi/async/test_auth_async.py | 4 ++-- .../dbapi/async/test_errors_async.py | 16 +++++++------- .../dbapi/async/test_queries_async.py | 22 +++++++++---------- tests/integration/dbapi/sync/test_errors.py | 4 ++-- tests/unit/async_db/test_connection.py | 20 ++++++++--------- tests/unit/client/test_auth.py | 6 ++--- tests/unit/db/test_connection.py | 4 ++-- 10 files changed, 53 insertions(+), 53 deletions(-) diff --git a/src/firebolt/async_db/connection.py b/src/firebolt/async_db/connection.py index f45b6e3e987..058b683e7e0 100644 --- a/src/firebolt/async_db/connection.py +++ b/src/firebolt/async_db/connection.py @@ -48,12 +48,12 @@ async def _resolve_engine_url( response.raise_for_status() return response.json()["engine"]["endpoint"] except HTTPStatusError as e: - # Engine error would be 404. + # Engine error would be 404 if e.response.status_code != 404: raise InterfaceError(f"unable to retrieve engine endpoint: {e}") # Once this is point is reached we've already authenticated with # the backend so it's safe to assume the cause of the error is - # missing engine. + # missing engine raise FireboltEngineError(f"Firebolt engine {engine_name} does not exist") except (JSONDecodeError, RequestError, RuntimeError, HTTPStatusError) as e: raise InterfaceError(f"unable to retrieve engine endpoint: {e}") @@ -82,25 +82,25 @@ async def connect_inner( api_endpoint(optional): Firebolt API endpoint. Used for authentication. Note: - Either `engine_name` or `engine_url` should be provided, but not both. + either `engine_name` or `engine_url` should be provided, but not both """ if engine_name and engine_url: raise InterfaceError( - "Both engine_name and engine_url are provided. " + "Both engine_name and engine_url are provided." "Provide only one to connect." ) if not engine_name and not engine_url: raise InterfaceError( - "Neither engine_name nor engine_url is provided. " + "Neither engine_name nor engine_url are provided." "Provide one to connect." ) api_endpoint = fix_url_schema(api_endpoint) - # These parameters are optional in function signature + # This parameters are optional in function signature, # but are required to connect. - # PEP 249 recommendeds making them kwargs. + # It's recommended to make them kwargs by PEP 249 for param, name in ( (database, "database"), (username, "username"), diff --git a/src/firebolt/common/exception.py b/src/firebolt/common/exception.py index 0ca437d84e5..87c8c8f329c 100644 --- a/src/firebolt/common/exception.py +++ b/src/firebolt/common/exception.py @@ -16,8 +16,8 @@ def __init__(self, method_name: str): def __str__(self) -> str: return ( - f"Unable to call {self.method_name}: " - f"Engine must to be attached to a database first." + f"unable to call {self.method_name}: " + f"engine must to be attached to a database first." ) @@ -43,8 +43,8 @@ def __init__(self, method_name: str): def __str__(self) -> str: return ( - f"Unable to call {self.method_name}: " - f"Engine must not be in starting or stopping state." + f"unable to call {self.method_name}: " + f"engine must not be in starting or stopping state." ) @@ -65,7 +65,7 @@ def __init__(self, method_name: str): self.method_name = method_name def __str__(self) -> str: - return f"Unable to call {self.method_name}: cursor closed." + return f"unable to call {self.method_name}: cursor closed" class QueryNotRunError(CursorError): @@ -73,7 +73,7 @@ def __init__(self, method_name: str): self.method_name = method_name def __str__(self) -> str: - return f"Unable to call {self.method_name}: need to run a query first." + return f"unable to call {self.method_name}: need to run a query first" class QueryError(CursorError): @@ -90,7 +90,7 @@ def __init__(self, cause: str, api_endpoint: str): self.api_endpoint = api_endpoint def __str__(self) -> str: - return f"Failed to authenticate at {self.api_endpoint}: {self.cause}." + return f"Failed to authenticate at {self.api_endpoint}: {self.cause}" # PEP-249 diff --git a/src/firebolt/db/connection.py b/src/firebolt/db/connection.py index b975b5f02b9..b03c050e249 100644 --- a/src/firebolt/db/connection.py +++ b/src/firebolt/db/connection.py @@ -60,7 +60,7 @@ def close(self) -> None: # Context manager support def __enter__(self) -> Connection: if self.closed: - raise ConnectionClosedError("Connection is already closed.") + raise ConnectionClosedError("Connection is already closed") return self def __exit__( diff --git a/tests/integration/dbapi/async/test_auth_async.py b/tests/integration/dbapi/async/test_auth_async.py index 5038cb7e956..351cfc7c655 100644 --- a/tests/integration/dbapi/async/test_auth_async.py +++ b/tests/integration/dbapi/async/test_auth_async.py @@ -23,10 +23,10 @@ async def test_refresh_token(connection: Connection) -> None: old = c._client.auth.token c._client.auth._expires = int(time()) - 1 - # Still works fine. + # Still works fine await c.execute("show tables") - assert c._client.auth.token != old, "Auth didn't update token on expiration." + assert c._client.auth.token != old, "Auth didn't update token on expiration" @mark.asyncio diff --git a/tests/integration/dbapi/async/test_errors_async.py b/tests/integration/dbapi/async/test_errors_async.py index 55afdd50f37..d2292220940 100644 --- a/tests/integration/dbapi/async/test_errors_async.py +++ b/tests/integration/dbapi/async/test_errors_async.py @@ -16,7 +16,7 @@ async def test_invalid_credentials( engine_url: str, database_name: str, username: str, password: str, api_endpoint: str ) -> None: - """Connection properly reacts to invalid credentials error.""" + """Connection properly reacts to invalid credentials error""" async with await connect( engine_url=engine_url, database=database_name, @@ -67,7 +67,7 @@ async def test_engine_url_not_exists( account_name: str, api_endpoint: str, ) -> None: - """Connection properly reacts to invalid engine url error.""" + """Connection properly reacts to invalid engine url error""" async with await connect( engine_url=engine_url + "_", database=database_name, @@ -89,7 +89,7 @@ async def test_engine_name_not_exists( account_name: str, api_endpoint: str, ) -> None: - """Connection properly reacts to invalid engine name error.""" + """Connection properly reacts to invalid engine name error""" with raises(FireboltEngineError): async with await connect( engine_name=engine_name + "_________", @@ -111,7 +111,7 @@ async def test_engine_stopped( account_name: str, api_endpoint: str, ) -> None: - """Connection properly reacts to invalid engine name error.""" + """Connection properly reacts to invalid engine name error""" with raises(EngineNotRunningError): async with await connect( engine_url=stopped_engine_url, @@ -128,7 +128,7 @@ async def test_engine_stopped( async def test_database_not_exists( engine_url: str, database_name: str, username: str, password: str, api_endpoint: str ) -> None: - """Connection properly reacts to invalid database error.""" + """Connection properly reacts to invalid database error""" new_db_name = database_name + "_" async with await connect( engine_url=engine_url, @@ -142,16 +142,16 @@ async def test_database_not_exists( assert ( str(exc_info.value) == f"Database {new_db_name} does not exist" - ), "Invalid database name error message." + ), "Invalid database name error message" @mark.asyncio async def test_sql_error(connection: Connection) -> None: - """Connection properly reacts to sql execution error.""" + """Connection properly reacts to sql execution error""" with connection.cursor() as c: with raises(OperationalError) as exc_info: await c.execute("select ]") assert str(exc_info.value).startswith( "Error executing query" - ), "Invalid SQL error message." + ), "Invalid SQL error message" diff --git a/tests/integration/dbapi/async/test_queries_async.py b/tests/integration/dbapi/async/test_queries_async.py index ee19fbde3b1..42288bd5c4f 100644 --- a/tests/integration/dbapi/async/test_queries_async.py +++ b/tests/integration/dbapi/async/test_queries_async.py @@ -69,17 +69,17 @@ async def test_select( async def test_drop_create( connection: Connection, create_drop_description: List[Column] ) -> None: - """Create and drop table/index queries are handled properly.""" + """Create and drop table/index queries are handled propperly""" async def test_query(c: Cursor, query: str) -> None: - assert await c.execute(query) == 1, "Invalid row count returned." - assert c.rowcount == 1, "Invalid rowcount value." + assert await c.execute(query) == 1, "Invalid row count returned" + assert c.rowcount == 1, "Invalid rowcount value" assert_deep_eq( c.description, create_drop_description, - "Invalid create table query description.", + "Invalid create table query description", ) - assert len(await c.fetchall()) == 1, "Invalid data returned." + assert len(await c.fetchall()) == 1, "Invalid data returned" """Create table query is handled properly""" with connection.cursor() as c: @@ -131,12 +131,12 @@ async def test_query(c: Cursor, query: str) -> None: @mark.asyncio async def test_insert(connection: Connection) -> None: - """Insert and delete queries are handled properly.""" + """Insert and delete queries are handled propperly""" async def test_empty_query(c: Cursor, query: str) -> None: - assert await c.execute(query) == -1, "Invalid row count returned." - assert c.rowcount == -1, "Invalid rowcount value." - assert c.description is None, "Invalid description." + assert await c.execute(query) == -1, "Invalid row count returned" + assert c.rowcount == -1, "Invalid rowcount value" + assert c.description is None, "Invalid description" with raises(DataError): await c.fetchone() @@ -161,7 +161,7 @@ async def test_empty_query(c: Cursor, query: str) -> None: assert ( await c.execute("SELECT * FROM test_tb ORDER BY test_tb.id") == 1 - ), "Invalid data length in table after insert." + ), "Invalid data length in table after insert" assert_deep_eq( await c.fetchall(), @@ -176,5 +176,5 @@ async def test_empty_query(c: Cursor, query: str) -> None: [1, 2, 3], ], ], - "Invalid data in table after insert.", + "Invalid data in table after insert", ) diff --git a/tests/integration/dbapi/sync/test_errors.py b/tests/integration/dbapi/sync/test_errors.py index 268383afdf1..0322b4657f1 100644 --- a/tests/integration/dbapi/sync/test_errors.py +++ b/tests/integration/dbapi/sync/test_errors.py @@ -112,7 +112,7 @@ def test_engine_stopped( def test_database_not_exists( engine_url: str, database_name: str, username: str, password: str, api_endpoint: str ) -> None: - """Connection properly reacts to invalid database error.""" + """Connection properly reacts to invalid database error""" new_db_name = database_name + "_" with connect( engine_url=engine_url, @@ -130,7 +130,7 @@ def test_database_not_exists( def test_sql_error(connection: Connection) -> None: - """Connection properly reacts to sql execution error.""" + """Connection properly reacts to sql execution error""" with connection.cursor() as c: with raises(OperationalError) as exc_info: c.execute("select ]") diff --git a/tests/unit/async_db/test_connection.py b/tests/unit/async_db/test_connection.py index f2f2c8e81db..34c0c9bdde8 100644 --- a/tests/unit/async_db/test_connection.py +++ b/tests/unit/async_db/test_connection.py @@ -36,13 +36,13 @@ async def test_cursors_closed_on_close(connection: Connection) -> None: c1, c2 = connection.cursor(), connection.cursor() assert ( len(connection._cursors) == 2 - ), "Invalid number of cursors stored in connection." + ), "Invalid number of cursors stored in connection" await connection.aclose() - assert connection.closed, "Connection was not closed on close." - assert c1.closed, "Cursor was not closed on connection close." - assert c2.closed, "Cursor was not closed on connection close." - assert len(connection._cursors) == 0, "Cursors left in connection after close." + assert connection.closed, "Connection was not closed on close" + assert c1.closed, "Cursor was not closed on connection close" + assert c2.closed, "Cursor was not closed on connection close" + assert len(connection._cursors) == 0, "Cursors left in connection after close" await connection.aclose() @@ -59,7 +59,7 @@ async def test_cursor_initialized( account_id_url: str, python_query_data: List[List[ColType]], ) -> None: - """Connection initialised its cursors properly.""" + """Connection initialised it's cursors propperly""" httpx_mock.add_callback(auth_callback, url=auth_url) httpx_mock.add_callback(query_callback, url=query_url) # httpx_mock.add_callback(account_id_callback, url=account_id_url) @@ -78,7 +78,7 @@ async def test_cursor_initialized( cursor = connection.cursor() assert ( cursor.connection == connection - ), "Invalid cursor connection attribute." + ), "Invalid cursor connection attribute" assert ( cursor._client == connection._client ), "Invalid cursor _client attribute" @@ -88,7 +88,7 @@ async def test_cursor_initialized( cursor.close() assert ( cursor not in connection._cursors - ), "Cursor wasn't removed from connection after close." + ), "Cursor wasn't removed from connection after close" @mark.asyncio @@ -139,7 +139,7 @@ async def test_connect_engine_name( ): pass assert str(exc_info.value).startswith( - "Both engine_name and engine_url are provided." + "Both engine_name and engine_url are provided" ) with raises(InterfaceError) as exc_info: @@ -151,7 +151,7 @@ async def test_connect_engine_name( ): pass assert str(exc_info.value).startswith( - "Neither engine_name nor engine_url is provided." + "Neither engine_name nor engine_url are provided" ) httpx_mock.add_callback(auth_callback, url=auth_url) diff --git a/tests/unit/client/test_auth.py b/tests/unit/client/test_auth.py index 29cdf77fc37..9b775975857 100644 --- a/tests/unit/client/test_auth.py +++ b/tests/unit/client/test_auth.py @@ -52,7 +52,7 @@ def test_auth_refresh_on_expiration( execute_generator_requests(auth.auth_flow(Request("GET", "https://host"))) assert auth.token == test_token, "invalid access token" execute_generator_requests(auth.auth_flow(Request("GET", "https://host"))) - assert auth.token == test_token2, "Expired access token was not updated." + assert auth.token == test_token2, "expired access token was not updated" def test_auth_uses_same_token_if_valid( @@ -88,7 +88,7 @@ def test_auth_uses_same_token_if_valid( execute_generator_requests(auth.auth_flow(Request("GET", "https://host"))) assert auth.token == test_token, "invalid access token" execute_generator_requests(auth.auth_flow(Request("GET", "https://host"))) - assert auth.token == test_token, "Shoud not update token until it expires." + assert auth.token == test_token, "shoud not update token until it expires" httpx_mock.reset(False) @@ -129,7 +129,7 @@ def http_error(**kwargs): execute_generator_requests(auth.get_new_token_generator()) assert ( - str(excinfo.value) == "Failed to authenticate at https://host: firebolt." + str(excinfo.value) == "Failed to authenticate at https://host: firebolt" ), "Invalid authentication error message" httpx_mock.reset(True) diff --git a/tests/unit/db/test_connection.py b/tests/unit/db/test_connection.py index b9150588349..1d2a31a7556 100644 --- a/tests/unit/db/test_connection.py +++ b/tests/unit/db/test_connection.py @@ -121,7 +121,7 @@ def test_connect_engine_name( password="password", ) assert str(exc_info.value).startswith( - "Both engine_name and engine_url are provided." + "Both engine_name and engine_url are provided" ) with raises(InterfaceError) as exc_info: @@ -131,7 +131,7 @@ def test_connect_engine_name( password="password", ) assert str(exc_info.value).startswith( - "Neither engine_name nor engine_url is provided." + "Neither engine_name nor engine_url are provided" ) httpx_mock.add_callback(auth_callback, url=auth_url) From 3bc8541250dff1aa28fcc7af7cd1d7a5520d6701 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Tue, 21 Dec 2021 23:46:20 -0500 Subject: [PATCH 20/33] Fixed several typos in comments. --- src/firebolt/async_db/connection.py | 14 ++++++------ src/firebolt/common/exception.py | 14 ++++++------ src/firebolt/db/connection.py | 2 +- .../dbapi/async/test_auth_async.py | 4 ++-- .../dbapi/async/test_errors_async.py | 16 +++++++------- .../dbapi/async/test_queries_async.py | 22 +++++++++---------- tests/integration/dbapi/sync/test_errors.py | 4 ++-- tests/unit/async_db/test_connection.py | 20 ++++++++--------- tests/unit/client/test_auth.py | 6 ++--- tests/unit/db/test_connection.py | 4 ++-- 10 files changed, 53 insertions(+), 53 deletions(-) diff --git a/src/firebolt/async_db/connection.py b/src/firebolt/async_db/connection.py index 058b683e7e0..0023943a6b5 100644 --- a/src/firebolt/async_db/connection.py +++ b/src/firebolt/async_db/connection.py @@ -48,12 +48,12 @@ async def _resolve_engine_url( response.raise_for_status() return response.json()["engine"]["endpoint"] except HTTPStatusError as e: - # Engine error would be 404 + # Engine error would be 404. if e.response.status_code != 404: raise InterfaceError(f"unable to retrieve engine endpoint: {e}") # Once this is point is reached we've already authenticated with # the backend so it's safe to assume the cause of the error is - # missing engine + # missing engine. raise FireboltEngineError(f"Firebolt engine {engine_name} does not exist") except (JSONDecodeError, RequestError, RuntimeError, HTTPStatusError) as e: raise InterfaceError(f"unable to retrieve engine endpoint: {e}") @@ -82,25 +82,25 @@ async def connect_inner( api_endpoint(optional): Firebolt API endpoint. Used for authentication. Note: - either `engine_name` or `engine_url` should be provided, but not both + Either `engine_name` or `engine_url` should be provided, but not both. """ if engine_name and engine_url: raise InterfaceError( - "Both engine_name and engine_url are provided." + "Both engine_name and engine_url are provided. " "Provide only one to connect." ) if not engine_name and not engine_url: raise InterfaceError( - "Neither engine_name nor engine_url are provided." + "Neither engine_name nor engine_url is provided. " "Provide one to connect." ) api_endpoint = fix_url_schema(api_endpoint) - # This parameters are optional in function signature, + # These parameters are optional in function signature # but are required to connect. - # It's recommended to make them kwargs by PEP 249 + # PEP 249 recommends making them kwargs. for param, name in ( (database, "database"), (username, "username"), diff --git a/src/firebolt/common/exception.py b/src/firebolt/common/exception.py index 87c8c8f329c..0ca437d84e5 100644 --- a/src/firebolt/common/exception.py +++ b/src/firebolt/common/exception.py @@ -16,8 +16,8 @@ def __init__(self, method_name: str): def __str__(self) -> str: return ( - f"unable to call {self.method_name}: " - f"engine must to be attached to a database first." + f"Unable to call {self.method_name}: " + f"Engine must to be attached to a database first." ) @@ -43,8 +43,8 @@ def __init__(self, method_name: str): def __str__(self) -> str: return ( - f"unable to call {self.method_name}: " - f"engine must not be in starting or stopping state." + f"Unable to call {self.method_name}: " + f"Engine must not be in starting or stopping state." ) @@ -65,7 +65,7 @@ def __init__(self, method_name: str): self.method_name = method_name def __str__(self) -> str: - return f"unable to call {self.method_name}: cursor closed" + return f"Unable to call {self.method_name}: cursor closed." class QueryNotRunError(CursorError): @@ -73,7 +73,7 @@ def __init__(self, method_name: str): self.method_name = method_name def __str__(self) -> str: - return f"unable to call {self.method_name}: need to run a query first" + return f"Unable to call {self.method_name}: need to run a query first." class QueryError(CursorError): @@ -90,7 +90,7 @@ def __init__(self, cause: str, api_endpoint: str): self.api_endpoint = api_endpoint def __str__(self) -> str: - return f"Failed to authenticate at {self.api_endpoint}: {self.cause}" + return f"Failed to authenticate at {self.api_endpoint}: {self.cause}." # PEP-249 diff --git a/src/firebolt/db/connection.py b/src/firebolt/db/connection.py index b03c050e249..b975b5f02b9 100644 --- a/src/firebolt/db/connection.py +++ b/src/firebolt/db/connection.py @@ -60,7 +60,7 @@ def close(self) -> None: # Context manager support def __enter__(self) -> Connection: if self.closed: - raise ConnectionClosedError("Connection is already closed") + raise ConnectionClosedError("Connection is already closed.") return self def __exit__( diff --git a/tests/integration/dbapi/async/test_auth_async.py b/tests/integration/dbapi/async/test_auth_async.py index 351cfc7c655..5038cb7e956 100644 --- a/tests/integration/dbapi/async/test_auth_async.py +++ b/tests/integration/dbapi/async/test_auth_async.py @@ -23,10 +23,10 @@ async def test_refresh_token(connection: Connection) -> None: old = c._client.auth.token c._client.auth._expires = int(time()) - 1 - # Still works fine + # Still works fine. await c.execute("show tables") - assert c._client.auth.token != old, "Auth didn't update token on expiration" + assert c._client.auth.token != old, "Auth didn't update token on expiration." @mark.asyncio diff --git a/tests/integration/dbapi/async/test_errors_async.py b/tests/integration/dbapi/async/test_errors_async.py index d2292220940..1e6728229f0 100644 --- a/tests/integration/dbapi/async/test_errors_async.py +++ b/tests/integration/dbapi/async/test_errors_async.py @@ -16,7 +16,7 @@ async def test_invalid_credentials( engine_url: str, database_name: str, username: str, password: str, api_endpoint: str ) -> None: - """Connection properly reacts to invalid credentials error""" + """Connection properly reacts to invalid credentials error.""" async with await connect( engine_url=engine_url, database=database_name, @@ -67,7 +67,7 @@ async def test_engine_url_not_exists( account_name: str, api_endpoint: str, ) -> None: - """Connection properly reacts to invalid engine url error""" + """Connection properly reacts to invalid engine url error.""" async with await connect( engine_url=engine_url + "_", database=database_name, @@ -89,7 +89,7 @@ async def test_engine_name_not_exists( account_name: str, api_endpoint: str, ) -> None: - """Connection properly reacts to invalid engine name error""" + """Connection properly reacts to invalid engine name error.""" with raises(FireboltEngineError): async with await connect( engine_name=engine_name + "_________", @@ -111,7 +111,7 @@ async def test_engine_stopped( account_name: str, api_endpoint: str, ) -> None: - """Connection properly reacts to invalid engine name error""" + """Connection properly reacts to invalid engine name error.""" with raises(EngineNotRunningError): async with await connect( engine_url=stopped_engine_url, @@ -128,7 +128,7 @@ async def test_engine_stopped( async def test_database_not_exists( engine_url: str, database_name: str, username: str, password: str, api_endpoint: str ) -> None: - """Connection properly reacts to invalid database error""" + """Connection properly reacts to invalid database error.""" new_db_name = database_name + "_" async with await connect( engine_url=engine_url, @@ -142,16 +142,16 @@ async def test_database_not_exists( assert ( str(exc_info.value) == f"Database {new_db_name} does not exist" - ), "Invalid database name error message" + ), "Invalid database name error message." @mark.asyncio async def test_sql_error(connection: Connection) -> None: - """Connection properly reacts to sql execution error""" + """Connection properly reacts to SQL execution error.""" with connection.cursor() as c: with raises(OperationalError) as exc_info: await c.execute("select ]") assert str(exc_info.value).startswith( "Error executing query" - ), "Invalid SQL error message" + ), "Invalid SQL error message." diff --git a/tests/integration/dbapi/async/test_queries_async.py b/tests/integration/dbapi/async/test_queries_async.py index 42288bd5c4f..ee19fbde3b1 100644 --- a/tests/integration/dbapi/async/test_queries_async.py +++ b/tests/integration/dbapi/async/test_queries_async.py @@ -69,17 +69,17 @@ async def test_select( async def test_drop_create( connection: Connection, create_drop_description: List[Column] ) -> None: - """Create and drop table/index queries are handled propperly""" + """Create and drop table/index queries are handled properly.""" async def test_query(c: Cursor, query: str) -> None: - assert await c.execute(query) == 1, "Invalid row count returned" - assert c.rowcount == 1, "Invalid rowcount value" + assert await c.execute(query) == 1, "Invalid row count returned." + assert c.rowcount == 1, "Invalid rowcount value." assert_deep_eq( c.description, create_drop_description, - "Invalid create table query description", + "Invalid create table query description.", ) - assert len(await c.fetchall()) == 1, "Invalid data returned" + assert len(await c.fetchall()) == 1, "Invalid data returned." """Create table query is handled properly""" with connection.cursor() as c: @@ -131,12 +131,12 @@ async def test_query(c: Cursor, query: str) -> None: @mark.asyncio async def test_insert(connection: Connection) -> None: - """Insert and delete queries are handled propperly""" + """Insert and delete queries are handled properly.""" async def test_empty_query(c: Cursor, query: str) -> None: - assert await c.execute(query) == -1, "Invalid row count returned" - assert c.rowcount == -1, "Invalid rowcount value" - assert c.description is None, "Invalid description" + assert await c.execute(query) == -1, "Invalid row count returned." + assert c.rowcount == -1, "Invalid rowcount value." + assert c.description is None, "Invalid description." with raises(DataError): await c.fetchone() @@ -161,7 +161,7 @@ async def test_empty_query(c: Cursor, query: str) -> None: assert ( await c.execute("SELECT * FROM test_tb ORDER BY test_tb.id") == 1 - ), "Invalid data length in table after insert" + ), "Invalid data length in table after insert." assert_deep_eq( await c.fetchall(), @@ -176,5 +176,5 @@ async def test_empty_query(c: Cursor, query: str) -> None: [1, 2, 3], ], ], - "Invalid data in table after insert", + "Invalid data in table after insert.", ) diff --git a/tests/integration/dbapi/sync/test_errors.py b/tests/integration/dbapi/sync/test_errors.py index 0322b4657f1..268383afdf1 100644 --- a/tests/integration/dbapi/sync/test_errors.py +++ b/tests/integration/dbapi/sync/test_errors.py @@ -112,7 +112,7 @@ def test_engine_stopped( def test_database_not_exists( engine_url: str, database_name: str, username: str, password: str, api_endpoint: str ) -> None: - """Connection properly reacts to invalid database error""" + """Connection properly reacts to invalid database error.""" new_db_name = database_name + "_" with connect( engine_url=engine_url, @@ -130,7 +130,7 @@ def test_database_not_exists( def test_sql_error(connection: Connection) -> None: - """Connection properly reacts to sql execution error""" + """Connection properly reacts to sql execution error.""" with connection.cursor() as c: with raises(OperationalError) as exc_info: c.execute("select ]") diff --git a/tests/unit/async_db/test_connection.py b/tests/unit/async_db/test_connection.py index 34c0c9bdde8..f2f2c8e81db 100644 --- a/tests/unit/async_db/test_connection.py +++ b/tests/unit/async_db/test_connection.py @@ -36,13 +36,13 @@ async def test_cursors_closed_on_close(connection: Connection) -> None: c1, c2 = connection.cursor(), connection.cursor() assert ( len(connection._cursors) == 2 - ), "Invalid number of cursors stored in connection" + ), "Invalid number of cursors stored in connection." await connection.aclose() - assert connection.closed, "Connection was not closed on close" - assert c1.closed, "Cursor was not closed on connection close" - assert c2.closed, "Cursor was not closed on connection close" - assert len(connection._cursors) == 0, "Cursors left in connection after close" + assert connection.closed, "Connection was not closed on close." + assert c1.closed, "Cursor was not closed on connection close." + assert c2.closed, "Cursor was not closed on connection close." + assert len(connection._cursors) == 0, "Cursors left in connection after close." await connection.aclose() @@ -59,7 +59,7 @@ async def test_cursor_initialized( account_id_url: str, python_query_data: List[List[ColType]], ) -> None: - """Connection initialised it's cursors propperly""" + """Connection initialised its cursors properly.""" httpx_mock.add_callback(auth_callback, url=auth_url) httpx_mock.add_callback(query_callback, url=query_url) # httpx_mock.add_callback(account_id_callback, url=account_id_url) @@ -78,7 +78,7 @@ async def test_cursor_initialized( cursor = connection.cursor() assert ( cursor.connection == connection - ), "Invalid cursor connection attribute" + ), "Invalid cursor connection attribute." assert ( cursor._client == connection._client ), "Invalid cursor _client attribute" @@ -88,7 +88,7 @@ async def test_cursor_initialized( cursor.close() assert ( cursor not in connection._cursors - ), "Cursor wasn't removed from connection after close" + ), "Cursor wasn't removed from connection after close." @mark.asyncio @@ -139,7 +139,7 @@ async def test_connect_engine_name( ): pass assert str(exc_info.value).startswith( - "Both engine_name and engine_url are provided" + "Both engine_name and engine_url are provided." ) with raises(InterfaceError) as exc_info: @@ -151,7 +151,7 @@ async def test_connect_engine_name( ): pass assert str(exc_info.value).startswith( - "Neither engine_name nor engine_url are provided" + "Neither engine_name nor engine_url is provided." ) httpx_mock.add_callback(auth_callback, url=auth_url) diff --git a/tests/unit/client/test_auth.py b/tests/unit/client/test_auth.py index 9b775975857..34c8c087568 100644 --- a/tests/unit/client/test_auth.py +++ b/tests/unit/client/test_auth.py @@ -52,7 +52,7 @@ def test_auth_refresh_on_expiration( execute_generator_requests(auth.auth_flow(Request("GET", "https://host"))) assert auth.token == test_token, "invalid access token" execute_generator_requests(auth.auth_flow(Request("GET", "https://host"))) - assert auth.token == test_token2, "expired access token was not updated" + assert auth.token == test_token2, "Expired access token was not updated." def test_auth_uses_same_token_if_valid( @@ -88,7 +88,7 @@ def test_auth_uses_same_token_if_valid( execute_generator_requests(auth.auth_flow(Request("GET", "https://host"))) assert auth.token == test_token, "invalid access token" execute_generator_requests(auth.auth_flow(Request("GET", "https://host"))) - assert auth.token == test_token, "shoud not update token until it expires" + assert auth.token == test_token, "Should not update token until it expires." httpx_mock.reset(False) @@ -129,7 +129,7 @@ def http_error(**kwargs): execute_generator_requests(auth.get_new_token_generator()) assert ( - str(excinfo.value) == "Failed to authenticate at https://host: firebolt" + str(excinfo.value) == "Failed to authenticate at https://host: firebolt." ), "Invalid authentication error message" httpx_mock.reset(True) diff --git a/tests/unit/db/test_connection.py b/tests/unit/db/test_connection.py index 1d2a31a7556..b9150588349 100644 --- a/tests/unit/db/test_connection.py +++ b/tests/unit/db/test_connection.py @@ -121,7 +121,7 @@ def test_connect_engine_name( password="password", ) assert str(exc_info.value).startswith( - "Both engine_name and engine_url are provided" + "Both engine_name and engine_url are provided." ) with raises(InterfaceError) as exc_info: @@ -131,7 +131,7 @@ def test_connect_engine_name( password="password", ) assert str(exc_info.value).startswith( - "Neither engine_name nor engine_url are provided" + "Neither engine_name nor engine_url is provided." ) httpx_mock.add_callback(auth_callback, url=auth_url) From 544d4bbe694344f0013e27bed847a93c4d8ebb2b Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Wed, 22 Dec 2021 01:07:06 -0500 Subject: [PATCH 21/33] Fixed typos in some error messages. --- src/firebolt/async_db/connection.py | 6 +++--- src/firebolt/async_db/cursor.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/firebolt/async_db/connection.py b/src/firebolt/async_db/connection.py index 0023943a6b5..e0fff61e693 100644 --- a/src/firebolt/async_db/connection.py +++ b/src/firebolt/async_db/connection.py @@ -50,13 +50,13 @@ async def _resolve_engine_url( except HTTPStatusError as e: # Engine error would be 404. if e.response.status_code != 404: - raise InterfaceError(f"unable to retrieve engine endpoint: {e}") + raise InterfaceError(f"Unable to retrieve engine endpoint: {e}.") # Once this is point is reached we've already authenticated with # the backend so it's safe to assume the cause of the error is # missing engine. - raise FireboltEngineError(f"Firebolt engine {engine_name} does not exist") + raise FireboltEngineError(f"Firebolt engine {engine_name} does not exist.") except (JSONDecodeError, RequestError, RuntimeError, HTTPStatusError) as e: - raise InterfaceError(f"unable to retrieve engine endpoint: {e}") + raise InterfaceError(f"Unable to retrieve engine endpoint: {e}.") def async_connect_factory(connection_class: Type) -> Callable: diff --git a/src/firebolt/async_db/cursor.py b/src/firebolt/async_db/cursor.py index 76a401d45e6..a6a35b55d33 100644 --- a/src/firebolt/async_db/cursor.py +++ b/src/firebolt/async_db/cursor.py @@ -203,7 +203,7 @@ async def _raise_if_error(self, resp: Response) -> None: if not await is_engine_running(self.connection, self.connection.engine_url): raise EngineNotRunningError( f"Firebolt engine {self.connection.engine_url} " - "needs to be running to run queries against it" + "needs to be running to run queries against it." ) resp.raise_for_status() From f443766aa14615cf7ffebb2955e790ce8f1a78c8 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Wed, 5 Jan 2022 12:51:41 -0500 Subject: [PATCH 22/33] Removed an extra call to retrieve account_id in connection.py. --- setup.cfg | 4 ++-- src/firebolt/async_db/connection.py | 2 +- src/firebolt/client/client.py | 2 -- src/firebolt/common/util.py | 16 ++++------------ tests/unit/client/test_client_async.py | 2 +- 5 files changed, 8 insertions(+), 18 deletions(-) diff --git a/setup.cfg b/setup.cfg index 470de21b51f..7d8c31fd961 100755 --- a/setup.cfg +++ b/setup.cfg @@ -24,11 +24,10 @@ project_urls = packages = find: install_requires = aiorwlock==1.1.0 - async-property==0.2.1 httpx[http2]==0.19.0 pydantic[dotenv]==1.8.2 readerwriterlock==1.0.9 - syncer==1.3.0 + trio==0.19.0 python_requires = >=3.7 include_package_data = True package_dir = @@ -49,6 +48,7 @@ dev = pytest-cov==3.0.0 pytest-httpx==0.13.0 pytest-mock==3.6.1 + trio-typing==0.7.0 release = argparse build diff --git a/src/firebolt/async_db/connection.py b/src/firebolt/async_db/connection.py index 058b683e7e0..57a06417e8c 100644 --- a/src/firebolt/async_db/connection.py +++ b/src/firebolt/async_db/connection.py @@ -33,7 +33,7 @@ async def _resolve_engine_url( api_endpoint=api_endpoint, ) as client: try: - account_id = await client.account_id + account_id = await client.account_id() response = await client.get( url=ACCOUNT_ENGINE_BY_NAME_URL.format(account_id=account_id), params={"engine_name": engine_name}, diff --git a/src/firebolt/client/client.py b/src/firebolt/client/client.py index f011d310697..d22bac87368 100644 --- a/src/firebolt/client/client.py +++ b/src/firebolt/client/client.py @@ -1,6 +1,5 @@ from typing import Any, Optional -from async_property import async_cached_property # type: ignore from httpx import AsyncClient as HttpxAsyncClient from httpx import Client as HttpxClient from httpx import _types @@ -81,7 +80,6 @@ class AsyncClient(FireboltClientMixin, HttpxAsyncClient): + (HttpxAsyncClient.__doc__ or "") """ - @async_cached_property async def account_id(self) -> str: if self.account_name is not None: response = await self.get( diff --git a/src/firebolt/common/util.py b/src/firebolt/common/util.py index dfeb7313b40..3ae028a3944 100644 --- a/src/firebolt/common/util.py +++ b/src/firebolt/common/util.py @@ -1,7 +1,8 @@ -from asyncio import get_event_loop, new_event_loop -from functools import lru_cache, wraps +from functools import lru_cache, partial, wraps from typing import TYPE_CHECKING, Any, Callable, Type, TypeVar +from trio import run as sync_run + T = TypeVar("T") @@ -40,15 +41,6 @@ def fix_url_schema(url: str) -> str: def async_to_sync(f: Callable) -> Callable: @wraps(f) def sync(*args: Any, **kwargs: Any) -> Any: - close = False - try: - loop = get_event_loop() - except RuntimeError: - loop = new_event_loop() - close = True - res = loop.run_until_complete(f(*args, **kwargs)) - if close: - loop.close() - return res + return sync_run(partial(f, *args, **kwargs)) return sync diff --git a/tests/unit/client/test_client_async.py b/tests/unit/client/test_client_async.py index 04a4c1dad8d..6aeb8bb4888 100644 --- a/tests/unit/client/test_client_async.py +++ b/tests/unit/client/test_client_async.py @@ -112,4 +112,4 @@ async def test_client_account_id( base_url=fix_url_schema(settings.server), api_endpoint=settings.server, ) as c: - assert await c.account_id == account_id, "Invalid account id returned" + assert await c.account_id() == account_id, "Invalid account id returned" From 38de019b4e2fe3e9a7291c472cae010929b62375 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Thu, 6 Jan 2022 10:56:27 -0500 Subject: [PATCH 23/33] Changed AccountError to AccountNotFoundError and used httpx.codes for no response error on missing account_name. --- src/firebolt/client/client.py | 15 ++++++++++----- src/firebolt/common/exception.py | 2 +- .../integration/dbapi/async/test_errors_async.py | 4 ++-- tests/integration/dbapi/sync/test_errors.py | 4 ++-- 4 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/firebolt/client/client.py b/src/firebolt/client/client.py index d22bac87368..958efd7ea53 100644 --- a/src/firebolt/client/client.py +++ b/src/firebolt/client/client.py @@ -1,5 +1,6 @@ from typing import Any, Optional +from https import codes as HttpxCodes from httpx import AsyncClient as HttpxAsyncClient from httpx import Client as HttpxClient from httpx import _types @@ -7,7 +8,7 @@ from firebolt.client.auth import Auth from firebolt.client.constants import DEFAULT_API_URL -from firebolt.common.exception import AccountError +from firebolt.common.exception import AccountNotFoundError from firebolt.common.urls import ACCOUNT_BY_NAME_URL, ACCOUNT_URL from firebolt.common.util import cached_property, fix_url_schema, mixin_for @@ -60,8 +61,10 @@ def account_id(self) -> str: response = self.get( url=ACCOUNT_BY_NAME_URL, params={"account_name": self.account_name} ) - if response.status_code != 200: - raise AccountError(self.account_name) + if response.status_code == HttpxCodes.NOT_FOUND: + raise AccountNotFoundError(self.account_name) + # process all other status codes + response.raise_for_status() return response.json()["account_id"] else: # account_name isn't set, use the default account. return self.get(url=ACCOUNT_URL).json()["account"]["id"] @@ -85,8 +88,10 @@ async def account_id(self) -> str: response = await self.get( url=ACCOUNT_BY_NAME_URL, params={"account_name": self.account_name} ) - if response.status_code != 200: - raise AccountError(self.account_name) + if response.status_code == HttpxCodes.NOT_FOUND: + raise AccountNotFoundError(self.account_name) + # process all other status codes + response.raise_for_status() return response.json()["account_id"] else: # account_name isn't set; use the default account. return (await self.get(url=ACCOUNT_URL)).json()["account"]["id"] diff --git a/src/firebolt/common/exception.py b/src/firebolt/common/exception.py index 87c8c8f329c..60f58d7fbde 100644 --- a/src/firebolt/common/exception.py +++ b/src/firebolt/common/exception.py @@ -29,7 +29,7 @@ class FireboltDatabaseError(FireboltError): pass -class AccountError(FireboltError): +class AccountNotFoundError(FireboltError): def __init__(self, method_name: str): self.method_name = method_name diff --git a/tests/integration/dbapi/async/test_errors_async.py b/tests/integration/dbapi/async/test_errors_async.py index d2292220940..1aee08187de 100644 --- a/tests/integration/dbapi/async/test_errors_async.py +++ b/tests/integration/dbapi/async/test_errors_async.py @@ -3,7 +3,7 @@ from firebolt.async_db import Connection, connect from firebolt.common.exception import ( - AccountError, + AccountNotFoundError, AuthenticationError, EngineNotRunningError, FireboltDatabaseError, @@ -42,7 +42,7 @@ async def test_invalid_account( ) -> None: """Connection properly reacts to invalid account error.""" account_name = "--" - with raises(AccountError) as exc_info: + with raises(AccountNotFoundError) as exc_info: async with await connect( database=database_name, engine_name=engine_name, # Omit engine_url to force account_id lookup. diff --git a/tests/integration/dbapi/sync/test_errors.py b/tests/integration/dbapi/sync/test_errors.py index 0322b4657f1..1da62711823 100644 --- a/tests/integration/dbapi/sync/test_errors.py +++ b/tests/integration/dbapi/sync/test_errors.py @@ -2,7 +2,7 @@ from pytest import raises from firebolt.common.exception import ( - AccountError, + AccountNotFoundError, AuthenticationError, EngineNotRunningError, FireboltDatabaseError, @@ -40,7 +40,7 @@ def test_invalid_account( ) -> None: """Connection properly reacts to invalid account error.""" account_name = "--" - with raises(AccountError) as exc_info: + with raises(AccountNotFoundError) as exc_info: with connect( database=database_name, engine_name=engine_name, # Omit engine_url to force account_id lookup. From 3508c74ba658d1380572f268555a9b3cdfab08b0 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Thu, 6 Jan 2022 14:25:40 -0500 Subject: [PATCH 24/33] Mistyped httpx in import in client.py. --- src/firebolt/client/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/firebolt/client/client.py b/src/firebolt/client/client.py index 958efd7ea53..55d7525717e 100644 --- a/src/firebolt/client/client.py +++ b/src/firebolt/client/client.py @@ -1,9 +1,9 @@ from typing import Any, Optional -from https import codes as HttpxCodes from httpx import AsyncClient as HttpxAsyncClient from httpx import Client as HttpxClient from httpx import _types +from httpx import codes as HttpxCodes from httpx._types import AuthTypes from firebolt.client.auth import Auth From 0f2bfffc81e24cf8c058a96a7d7143538caa8459 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Thu, 6 Jan 2022 15:30:32 -0500 Subject: [PATCH 25/33] Removed accidental change from asyncio to trio in common/utils.py. --- src/firebolt/common/util.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/firebolt/common/util.py b/src/firebolt/common/util.py index 3ae028a3944..c9507f0d7f8 100644 --- a/src/firebolt/common/util.py +++ b/src/firebolt/common/util.py @@ -1,8 +1,7 @@ -from functools import lru_cache, partial, wraps +from asyncio import get_event_loop, new_event_loop, set_event_loop +from functools import lru_cache, wraps from typing import TYPE_CHECKING, Any, Callable, Type, TypeVar -from trio import run as sync_run - T = TypeVar("T") @@ -41,6 +40,12 @@ def fix_url_schema(url: str) -> str: def async_to_sync(f: Callable) -> Callable: @wraps(f) def sync(*args: Any, **kwargs: Any) -> Any: - return sync_run(partial(f, *args, **kwargs)) + try: + loop = get_event_loop() + except RuntimeError: + loop = new_event_loop() + set_event_loop(loop) + res = loop.run_until_complete(f(*args, **kwargs)) + return res return sync From 5f92642b0ea056c852ff2312574a06a8df1d0879 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Fri, 7 Jan 2022 00:24:10 -0500 Subject: [PATCH 26/33] Removed account_name completely from Connection class. --- src/firebolt/async_db/connection.py | 8 +------- src/firebolt/db/connection.py | 2 -- tests/unit/db/test_connection.py | 2 +- 3 files changed, 2 insertions(+), 10 deletions(-) diff --git a/src/firebolt/async_db/connection.py b/src/firebolt/async_db/connection.py index 57a06417e8c..f8f20160468 100644 --- a/src/firebolt/async_db/connection.py +++ b/src/firebolt/async_db/connection.py @@ -126,9 +126,7 @@ async def connect_inner( assert engine_url is not None engine_url = fix_url_schema(engine_url) - return connection_class( - engine_url, database, username, password, account_name, api_endpoint - ) + return connection_class(engine_url, database, username, password, api_endpoint) return connect_inner @@ -141,7 +139,6 @@ class BaseConnection: "_cursors", "database", "engine_url", - "account_name", "api_endpoint", "_is_closed", ) @@ -152,13 +149,11 @@ def __init__( database: str, # TODO: Get by engine name username: str, password: str, - account_name: str, api_endpoint: str = DEFAULT_API_URL, ): self._client = AsyncClient( auth=(username, password), base_url=engine_url, - account_name=account_name, api_endpoint=api_endpoint, timeout=Timeout(DEFAULT_TIMEOUT_SECONDS, read=None), ) @@ -224,7 +219,6 @@ class Connection(BaseConnection): database: Firebolt database name username: Firebolt account username password: Firebolt account password - account_name: Necessary for entities with more than one account. api_endpoint: Optional. Firebolt API endpoint. Used for authentication. Note: diff --git a/src/firebolt/db/connection.py b/src/firebolt/db/connection.py index b03c050e249..27fc197c51c 100644 --- a/src/firebolt/db/connection.py +++ b/src/firebolt/db/connection.py @@ -26,8 +26,6 @@ class Connection(AsyncBaseConnection): database: Firebolt database name username: Firebolt account username password: Firebolt account password - account_name: Necessary for customers with multiple accounts; - if blank uses default. api_endpoint: Optional. Firebolt API endpoint. Used for authentication. Note: diff --git a/tests/unit/db/test_connection.py b/tests/unit/db/test_connection.py index 1d2a31a7556..47f0dc7dc5e 100644 --- a/tests/unit/db/test_connection.py +++ b/tests/unit/db/test_connection.py @@ -162,7 +162,7 @@ def test_connect_engine_name( def test_connection_unclosed_warnings(): - c = Connection("", "", "", "", "", "") + c = Connection("", "", "", "", "") with warns(UserWarning) as winfo: del c From 180bf05349bdc85e3941e2e43f9912d4681e0b01 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Mon, 10 Jan 2022 12:12:31 -0500 Subject: [PATCH 27/33] Small cleanups to deal with a failed rebase. --- setup.cfg | 2 -- 1 file changed, 2 deletions(-) diff --git a/setup.cfg b/setup.cfg index 7d8c31fd961..64819164754 100755 --- a/setup.cfg +++ b/setup.cfg @@ -27,7 +27,6 @@ install_requires = httpx[http2]==0.19.0 pydantic[dotenv]==1.8.2 readerwriterlock==1.0.9 - trio==0.19.0 python_requires = >=3.7 include_package_data = True package_dir = @@ -48,7 +47,6 @@ dev = pytest-cov==3.0.0 pytest-httpx==0.13.0 pytest-mock==3.6.1 - trio-typing==0.7.0 release = argparse build From 0ee2d4ece9b08a63ac36e5562d3cd42e732aceca Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Mon, 10 Jan 2022 12:39:24 -0500 Subject: [PATCH 28/33] Add @async_cached_property back to client.py. --- src/firebolt/client/client.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/firebolt/client/client.py b/src/firebolt/client/client.py index 55d7525717e..53d63a461d3 100644 --- a/src/firebolt/client/client.py +++ b/src/firebolt/client/client.py @@ -1,5 +1,6 @@ from typing import Any, Optional +from async_property import async_cached_property # type: ignore from httpx import AsyncClient as HttpxAsyncClient from httpx import Client as HttpxClient from httpx import _types @@ -83,6 +84,7 @@ class AsyncClient(FireboltClientMixin, HttpxAsyncClient): + (HttpxAsyncClient.__doc__ or "") """ + @async_cached_property async def account_id(self) -> str: if self.account_name is not None: response = await self.get( From c07aef715b3300f0c8cde3166236d2008c530b18 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Mon, 10 Jan 2022 17:33:13 -0500 Subject: [PATCH 29/33] Fixed await error caused by last commit. --- src/firebolt/async_db/connection.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/firebolt/async_db/connection.py b/src/firebolt/async_db/connection.py index f8f20160468..88230455ee3 100644 --- a/src/firebolt/async_db/connection.py +++ b/src/firebolt/async_db/connection.py @@ -33,7 +33,7 @@ async def _resolve_engine_url( api_endpoint=api_endpoint, ) as client: try: - account_id = await client.account_id() + account_id = await client.account_id response = await client.get( url=ACCOUNT_ENGINE_BY_NAME_URL.format(account_id=account_id), params={"engine_name": engine_name}, From 4c960783d5b4c02ac9893eaedc0aeb7547793693 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Mon, 10 Jan 2022 17:34:10 -0500 Subject: [PATCH 30/33] account_id_callback is unused in test_cursor_initialized in asynch_db/test_connection. Commented it out for now. --- tests/unit/async_db/test_connection.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/async_db/test_connection.py b/tests/unit/async_db/test_connection.py index 34c0c9bdde8..1847b3951cd 100644 --- a/tests/unit/async_db/test_connection.py +++ b/tests/unit/async_db/test_connection.py @@ -55,8 +55,8 @@ async def test_cursor_initialized( auth_url: str, query_callback: Callable, query_url: str, - account_id_callback: Callable, - account_id_url: str, + # account_id_callback: Callable, + # account_id_url: str, python_query_data: List[List[ColType]], ) -> None: """Connection initialised it's cursors propperly""" From ea613004c4b2791182b61e244fe9d779563d8416 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Mon, 10 Jan 2022 20:55:54 -0500 Subject: [PATCH 31/33] Fixed unit test errors caused by 0ee2d4ece9b08a63ac36e5562d3cd42e732aceca. --- tests/unit/async_db/test_connection.py | 3 --- tests/unit/client/test_client_async.py | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/unit/async_db/test_connection.py b/tests/unit/async_db/test_connection.py index 1847b3951cd..fbdf2dc3c73 100644 --- a/tests/unit/async_db/test_connection.py +++ b/tests/unit/async_db/test_connection.py @@ -55,14 +55,11 @@ async def test_cursor_initialized( auth_url: str, query_callback: Callable, query_url: str, - # account_id_callback: Callable, - # account_id_url: str, python_query_data: List[List[ColType]], ) -> None: """Connection initialised it's cursors propperly""" httpx_mock.add_callback(auth_callback, url=auth_url) httpx_mock.add_callback(query_callback, url=query_url) - # httpx_mock.add_callback(account_id_callback, url=account_id_url) for url in (settings.server, f"https://{settings.server}"): async with ( diff --git a/tests/unit/client/test_client_async.py b/tests/unit/client/test_client_async.py index 6aeb8bb4888..9c563d3c511 100644 --- a/tests/unit/client/test_client_async.py +++ b/tests/unit/client/test_client_async.py @@ -112,4 +112,4 @@ async def test_client_account_id( base_url=fix_url_schema(settings.server), api_endpoint=settings.server, ) as c: - assert await c.account_id() == account_id, "Invalid account id returned" + assert await c.account_id == account_id, "Invalid account id returned." From 852ca2a934ccdbe3ecb07fd3c071d4d3b0057784 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Tue, 11 Jan 2022 09:33:10 -0500 Subject: [PATCH 32/33] Updated logic on getting account_id from account_name in both unit/conftest.py and client/client.py. Added async-property==0.2.1 back into setup.cfg. --- setup.cfg | 1 + src/firebolt/client/client.py | 4 ++-- tests/unit/conftest.py | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/setup.cfg b/setup.cfg index 64819164754..9583d04c4e4 100755 --- a/setup.cfg +++ b/setup.cfg @@ -24,6 +24,7 @@ project_urls = packages = find: install_requires = aiorwlock==1.1.0 + async-property==0.2.1 httpx[http2]==0.19.0 pydantic[dotenv]==1.8.2 readerwriterlock==1.0.9 diff --git a/src/firebolt/client/client.py b/src/firebolt/client/client.py index 53d63a461d3..c751a7e59f9 100644 --- a/src/firebolt/client/client.py +++ b/src/firebolt/client/client.py @@ -58,7 +58,7 @@ class Client(FireboltClientMixin, HttpxClient): @cached_property def account_id(self) -> str: - if self.account_name is not None: + if self.account_name: response = self.get( url=ACCOUNT_BY_NAME_URL, params={"account_name": self.account_name} ) @@ -86,7 +86,7 @@ class AsyncClient(FireboltClientMixin, HttpxAsyncClient): @async_cached_property async def account_id(self) -> str: - if self.account_name is not None: + if self.account_name: response = await self.get( url=ACCOUNT_BY_NAME_URL, params={"account_name": self.account_name} ) diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index be461edb9a1..ebf99c3a4b2 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -126,7 +126,7 @@ def db_name() -> str: @pytest.fixture def account_id_url(settings: Settings) -> str: - if settings.account_name is None: + if not settings.account_name: # if None or '' return f"https://{settings.server}{ACCOUNT_URL}" else: return ( From 3c1c6a8a88ac44cc23f23e769db56c1e6d7cdd02 Mon Sep 17 00:00:00 2001 From: ericf-firebolt Date: Wed, 12 Jan 2022 08:48:57 -0500 Subject: [PATCH 33/33] More minor edits. --- tests/integration/dbapi/sync/test_queries.py | 4 ++-- tests/unit/async_db/test_connection.py | 2 +- tests/unit/client/test_client.py | 4 ++-- tests/unit/client/test_client_async.py | 4 ++-- tests/unit/db/test_connection.py | 4 ++-- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/integration/dbapi/sync/test_queries.py b/tests/integration/dbapi/sync/test_queries.py index 9bf0fbed802..0f3d151784f 100644 --- a/tests/integration/dbapi/sync/test_queries.py +++ b/tests/integration/dbapi/sync/test_queries.py @@ -65,7 +65,7 @@ def test_select( def test_drop_create( connection: Connection, create_drop_description: List[Column] ) -> None: - """Create and drop table/index queries are handled propperly""" + """Create and drop table/index queries are handled properly.""" def test_query(c: Cursor, query: str) -> None: assert c.execute(query) == 1, "Invalid row count returned" @@ -124,7 +124,7 @@ def test_query(c: Cursor, query: str) -> None: def test_insert(connection: Connection) -> None: - """Insert and delete queries are handled propperly""" + """Insert and delete queries are handled properly.""" def test_empty_query(c: Cursor, query: str) -> None: assert c.execute(query) == -1, "Invalid row count returned" diff --git a/tests/unit/async_db/test_connection.py b/tests/unit/async_db/test_connection.py index 1522f7f6586..e45ca0abe47 100644 --- a/tests/unit/async_db/test_connection.py +++ b/tests/unit/async_db/test_connection.py @@ -32,7 +32,7 @@ async def test_closed_connection(connection: Connection) -> None: @mark.asyncio async def test_cursors_closed_on_close(connection: Connection) -> None: - """Connection closes all it's cursors on close.""" + """Connection closes all its cursors on close.""" c1, c2 = connection.cursor(), connection.cursor() assert ( len(connection._cursors) == 2 diff --git a/tests/unit/client/test_client.py b/tests/unit/client/test_client.py index cb72c32bca2..2d914e10a99 100644 --- a/tests/unit/client/test_client.py +++ b/tests/unit/client/test_client.py @@ -57,11 +57,11 @@ def test_client_different_auths( test_password: str, ): """ - Client propperly handles such auth types: + Client properly handles such auth types: - tuple(username, password) - Auth - None - All other types should raise TypeError + All other types should raise TypeError. """ httpx_mock.add_callback( diff --git a/tests/unit/client/test_client_async.py b/tests/unit/client/test_client_async.py index 9c563d3c511..913a598126c 100644 --- a/tests/unit/client/test_client_async.py +++ b/tests/unit/client/test_client_async.py @@ -59,11 +59,11 @@ async def test_client_different_auths( test_password: str, ): """ - Client propperly handles such auth types: + Client properly handles such auth types: - tuple(username, password) - Auth - None - All other types should raise TypeError + All other types should raise TypeError. """ httpx_mock.add_callback( diff --git a/tests/unit/db/test_connection.py b/tests/unit/db/test_connection.py index 168d214fc73..3a042fe52f6 100644 --- a/tests/unit/db/test_connection.py +++ b/tests/unit/db/test_connection.py @@ -26,7 +26,7 @@ def test_closed_connection(connection: Connection) -> None: def test_cursors_closed_on_close(connection: Connection) -> None: - """Connection closes all it's cursors on close.""" + """Connection closes all its cursors on close.""" c1, c2 = connection.cursor(), connection.cursor() assert ( len(connection._cursors) == 2 @@ -49,7 +49,7 @@ def test_cursor_initialized( query_url: str, python_query_data: List[List[ColType]], ) -> None: - """Connection initialised it's cursors propperly""" + """Connection initialised its cursors properly.""" httpx_mock.add_callback(auth_callback, url=auth_url) httpx_mock.add_callback(query_callback, url=query_url)