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

sfc-gh-mchok · 2024-11-19T02:16:50Z

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

Move sql calls that execute setup scripts (CREATE APPLICATION and ALTER APPLICATION UPGRADE) to the SQL Facade, and reclassify all ProgrammingErrors coming out of these calls as UserInputErrors
Update tests that use these calls to mock the sql facade

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

src/snowflake/cli/_plugins/nativeapp/entities/application.py

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

src/snowflake/cli/_plugins/nativeapp/entities/application.py

sfc-gh-melnacouzi · 2024-11-21T14:06:05Z

tests/nativeapp/test_event_sharing.py

+        *mock_sql_facade_create_application.mock_calls,
+    ] == expected
+
+    mock_console.warning.assert_called_once_with(


why is application creation a warning? or has it always been the case?

I believe it's always been the case if we look here, but we've always had the mock return None (so that the warning isn't called), which I don't think reflects reality, so I updated the tests for that

sfc-gh-melnacouzi · 2024-11-21T14:08:19Z

tests/nativeapp/test_event_sharing.py

+            DEFAULT_UPGRADE_SUCCESS_MESSAGE
+            if is_upgrade
+            else DEFAULT_CREATE_SUCCESS_MESSAGE


for simplicity, you can have the same message since this is within the same test. We already check the mocks for upgrade/create, so it's hard to mess this up.

For sure, updated here: 17f761b

sfc-gh-pjafari

stil reading :D

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

src/snowflake/cli/_plugins/nativeapp/entities/application.py

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

…n_processor

…proper identifier when number bug, pending event sharing unit tests

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

src/snowflake/cli/_plugins/nativeapp/entities/application.py

sfc-gh-pjafari · 2024-11-26T14:42:52Z

src/snowflake/cli/_plugins/nativeapp/entities/application.py

        with sql_executor.use_role(self.role):
            event_sharing = EventSharingHandler(
-                telemetry_definition=model.telemetry,
+                telemetry_definition=self._entity_model.telemetry,
                deploy_root=package.deploy_root,
                install_method=install_method,
-                console=console,
+                console=self._workspace_ctx.console,
            )


So about this use_role block :D
instantiating an EventSharingHandler shouldn't need to be in a use_role block. If it needs to be done with a specific role, pass the role in.

Looking at implementation of get_existing_app_info, it seems that it could be moved as a whole to the facade. Just make sure you pass an optional role in, similar to other methods in the facade.

For _upgrade_app and _create_app, you're already doing role handling in them.

It seems that this use_role is trivial now upon changes above and can be removed.

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

sfc-gh-pjafari · 2024-11-26T14:51:34Z

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

+# 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,
+}
+


You can move these to sf_facade_exceptions file :)

sfc-gh-pjafari · 2024-11-26T15:02:53Z

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

+            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}.")


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

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

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.

…facade exceptions

sfc-gh-melnacouzi · 2024-11-26T18:37:12Z

src/snowflake/cli/_plugins/nativeapp/entities/application.py

                # 3. If existing application is found, perform a few validations and upgrade the application object.
+                create_or_upgrade_result = None


if we remove the self._upgrade_app() and _create_app() functions (since they do not do much), would this logic become clearer?

I think it's ok as it is now - since create and upgrade do different things, e.g. create needs permission grants and upgrade has specific error handling. Either way I don't have strong feelings about it :D

sfc-gh-melnacouzi · 2024-11-26T18:41:39Z

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

+        """
+
+        if package_role != role:
+            self._grant_privileges_for_create_application(


i don't think this belongs here tbh - I feel this should be called outside of this function, since it's supposed to "create_application" with specific parameters (not grant stuff)

… docstring to match others

sfc-gh-mchok · 2024-11-29T22:26:39Z

src/snowflake/cli/_plugins/nativeapp/entities/application.py

+        sql_executor = get_sql_executor()
+        with sql_executor.use_role(self.role):


Just adding this line at the top and indenting the rest of the function, since the rest of the calls are to the SQL Facade now we don't have a top-level use role. I believe this doesn't need to use the app warehouse from my testing so I did not add it, but please let me know if I am mistaken

sfc-gh-mchok · 2024-12-02T13:54:09Z

src/snowflake/cli/api/constants.py

+    APPLICATION = ObjectNames("application", "application", "applications")
+    APPLICATION_PACKAGE = ObjectNames(
+        "application-package", "application package", "application packages"
+    )

    def __str__(self):
        """This makes using this Enum easier in formatted string"""
        return self.value.cli_name


 OBJECT_TO_NAMES = {o.value.cli_name: o.value for o in ObjectType}
-SUPPORTED_OBJECTS = sorted(OBJECT_TO_NAMES.keys())
+UNSUPPORTED_OBJECTS = {
+    ObjectType.APPLICATION.value.cli_name,
+    ObjectType.APPLICATION_PACKAGE.value.cli_name,
+}
+SUPPORTED_OBJECTS = sorted(OBJECT_TO_NAMES.keys() - UNSUPPORTED_OBJECTS)


I wanted to add application and application package as constants to support the new sql facade operations with a cleaner API but I did not want to have them be supported in commands like object list/drop/etc., unsure if there's a better way of handling this

sfc-gh-melnacouzi · 2024-12-02T15:25:13Z

src/snowflake/cli/_plugins/nativeapp/entities/application.py

-            return sql_executor.show_specific_object(
-                "applications", self.name, name_col=NAME_COL
-            )
+        return get_snowflake_facade().get_existing_app_info(self.name, self.role)


i am confused, why do we use facade version sometimes, and sometimes use the application one (self.get_existing_app_info)?

My mistake, I forgot to refactor the other places where we call that method, I just replaced them all now

sfc-gh-melnacouzi · 2024-12-02T15:26:02Z

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

@@ -503,6 +553,198 @@ def show_release_directives(
                )
            return cursor.fetchall()

+    def get_existing_app_info(self, name: str, role: str) -> dict | None:


why is this different than show_specific_object?

The goal was to also move show_specific_object calls to the facade eventually, this is one of those instances, see #1870 (comment) for initial context

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

…s cause by setup script execution (#1870)

sfc-gh-mchok commented Nov 20, 2024

View reviewed changes

src/snowflake/cli/_plugins/nativeapp/same_account_install_method.py Show resolved Hide resolved

sfc-gh-mchok commented Nov 20, 2024

View reviewed changes

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

sfc-gh-mchok changed the title ~~Mchok create or alter app to sqlfacade~~ move create/alter app upgrade calls to sql facade to reclassify errors cause by setup script execution Nov 20, 2024

sfc-gh-mchok marked this pull request as ready for review November 20, 2024 21:11

sfc-gh-mchok requested review from a team as code owners November 20, 2024 21:12

sfc-gh-melnacouzi reviewed Nov 21, 2024

View reviewed changes

sfc-gh-pjafari reviewed Nov 21, 2024

View reviewed changes

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

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

sfc-gh-mchok added 6 commits November 25, 2024 10:37

convert create/upgrade app calls to sqlfacade; TODO: refactor test_ru…

b5db358

…n_processor

migrate all tests to sql facade for create/upgrade

4f48b11

update event sharing tests for refactor

5c0d52d

merge success messages for tests

6595561

move error handling to facade

a71f4a9

further overhauls to create/upgrade to make uniform, fix version not …

f1f00f0

…proper identifier when number bug, pending event sharing unit tests

sfc-gh-mchok force-pushed the mchok-create-or-alter-app-to-sqlfacade branch from 23d0bfe to f1f00f0 Compare November 25, 2024 15:43

sfc-gh-mchok commented Nov 25, 2024

View reviewed changes

src/snowflake/cli/_plugins/nativeapp/same_account_install_method.py Show resolved Hide resolved

sfc-gh-mchok added 3 commits November 25, 2024 17:23

update event sharing tests

cab6e8b

remove unused mock arg

945544f

Merge branch 'main' into mchok-create-or-alter-app-to-sqlfacade

f345027

sfc-gh-pjafari reviewed Nov 26, 2024

View reviewed changes

sfc-gh-mchok added 2 commits November 26, 2024 11:18

extract app entity properties, move upgrade restriction codes to sql …

ef653ab

…facade exceptions

move grants for application to sf facade

da876f0

sfc-gh-melnacouzi reviewed Nov 26, 2024

View reviewed changes

sfc-gh-mchok added 2 commits November 26, 2024 13:54

move grant privileges call to app entity

34bf540

separate telemetry error check from rest of upgrade logic and rewrite…

cc7aeee

… docstring to match others

sfc-gh-melnacouzi previously approved these changes Nov 26, 2024

View reviewed changes

move get_existing_app_info to sql facade

8d4e35b

sfc-gh-mchok added 3 commits November 28, 2024 09:50

Merge branch 'main' into mchok-create-or-alter-app-to-sqlfacade

57b7f3d

unclassify user errors for get existing app info

6205d72

remove error codes to err on the side of caution

cef4337

sfc-gh-melnacouzi previously approved these changes Nov 28, 2024

View reviewed changes

sfc-gh-mchok added 2 commits November 28, 2024 14:31

Merge branch 'main' into mchok-create-or-alter-app-to-sqlfacade

8c2a475

tests for sql facade (1/3)

7639f2e

sfc-gh-mchok dismissed sfc-gh-melnacouzi’s stale review via 7639f2e November 28, 2024 19:42

refactor test_run_processor tests (2/3)

94e797f

sfc-gh-mchok commented Nov 29, 2024

View reviewed changes

sfc-gh-mchok added 4 commits November 30, 2024 17:48

refactor event sharing tests

1248c2b

add grant privileges to expected calls

d5159d7

Merge branch 'main' into mchok-create-or-alter-app-to-sqlfacade

92c20e0

add support exclusion for application( package) object types

0b4c0fd

sfc-gh-mchok commented Dec 2, 2024

View reviewed changes

sfc-gh-melnacouzi reviewed Dec 2, 2024

View reviewed changes

replace app method of get existing info with facade one

f2d4ff9

sfc-gh-melnacouzi previously approved these changes Dec 2, 2024

View reviewed changes

fix tests after replacing get existing app info with facade version

c9f090e

sfc-gh-mchok dismissed sfc-gh-melnacouzi’s stale review via c9f090e December 2, 2024 16:24

sfc-gh-melnacouzi previously approved these changes Dec 2, 2024

View reviewed changes

sfc-gh-pjafari reviewed Dec 2, 2024

View reviewed changes

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

use _same_identifier for comparison

0af4869

sfc-gh-mchok dismissed sfc-gh-melnacouzi’s stale review via 0af4869 December 2, 2024 21:43

sfc-gh-melnacouzi approved these changes Dec 3, 2024

View reviewed changes

sfc-gh-melnacouzi enabled auto-merge (squash) December 3, 2024 03:41

Merge branch 'main' into mchok-create-or-alter-app-to-sqlfacade

c051f5a

sfc-gh-jsikorski approved these changes Dec 3, 2024

View reviewed changes

sfc-gh-melnacouzi merged commit 6475e23 into main Dec 3, 2024
20 checks passed

sfc-gh-melnacouzi deleted the mchok-create-or-alter-app-to-sqlfacade branch December 3, 2024 15:30

sfc-gh-gbloom pushed a commit that referenced this pull request Dec 4, 2024

move create/alter app upgrade calls to sql facade to reclassify error…

e0df7d3

…s cause by setup script execution (#1870)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sfc-gh-mchok commented Nov 19, 2024 •

edited

Loading

sfc-gh-melnacouzi Nov 21, 2024

sfc-gh-mchok Nov 21, 2024 •

edited

Loading

sfc-gh-melnacouzi Nov 21, 2024

sfc-gh-mchok Nov 21, 2024

sfc-gh-pjafari left a comment

sfc-gh-pjafari Nov 26, 2024

sfc-gh-pjafari Nov 26, 2024 •

edited

Loading

sfc-gh-pjafari Nov 26, 2024

sfc-gh-mchok Nov 26, 2024

sfc-gh-melnacouzi Nov 26, 2024

sfc-gh-melnacouzi Nov 26, 2024

sfc-gh-pjafari Nov 26, 2024

sfc-gh-melnacouzi Nov 26, 2024

sfc-gh-mchok Nov 29, 2024 •

edited

Loading

sfc-gh-mchok Dec 2, 2024

sfc-gh-melnacouzi Dec 2, 2024

sfc-gh-mchok Dec 2, 2024

sfc-gh-melnacouzi Dec 2, 2024

sfc-gh-mchok Dec 2, 2024

		# 3. If existing application is found, perform a few validations and upgrade the application object.
		create_or_upgrade_result = None

		sql_executor = get_sql_executor()
		with sql_executor.use_role(self.role):

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

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

Conversation

sfc-gh-mchok commented Nov 19, 2024 • edited Loading

Pre-review checklist

Changes description

Choose a reason for hiding this comment

sfc-gh-mchok Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-pjafari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-pjafari Nov 26, 2024 • edited Loading

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-mchok Nov 29, 2024 • edited Loading

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-mchok commented Nov 19, 2024 •

edited

Loading

sfc-gh-mchok Nov 21, 2024 •

edited

Loading

sfc-gh-pjafari Nov 26, 2024 •

edited

Loading

sfc-gh-mchok Nov 29, 2024 •

edited

Loading