From 037f2620d1d16da8b40f759713d62c692403b219 Mon Sep 17 00:00:00 2001 From: ptiurin Date: Mon, 21 Aug 2023 16:41:57 +0100 Subject: [PATCH 01/10] feat: Identity compatibility --- setup.cfg | 2 +- src/firebolt_db/firebolt_dialect.py | 12 ++-- tests/integration/conftest.py | 63 ++++++++++--------- .../test_sqlalchemy_integration.py | 13 ++-- tests/unit/test_firebolt_dialect.py | 20 +++--- 5 files changed, 59 insertions(+), 51 deletions(-) diff --git a/setup.cfg b/setup.cfg index c094b3d..db8948b 100644 --- a/setup.cfg +++ b/setup.cfg @@ -25,7 +25,7 @@ project_urls = [options] packages = find: install_requires = - firebolt-sdk>=0.9.2 + firebolt-sdk@git+https://github.com/firebolt-db/firebolt-python-sdk@main sqlalchemy>=1.0.0 python_requires = >=3.7 package_dir = diff --git a/src/firebolt_db/firebolt_dialect.py b/src/firebolt_db/firebolt_dialect.py index 049013c..3c00572 100644 --- a/src/firebolt_db/firebolt_dialect.py +++ b/src/firebolt_db/firebolt_dialect.py @@ -5,7 +5,7 @@ import firebolt.db as dbapi import sqlalchemy.types as sqltypes -from firebolt.client.auth import Auth, ServiceAccount, UsernamePassword +from firebolt.client.auth import Auth, ClientCredentials from firebolt.db import Cursor from sqlalchemy.engine import Connection as AlchemyConnection from sqlalchemy.engine import ExecutionContext, default @@ -136,17 +136,13 @@ def dbapi(cls) -> ModuleType: def create_connect_args(self, url: URL) -> Tuple[List, Dict]: """ Build firebolt-sdk compatible connection arguments. - URL format : firebolt://username:password@host:port/db_name + URL format : firebolt://id:secret@host:port/db_name """ parameters = dict(url.query) # parameters are all passed as a string, we need to convert # bool flag to boolean for SDK compatibility token_cache_flag = bool(strtobool(parameters.pop("use_token_cache", "True"))) - auth = ( - ServiceAccount(url.username, url.password, token_cache_flag) - if "@" not in url.username - else UsernamePassword(url.username, url.password, token_cache_flag) - ) + auth = ClientCredentials(url.username, url.password, token_cache_flag) kwargs: Dict[str, Union[str, Auth, Dict[str, Any], None]] = { "database": url.host or None, "auth": auth, @@ -156,6 +152,8 @@ def create_connect_args(self, url: URL) -> Tuple[List, Dict]: additional_parameters = {} if "account_name" in parameters: kwargs["account_name"] = parameters.pop("account_name") + else: + raise Exception("account_name parameter must be provided to authenticate") self._set_parameters = parameters # If URL override is not provided leave it to the sdk to determine the endpoint if "FIREBOLT_BASE_URL" in os.environ: diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 9e3c9fa..30a3379 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -1,4 +1,3 @@ -import asyncio import urllib.parse from logging import getLogger from os import environ @@ -13,10 +12,9 @@ ENGINE_NAME_ENV = "ENGINE_NAME" DATABASE_NAME_ENV = "DATABASE_NAME" -USERNAME_ENV = "USER_NAME" -PASSWORD_ENV = "PASSWORD" -SERVICE_ID = "SERVICE_ID" -SERVICE_SECRET = "SERVICE_SECRET" +ACCOUNT_NAME_ENV = "ACCOUNT_NAME" +CLIENT_ID_ENV = "CLIENT_ID" +CLIENT_KEY_ENV = "CLIENT_KEY" def must_env(var_name: str) -> str: @@ -36,40 +34,45 @@ def database_name() -> str: @fixture(scope="session") -def username() -> str: - return must_env(USERNAME_ENV) +def client_id() -> str: + return must_env(CLIENT_ID_ENV) @fixture(scope="session") -def password() -> str: - return urllib.parse.quote_plus(must_env(PASSWORD_ENV)) +def client_key() -> str: + return urllib.parse.quote_plus(must_env(CLIENT_KEY_ENV)) @fixture(scope="session") -def service_id() -> str: - return must_env(SERVICE_ID) - - -@fixture(scope="session") -def service_secret() -> str: - return must_env(SERVICE_SECRET) +def account_name() -> str: + return must_env(ACCOUNT_NAME_ENV) @fixture(scope="session") def engine( - username: str, password: str, database_name: str, engine_name: str + client_id: str, + client_key: str, + database_name: str, + engine_name: str, + account_name: str, ) -> Engine: return create_engine( - f"firebolt://{username}:{password}@{database_name}/{engine_name}" + f"firebolt://{client_id}:{client_key}@{database_name}/{engine_name}" + + f"?account_name={account_name}" ) @fixture(scope="session") def engine_service_account( - service_id: str, service_secret: str, database_name: str, engine_name: str + client_id: str, + client_key: str, + database_name: str, + engine_name: str, + account_name: str, ) -> Engine: return create_engine( - f"firebolt://{service_id}:{service_secret}@{database_name}/{engine_name}" + f"firebolt://{client_id}:{client_key}@{database_name}/{engine_name}" + + f"?account_name={account_name}" ) @@ -85,23 +88,21 @@ def connection_service_account(engine_service_account: Engine) -> Connection: yield c -@fixture(scope="session") -def event_loop(): - loop = asyncio.get_event_loop() - yield loop - loop.close() - - -@fixture(scope="session") +@fixture() def async_engine( - username: str, password: str, database_name: str, engine_name: str + client_id: str, + client_key: str, + database_name: str, + engine_name: str, + account_name: str, ) -> Engine: return create_async_engine( - f"asyncio+firebolt://{username}:{password}@{database_name}/{engine_name}" + f"asyncio+firebolt://{client_id}:{client_key}@{database_name}/{engine_name}" + + f"?account_name={account_name}" ) -@fixture(scope="session") +@fixture() async def async_connection( async_engine: Engine, ) -> Connection: diff --git a/tests/integration/test_sqlalchemy_integration.py b/tests/integration/test_sqlalchemy_integration.py index 202b8da..af747ff 100644 --- a/tests/integration/test_sqlalchemy_integration.py +++ b/tests/integration/test_sqlalchemy_integration.py @@ -21,15 +21,20 @@ def test_create_ex_table( assert not engine.dialect.has_table(connection, ex_table_name) def test_set_params( - self, username: str, password: str, database_name: str, engine_name: str + self, + client_id: str, + client_key: str, + database_name: str, + engine_name: str, + account_name: str, ): engine = create_engine( - f"firebolt://{username}:{password}@{database_name}/{engine_name}" + f"firebolt://{client_id}:{client_key}@{database_name}/{engine_name}" + + f"?account_name={account_name}" ) with engine.connect() as connection: connection.execute(text("SET advanced_mode=1")) - connection.execute(text("SET use_standard_sql=0")) - result = connection.execute(text("SELECT sleepEachRow(1) from numbers(1)")) + result = connection.execute(text("SELECT 1")) assert len(result.fetchall()) == 1 engine.dispose() diff --git a/tests/unit/test_firebolt_dialect.py b/tests/unit/test_firebolt_dialect.py index 8d67cf4..289f37a 100644 --- a/tests/unit/test_firebolt_dialect.py +++ b/tests/unit/test_firebolt_dialect.py @@ -48,14 +48,15 @@ def test_create_connect_args_service_account(self, dialect: FireboltDialect): def test_create_connect_args(self, dialect: FireboltDialect): connection_url = ( - "test_engine://test_user@email:test_password@test_db_name/test_engine_name?" + "test_engine://aabbb2bccc-kkkn3nbbb-iii4lll:test_password@" + + "test_db_name/test_engine_name?" ) u = url.make_url(connection_url) with mock.patch.dict(os.environ, {"FIREBOLT_BASE_URL": "test_url"}): result_list, result_dict = dialect.create_connect_args(u) assert result_dict["engine_name"] == "test_engine_name" - assert result_dict["auth"].username == "test_user@email" - assert result_dict["auth"].password == "test_password" + assert result_dict["auth"].client_id == "aabbb2bccc-kkkn3nbbb-iii4ll" + assert result_dict["auth"].client_secret == "test_password" assert result_dict["auth"]._use_token_cache is True assert result_dict["database"] == "test_db_name" assert result_dict["api_endpoint"] == "test_url" @@ -71,7 +72,8 @@ def test_create_connect_args(self, dialect: FireboltDialect): def test_create_connect_args_set_params(self, dialect: FireboltDialect): connection_url = ( - "test_engine://test_user@email:test_password@test_db_name/test_engine_name" + "test_engine://aabbb2bccc-kkkn3nbbb-iii4ll:test_password@" + "test_db_name/test_engine_name" "?account_name=FB¶m1=1¶m2=2" ) u = url.make_url(connection_url) @@ -83,7 +85,8 @@ def test_create_connect_args_set_params(self, dialect: FireboltDialect): def test_create_connect_args_driver_override(self, dialect: FireboltDialect): connection_url = ( - "test_engine://test_user@email:test_password@test_db_name/test_engine_name" + "test_engine://aabbb2bccc-kkkn3nbbb-iii4ll:test_password@" + "test_db_name/test_engine_name" "?user_drivers=DriverA:1.0.2&user_clients=ClientB:2.0.9" ) u = url.make_url(connection_url) @@ -105,13 +108,14 @@ def test_create_connect_args_token_cache( self, token, expected, dialect: FireboltDialect ): connection_url = ( - "test_engine://test_user@email:test_password@test_db_name/test_engine_name" + "test_engine://aabbb2bccc-kkkn3nbbb-iii4ll:test_password@" + "test_db_name/test_engine_name" f"?use_token_cache={token}¶m1=1¶m2=2" ) u = url.make_url(connection_url) result_list, result_dict = dialect.create_connect_args(u) - assert result_dict["auth"].username == "test_user@email" - assert result_dict["auth"].password == "test_password" + assert result_dict["auth"].client_id == "aabbb2bccc-kkkn3nbbb-iii4ll" + assert result_dict["auth"].client_secret == "test_password" assert result_dict["auth"]._use_token_cache == expected assert dialect._set_parameters == {"param1": "1", "param2": "2"} From 4b87d2a2b37fc77e95d64505949e2e0d3dc0c150 Mon Sep 17 00:00:00 2001 From: ptiurin Date: Tue, 29 Aug 2023 10:03:45 +0100 Subject: [PATCH 02/10] fix unit tests --- setup.cfg | 4 ++-- tests/unit/test_firebolt_dialect.py | 13 +++++++++---- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/setup.cfg b/setup.cfg index db8948b..100b6cc 100644 --- a/setup.cfg +++ b/setup.cfg @@ -46,9 +46,9 @@ dev = mock==4.0.3 mypy==0.910 pre-commit==2.15.0 - pytest==6.2.5 - pytest-asyncio==0.16.0 + pytest==7.2.0 pytest-cov==3.0.0 + pytest-trio==0.8.0 sqlalchemy-stubs==0.4 [mypy] diff --git a/tests/unit/test_firebolt_dialect.py b/tests/unit/test_firebolt_dialect.py index 289f37a..755d327 100644 --- a/tests/unit/test_firebolt_dialect.py +++ b/tests/unit/test_firebolt_dialect.py @@ -33,6 +33,7 @@ def test_create_dialect(self, dialect: FireboltDialect): def test_create_connect_args_service_account(self, dialect: FireboltDialect): u = url.make_url( "test_engine://test-sa-user-key:test_password@test_db_name/test_engine_name" + "?account_name=dummy" ) with mock.patch.dict(os.environ, {"FIREBOLT_BASE_URL": "test_url"}): result_list, result_dict = dialect.create_connect_args(u) @@ -50,12 +51,14 @@ def test_create_connect_args(self, dialect: FireboltDialect): connection_url = ( "test_engine://aabbb2bccc-kkkn3nbbb-iii4lll:test_password@" + "test_db_name/test_engine_name?" + "account_name=dummy" ) u = url.make_url(connection_url) with mock.patch.dict(os.environ, {"FIREBOLT_BASE_URL": "test_url"}): result_list, result_dict = dialect.create_connect_args(u) assert result_dict["engine_name"] == "test_engine_name" - assert result_dict["auth"].client_id == "aabbb2bccc-kkkn3nbbb-iii4ll" + assert result_dict["account_name"] == "dummy" + assert result_dict["auth"].client_id == "aabbb2bccc-kkkn3nbbb-iii4lll" assert result_dict["auth"].client_secret == "test_password" assert result_dict["auth"]._use_token_cache is True assert result_dict["database"] == "test_db_name" @@ -74,7 +77,7 @@ def test_create_connect_args_set_params(self, dialect: FireboltDialect): connection_url = ( "test_engine://aabbb2bccc-kkkn3nbbb-iii4ll:test_password@" "test_db_name/test_engine_name" - "?account_name=FB¶m1=1¶m2=2" + "?account_name=dummy¶m1=1¶m2=2" ) u = url.make_url(connection_url) result_list, result_dict = dialect.create_connect_args(u) @@ -87,7 +90,8 @@ def test_create_connect_args_driver_override(self, dialect: FireboltDialect): connection_url = ( "test_engine://aabbb2bccc-kkkn3nbbb-iii4ll:test_password@" "test_db_name/test_engine_name" - "?user_drivers=DriverA:1.0.2&user_clients=ClientB:2.0.9" + "?account_name=dummy" + "&user_drivers=DriverA:1.0.2&user_clients=ClientB:2.0.9" ) u = url.make_url(connection_url) result_list, result_dict = dialect.create_connect_args(u) @@ -110,7 +114,8 @@ def test_create_connect_args_token_cache( connection_url = ( "test_engine://aabbb2bccc-kkkn3nbbb-iii4ll:test_password@" "test_db_name/test_engine_name" - f"?use_token_cache={token}¶m1=1¶m2=2" + "?account_name=dummy" + f"&use_token_cache={token}¶m1=1¶m2=2" ) u = url.make_url(connection_url) result_list, result_dict = dialect.create_connect_args(u) From e734bd546a6cd8c28dde7f4b755aadc424af06e3 Mon Sep 17 00:00:00 2001 From: ptiurin Date: Tue, 29 Aug 2023 22:45:08 +0100 Subject: [PATCH 03/10] async fix --- setup.cfg | 1 + src/firebolt_db/firebolt_async_dialect.py | 8 ++++++-- .../test_sqlalchemy_async_integration.py | 13 +------------ 3 files changed, 8 insertions(+), 14 deletions(-) diff --git a/setup.cfg b/setup.cfg index 100b6cc..49b1836 100644 --- a/setup.cfg +++ b/setup.cfg @@ -50,6 +50,7 @@ dev = pytest-cov==3.0.0 pytest-trio==0.8.0 sqlalchemy-stubs==0.4 + trio-typing==0.9.0 [mypy] disallow_untyped_defs = True diff --git a/src/firebolt_db/firebolt_async_dialect.py b/src/firebolt_db/firebolt_async_dialect.py index 9ca2bf2..f25072d 100644 --- a/src/firebolt_db/firebolt_async_dialect.py +++ b/src/firebolt_db/firebolt_async_dialect.py @@ -1,6 +1,7 @@ from __future__ import annotations from asyncio import Lock +from functools import partial from types import ModuleType from typing import Any, Dict, Iterator, List, Optional, Tuple @@ -11,6 +12,7 @@ # and util.concurrency from sqlalchemy.engine import AdaptedConnection # type: ignore[attr-defined] from sqlalchemy.util.concurrency import await_only # type: ignore[import] +from trio import run from firebolt_db.firebolt_dialect import FireboltDialect @@ -150,8 +152,10 @@ def _init_dbapi_attributes(self) -> None: setattr(self, name, getattr(self.dbapi, name)) def connect(self, *arg: Any, **kw: Any) -> AsyncConnectionWrapper: - - connection = await_only(self.dbapi.connect(*arg, **kw)) # type: ignore[attr-defined] # noqa: F821,E501 + # Synchronously establish a connection that can execute + # asynchronous queries later + conn_func = partial(self.dbapi.connect, *arg, **kw) # type: ignore[attr-defined] # noqa: F821,E501 + connection = run(conn_func) return AsyncConnectionWrapper( self, connection, diff --git a/tests/integration/test_sqlalchemy_async_integration.py b/tests/integration/test_sqlalchemy_async_integration.py index bb5a051..e1c9cb4 100644 --- a/tests/integration/test_sqlalchemy_async_integration.py +++ b/tests/integration/test_sqlalchemy_async_integration.py @@ -1,16 +1,13 @@ from typing import Dict, List -import pytest from sqlalchemy import inspect, text from sqlalchemy.engine.base import Connection, Engine class TestAsyncFireboltDialect: - @pytest.mark.asyncio async def test_create_ex_table( self, async_connection: Connection, - async_engine: Engine, ex_table_query: str, ex_table_name: str, ): @@ -25,7 +22,6 @@ def has_test_table(conn: Connection) -> bool: await async_connection.execute(text(f"DROP TABLE {ex_table_name}")) assert not await async_connection.run_sync(has_test_table) - @pytest.mark.asyncio async def test_data_write(self, async_connection: Connection, fact_table_name: str): result = await async_connection.execute( text(f"INSERT INTO {fact_table_name}(idx, dummy) VALUES (1, 'some_text')") @@ -36,18 +32,12 @@ async def test_data_write(self, async_connection: Connection, fact_table_name: s assert result.rowcount == 1 assert len(result.fetchall()) == 1 - @pytest.mark.asyncio async def test_set_params(self, async_connection: Engine): await async_connection.execute(text("SET advanced_mode=1")) - await async_connection.execute(text("SET use_standard_sql=0")) - result = await async_connection.execute( - text("SELECT sleepEachRow(1) from numbers(1)") - ) + result = await async_connection.execute(text("SELECT 1")) assert len(result.fetchall()) == 1 - await async_connection.execute(text("SET use_standard_sql=1")) await async_connection.execute(text("SET advanced_mode=0")) - @pytest.mark.asyncio async def test_get_table_names(self, async_connection: Connection): def get_table_names(conn: Connection) -> bool: inspector = inspect(conn) @@ -56,7 +46,6 @@ def get_table_names(conn: Connection) -> bool: results = await async_connection.run_sync(get_table_names) assert len(results) > 0 - @pytest.mark.asyncio async def test_get_columns( self, async_connection: Connection, fact_table_name: str ): From 49f9815638a0f415b60e2c98d5c9b4cd25438c56 Mon Sep 17 00:00:00 2001 From: ptiurin Date: Wed, 30 Aug 2023 09:33:33 +0100 Subject: [PATCH 04/10] Skip test --- tests/unit/test_firebolt_async_dialect.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/tests/unit/test_firebolt_async_dialect.py b/tests/unit/test_firebolt_async_dialect.py index 977976d..3753024 100644 --- a/tests/unit/test_firebolt_async_dialect.py +++ b/tests/unit/test_firebolt_async_dialect.py @@ -32,7 +32,7 @@ def test_create_dialect(self, async_dialect: AsyncFireboltDialect): assert isinstance(async_dialect.type_compiler, FireboltTypeCompiler) assert async_dialect.context == {} - @pytest.mark.asyncio + @pytest.mark.skip("Failing with nested run() in trino") async def test_create_api_wrapper(self, async_api: AsyncMock(spec=MockAsyncDBApi)): def test_connect() -> AsyncAPIWrapper: async_api.paramstyle = "quoted" @@ -45,7 +45,6 @@ def test_connect() -> AsyncAPIWrapper: assert wrapper.paramstyle == "quoted" async_api.connect.assert_called_once_with("test arg") - @pytest.mark.asyncio async def test_connection_wrapper(self, async_api: AsyncMock(spec=MockAsyncDBApi)): def test_connection() -> AsyncConnectionWrapper: wrapper = AsyncConnectionWrapper(async_api, await_only(async_api.connect())) @@ -60,7 +59,6 @@ def test_connection() -> AsyncConnectionWrapper: async_api.connect.return_value.commit.assert_called_once() async_api.connect.return_value.aclose.assert_awaited_once() - @pytest.mark.asyncio async def test_cursor_execute( self, async_api: AsyncMock(spec=MockAsyncDBApi), @@ -84,7 +82,6 @@ def test_cursor() -> AsyncCursorWrapper: ) async_cursor.fetchall.assert_awaited_once() - @pytest.mark.asyncio async def test_cursor_execute_no_fetch( self, async_api: AsyncMock(spec=MockAsyncDBApi), @@ -109,7 +106,6 @@ def test_cursor() -> AsyncCursorWrapper: ) async_cursor.fetchall.assert_not_awaited() - @pytest.mark.asyncio async def test_cursor_close( self, async_api: AsyncMock(spec=MockAsyncDBApi), @@ -127,7 +123,6 @@ def test_cursor(): await greenlet_spawn(test_cursor) - @pytest.mark.asyncio async def test_cursor_executemany( self, async_api: AsyncMock(spec=MockAsyncDBApi), @@ -145,7 +140,6 @@ def test_cursor(): await greenlet_spawn(test_cursor) - @pytest.mark.asyncio async def test_cursor_fetch( self, async_api: AsyncMock(spec=MockAsyncDBApi), From 368de4516d04e6babd6660ccf28de25d90f8e751 Mon Sep 17 00:00:00 2001 From: ptiurin Date: Wed, 30 Aug 2023 09:37:28 +0100 Subject: [PATCH 05/10] Integration test CI --- .../workflows/python-integration-tests.yml | 59 +++++++++++++++---- 1 file changed, 47 insertions(+), 12 deletions(-) diff --git a/.github/workflows/python-integration-tests.yml b/.github/workflows/python-integration-tests.yml index 44bf374..76a5085 100644 --- a/.github/workflows/python-integration-tests.yml +++ b/.github/workflows/python-integration-tests.yml @@ -2,6 +2,34 @@ name: Integration tests on: workflow_dispatch: + inputs: + environment: + description: 'Environment to run the tests against' + type: choice + required: true + default: 'dev' + options: + - dev + - staging + workflow_call: + inputs: + environment: + default: 'staging' + required: false + type: string + branch: + required: false + type: string + description: 'Branch to run on' + secrets: + FIREBOLT_CLIENT_ID_STG_NEW_IDN: + required: true + FIREBOLT_CLIENT_SECRET_STG_NEW_IDN: + required: true + FIREBOLT_CLIENT_ID_NEW_IDN: + required: true + FIREBOLT_CLIENT_SECRET_NEW_IDN: + required: true jobs: integration-tests: @@ -20,14 +48,24 @@ jobs: python -m pip install --upgrade pip pip install ".[dev]" + - name: Determine env variables + run: | + if [ "${{ inputs.environment }}" == 'staging' ]; then + echo "CLIENT_ID=${{ secrets.FIREBOLT_CLIENT_ID_STG_NEW_IDN }}" >> "$GITHUB_ENV" + echo "CLIENT_SECRET=${{ secrets.FIREBOLT_CLIENT_SECRET_STG_NEW_IDN }}" >> "$GITHUB_ENV" + else + echo "CLIENT_ID=${{ secrets.FIREBOLT_CLIENT_ID_NEW_IDN }}" >> "$GITHUB_ENV" + echo "CLIENT_SECRET=${{ secrets.FIREBOLT_CLIENT_SECRET_NEW_IDN }}" >> "$GITHUB_ENV" + fi + - name: Setup database and engine id: setup - uses: firebolt-db/integration-testing-setup@v1 + uses: firebolt-db/integration-testing-setup@v2 with: - firebolt-username: ${{ secrets.FIREBOLT_USERNAME }} - firebolt-password: ${{ secrets.FIREBOLT_PASSWORD }} - api-endpoint: "api.dev.firebolt.io" - region: "us-east-1" + firebolt-client-id: ${{ env.CLIENT_ID }} + firebolt-client-secret: ${{ env.CLIENT_SECRET }} + account: "developer" + api-endpoint: "api.${{ inputs.environment }}.firebolt.io" - name: Restore cached failed tests id: cache-tests-restore @@ -39,16 +77,13 @@ jobs: - name: Run integration tests env: - USER_NAME: ${{ secrets.FIREBOLT_USERNAME }} - PASSWORD: ${{ secrets.FIREBOLT_PASSWORD }} - SERVICE_ID: ${{ secrets.SERVICE_ID }} - SERVICE_SECRET: ${{ secrets.SERVICE_SECRET }} + SERVICE_ID: ${{ env.CLIENT_ID }} + SERVICE_SECRET: ${{ env.CLIENT_SECRET }} DATABASE_NAME: ${{ steps.setup.outputs.database_name }} ENGINE_NAME: ${{ steps.setup.outputs.engine_name }} - ENGINE_URL: ${{ steps.setup.outputs.engine_url }} STOPPED_ENGINE_NAME: ${{ steps.setup.outputs.stopped_engine_name }} - STOPPED_ENGINE_URL: ${{ steps.setup.outputs.stopped_engine_url }} - FIREBOLT_BASE_URL: "api.dev.firebolt.io" + FIREBOLT_BASE_URL: "api.${{ inputs.environment }}.firebolt.io" + ACCOUNT_NAME: "developer" run: | pytest --last-failed -o log_cli=true -o log_cli_level=INFO tests/integration From b97bc506c7f70d9f417bb20494c67ed80100130d Mon Sep 17 00:00:00 2001 From: ptiurin Date: Wed, 30 Aug 2023 09:41:01 +0100 Subject: [PATCH 06/10] adding v0 integration tests --- .../workflows/python-integration-tests-v0.yml | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 .github/workflows/python-integration-tests-v0.yml diff --git a/.github/workflows/python-integration-tests-v0.yml b/.github/workflows/python-integration-tests-v0.yml new file mode 100644 index 0000000..a87c25d --- /dev/null +++ b/.github/workflows/python-integration-tests-v0.yml @@ -0,0 +1,74 @@ +name: v0.x Integration tests + +on: + workflow_dispatch: + workflow_call: + secrets: + FIREBOLT_USERNAME: + required: true + FIREBOLT_PASSWORD: + required: true + SERVICE_ID: + required: true + SERVICE_SECRET: + required: true + +jobs: + integration-tests: + runs-on: ubuntu-latest + steps: + - name: Check out code + uses: actions/checkout@v2 + with: + ref: 0.x + + - name: Set up Python 3.7 + uses: actions/setup-python@v2 + with: + python-version: 3.7 + + - name: Install dependencies + run: | + python -m pip install --upgrade pip + pip install ".[dev]" + + - name: Setup database and engine + id: setup + uses: firebolt-db/integration-testing-setup@v1 + with: + firebolt-username: ${{ secrets.FIREBOLT_USERNAME }} + firebolt-password: ${{ secrets.FIREBOLT_PASSWORD }} + api-endpoint: "api.dev.firebolt.io" + region: "us-east-1" + + - name: Restore cached failed tests + id: cache-tests-restore + uses: actions/cache/restore@v3 + with: + path: | + .pytest_cache/v/cache/lastfailed + key: ${{ runner.os }}-pytest-restore-failed-${{ github.ref }}-${{ github.sha }} + + - name: Run integration tests + env: + USER_NAME: ${{ secrets.FIREBOLT_USERNAME }} + PASSWORD: ${{ secrets.FIREBOLT_PASSWORD }} + SERVICE_ID: ${{ secrets.SERVICE_ID }} + SERVICE_SECRET: ${{ secrets.SERVICE_SECRET }} + DATABASE_NAME: ${{ steps.setup.outputs.database_name }} + ENGINE_NAME: ${{ steps.setup.outputs.engine_name }} + ENGINE_URL: ${{ steps.setup.outputs.engine_url }} + STOPPED_ENGINE_NAME: ${{ steps.setup.outputs.stopped_engine_name }} + STOPPED_ENGINE_URL: ${{ steps.setup.outputs.stopped_engine_url }} + FIREBOLT_BASE_URL: "api.dev.firebolt.io" + run: | + pytest --last-failed -o log_cli=true -o log_cli_level=INFO tests/integration + + - name: Save failed tests + id: cache-tests-save + uses: actions/cache/save@v3 + if: failure() + with: + path: | + .pytest_cache/v/cache/lastfailed + key: ${{ steps.cache-tests-restore.outputs.cache-primary-key }} \ No newline at end of file From ed09717f582b2f54049dfcf2274f201e4cdfb3c3 Mon Sep 17 00:00:00 2001 From: ptiurin Date: Wed, 30 Aug 2023 16:22:38 +0100 Subject: [PATCH 07/10] better tests --- src/firebolt_db/firebolt_dialect.py | 6 ++- tests/unit/test_firebolt_dialect.py | 61 ++++++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 4 deletions(-) diff --git a/src/firebolt_db/firebolt_dialect.py b/src/firebolt_db/firebolt_dialect.py index 3c00572..73cd1c2 100644 --- a/src/firebolt_db/firebolt_dialect.py +++ b/src/firebolt_db/firebolt_dialect.py @@ -10,6 +10,7 @@ from sqlalchemy.engine import Connection as AlchemyConnection from sqlalchemy.engine import ExecutionContext, default from sqlalchemy.engine.url import URL +from sqlalchemy.exc import ArgumentError from sqlalchemy.sql import compiler, text from sqlalchemy.types import ( ARRAY, @@ -153,7 +154,9 @@ def create_connect_args(self, url: URL) -> Tuple[List, Dict]: if "account_name" in parameters: kwargs["account_name"] = parameters.pop("account_name") else: - raise Exception("account_name parameter must be provided to authenticate") + raise ArgumentError( + "account_name parameter must be provided to authenticate" + ) self._set_parameters = parameters # If URL override is not provided leave it to the sdk to determine the endpoint if "FIREBOLT_BASE_URL" in os.environ: @@ -222,7 +225,6 @@ def get_columns( schema: Optional[str] = None, **kwargs: Any ) -> List[Dict]: - query = """ select column_name, data_type, diff --git a/tests/unit/test_firebolt_dialect.py b/tests/unit/test_firebolt_dialect.py index 755d327..c1a442d 100644 --- a/tests/unit/test_firebolt_dialect.py +++ b/tests/unit/test_firebolt_dialect.py @@ -2,9 +2,11 @@ from unittest import mock import sqlalchemy +import sqlalchemy.types as sqltypes from conftest import MockCursor, MockDBApi -from pytest import mark +from pytest import mark, raises from sqlalchemy.engine import url +from sqlalchemy.exc import ArgumentError from sqlalchemy.sql import text import firebolt_db # SQLAlchemy package @@ -15,6 +17,7 @@ FireboltTypeCompiler, ) from firebolt_db.firebolt_dialect import dialect as dialect_definition +from firebolt_db.firebolt_dialect import resolve_type class TestFireboltDialect: @@ -47,6 +50,13 @@ def test_create_connect_args_service_account(self, dialect: FireboltDialect): assert "password" not in result_dict assert result_list == [] + def test_create_connect_no_account(self, dialect: FireboltDialect): + u = url.make_url( + "test_engine://test-sa-user-key:test_password@test_db_name/test_engine_name" + ) + with raises(ArgumentError): + dialect.create_connect_args(u) + def test_create_connect_args(self, dialect: FireboltDialect): connection_url = ( "test_engine://aabbb2bccc-kkkn3nbbb-iii4lll:test_password@" @@ -214,7 +224,6 @@ def getitem(self, idx): expected_query_schema, ), ): - assert call() == [ { "name": "name1", @@ -235,6 +244,22 @@ def getitem(self, idx): ) connection.execute.reset_mock() + def test_has_table( + self, dialect: FireboltDialect, connection: mock.Mock(spec=MockDBApi) + ): + connection.execute.return_value.fetchone.return_value.exists_ = True + assert dialect.has_table(connection, "dummy") + assert "dummy" in str(connection.execute.call_args[0][0].compile()) + + def test_noop( + self, dialect: FireboltDialect, connection: mock.Mock(spec=MockDBApi) + ): + dialect.get_view_definition(connection, "dummy") + dialect.do_rollback(connection) + dialect.do_commit(connection) + connection.assert_not_called() + connection.execute.assert_not_called() + def test_pk_constraint( self, dialect: FireboltDialect, connection: mock.Mock(spec=MockDBApi) ): @@ -294,3 +319,35 @@ def test_types(): assert firebolt_db.firebolt_dialect.BOOLEAN is sqlalchemy.sql.sqltypes.BOOLEAN assert firebolt_db.firebolt_dialect.REAL is sqlalchemy.sql.sqltypes.REAL assert issubclass(firebolt_db.firebolt_dialect.ARRAY, sqlalchemy.types.TypeEngine) + + +@mark.parametrize( + ["firebolt_type", "alchemy_type"], + [ + ("TEXT", sqltypes.TEXT), + ("LONG", sqltypes.BIGINT), + ("DECIMAL", sqltypes.NUMERIC), + ("INT", sqltypes.INTEGER), + ("TIMESTAMP", sqltypes.TIMESTAMP), + ("TIMESTAMPTZ", sqltypes.TIMESTAMP), + ("TIMESTAMPNTZ", sqltypes.TIMESTAMP), + ], +) +def test_resolve_type(firebolt_type: str, alchemy_type: sqltypes.TypeEngine): + assert resolve_type(firebolt_type.lower()) == alchemy_type + + +@mark.parametrize( + ["firebolt_type", "item_type", "dimensions"], + [ + ("ARRAY(INT NOT NULL)", sqltypes.INTEGER, 1), + ("ARRAY(INT NULL)", sqltypes.INTEGER, 1), + ("ARRAY(ARRAY(INT NULL))", sqltypes.INTEGER, 2), + ], +) +def test_resolve_array_type( + firebolt_type: str, item_type: sqltypes.TypeEngine, dimensions: int +): + resolved_type = resolve_type(firebolt_type.lower()) + assert type(resolved_type.item_type) == item_type + assert resolved_type.dimensions == dimensions From 992312f5e153aef57c3bddb0018a2f3f1a7b345c Mon Sep 17 00:00:00 2001 From: ptiurin Date: Wed, 30 Aug 2023 16:22:55 +0100 Subject: [PATCH 08/10] ci --- .github/workflows/code-check.yml | 9 ++- .github/workflows/nightly-v0.yml | 103 ++++++++++++++++++++++++++++ .github/workflows/nightly.yml | 18 ++--- .github/workflows/security-scan.yml | 7 ++ .github/workflows/unit-tests.yml | 9 ++- 5 files changed, 133 insertions(+), 13 deletions(-) create mode 100644 .github/workflows/nightly-v0.yml diff --git a/.github/workflows/code-check.yml b/.github/workflows/code-check.yml index a72db28..a2f1e46 100644 --- a/.github/workflows/code-check.yml +++ b/.github/workflows/code-check.yml @@ -2,8 +2,13 @@ name: Code quality checks on: workflow_call: + inputs: + branch: + required: false + type: string + description: 'Branch to run on' push: - branches: [ main ] + branches: [ main, 0.x ] jobs: check-code: @@ -11,6 +16,8 @@ jobs: steps: - name: Check out code uses: actions/checkout@v2 + with: + ref: ${{ inputs.branch }} - name: Set up Python 3.8 uses: actions/setup-python@v2 diff --git a/.github/workflows/nightly-v0.yml b/.github/workflows/nightly-v0.yml new file mode 100644 index 0000000..99df3d6 --- /dev/null +++ b/.github/workflows/nightly-v0.yml @@ -0,0 +1,103 @@ +name: v0.x Nightly code check +on: + workflow_dispatch: + schedule: + - cron: '0 4 * * *' # 4 am UTC every day +jobs: + code-check: + uses: ./.github/workflows/code-check.yml + with: + branch: 0.x + unit-tests: + uses: ./.github/workflows/unit-tests.yml + with: + branch: 0.x + secrets: + GIST_PAT: ${{ secrets.GIST_PAT }} + security-scan: + needs: [unit-tests] + uses: ./.github/workflows/security-scan.yml + with: + branch: 0.x + secrets: + FOSSA_TOKEN: ${{ secrets.FOSSA_TOKEN }} + SONARCLOUD_TOKEN: ${{ secrets.SONARCLOUD_TOKEN }} + tests: + runs-on: ${{ matrix.os }} + strategy: + fail-fast: false # finish all jobs even if one fails + max-parallel: 2 + matrix: + os: ['ubuntu-latest', 'macos-latest', 'windows-latest'] + python-version: ['3.7', '3.8', '3.9', '3.10'] + steps: + - name: Check out code + uses: actions/checkout@v2 + + - name: Set up Python 3.7 + uses: actions/setup-python@v2 + with: + python-version: ${{ matrix.python-version }} + + - name: Install dependencies + run: | + python -m pip install --upgrade pip + python -m pip install ".[dev]" + + - name: Test with pytest + run: | + pytest tests/unit + + - name: Setup database and engine + id: setup + uses: firebolt-db/integration-testing-setup@v1 + with: + firebolt-username: ${{ secrets.FIREBOLT_USERNAME }} + firebolt-password: ${{ secrets.FIREBOLT_PASSWORD }} + api-endpoint: "api.dev.firebolt.io" + region: "us-east-1" + db_suffix: ${{ format('{0}_{1}', matrix.os, matrix.python-version) }} + + - name: Restore cached failed tests + id: cache-tests-restore + uses: actions/cache/restore@v3 + with: + path: | + .pytest_cache/v/cache/lastfailed + key: ${{ runner.os }}-pytest-restore-failed-${{ github.ref }}-${{ github.sha }} + + - name: Run integration tests + env: + USER_NAME: ${{ secrets.FIREBOLT_USERNAME }} + PASSWORD: ${{ secrets.FIREBOLT_PASSWORD }} + SERVICE_ID: ${{ secrets.SERVICE_ID }} + SERVICE_SECRET: ${{ secrets.SERVICE_SECRET }} + DATABASE_NAME: ${{ steps.setup.outputs.database_name }} + ENGINE_NAME: ${{ steps.setup.outputs.engine_name }} + ENGINE_URL: ${{ steps.setup.outputs.engine_url }} + STOPPED_ENGINE_NAME: ${{ steps.setup.outputs.stopped_engine_name }} + STOPPED_ENGINE_URL: ${{ steps.setup.outputs.stopped_engine_url }} + ACCOUNT_NAME: "firebolt" + FIREBOLT_BASE_URL: "api.dev.firebolt.io" + run: | + pytest --last-failed -o log_cli=true -o log_cli_level=INFO --junit-xml=report/junit.xml tests/integration + + - name: Save failed tests + id: cache-tests-save + uses: actions/cache/save@v3 + if: failure() + with: + path: | + .pytest_cache/v/cache/lastfailed + key: ${{ steps.cache-tests-restore.outputs.cache-primary-key }} + + - name: Slack Notify of failure + if: failure() + id: slack + uses: firebolt-db/action-slack-nightly-notify@v1 + with: + os: ${{ matrix.os }} + programming-language: Python + language-version: ${{ matrix.python-version }} + notifications-channel: 'ecosystem-ci-notifications' + slack-api-key: ${{ secrets.SLACK_BOT_TOKEN }} diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index adfbd42..a45e751 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -44,12 +44,12 @@ jobs: - name: Setup database and engine id: setup - uses: firebolt-db/integration-testing-setup@v1 + uses: firebolt-db/integration-testing-setup@v2 with: - firebolt-username: ${{ secrets.FIREBOLT_USERNAME }} - firebolt-password: ${{ secrets.FIREBOLT_PASSWORD }} + firebolt-client-id: ${{ secrets.FIREBOLT_CLIENT_ID_NEW_IDN }} + firebolt-client-secret: ${{ secrets.FIREBOLT_CLIENT_SECRET_NEW_IDN }} + account: "developer" api-endpoint: "api.dev.firebolt.io" - region: "us-east-1" db_suffix: ${{ format('{0}_{1}', matrix.os, matrix.python-version) }} - name: Restore cached failed tests @@ -62,17 +62,13 @@ jobs: - name: Run integration tests env: - USER_NAME: ${{ secrets.FIREBOLT_USERNAME }} - PASSWORD: ${{ secrets.FIREBOLT_PASSWORD }} - SERVICE_ID: ${{ secrets.SERVICE_ID }} - SERVICE_SECRET: ${{ secrets.SERVICE_SECRET }} + SERVICE_ID: ${{ secrets.FIREBOLT_CLIENT_ID_NEW_IDN }} + SERVICE_SECRET: ${{ secrets.FIREBOLT_CLIENT_SECRET_NEW_IDN }} DATABASE_NAME: ${{ steps.setup.outputs.database_name }} ENGINE_NAME: ${{ steps.setup.outputs.engine_name }} - ENGINE_URL: ${{ steps.setup.outputs.engine_url }} STOPPED_ENGINE_NAME: ${{ steps.setup.outputs.stopped_engine_name }} - STOPPED_ENGINE_URL: ${{ steps.setup.outputs.stopped_engine_url }} - ACCOUNT_NAME: "firebolt" FIREBOLT_BASE_URL: "api.dev.firebolt.io" + ACCOUNT_NAME: "developer" run: | pytest --last-failed -o log_cli=true -o log_cli_level=INFO --junit-xml=report/junit.xml tests/integration diff --git a/.github/workflows/security-scan.yml b/.github/workflows/security-scan.yml index 121fda9..04ca88a 100644 --- a/.github/workflows/security-scan.yml +++ b/.github/workflows/security-scan.yml @@ -2,6 +2,11 @@ name: Firebolt Security Scan on: workflow_call: + inputs: + branch: + required: false + type: string + description: 'Branch to run on' secrets: FOSSA_TOKEN: required: true @@ -14,6 +19,8 @@ jobs: steps: - name: "Checkout Code" uses: actions/checkout@v2 + with: + ref: ${{ inputs.branch }} - name: "Download coverage report" uses: actions/download-artifact@v2 diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index d529ee6..3186242 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -2,11 +2,16 @@ name: Unit tests on: workflow_call: + inputs: + branch: + required: false + type: string + description: 'Branch to run on' secrets: GIST_PAT: required: true push: - branches: [ main ] + branches: [ main, 0.x ] jobs: unit-tests: @@ -16,6 +21,8 @@ jobs: steps: - name: Check out code uses: actions/checkout@v2 + with: + ref: ${{ inputs.branch }} - name: Set up Python 3.7 uses: actions/setup-python@v2 From ba93bb0c9f728230ad66fe92300d41bb10a850ba Mon Sep 17 00:00:00 2001 From: ptiurin Date: Wed, 30 Aug 2023 16:49:55 +0100 Subject: [PATCH 09/10] fix secret name --- tests/integration/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 30a3379..9cdfbeb 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -14,7 +14,7 @@ DATABASE_NAME_ENV = "DATABASE_NAME" ACCOUNT_NAME_ENV = "ACCOUNT_NAME" CLIENT_ID_ENV = "CLIENT_ID" -CLIENT_KEY_ENV = "CLIENT_KEY" +CLIENT_KEY_ENV = "CLIENT_SECRET" def must_env(var_name: str) -> str: From b35f2456a200e0b41b5362b0e0806e37773fadf0 Mon Sep 17 00:00:00 2001 From: ptiurin Date: Wed, 30 Aug 2023 17:07:28 +0100 Subject: [PATCH 10/10] docs: modify docs --- README.md | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 1175bde..013faaa 100644 --- a/README.md +++ b/README.md @@ -30,24 +30,18 @@ pip install firebolt-sqlalchemy Connection strings use the following structure: ``` -firebolt://{username}:{password}@{database}[/{engine_name}][?account_name={name}}] +firebolt://{client_id}:{client_secret}@{database}[/{engine_name}]?account_name={name} ``` -`engine_name` is optional. If omitted, Firebolt will use the default engine for the database. +`engine_name` is optional. -`account_name` is optional. If omitted a default account will be used for connection. +`account_name` is required. Examples: ``` -firebolt://email@domain:password@sample_database -firebolt://email@domain:password@sample_database/sample_engine -``` - -If a different account name is required, it can be specified in the connection string - -``` -firebolt://email@domain:password@sample_database/sample_engine?account_name=my_account +firebolt://aaa-bbb-ccc-222:$ecret@sample_database?account_name=my_account +firebolt://aaa-bbb-ccc-222:$ecret@sample_database/sample_engine?account_name=my_account ``` To override the API URL (e.g. for dev testing): @@ -56,11 +50,11 @@ To override the API URL (e.g. for dev testing): export FIREBOLT_BASE_URL= ``` -If your password contains % or / characters they need to be sanitised as per https://docs.sqlalchemy.org/en/20/core/engines.html#database-urls +If your secret contains % or / characters they need to be sanitised as per https://docs.sqlalchemy.org/en/20/core/engines.html#database-urls ```python -my_pass = "0920%/2" +my_secret = "0920%/2" import urllib.parse -new_pass = urllib.parse.quote_plus(my_pass) +new_secret = urllib.parse.quote_plus(my_secret) ``` ## Quick Start @@ -69,8 +63,8 @@ new_pass = urllib.parse.quote_plus(my_pass) import urllib.parse from sqlalchemy import create_engine -password = urllib.parse.quote_plus("your_password_here") -engine = create_engine("firebolt://email@domain:" + password + "@sample_database/sample_engine") +secret = urllib.parse.quote_plus("your_secret_here") +engine = create_engine("firebolt://aaa-bbb-ccc-222:" + secret + "@sample_database/sample_engine?account_name=my_account") connection = engine.connect() connection.execute("CREATE FACT TABLE example(dummy int) PRIMARY INDEX dummy") @@ -87,8 +81,8 @@ import urllib.parse from sqlalchemy import text from sqlalchemy.ext.asyncio import create_async_engine -password = urllib.parse.quote_plus("your_password_here") -engine = create_async_engine("asyncio+firebolt://email@domain:" + password + "@sample_database/sample_engine") +secret = urllib.parse.quote_plus("your_secret_here") +engine = create_async_engine("asyncio+firebolt://aaa-bbb-ccc-222:" + secret + "@sample_database/sample_engine?account_name=my_account") async with engine.connect() as conn: