Skip to content

Commit

Permalink
refactor: FIR-35468 remove account resolve endpoint from python sdk (#…
Browse files Browse the repository at this point in the history
  • Loading branch information
stepansergeevitch authored Aug 23, 2024
1 parent 041e333 commit 559f526
Show file tree
Hide file tree
Showing 21 changed files with 23 additions and 465 deletions.
6 changes: 1 addition & 5 deletions src/firebolt/async_db/connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,7 @@
from firebolt.client.auth import Auth
from firebolt.client.client import AsyncClient, AsyncClientV1, AsyncClientV2
from firebolt.common.base_connection import BaseConnection
from firebolt.common.cache import (
_firebolt_account_info_cache,
_firebolt_system_engine_cache,
)
from firebolt.common.cache import _firebolt_system_engine_cache
from firebolt.common.constants import DEFAULT_TIMEOUT_SECONDS
from firebolt.utils.exception import ConfigurationError, ConnectionClosedError
from firebolt.utils.usage_tracker import get_user_agent_header
Expand Down Expand Up @@ -135,7 +132,6 @@ async def connect(
user_agent_header = get_user_agent_header(user_drivers, user_clients)
if disable_cache:
_firebolt_system_engine_cache.disable()
_firebolt_account_info_cache.disable()
# Use v2 if auth is ClientCredentials
# Use v1 if auth is ServiceAccount or UsernamePassword
auth_version = auth.get_firebolt_version()
Expand Down
8 changes: 0 additions & 8 deletions src/firebolt/async_db/cursor.py
Original file line number Diff line number Diff line change
Expand Up @@ -136,14 +136,6 @@ async def _parse_response_headers(self, headers: Headers) -> None:
endpoint, params = _parse_update_endpoint(
headers.get(UPDATE_ENDPOINT_HEADER)
)
if (
params.get("account_id", await self._client.account_id)
!= await self._client.account_id
):
raise OperationalError(
"USE ENGINE command failed. Account parameter mismatch. "
"Contact support"
)
self._update_set_parameters(params)
self.engine_url = endpoint

Expand Down
110 changes: 3 additions & 107 deletions src/firebolt/client/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,12 @@
AsyncKeepaliveTransport,
KeepaliveTransport,
)
from firebolt.common._types import _AccountInfo
from firebolt.common.cache import _firebolt_account_info_cache
from firebolt.utils.exception import (
AccountNotFoundError,
AccountNotFoundOrNoAccessError,
FireboltEngineError,
InterfaceError,
)
from firebolt.utils.urls import (
ACCOUNT_BY_NAME_URL,
ACCOUNT_BY_NAME_URL_V1,
ACCOUNT_ENGINE_ID_BY_NAME_URL,
ACCOUNT_ENGINE_URL,
Expand All @@ -47,13 +43,6 @@
FireboltClientMixinBase = mixin_for(HttpxClient) # type: Any


def parse_response_for_account_info(response: Response) -> _AccountInfo:
"""Construct account info object from the API response."""
account_id = response.json()["id"]
account_version = int(response.json().get("infraVersion", 2))
return _AccountInfo(id=account_id, version=account_version)


class FireboltClientMixin(FireboltClientMixinBase):
"""HttpxAsyncClient mixin with Firebolt authentication functionality."""

Expand Down Expand Up @@ -101,7 +90,7 @@ def _merge_auth_request(self, request: Request) -> Request:
return request

def _enforce_trailing_slash(self, url: URL) -> URL:
"""Don't automatically append trailing slach to a base url"""
"""Don't automatically append trailing slash to a base url"""
return url

def clone(self) -> "Client":
Expand All @@ -124,11 +113,6 @@ def __init__(self, *args: Any, **kwargs: Any):
def account_id(self) -> str:
...

@property
@abstractmethod
def _account_version(self) -> int:
...

def _send_handling_redirects(
self, request: Request, *args: Any, **kwargs: Any
) -> Response:
Expand Down Expand Up @@ -161,40 +145,6 @@ def __init__(
**kwargs,
)

@property
def _account_info(self) -> _AccountInfo:
if account_info := _firebolt_account_info_cache.get(
[self.account_name, self._api_endpoint.host]
):
return account_info
response = self.get(
url=self._api_endpoint.copy_with(
path=ACCOUNT_BY_NAME_URL.format(account_name=self.account_name)
)
)
if response.status_code == HttpxCodes.NOT_FOUND:
assert self.account_name is not None
raise AccountNotFoundOrNoAccessError(self.account_name)
# process all other status codes
response.raise_for_status()
account_info = parse_response_for_account_info(response)
_firebolt_account_info_cache.set(
key=[self.account_name, self._api_endpoint.host], value=account_info
)
return account_info

@property
def _account_version(self) -> int:
"""User account version. 2 means both database and engine v2 are supported.
Returns:
int: Account version
Raises:
AccountNotFoundError: No account found with provided name
"""
return self._account_info.version

@property
def account_id(self) -> str:
"""User account ID.
Expand All @@ -208,7 +158,7 @@ def account_id(self) -> str:
Raises:
AccountNotFoundError: No account found with provided name
"""
return self._account_info.id
return ""


class ClientV1(Client):
Expand Down Expand Up @@ -236,13 +186,6 @@ def __init__(
)
self._auth_endpoint = URL(fix_url_schema(api_endpoint))

@property
def _account_version(self) -> int:
"""User account version. Hardcoded since it's not returned
by the backend for V1.
"""
return 1

@cached_property
def account_id(self) -> str:
"""User account ID.
Expand Down Expand Up @@ -333,11 +276,6 @@ def __init__(self, *args: Any, **kwargs: Any):
async def account_id(self) -> str:
...

@property
@abstractmethod
async def _account_version(self) -> int:
...

async def _send_handling_redirects(
self, request: Request, *args: Any, **kwargs: Any
) -> Response:
Expand Down Expand Up @@ -370,30 +308,6 @@ def __init__(
**kwargs,
)

async def _account_info(self) -> _AccountInfo:
# manual caching to avoid async_cached_property issues
if account_info := _firebolt_account_info_cache.get(
[self.account_name, self._api_endpoint.host]
):
return account_info

response = await self.get(
url=self._api_endpoint.copy_with(
path=ACCOUNT_BY_NAME_URL.format(account_name=self.account_name)
)
)
if response.status_code == HttpxCodes.NOT_FOUND:
assert self.account_name is not None
raise AccountNotFoundOrNoAccessError(self.account_name)
# process all other status codes
response.raise_for_status()
account_info = parse_response_for_account_info(response)
# cache for future use
_firebolt_account_info_cache.set(
key=[self.account_name, self._api_endpoint.host], value=account_info
)
return account_info

@property
async def account_id(self) -> str:
"""User account ID.
Expand All @@ -407,19 +321,7 @@ async def account_id(self) -> str:
Raises:
AccountNotFoundError: No account found with provided name
"""
return (await self._account_info()).id

@property
async def _account_version(self) -> int:
"""User account version. 2 means both database and engine v2 are supported.
Returns:
int: Account version
Raises:
AccountNotFoundError: No account found with provided name
"""
return (await self._account_info()).version
return ""


class AsyncClientV1(AsyncClient):
Expand Down Expand Up @@ -448,12 +350,6 @@ def __init__(
self.account_id_cache: Dict[str, str] = {}
self._auth_endpoint = URL(fix_url_schema(api_endpoint))

@property
async def _account_version(self) -> int:
"""User account version. Hardcoded since it's not returned
by the backend for V1."""
return 1

@property
async def account_id(self) -> str:
"""User account ID.
Expand Down
3 changes: 0 additions & 3 deletions src/firebolt/common/cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@
TypeVar,
)

from firebolt.common._types import _AccountInfo

T = TypeVar("T")


Expand Down Expand Up @@ -85,4 +83,3 @@ def __contains__(self, key: str) -> bool:
_firebolt_system_engine_cache = UtilCache[Tuple[str, Dict[str, str]]](
cache_name="system_engine"
)
_firebolt_account_info_cache = UtilCache[_AccountInfo](cache_name="account_info")
6 changes: 1 addition & 5 deletions src/firebolt/db/connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,7 @@
from firebolt.client import DEFAULT_API_URL, Client, ClientV1, ClientV2
from firebolt.client.auth import Auth
from firebolt.common.base_connection import BaseConnection
from firebolt.common.cache import (
_firebolt_account_info_cache,
_firebolt_system_engine_cache,
)
from firebolt.common.cache import _firebolt_system_engine_cache
from firebolt.common.constants import DEFAULT_TIMEOUT_SECONDS
from firebolt.db.cursor import Cursor, CursorV1, CursorV2
from firebolt.db.util import _get_system_engine_url_and_params
Expand Down Expand Up @@ -48,7 +45,6 @@ def connect(
auth_version = auth.get_firebolt_version()
if disable_cache:
_firebolt_system_engine_cache.disable()
_firebolt_account_info_cache.disable()
# Use v2 if auth is ClientCredentials
# Use v1 if auth is ServiceAccount or UsernamePassword
if auth_version == 2:
Expand Down
8 changes: 0 additions & 8 deletions src/firebolt/db/cursor.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,14 +135,6 @@ def _parse_response_headers(self, headers: Headers) -> None:
endpoint, params = _parse_update_endpoint(
headers.get(UPDATE_ENDPOINT_HEADER)
)
if (
params.get("account_id", self._client.account_id)
!= self._client.account_id
):
raise OperationalError(
"USE ENGINE command failed. Account parameter mismatch. "
"Contact support"
)
self._update_set_parameters(params)
self.engine_url = endpoint

Expand Down
2 changes: 0 additions & 2 deletions src/firebolt/utils/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
ENGINES_URL = "/core/v1/account/engines"
ENGINES_BY_IDS_URL = "/core/v1/engines:getByIds"

ACCOUNT_BY_NAME_URL = "/web/v3/account/{account_name}/resolve"

ACCOUNT_DATABASES_URL = "/core/v1/accounts/{account_id}/databases"
ACCOUNT_DATABASE_URL = "/core/v1/accounts/{account_id}/databases/{database_id}"
ACCOUNT_DATABASE_BINDING_URL = ACCOUNT_DATABASE_URL + "/bindings/{engine_id}"
Expand Down
9 changes: 0 additions & 9 deletions tests/integration/dbapi/async/V2/test_system_engine_async.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,15 +77,6 @@ async def test_system_engine_no_db(
)


async def test_system_engine_account(connection_system_engine: Connection):
assert (
await connection_system_engine._client.account_id
), "Can't get account id explicitly"
assert (
await connection_system_engine._client._account_version
) == 2, "Invalid account version"


async def test_system_engine_use_engine(
connection_system_engine: Connection, database_name: str, engine_name: str
):
Expand Down
9 changes: 0 additions & 9 deletions tests/integration/dbapi/sync/V2/test_system_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,6 @@ def test_system_engine_no_db(
)


def test_system_engine_account(connection_system_engine: Connection):
assert (
connection_system_engine._client.account_id
), "Can't get account id explicitly"
assert (
connection_system_engine._client._account_version == 2
), "Invalid account version"


def test_system_engine_use_engine(
connection_system_engine: Connection, database_name: str, engine_name: str
):
Expand Down
4 changes: 2 additions & 2 deletions tests/unit/V1/client/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from pytest import fixture

from firebolt.utils.exception import AccountNotFoundError
from firebolt.utils.urls import ACCOUNT_BY_NAME_URL, ACCOUNT_URL, AUTH_URL
from firebolt.utils.urls import ACCOUNT_BY_NAME_URL_V1, ACCOUNT_URL, AUTH_URL
from tests.unit.response import Response


Expand Down Expand Up @@ -91,7 +91,7 @@ def check_credentials(

@fixture
def account_id_url(api_endpoint: str) -> Pattern:
base = f"https://{api_endpoint}{ACCOUNT_BY_NAME_URL}?account_name="
base = f"https://{api_endpoint}{ACCOUNT_BY_NAME_URL_V1}?account_name="
default_base = f"https://{api_endpoint}{ACCOUNT_URL}"
base = base.replace("/", "\\/").replace("?", "\\?")
default_base = default_base.replace("/", "\\/").replace("?", "\\?")
Expand Down
1 change: 0 additions & 1 deletion tests/unit/V1/client/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ def test_client_account_id(
api_endpoint=api_endpoint,
) as c:
assert c.account_id == account_id, "Invalid account id returned"
assert c._account_version == 1, "Invalid account version returned"


# FIR-14945
Expand Down
1 change: 0 additions & 1 deletion tests/unit/V1/client/test_client_async.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,6 @@ async def test_client_account_id(
api_endpoint=api_endpoint,
) as c:
assert await c.account_id == account_id, "Invalid account id returned."
assert await c._account_version == 1, "Invalid account version returned."


async def test_concurent_auth_lock(
Expand Down
4 changes: 2 additions & 2 deletions tests/unit/V1/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from firebolt.client.auth import Auth, UsernamePassword
from firebolt.utils.exception import AccountNotFoundError
from firebolt.utils.urls import (
ACCOUNT_BY_NAME_URL,
ACCOUNT_BY_NAME_URL_V1,
ACCOUNT_DATABASE_BY_NAME_URL,
ACCOUNT_ENGINE_URL,
ACCOUNT_ENGINE_URL_BY_DATABASE_NAME_V1,
Expand Down Expand Up @@ -148,7 +148,7 @@ def do_mock(

@fixture
def account_id_url(api_endpoint: str) -> Pattern:
base = f"https://{api_endpoint}{ACCOUNT_BY_NAME_URL}?account_name="
base = f"https://{api_endpoint}{ACCOUNT_BY_NAME_URL_V1}?account_name="
default_base = f"https://{api_endpoint}{ACCOUNT_URL}"
base = base.replace("/", "\\/").replace("?", "\\?")
default_base = default_base.replace("/", "\\/").replace("?", "\\?")
Expand Down
4 changes: 2 additions & 2 deletions tests/unit/V1/service/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
from firebolt.utils.exception import AccountNotFoundError
from firebolt.utils.urls import (
ACCOUNT_BINDINGS_URL,
ACCOUNT_BY_NAME_URL,
ACCOUNT_BY_NAME_URL_V1,
ACCOUNT_DATABASE_BINDING_URL,
ACCOUNT_DATABASE_BY_NAME_URL,
ACCOUNT_DATABASE_URL,
Expand Down Expand Up @@ -599,7 +599,7 @@ def check_credentials(

@fixture
def account_id_url(api_endpoint: str) -> Pattern:
base = f"https://{api_endpoint}{ACCOUNT_BY_NAME_URL}?account_name="
base = f"https://{api_endpoint}{ACCOUNT_BY_NAME_URL_V1}?account_name="
default_base = f"https://{api_endpoint}{ACCOUNT_URL}"
base = base.replace("/", "\\/").replace("?", "\\?")
default_base = default_base.replace("/", "\\/").replace("?", "\\?")
Expand Down
Loading

0 comments on commit 559f526

Please sign in to comment.