Skip to content
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

move create/alter app upgrade calls to sql facade to reclassify errors cause by setup script execution #1870

Merged
merged 29 commits into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
b5db358
convert create/upgrade app calls to sqlfacade; TODO: refactor test_ru…
sfc-gh-mchok Nov 15, 2024
4f48b11
migrate all tests to sql facade for create/upgrade
sfc-gh-mchok Nov 19, 2024
5c0d52d
update event sharing tests for refactor
sfc-gh-mchok Nov 20, 2024
6595561
merge success messages for tests
sfc-gh-mchok Nov 21, 2024
a71f4a9
move error handling to facade
sfc-gh-mchok Nov 21, 2024
f1f00f0
further overhauls to create/upgrade to make uniform, fix version not …
sfc-gh-mchok Nov 22, 2024
cab6e8b
update event sharing tests
sfc-gh-mchok Nov 25, 2024
945544f
remove unused mock arg
sfc-gh-mchok Nov 25, 2024
f345027
Merge branch 'main' into mchok-create-or-alter-app-to-sqlfacade
sfc-gh-mchok Nov 26, 2024
ef653ab
extract app entity properties, move upgrade restriction codes to sql …
sfc-gh-mchok Nov 26, 2024
da876f0
move grants for application to sf facade
sfc-gh-mchok Nov 26, 2024
34bf540
move grant privileges call to app entity
sfc-gh-mchok Nov 26, 2024
cc7aeee
separate telemetry error check from rest of upgrade logic and rewrite…
sfc-gh-mchok Nov 26, 2024
8d4e35b
move get_existing_app_info to sql facade
sfc-gh-mchok Nov 26, 2024
52af246
change error handling to be default our fault, add list of error code…
sfc-gh-mchok Nov 27, 2024
57b7f3d
Merge branch 'main' into mchok-create-or-alter-app-to-sqlfacade
sfc-gh-mchok Nov 28, 2024
6205d72
unclassify user errors for get existing app info
sfc-gh-mchok Nov 28, 2024
cef4337
remove error codes to err on the side of caution
sfc-gh-mchok Nov 28, 2024
8c2a475
Merge branch 'main' into mchok-create-or-alter-app-to-sqlfacade
sfc-gh-mchok Nov 28, 2024
7639f2e
tests for sql facade (1/3)
sfc-gh-mchok Nov 28, 2024
94e797f
refactor test_run_processor tests (2/3)
sfc-gh-mchok Nov 29, 2024
1248c2b
refactor event sharing tests
sfc-gh-mchok Nov 30, 2024
d5159d7
add grant privileges to expected calls
sfc-gh-mchok Nov 30, 2024
92c20e0
Merge branch 'main' into mchok-create-or-alter-app-to-sqlfacade
sfc-gh-mchok Dec 2, 2024
0b4c0fd
add support exclusion for application( package) object types
sfc-gh-mchok Dec 2, 2024
f2d4ff9
replace app method of get existing info with facade one
sfc-gh-mchok Dec 2, 2024
c9f090e
fix tests after replacing get existing app info with facade version
sfc-gh-mchok Dec 2, 2024
0af4869
use _same_identifier for comparison
sfc-gh-mchok Dec 2, 2024
c051f5a
Merge branch 'main' into mchok-create-or-alter-app-to-sqlfacade
sfc-gh-melnacouzi Dec 3, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
298 changes: 107 additions & 191 deletions src/snowflake/cli/_plugins/nativeapp/entities/application.py

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from dataclasses import dataclass
from typing import Optional

from snowflake.cli._plugins.nativeapp.constants import (
Expand All @@ -8,25 +9,15 @@
ApplicationCreatedExternallyError,
)
from snowflake.cli._plugins.stage.manager import StageManager
from snowflake.cli.api.project.util import to_identifier


@dataclass
sfc-gh-mchok marked this conversation as resolved.
Show resolved Hide resolved
class SameAccountInstallMethod:
_requires_created_by_cli: bool
_from_release_directive: bool
version: Optional[str]
patch: Optional[int]

def __init__(
self,
requires_created_by_cli: bool,
version: Optional[str] = None,
patch: Optional[int] = None,
from_release_directive: bool = False,
):
self._requires_created_by_cli = requires_created_by_cli
self.version = version
self.patch = patch
self._from_release_directive = from_release_directive
version: Optional[str] = None
patch: Optional[int] = None
_from_release_directive: bool = False

sfc-gh-pjafari marked this conversation as resolved.
Show resolved Hide resolved
@classmethod
def unversioned_dev(cls):
Expand All @@ -39,7 +30,7 @@ def versioned_dev(cls, version: str, patch: Optional[int] = None):

@classmethod
def release_directive(cls):
return cls(False, from_release_directive=True)
return cls(False, _from_release_directive=True)

@property
def is_dev_mode(self) -> bool:
Expand All @@ -53,8 +44,9 @@ def using_clause(
return ""

if self.version:
version_clause = f"version {to_identifier(self.version)}"
sfc-gh-mchok marked this conversation as resolved.
Show resolved Hide resolved
patch_clause = f"patch {self.patch}" if self.patch else ""
return f"using version {self.version} {patch_clause}"
return f"using {version_clause} {patch_clause}"

stage_name = StageManager.quote_stage_name(stage_fqn)
return f"using {stage_name}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,12 @@ def __init__(
if role:
message += f" using role: {role}"
super().__init__(message)


class UpgradeApplicationRestrictionError(UserInputError):
"""
Raised when an alter application ... upgrade fails due to user error.
Must be caught and handled by the caller of an upgrade_application
"""

pass
159 changes: 159 additions & 0 deletions src/snowflake/cli/_plugins/nativeapp/sf_sql_facade.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,38 @@
from textwrap import dedent
from typing import Any, Dict, List

from snowflake.cli._plugins.nativeapp.constants import (
AUTHORIZE_TELEMETRY_COL,
SPECIAL_COMMENT,
)
from snowflake.cli._plugins.nativeapp.same_account_install_method import (
SameAccountInstallMethod,
)
from snowflake.cli._plugins.nativeapp.sf_facade_constants import UseObjectType
from snowflake.cli._plugins.nativeapp.sf_facade_exceptions import (
CouldNotUseObjectError,
InsufficientPrivilegesError,
UnexpectedResultError,
UpgradeApplicationRestrictionError,
UserInputError,
UserScriptError,
handle_unclassified_error,
)
from snowflake.cli.api.cli_global_context import get_cli_context
from snowflake.cli.api.errno import (
APPLICATION_NO_LONGER_AVAILABLE,
APPLICATION_REQUIRES_TELEMETRY_SHARING,
CANNOT_DISABLE_MANDATORY_TELEMETRY,
CANNOT_UPGRADE_FROM_LOOSE_FILES_TO_VERSION,
CANNOT_UPGRADE_FROM_VERSION_TO_LOOSE_FILES,
DOES_NOT_EXIST_OR_CANNOT_BE_PERFORMED,
INSUFFICIENT_PRIVILEGES,
NO_WAREHOUSE_SELECTED_IN_SESSION,
NOT_SUPPORTED_ON_DEV_MODE_APPLICATIONS,
ONLY_SUPPORTED_ON_DEV_MODE_APPLICATIONS,
)
from snowflake.cli.api.identifiers import FQN
from snowflake.cli.api.metrics import CLICounterField
from snowflake.cli.api.project.util import (
identifier_to_show_like_pattern,
is_valid_unquoted_identifier,
Expand All @@ -42,6 +60,15 @@
from snowflake.cli.api.sql_execution import BaseSqlExecutor, SqlExecutor
from snowflake.connector import DictCursor, ProgrammingError

# Reasons why an `alter application ... upgrade` might fail
UPGRADE_RESTRICTION_CODES = {
CANNOT_UPGRADE_FROM_LOOSE_FILES_TO_VERSION,
CANNOT_UPGRADE_FROM_VERSION_TO_LOOSE_FILES,
ONLY_SUPPORTED_ON_DEV_MODE_APPLICATIONS,
NOT_SUPPORTED_ON_DEV_MODE_APPLICATIONS,
APPLICATION_NO_LONGER_AVAILABLE,
}

Copy link
Contributor

@sfc-gh-pjafari sfc-gh-pjafari Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can move these to sf_facade_exceptions file :)


class SnowflakeSQLFacade:
def __init__(self, sql_executor: SqlExecutor | None = None):
Expand Down Expand Up @@ -503,6 +530,138 @@ def show_release_directives(
)
return cursor.fetchall()

def upgrade_application(
self,
name: str,
current_app_row: dict,
install_method: SameAccountInstallMethod,
stage_fqn: str,
debug_mode: bool | None,
should_authorize_event_sharing: bool | None,
role: str | None = None,
warehouse: str | None = None,
) -> list[tuple[str]]:
"""
Upgrades an application object using the provided clauses
"""
install_method.ensure_app_usable(
app_name=name,
app_role=role,
show_app_row=current_app_row,
)
# If all the above checks are in order, proceed to upgrade

with self._use_role_optional(role), self._use_warehouse_optional(warehouse):
try:
using_clause = install_method.using_clause(stage_fqn)
upgrade_cursor = self._sql_executor.execute_query(
f"alter application {name} upgrade {using_clause}",
)

# if debug_mode is present (controlled), ensure it is up-to-date
if install_method.is_dev_mode:
if debug_mode is not None:
self._sql_executor.execute_query(
f"alter application {name} set debug_mode = {debug_mode}"
)

# Only update event sharing if the current value is different as the one we want to set
if should_authorize_event_sharing is not None:
current_authorize_event_sharing = (
self.get_app_properties(name, role)
.get(AUTHORIZE_TELEMETRY_COL, "false")
.lower()
== "true"
)
if (
current_authorize_event_sharing
!= should_authorize_event_sharing
):
self._log.info(
"Setting telemetry sharing authorization to %s",
should_authorize_event_sharing,
)
self._sql_executor.execute_query(
f"alter application {name} set AUTHORIZE_TELEMETRY_EVENT_SHARING = {str(should_authorize_event_sharing).upper()}"
)
except ProgrammingError as err:
if err.errno in UPGRADE_RESTRICTION_CODES:
raise UpgradeApplicationRestrictionError(err.msg) from err
elif err.errno == CANNOT_DISABLE_MANDATORY_TELEMETRY:
get_cli_context().metrics.set_counter(
CLICounterField.EVENT_SHARING_ERROR, 1
)
raise UserInputError(
"Could not disable telemetry event sharing for the application because it contains mandatory events. Please set 'share_mandatory_events' to true in the application telemetry section of the project definition file."
) from err

raise UserInputError(
f"Failed to upgrade application {name} with the following error message:\n"
f"{err.msg}"
) from err
except Exception as err:
handle_unclassified_error(err, f"Failed to upgrade application {name}.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In create case, it makes sense that you have all the error handling in one block since you send one query with the clause.
Here however, you can separate your sql calls in try blocks of their own to make the error handling more readable.
You can also move the telemetry upgrade block into its own private method here, like _update_telemetry_authorization if you want

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I extracted the logic without creating a helper, I think it looks alright as it is right now, but might be weird with the error raising duplication, but i made the error messages more specialized. Let me know what you think

cc @sfc-gh-melnacouzi

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • separate error handling makes sense if there are no shared errors -might look ugly though if there is shared error handling that we need to repeat.
  • separating telemetry authorization is optional given that it is not used somewhere else yet.
    I think you can make the call on this @sfc-gh-mchok based on which one is more readable.

return upgrade_cursor.fetchall()

def create_application(
self,
name: str,
package_name: str,
install_method: SameAccountInstallMethod,
stage_fqn: str,
debug_mode: bool | None,
should_authorize_event_sharing: bool | None,
role: str | None = None,
warehouse: str | None = None,
) -> list[tuple[str]]:
"""
Creates a new application object using an application package,
running the setup script of the application package
"""
# by default, applications are created in debug mode when possible;
# this can be overridden in the project definition
debug_mode_clause = ""
if install_method.is_dev_mode:
initial_debug_mode = debug_mode if debug_mode is not None else True
debug_mode_clause = f"debug_mode = {initial_debug_mode}"

authorize_telemetry_clause = ""
if should_authorize_event_sharing is not None:
self._log.info(
"Setting AUTHORIZE_TELEMETRY_EVENT_SHARING to %s",
should_authorize_event_sharing,
)
authorize_telemetry_clause = f" AUTHORIZE_TELEMETRY_EVENT_SHARING = {str(should_authorize_event_sharing).upper()}"

using_clause = install_method.using_clause(stage_fqn)
with self._use_role_optional(role), self._use_warehouse_optional(warehouse):
try:
create_cursor = self._sql_executor.execute_query(
dedent(
f"""\
create application {name}
from application package {package_name} {using_clause} {debug_mode_clause}{authorize_telemetry_clause}
comment = {SPECIAL_COMMENT}
"""
),
)
except ProgrammingError as err:
if err.errno == APPLICATION_REQUIRES_TELEMETRY_SHARING:
get_cli_context().metrics.set_counter(
CLICounterField.EVENT_SHARING_ERROR, 1
)
raise UserInputError(
"The application package requires event sharing to be authorized. Please set 'share_mandatory_events' to true in the application telemetry section of the project definition file."
) from err

raise UserInputError(
f"Failed to create application {name} with the following error message:\n"
f"{err.msg}"
) from err
except Exception as err:
handle_unclassified_error(err, f"Failed to create application {name}.")
return create_cursor.fetchall()


# TODO move this to src/snowflake/cli/api/project/util.py in a separate
# PR since it's codeowned by the CLI team
Expand Down
9 changes: 3 additions & 6 deletions src/snowflake/cli/api/entities/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
)
from snowflake.cli.api.secure_path import UNLIMITED, SecurePath
from snowflake.connector import ProgrammingError
from snowflake.connector.cursor import SnowflakeCursor


def generic_sql_error_handler(err: ProgrammingError) -> NoReturn:
Expand Down Expand Up @@ -325,17 +324,15 @@ def drop_generic_object(
console.message(f"Dropped {object_type} {object_name} successfully.")


def print_messages(
console: AbstractConsole, create_or_upgrade_cursor: Optional[SnowflakeCursor]
):
def print_messages(console: AbstractConsole, cursor_results: list[tuple[str]]):
"""
Shows messages in the console returned by the CREATE or UPGRADE
APPLICATION command.
"""
if not create_or_upgrade_cursor:
if not cursor_results:
return

messages = [row[0] for row in create_or_upgrade_cursor.fetchall()]
messages = [row[0] for row in cursor_results]
for message in messages:
console.warning(message)
console.message("")
1 change: 1 addition & 0 deletions src/snowflake/cli/api/errno.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,4 @@
APPLICATION_OWNS_EXTERNAL_OBJECTS = 93128
APPLICATION_REQUIRES_TELEMETRY_SHARING = 93321
CANNOT_DISABLE_MANDATORY_TELEMETRY = 93329
APPLICATION_INSTANCE_FAILED_TO_RUN_SETUP_SCRIPT = 93082
Loading