From 876076b9702d45942e40f96d9b1f2b8417689bbc Mon Sep 17 00:00:00 2001 From: Mila Page <67295367+VersusFacit@users.noreply.github.com> Date: Thu, 1 Jun 2023 16:34:29 -0700 Subject: [PATCH] Restore transaction semantics used by dbt-redshift prior to 1.5 (#475) * Rename flag and get autocommit on * Add changie * Add test for issue 451 * Add test for issue #454 * Fix spelling. * Revert names * update tests * update the comment to use pep for rationale --------- Co-authored-by: Mila Page --- .../unreleased/Fixes-20230531-153347.yaml | 7 + dbt/adapters/redshift/connections.py | 3 +- tests/functional/adapter/test_autocommit.py | 60 ------ tests/functional/test_autocommit.py | 171 ++++++++++++++++++ 4 files changed, 180 insertions(+), 61 deletions(-) create mode 100644 .changes/unreleased/Fixes-20230531-153347.yaml delete mode 100644 tests/functional/adapter/test_autocommit.py create mode 100644 tests/functional/test_autocommit.py diff --git a/.changes/unreleased/Fixes-20230531-153347.yaml b/.changes/unreleased/Fixes-20230531-153347.yaml new file mode 100644 index 00000000..4c4d324a --- /dev/null +++ b/.changes/unreleased/Fixes-20230531-153347.yaml @@ -0,0 +1,7 @@ +kind: Fixes +body: Get autocommit on by default to restore old semantics users had relied on prior + to 1.5. Add tests. +time: 2023-05-31T15:33:47.180508-07:00 +custom: + Author: versusfacit + Issue: "425" diff --git a/dbt/adapters/redshift/connections.py b/dbt/adapters/redshift/connections.py index 509fd491..0c25ec08 100644 --- a/dbt/adapters/redshift/connections.py +++ b/dbt/adapters/redshift/connections.py @@ -150,7 +150,8 @@ class RedshiftCredentials(Credentials): sslmode: Optional[UserSSLMode] = field(default_factory=UserSSLMode.default) retries: int = 1 region: Optional[str] = None # if not provided, will be determined from host - autocommit: Optional[bool] = False + # opt-in by default per team deliberation on https://peps.python.org/pep-0249/#autocommit + autocommit: Optional[bool] = True _ALIASES = {"dbname": "database", "pass": "password"} diff --git a/tests/functional/adapter/test_autocommit.py b/tests/functional/adapter/test_autocommit.py deleted file mode 100644 index 78f33e0f..00000000 --- a/tests/functional/adapter/test_autocommit.py +++ /dev/null @@ -1,60 +0,0 @@ -import os -import pytest - -from dbt.tests.util import run_dbt_and_capture - -_MACROS__CREATE_DB = """ -{% macro create_db_fake() %} - -{% set database = "db_for_test__do_delete_if_you_see_this" %} - -{# IF NOT EXISTS not avaiable but Redshift merely returns an error for trying to overwrite #} -{% set create_command %} - CREATE DATABASE {{ database }} -{% endset %} - -{{ log(create_command, info=True) }} - -{% do run_query(create_command) %} - -{{ log("Created redshift database " ~ database, info=True) }} - -{% endmacro %} -""" - - -class TestAutocommitWorksWithTransactionBlocks: - @pytest.fixture(scope="class") - def macros(self): - return {"macro.sql": _MACROS__CREATE_DB} - - @pytest.fixture(scope="class") - def dbt_profile_target(self): - return { - "type": "redshift", - "threads": 1, - "retries": 6, - "host": os.getenv("REDSHIFT_TEST_HOST"), - "port": int(os.getenv("REDSHIFT_TEST_PORT")), - "user": os.getenv("REDSHIFT_TEST_USER"), - "pass": os.getenv("REDSHIFT_TEST_PASS"), - "dbname": os.getenv("REDSHIFT_TEST_DBNAME"), - "autocommit": True, - } - - def test_autocommit_allows_for_more_commands(self, project): - """Scenario: user has autocommit=True in their target to run macros with normally - forbidden commands like CREATE DATABASE and VACUUM""" - result, out = run_dbt_and_capture(["run-operation", "create_db_fake"], expect_pass=False) - assert "CREATE DATABASE cannot run inside a transaction block" not in out - - -class TestTransactionBlocksPreventCertainCommands: - @pytest.fixture(scope="class") - def macros(self): - return {"macro.sql": _MACROS__CREATE_DB} - - def test_normally_create_db_disallowed(self, project): - """Monitor if status quo in Redshift connector changes""" - result, out = run_dbt_and_capture(["run-operation", "create_db_fake"], expect_pass=False) - assert "CREATE DATABASE cannot run inside a transaction block" in out diff --git a/tests/functional/test_autocommit.py b/tests/functional/test_autocommit.py new file mode 100644 index 00000000..e5e54a34 --- /dev/null +++ b/tests/functional/test_autocommit.py @@ -0,0 +1,171 @@ +import os +import pytest + +from dbt.tests.util import run_dbt, run_dbt_and_capture + +_MACROS__CREATE_DB = """ +{% macro create_db_fake() %} + +{% set database = "db_for_test__do_delete_if_you_see_this" %} + +{# IF NOT EXISTS not avaiable but Redshift merely returns an error for trying to overwrite #} +{% set create_command %} + CREATE DATABASE {{ database }} +{% endset %} + +{{ log(create_command, info=True) }} + +{% do run_query(create_command) %} + +{{ log("Created redshift database " ~ database, info=True) }} + +{% endmacro %} +""" + +_MACROS__UPDATE_MY_MODEL = """ +{% macro update_some_model(alert_ids, sent_at, table_name) %} + {% set update_query %} + UPDATE {{ ref('my_model') }} set status = 'sent' + {% endset %} + {% do run_query(update_query) %} +{% endmacro %} +""" + +_MACROS__UPDATE_MY_SEED = """ +{% macro update_my_seed() %} +update {{ ref("my_seed") }} set status = 'done' +{% endmacro %} +""" + +_MODELS__MY_MODEL = """ +{{ config(materialized="table") }} + +select 1 as id, 'pending' as status +""" + +_MODELS__AFTER_COMMIT = """ +{{ + config( + post_hook=after_commit("{{ update_my_seed() }}") + ) +}} + +select 1 as id +""" + +_SEEDS_MY_SEED = """ +id,status +1,pending +""".lstrip() + + +class TestTransactionBlocksPreventCertainCommands: + @pytest.fixture(scope="class") + def macros(self): + return {"macro.sql": _MACROS__CREATE_DB} + + def test_autocommit_deactivated_prevents_DDL(self, project): + """Scenario: user has autocommit=True in their target to run macros with normally + forbidden commands like CREATE DATABASE and VACUUM""" + result, out = run_dbt_and_capture(["run-operation", "create_db_fake"], expect_pass=False) + assert "CREATE DATABASE cannot run inside a transaction block" not in out + + +class TestAutocommitUnblocksDDLInTransactions: + @pytest.fixture(scope="class") + def dbt_profile_target(self): + return { + "type": "redshift", + "threads": 1, + "retries": 6, + "host": os.getenv("REDSHIFT_TEST_HOST"), + "port": int(os.getenv("REDSHIFT_TEST_PORT")), + "user": os.getenv("REDSHIFT_TEST_USER"), + "pass": os.getenv("REDSHIFT_TEST_PASS"), + "dbname": os.getenv("REDSHIFT_TEST_DBNAME"), + "autocommit": False, + } + + @pytest.fixture(scope="class") + def macros(self): + return {"macro.sql": _MACROS__CREATE_DB} + + def test_default_setting_allows_DDL(self, project): + """Monitor if status quo in Redshift connector changes""" + result, out = run_dbt_and_capture(["run-operation", "create_db_fake"], expect_pass=False) + assert "CREATE DATABASE cannot run inside a transaction block" in out + + +class TestUpdateDDLCommits: + @pytest.fixture(scope="class") + def macros(self): + return {"macro.sql": _MACROS__UPDATE_MY_MODEL} + + @pytest.fixture(scope="class") + def models(self): + return {"my_model.sql": _MODELS__MY_MODEL} + + def test_update_will_go_through(self, project): + run_dbt() + run_dbt(["run-operation", "update_some_model"]) + _, out = run_dbt_and_capture( + ["show", "--inline", "select * from {}.my_model".format(project.test_schema)] + ) + assert "1 | sent" in out + + +class TestUpdateDDLDoesNotCommitWithoutAutocommit: + @pytest.fixture(scope="class") + def dbt_profile_target(self): + return { + "type": "redshift", + "host": os.getenv("REDSHIFT_TEST_HOST"), + "port": int(os.getenv("REDSHIFT_TEST_PORT")), + "user": os.getenv("REDSHIFT_TEST_USER"), + "pass": os.getenv("REDSHIFT_TEST_PASS"), + "dbname": os.getenv("REDSHIFT_TEST_DBNAME"), + "autocommit": False, + } + + @pytest.fixture(scope="class") + def macros(self): + return {"macro.sql": _MACROS__UPDATE_MY_MODEL} + + @pytest.fixture(scope="class") + def models(self): + return {"my_model.sql": _MODELS__MY_MODEL} + + def test_update_will_not_go_through(self, project): + run_dbt() + run_dbt(["run-operation", "update_some_model"]) + _, out = run_dbt_and_capture( + ["show", "--inline", "select * from {}.my_model".format(project.test_schema)] + ) + assert "1 | pending" in out + + +class TestAfterCommitMacroTakesEffect: + @pytest.fixture(scope="class") + def macros(self): + return {"macro.sql": _MACROS__UPDATE_MY_SEED} + + @pytest.fixture(scope="class") + def models(self): + return {"my_model.sql": _MODELS__AFTER_COMMIT} + + @pytest.fixture(scope="class") + def seeds(self): + return {"my_seed.csv": _SEEDS_MY_SEED} + + def test_update_happens_via_macro_in_config(self, project): + run_dbt(["seed"]) + _, out = run_dbt_and_capture( + ["show", "--inline", "select * from {}.my_seed".format(project.test_schema)] + ) + assert "1 | pending" in out + + run_dbt() + _, out = run_dbt_and_capture( + ["show", "--inline", "select * from {}.my_seed".format(project.test_schema)] + ) + assert "1 | done" in out