-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Execute user script refactor #1695
Closed
sfc-gh-pjafari
wants to merge
17
commits into
mchok-improve-ProgrammingError
from
pj-user-script-refactor
Closed
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
964117e
first commit - make contract
sfc-gh-pjafari 15a123a
rewrite tests
sfc-gh-pjafari e5f3a98
remove comment
sfc-gh-pjafari a56fe0e
add error handling
sfc-gh-pjafari 7fd543d
move and rename class
sfc-gh-pjafari 1974494
re-wrote tests
sfc-gh-pjafari 1ae721a
add exit_code check to test
sfc-gh-pjafari 76d1709
add happy path test for execute_user_script
sfc-gh-pjafari 6939f73
add todo comment to add tests
sfc-gh-pjafari ddf215c
add docstrings
sfc-gh-pjafari 412467a
remove comment
sfc-gh-pjafari cf14589
error handle all use calls
sfc-gh-pjafari 429df76
udpate test assert
sfc-gh-pjafari 75ae306
add test coverage for sqlfacade
sfc-gh-pjafari fb084f5
update unknownsqlerror type, don't pass in sql_executor
sfc-gh-pjafari deee3d3
Merge branch 'mchok-improve-ProgrammingError' into pj-user-script-ref…
sfc-gh-pjafari bb0d83a
update test
sfc-gh-pjafari File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
from snowflake.cli._plugins.nativeapp.sf_sql_facade import SnowflakeSQLFacade | ||
|
||
|
||
def get_snowflake_facade() -> SnowflakeSQLFacade: | ||
"""Returns a Snowflake Facade""" | ||
return SnowflakeSQLFacade() |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,204 @@ | ||
# Copyright (c) 2024 Snowflake Inc. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
import logging | ||
from contextlib import contextmanager | ||
|
||
from click import ClickException | ||
from cryptography.utils import cached_property | ||
from snowflake.cli.api.constants import ObjectType | ||
from snowflake.cli.api.entities.common import get_sql_executor | ||
from snowflake.cli.api.errno import ( | ||
DOES_NOT_EXIST_OR_CANNOT_BE_PERFORMED, | ||
NO_WAREHOUSE_SELECTED_IN_SESSION, | ||
) | ||
from snowflake.cli.api.exceptions import CouldNotUseObjectError | ||
from snowflake.cli.api.project.util import to_identifier | ||
from snowflake.cli.api.sql_execution import SqlExecutor | ||
from snowflake.connector import DatabaseError, ProgrammingError | ||
|
||
|
||
class UnknownSQLError(DatabaseError): | ||
"""Exception raised when the root of the SQL error is unidentified by us.""" | ||
|
||
exit_code = 3 | ||
|
||
def __init__(self, msg): | ||
self.msg = f"Unknown SQL error occurred. {msg}" | ||
super().__init__(self.msg) | ||
|
||
def __str__(self): | ||
return self.msg | ||
|
||
|
||
class UserScriptError(ClickException): | ||
"""Exception raised when user-provided scripts fail.""" | ||
|
||
def __init__(self, script_name, msg): | ||
super().__init__(f"Failed to run script {script_name}. {msg}") | ||
|
||
|
||
class SnowflakeSQLFacade: | ||
def __init__(self, sql_executor: SqlExecutor | None = None): | ||
self._sql_executor = ( | ||
sql_executor if sql_executor is not None else get_sql_executor() | ||
) | ||
|
||
@cached_property | ||
def _log(self): | ||
return logging.getLogger(__name__) | ||
|
||
def _use_object(self, object_type: ObjectType, name: str): | ||
""" | ||
Call sql to use snowflake object with error handling | ||
@param object_type: ObjectType, type of snowflake object to use | ||
@param name: object name, has to be a valid snowflake identifier. | ||
""" | ||
try: | ||
self._sql_executor.execute_query(f"use {object_type} {name}") | ||
except ProgrammingError as err: | ||
if err.errno == DOES_NOT_EXIST_OR_CANNOT_BE_PERFORMED: | ||
raise CouldNotUseObjectError(object_type, name) from err | ||
|
||
raise ProgrammingError(f"Failed to use {object_type} {name}") from err | ||
except Exception as err: | ||
raise UnknownSQLError(f"Failed to use {object_type} {name}") from err | ||
|
||
@contextmanager | ||
def _use_warehouse_optional(self, new_wh: str | None): | ||
""" | ||
Switches to a different warehouse for a while, then switches back. | ||
This is a no-op if the requested warehouse is already active or if no warehouse is passed in. | ||
@param new_wh: Name of the warehouse to use. If not a valid Snowflake identifier, will be converted before use. | ||
""" | ||
if new_wh is None: | ||
yield | ||
return | ||
|
||
valid_wh_name = to_identifier(new_wh) | ||
|
||
wh_result = self._sql_executor.execute_query( | ||
f"select current_warehouse()" | ||
).fetchone() | ||
|
||
# If user has an assigned default warehouse, prev_wh will contain a value even if the warehouse is suspended. | ||
try: | ||
prev_wh = wh_result[0] | ||
except: | ||
prev_wh = None | ||
# new_wh is not None, and should already be a valid identifier, no additional check is performed here. | ||
is_different_wh = valid_wh_name != prev_wh | ||
if is_different_wh: | ||
self._log.debug(f"Using warehouse: {valid_wh_name}") | ||
self._use_object(ObjectType.WAREHOUSE, valid_wh_name) | ||
try: | ||
yield | ||
finally: | ||
if is_different_wh and prev_wh is not None: | ||
self._log.debug(f"Switching back to warehouse:{prev_wh}") | ||
self._use_object(ObjectType.WAREHOUSE, prev_wh) | ||
|
||
@contextmanager | ||
def _use_role_optional(self, new_role: str | None): | ||
""" | ||
Switches to a different role for a while, then switches back. | ||
This is a no-op if the requested role is already active or if no role is passed in. | ||
@param new_role: Name of the role to use. If not a valid Snowflake identifier, will be converted before use. | ||
""" | ||
if new_role is None: | ||
yield | ||
return | ||
|
||
valid_role_name = to_identifier(new_role) | ||
|
||
prev_role = self._sql_executor.current_role() | ||
|
||
is_different_role = valid_role_name.lower() != prev_role.lower() | ||
if is_different_role: | ||
self._log.debug(f"Assuming different role: {valid_role_name}") | ||
self._use_object(ObjectType.ROLE, valid_role_name) | ||
try: | ||
yield | ||
finally: | ||
if is_different_role: | ||
self._log.debug(f"Switching back to role:{prev_role}") | ||
self._use_object(ObjectType.ROLE, prev_role) | ||
|
||
@contextmanager | ||
def _use_database_optional(self, database_name: str | None): | ||
""" | ||
Switch to database `database_name`, then switches back. | ||
This is a no-op if the requested database is already selected or if no database_name is passed in. | ||
@param database_name: Name of the database to use. If not a valid Snowflake identifier, will be converted before use. | ||
""" | ||
|
||
if database_name is None: | ||
yield | ||
return | ||
|
||
valid_name = to_identifier(database_name) | ||
|
||
db_result = self._sql_executor.execute_query( | ||
f"select current_database()" | ||
).fetchone() | ||
try: | ||
prev_db = db_result[0] | ||
except: | ||
prev_db = None | ||
|
||
is_different_db = valid_name != prev_db | ||
if is_different_db: | ||
self._log.debug(f"Using database {valid_name}") | ||
self._use_object(ObjectType.DATABASE, valid_name) | ||
|
||
try: | ||
yield | ||
finally: | ||
if is_different_db and prev_db is not None: | ||
self._log.debug(f"Switching back to database:{prev_db}") | ||
self._use_object(ObjectType.DATABASE, prev_db) | ||
|
||
def execute_user_script( | ||
self, | ||
queries: str, | ||
script_name: str, | ||
role: str | None = None, | ||
warehouse: str | None = None, | ||
database: str | None = None, | ||
): | ||
""" | ||
Runs the user-provided sql script. | ||
@param queries: Queries to run in this script | ||
@param script_name: Name of the file containing the script. Used to show logs to the user. | ||
@param [Optional] role: Role to switch to while running this script. Current role will be used if no role is passed in. | ||
@param [Optional] warehouse: Warehouse to use while running this script. | ||
@param [Optional] database: Database to use while running this script. | ||
""" | ||
with self._use_role_optional(role): | ||
with self._use_warehouse_optional(warehouse): | ||
with self._use_database_optional(database): | ||
try: | ||
self._sql_executor.execute_queries(queries) | ||
except ProgrammingError as err: | ||
if err.errno == NO_WAREHOUSE_SELECTED_IN_SESSION: | ||
raise UserScriptError( | ||
script_name, | ||
f"{err.msg}. Please provide a warehouse in your project definition file, config.toml file, or via command line", | ||
) from err | ||
else: | ||
raise UserScriptError(script_name, err.msg) from err | ||
except Exception as err: | ||
raise UnknownSQLError( | ||
f"Failed to run script {script_name}" | ||
) from err |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.