Execute user script refactor #1695

sfc-gh-pjafari · 2024-10-09T02:12:07Z

Pre-review checklist

I've confirmed that instructions included in README.md are still correct after my changes in the codebase.
I've added or updated automated unit tests to verify correctness of my new code.
I've added or updated integration tests to verify correctness of my new code.
I've confirmed that my changes are working by executing CLI's commands manually on MacOS.
I've confirmed that my changes are working by executing CLI's commands manually on Windows.
I've confirmed that my changes are up-to-date with the target branch.
I've described my changes in the release notes.
I've described my changes in the section below.

Changes description

NOTE - This is a PR against @sfc-gh-mchok's telemetry branch, since I use some error types he's introduced

Started a SQLService abstraction layer. The goal is to extract execute_query calls that are embedded in the logic out to their own unit. This PR uses package scripts to show an example of the restructuring.
use role, warehouse, and database calls are moved to this layer. See test_package_scripts_with_conn_warehouse for and example of an updated test.

NOTE - Since the start of this work, apply_package_scripts is no longer invoked by any commands (in favor of post-deploy hooks). That's why there are no integration tests associated with this change, even though some error handling has changed.
Exhaustive test cases for execute_user_script as well as integ tests to be added when making similar change for running post_deploy hooks in an upcoming PR.
...

sfc-gh-fcampbell

just some drive-by suggestions

src/snowflake/cli/api/sql_contract.py

src/snowflake/cli/_plugins/nativeapp/sf_sql_facade.py

sfc-gh-bdufour · 2024-10-15T13:45:08Z

src/snowflake/cli/_plugins/nativeapp/sf_facade.py

+def get_snowflake_facade(sql_executor: SqlExecutor | None) -> SnowflakeSQLFacade:
+    """Returns a Snowflake Facade"""
+    return SnowflakeSQLFacade(sql_executor)


why require an executor at all? At least we should use None by default IMHO.

see @sfc-gh-fcampbell's comment here #1695 (comment)
Though we are currently always creating a new instance of sql_executor where we need it. imo the way we are using it rn either way ends up the same. But we can make a design decision as to if we want to pass the sql executor in or instantiate it in SnowflakeSQLFacade.

IMHO the executor is pretty much an implementation detail here, and I'd only expect it to be replaced for tests so we can inspect our interaction with a low-level executor. I do like the simplicity of just acquiring a facade object (could be a global or per-thread singleton) from anywhere in the code, as long as we can easily mock it out when we need to test code that interacts with the facade.

sfc-gh-bdufour · 2024-10-15T13:47:37Z

src/snowflake/cli/_plugins/nativeapp/sf_sql_facade.py

+        wh_result = self._sql_executor.execute_query(
+            f"select current_warehouse()"
+        ).fetchone()


what should be the behaviour if this fails? It shouldn't fail, I know, but we should control the contract if it ever does. Right now we'd probably just let a ProgrammingError bubble up. Is that what we want?

I think with our error classifications now bubbbling it up as a ProgrammingError is fair (categorizes it as not 100% the user's fault). Would you say if we get a failure here it's fair to assume we need act on it and see what's wrong?

It's a weird state for sure if something fails, but I'm not convinced that ProgrammingError is the right contract. We know what failed here, and it's not wrong SQL.

sfc-gh-bdufour · 2024-10-15T15:14:44Z

src/snowflake/cli/_plugins/nativeapp/sf_sql_facade.py

+        finally:
+            if is_different_wh and prev_wh is not None:
+                self._log.debug(f"Switching back to warehouse:{prev_wh}")
+                self._sql_executor.execute_query(f"use warehouse {prev_wh}")


ditto, what happens if this fails? Technically the user script could drop the warehouse or otherwise make it inaccessible.

good point. and the same can be said about the use database case. See update. made a _use_object method that has the error handling it and using it for all use object_type object_name calls.

tests/nativeapp/test_package_scripts.py

sfc-gh-bdufour · 2024-10-15T15:26:02Z

tests/nativeapp/test_sf_sql_facade.py

+# TODO: Add more test coverage + define contract
+# TODO: Add test for non-identifier names


fast-follow PRs? What's the plan here?

Yes, see note on my PR description. This class is not used in any code path (since Frank has removed apply_package_script in the meantime). So more tests and integ tests to be added in the upcoming PR for post deploy hooks.

We should be able to cover the facade in isolation though...

added tests

sfc-gh-bdufour · 2024-10-15T15:26:52Z

tests/nativeapp/test_sf_sql_facade.py

+    role = "mock_role"
+    wh = "mock_wh"
+    database = "mock_db"
+    side_effects, expected = mock_execute_helper(


if you have a better pattern, use it instead. This is pretty much NADE legacy at this point. We could also create a new abstraction / helper for SQL testing in this file, since it will be used here almost exclusively.

I think using this helper is OK for when we actually want to test the sql.
And its usage in other tests will be reduced and removed as I refactor more SQL calls into this class.
Of course there are small improvements I can do to it like switching the order of expected call and side_effect but not sure how much we'll gain from that.

tests/nativeapp/test_package_scripts.py

…actor

sfc-gh-pjafari · 2024-10-16T20:24:32Z

closing this PR, new PR on main here #1732

sfc-gh-pjafari marked this pull request as ready for review October 9, 2024 17:39

sfc-gh-pjafari requested review from a team as code owners October 9, 2024 17:39

sfc-gh-fcampbell reviewed Oct 9, 2024

View reviewed changes

sfc-gh-pjafari marked this pull request as draft October 9, 2024 19:02

sfc-gh-pjafari changed the base branch from mchok-improve-ProgrammingError to main October 11, 2024 19:47

sfc-gh-pjafari changed the base branch from main to mchok-improve-ProgrammingError October 11, 2024 19:47

sfc-gh-pjafari added 10 commits October 11, 2024 15:49

first commit - make contract

964117e

rewrite tests

15a123a

remove comment

e5f3a98

add error handling

a56fe0e

move and rename class

7fd543d

re-wrote tests

1974494

add exit_code check to test

1ae721a

add happy path test for execute_user_script

76d1709

add todo comment to add tests

6939f73

add docstrings

ddf215c

sfc-gh-pjafari force-pushed the pj-user-script-refactor branch from 0519636 to ddf215c Compare October 11, 2024 19:49

sfc-gh-pjafari commented Oct 11, 2024

View reviewed changes

src/snowflake/cli/_plugins/nativeapp/sf_sql_facade.py Outdated Show resolved Hide resolved

remove comment

412467a

sfc-gh-pjafari marked this pull request as ready for review October 11, 2024 19:52

sfc-gh-bdufour reviewed Oct 15, 2024

View reviewed changes

error handle all use calls

cf14589

sfc-gh-bdufour reviewed Oct 15, 2024

View reviewed changes

tests/nativeapp/test_package_scripts.py Outdated Show resolved Hide resolved

sfc-gh-pjafari added 5 commits October 15, 2024 13:56

udpate test assert

429df76

add test coverage for sqlfacade

75ae306

update unknownsqlerror type, don't pass in sql_executor

fb084f5

Merge branch 'mchok-improve-ProgrammingError' into pj-user-script-ref…

deee3d3

…actor

update test

bb0d83a

sfc-gh-pjafari closed this Oct 16, 2024

sfc-gh-pjafari deleted the pj-user-script-refactor branch October 23, 2024 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Execute user script refactor #1695

Execute user script refactor #1695

sfc-gh-pjafari commented Oct 9, 2024 •

edited

Loading

sfc-gh-fcampbell left a comment

sfc-gh-bdufour Oct 15, 2024

sfc-gh-pjafari Oct 15, 2024

sfc-gh-bdufour Oct 15, 2024

sfc-gh-pjafari Oct 15, 2024

sfc-gh-bdufour Oct 15, 2024

sfc-gh-pjafari Oct 15, 2024

sfc-gh-bdufour Oct 15, 2024

sfc-gh-bdufour Oct 15, 2024

sfc-gh-pjafari Oct 15, 2024

sfc-gh-bdufour Oct 15, 2024

sfc-gh-pjafari Oct 15, 2024

sfc-gh-bdufour Oct 15, 2024

sfc-gh-pjafari Oct 15, 2024

sfc-gh-bdufour Oct 15, 2024

sfc-gh-pjafari Oct 15, 2024

sfc-gh-pjafari commented Oct 16, 2024

		# TODO: Add more test coverage + define contract
		# TODO: Add test for non-identifier names

Execute user script refactor #1695

Execute user script refactor #1695

Conversation

sfc-gh-pjafari commented Oct 9, 2024 • edited Loading

Pre-review checklist

Changes description

sfc-gh-fcampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-pjafari commented Oct 16, 2024

sfc-gh-pjafari commented Oct 9, 2024 •

edited

Loading