feat: Update interface error messages (#195)

* Broke out various errors when resolving engine urls and added status codes to InterfaceErrors to try to determine why SDK is failing to get engine endpoints.

* Updated version number to 0.9.3.

* Trying to send engine endpoint, whether get url or get-url-by-name.

* Trying to break out request errors.

* Added return.

* Added a comment.

* Updated exception handling logic.

* Slight refactoring of error catching code in _resolve_engine_url().

* Slight refactoring of error catching code in _resolve_engine_url() to display url in all cases of failure.

* Renamed GET_ENGINE_URL to GET_ENGINE_BY_NAME_URL to clarify it for me as I debugged.

* Upped default timeout to 10 seconds.

* Removed references to  from error messages in timeouts.

* Removed some print()s and trying to get tests/unit/test_connection::test_connect_engine_name to work.

* Still trying to get test_connect_engine_name() to pass.

* Passed final unit tests. Whew.

* Switched the order of arguments in an and clause for better short-circuiting.

Author: ericf-firebolt

src/firebolt/async_db/connection.py

@@ -20,14 +20,14 @@
     InterfaceError,
 )
 from firebolt.utils.urls import (
-    ACCOUNT_ENGINE_BY_NAME_URL,
+    ACCOUNT_ENGINE_ID_BY_NAME_URL,
     ACCOUNT_ENGINE_URL,
     ACCOUNT_ENGINE_URL_BY_DATABASE_NAME,
 )
 from firebolt.utils.usage_tracker import get_user_agent_header
 from firebolt.utils.util import fix_url_schema
 
-DEFAULT_TIMEOUT_SECONDS: int = 5
+DEFAULT_TIMEOUT_SECONDS: int = 60
 KEEPALIVE_FLAG: int = 1
 KEEPIDLE_RATE: int = 60  # seconds
 AUTH_CREDENTIALS_DEPRECATION_MESSAGE = """ Passing connection credentials
@@ -56,32 +56,37 @@ async def _resolve_engine_url(
         base_url=api_endpoint,
         account_name=account_name,
         api_endpoint=api_endpoint,
+        timeout=Timeout(DEFAULT_TIMEOUT_SECONDS),
     ) as client:
+        account_id = await client.account_id
+        url = ACCOUNT_ENGINE_ID_BY_NAME_URL.format(account_id=account_id)
         try:
-            account_id = await client.account_id
             response = await client.get(
-                url=ACCOUNT_ENGINE_BY_NAME_URL.format(account_id=account_id),
+                url=url,
                 params={"engine_name": engine_name},
             )
             response.raise_for_status()
             engine_id = response.json()["engine_id"]["engine_id"]
-            response = await client.get(
-                url=ACCOUNT_ENGINE_URL.format(
-                    account_id=account_id, engine_id=engine_id
-                ),
-            )
+            url = ACCOUNT_ENGINE_URL.format(account_id=account_id, engine_id=engine_id)
+            response = await client.get(url=url)
             response.raise_for_status()
             return response.json()["engine"]["endpoint"]
         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"Error {e.__class__.__name__}: Unable to retrieve engine "
+                    f"endpoint {url}."
+                )
             # 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.")
         except (JSONDecodeError, RequestError, RuntimeError, HTTPStatusError) as e:
-            raise InterfaceError(f"Unable to retrieve engine endpoint: {e}.")
+            raise InterfaceError(
+                f"Error {e.__class__.__name__}: "
+                f"Unable to retrieve engine endpoint {url}."
+            )
 
 
 async def _get_database_default_engine_url(
@@ -95,6 +100,7 @@ async def _get_database_default_engine_url(
         base_url=api_endpoint,
         account_name=account_name,
         api_endpoint=api_endpoint,
+        timeout=Timeout(DEFAULT_TIMEOUT_SECONDS),
     ) as client:
         try:
             account_id = await client.account_id

src/firebolt/db/connection.py

@@ -13,8 +13,6 @@
 from firebolt.utils.exception import ConnectionClosedError
 from firebolt.utils.util import AsyncJobThread, async_to_sync
 
-DEFAULT_TIMEOUT_SECONDS: int = 5
-
 
 class Connection(AsyncBaseConnection):
     """

src/firebolt/service/engine.py

@@ -11,9 +11,9 @@
 from firebolt.service.types import EngineOrder, EngineType, WarmupMethod
 from firebolt.utils.exception import FireboltError
 from firebolt.utils.urls import (
-    ACCOUNT_ENGINE_BY_NAME_URL,
+    ACCOUNT_ENGINE_ID_BY_NAME_URL,
     ACCOUNT_ENGINE_URL,
-    ACCOUNT_ENGINES_URL,
+    ACCOUNT_LIST_ENGINES_URL,
     ENGINES_BY_IDS_URL,
 )
 from firebolt.utils.util import prune_dict
@@ -51,7 +51,7 @@ def get_by_name(self, name: str) -> Engine:
         """Get an engine from Firebolt by its name."""
 
         response = self.client.get(
-            url=ACCOUNT_ENGINE_BY_NAME_URL.format(account_id=self.account_id),
+            url=ACCOUNT_ENGINE_ID_BY_NAME_URL.format(account_id=self.account_id),
             params={"engine_name": name},
         )
         engine_id = response.json()["engine_id"]["engine_id"]
@@ -88,7 +88,7 @@ def get_many(
             ).key.region_id
 
         response = self.client.get(
-            url=ACCOUNT_ENGINES_URL.format(account_id=self.account_id),
+            url=ACCOUNT_LIST_ENGINES_URL.format(account_id=self.account_id),
             params=prune_dict(
                 {
                     "page.first": 5000,  # FUTURE: pagination support w/ generator
@@ -210,7 +210,7 @@ def _send_create_engine(
         """
 
         response = self.client.post(
-            url=ACCOUNT_ENGINES_URL.format(account_id=self.account_id),
+            url=ACCOUNT_LIST_ENGINES_URL.format(account_id=self.account_id),
             headers={"Content-type": "application/json"},
             json=_EngineCreateRequest(
                 account_id=self.account_id,

src/firebolt/utils/urls.py

@@ -12,10 +12,10 @@
 ACCOUNT_ENGINE_START_URL = ACCOUNT_ENGINE_URL + ":start"
 ACCOUNT_ENGINE_RESTART_URL = ACCOUNT_ENGINE_URL + ":restart"
 ACCOUNT_ENGINE_STOP_URL = ACCOUNT_ENGINE_URL + ":stop"
-ACCOUNT_ENGINES_URL = "/core/v1/accounts/{account_id}/engines"
-ACCOUNT_ENGINE_BY_NAME_URL = ACCOUNT_ENGINES_URL + ":getIdByName"
+ACCOUNT_LIST_ENGINES_URL = "/core/v1/accounts/{account_id}/engines"
+ACCOUNT_ENGINE_ID_BY_NAME_URL = ACCOUNT_LIST_ENGINES_URL + ":getIdByName"
 ACCOUNT_ENGINE_REVISION_URL = ACCOUNT_ENGINE_URL + "/engineRevisions/{revision_id}"
-ACCOUNT_ENGINE_URL_BY_DATABASE_NAME = ACCOUNT_ENGINES_URL + ":getURLByDatabaseName"
+ACCOUNT_ENGINE_URL_BY_DATABASE_NAME = ACCOUNT_LIST_ENGINES_URL + ":getURLByDatabaseName"
 
 ACCOUNT_DATABASES_URL = "/core/v1/accounts/{account_id}/databases"
 ACCOUNT_DATABASE_URL = "/core/v1/accounts/{account_id}/databases/{database_id}"

tests/integration/dbapi/async/test_queries_async.py

@@ -427,7 +427,7 @@ async def test_server_side_async_execution_query(connection: Connection) -> None
     with connection.cursor() as c:
         query_id = await c.execute("SELECT 1", [], async_execution=True)
     assert (
-        type(query_id) is str and query_id
+        query_id and type(query_id) is str
     ), "Invalid query id was returned from server-side async query."
 
 

tests/integration/dbapi/sync/test_queries.py

@@ -423,7 +423,7 @@ def test_server_side_async_execution_query(connection: Connection) -> None:
     with connection.cursor() as c:
         query_id = c.execute("SELECT 1", [], async_execution=True)
     assert (
-        type(query_id) is str and query_id
+        query_id and type(query_id) is str
     ), "Invalid query id was returned from server-side async query."
 
 

tests/unit/async_db/test_connection.py

@@ -18,7 +18,7 @@
     FireboltEngineError,
 )
 from firebolt.utils.token_storage import TokenSecureStorage
-from firebolt.utils.urls import ACCOUNT_ENGINE_BY_NAME_URL
+from firebolt.utils.urls import ACCOUNT_ENGINE_ID_BY_NAME_URL
 
 
 async def test_closed_connection(connection: Connection) -> None:
@@ -145,8 +145,8 @@ async def test_connect_engine_name(
     account_id_url: Pattern,
     account_id_callback: Callable,
     engine_id: str,
-    get_engine_url: str,
-    get_engine_callback: Callable,
+    get_engine_url_by_id_url: str,
+    get_engine_url_by_id_callback: Callable,
     python_query_data: List[List[ColType]],
     account_id: str,
 ):
@@ -166,14 +166,14 @@ async def test_connect_engine_name(
     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(get_engine_callback, url=get_engine_url)
+    httpx_mock.add_callback(get_engine_url_by_id_callback, url=get_engine_url_by_id_url)
 
     engine_name = settings.server.split(".")[0]
 
     # Mock engine id lookup error
     httpx_mock.add_response(
         url=f"https://{settings.server}"
-        + ACCOUNT_ENGINE_BY_NAME_URL.format(account_id=account_id)
+        + ACCOUNT_ENGINE_ID_BY_NAME_URL.format(account_id=account_id)
         + f"?engine_name={engine_name}",
         status_code=codes.NOT_FOUND,
     )
@@ -192,7 +192,7 @@ async def test_connect_engine_name(
     # Mock engine id lookup by name
     httpx_mock.add_response(
         url=f"https://{settings.server}"
-        + ACCOUNT_ENGINE_BY_NAME_URL.format(account_id=account_id)
+        + ACCOUNT_ENGINE_ID_BY_NAME_URL.format(account_id=account_id)
         + f"?engine_name={engine_name}",
         status_code=codes.OK,
         json={"engine_id": {"engine_id": engine_id}},
@@ -219,15 +219,8 @@ async def test_connect_default_engine(
     query_url: str,
     account_id_url: Pattern,
     account_id_callback: Callable,
-    engine_id: str,
-    get_engine_url: str,
-    get_engine_callback: Callable,
-    database_by_name_url: str,
-    database_by_name_callback: Callable,
-    database_id: str,
     engine_by_db_url: str,
     python_query_data: List[List[ColType]],
-    account_id: str,
 ):
     httpx_mock.add_callback(auth_callback, url=auth_url)
     httpx_mock.add_callback(query_callback, url=query_url)

tests/unit/conftest.py

@@ -34,10 +34,8 @@
     AUTH_URL,
     DATABASES_URL,
     ENGINES_URL,
-    PROVIDERS_URL,
 )
 from tests.unit.db_conftest import *  # noqa
-from tests.unit.util import list_to_paginated_response
 
 
 # Register nofakefs mark
@@ -197,25 +195,46 @@ def do_mock(
 
 @fixture
 def engine_id() -> str:
-    return "engine_id"
+    return "mock_engine_id"
 
 
 @fixture
-def get_engine_url(settings: Settings, account_id: str, engine_id: str) -> str:
+def engine_endpoint() -> str:
+    return "mock_engine_endpoint"
+
+
+@fixture
+def engine_name() -> str:
+    return "mock_engine_name"
+
+
+@fixture
+def get_engine_name_by_id_url(
+    settings: Settings, account_id: str, engine_id: str
+) -> str:
+    return f"https://{settings.server}" + ACCOUNT_ENGINE_URL.format(
+        account_id=account_id, engine_id=engine_id
+    )
+
+
+@fixture
+def get_engine_url_by_id_url(
+    settings: Settings, account_id: str, engine_id: str
+) -> str:
     return f"https://{settings.server}" + ACCOUNT_ENGINE_URL.format(
         account_id=account_id, engine_id=engine_id
     )
 
 
 @fixture
-def get_engine_callback(
-    get_engine_url: str, engine_id: str, settings: Settings
+def get_engine_url_by_id_callback(
+    get_engine_url_by_id_url: str, engine_id: str, settings: Settings
 ) -> Callable:
     def do_mock(
         request: Request = None,
         **kwargs,
     ) -> Response:
-        assert request.url == get_engine_url
+        assert request.url == get_engine_url_by_id_url
         return Response(
             status_code=httpx.codes.OK,
             json={
@@ -240,26 +259,6 @@ def do_mock(
     return do_mock
 
 
-@fixture
-def get_providers_url(settings: Settings, account_id: str, engine_id: str) -> str:
-    return f"https://{settings.server}{PROVIDERS_URL}"
-
-
-@fixture
-def get_providers_callback(get_providers_url: str, provider: Provider) -> Callable:
-    def do_mock(
-        request: Request = None,
-        **kwargs,
-    ) -> Response:
-        assert request.url == get_providers_url
-        return Response(
-            status_code=httpx.codes.OK,
-            json=list_to_paginated_response([provider]),
-        )
-
-    return do_mock
-
-
 @fixture
 def get_engines_url(settings: Settings) -> str:
     return f"https://{settings.server}{ENGINES_URL}"

tests/unit/db/test_connection.py

@@ -16,9 +16,10 @@
     AccountNotFoundError,
     ConfigurationError,
     ConnectionClosedError,
+    FireboltEngineError,
 )
 from firebolt.utils.token_storage import TokenSecureStorage
-from firebolt.utils.urls import ACCOUNT_ENGINE_BY_NAME_URL
+from firebolt.utils.urls import ACCOUNT_ENGINE_ID_BY_NAME_URL
 
 
 def test_closed_connection(connection: Connection) -> None:
@@ -143,10 +144,8 @@ def test_connect_engine_name(
     account_id_url: Pattern,
     account_id_callback: Callable,
     engine_id: str,
-    get_engine_url: str,
-    get_engine_callback: Callable,
-    get_providers_url: str,
-    get_providers_callback: Callable,
+    get_engine_url_by_id_url: str,
+    get_engine_url_by_id_callback: Callable,
     python_query_data: List[List[ColType]],
     account_id: str,
 ):
@@ -164,14 +163,32 @@ def test_connect_engine_name(
     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(get_engine_callback, url=get_engine_url)
+    httpx_mock.add_callback(get_engine_url_by_id_callback, url=get_engine_url_by_id_url)
 
     engine_name = settings.server.split(".")[0]
 
+    # Mock engine id lookup error
+    httpx_mock.add_response(
+        url=f"https://{settings.server}"
+        + ACCOUNT_ENGINE_ID_BY_NAME_URL.format(account_id=account_id)
+        + f"?engine_name={engine_name}",
+        status_code=codes.NOT_FOUND,
+    )
+
+    with raises(FireboltEngineError):
+        connect(
+            database="db",
+            username="username",
+            password="password",
+            engine_name=engine_name,
+            account_name=settings.account_name,
+            api_endpoint=settings.server,
+        )
+
     # Mock engine id lookup by name
     httpx_mock.add_response(
         url=f"https://{settings.server}"
-        + ACCOUNT_ENGINE_BY_NAME_URL.format(account_id=account_id)
+        + ACCOUNT_ENGINE_ID_BY_NAME_URL.format(account_id=account_id)
         + f"?engine_name={engine_name}",
         status_code=codes.OK,
         json={"engine_id": {"engine_id": engine_id}},
@@ -198,11 +215,6 @@ def test_connect_default_engine(
     query_url: str,
     account_id_url: Pattern,
     account_id_callback: Callable,
-    engine_id: str,
-    get_engine_url: str,
-    get_engine_callback: Callable,
-    database_by_name_url: str,
-    database_by_name_callback: Callable,
     database_id: str,
     engine_by_db_url: str,
     python_query_data: List[List[ColType]],

tests/unit/service/conftest.py

@@ -23,8 +23,8 @@
     ACCOUNT_DATABASE_URL,
     ACCOUNT_DATABASES_URL,
     ACCOUNT_ENGINE_URL,
-    ACCOUNT_ENGINES_URL,
     ACCOUNT_INSTANCE_TYPES_URL,
+    ACCOUNT_LIST_ENGINES_URL,
     PROVIDERS_URL,
     REGIONS_URL,
 )
@@ -261,7 +261,7 @@ def do_mock(
 
 @fixture
 def engine_url(settings: Settings, account_id) -> str:
-    return f"https://{settings.server}" + ACCOUNT_ENGINES_URL.format(
+    return f"https://{settings.server}" + ACCOUNT_LIST_ENGINES_URL.format(
         account_id=account_id
     )