Merge branch 'main' into snowpark-clean-up-1

RELEASE-NOTES.md

@@ -42,6 +42,7 @@
 * Fix the typo in spcs service name argument description. It is the identifier of the **service** instead of the **service pool**.
 * Fix error handling and improve messaging when no artifacts provided.
 * Improved error message for incompatible parameters.
+* Fixed SQL error when running `snow app version create` and `snow app version drop` with a version name that isn't a valid Snowflake unquoted identifier
 
 
 # v2.8.0

src/snowflake/cli/_plugins/nativeapp/version/version_processor.py

@@ -39,10 +39,9 @@
 from snowflake.cli.api.console import cli_console as cc
 from snowflake.cli.api.exceptions import SnowflakeSQLExecutionError
 from snowflake.cli.api.project.schemas.native_app.native_app import NativeApp
-from snowflake.cli.api.project.util import unquote_identifier
+from snowflake.cli.api.project.util import to_identifier, unquote_identifier
 from snowflake.cli.api.utils.cursor import (
     find_all_rows,
-    find_first_row,
 )
 from snowflake.connector import ProgrammingError
 from snowflake.connector.cursor import DictCursor
@@ -121,6 +120,8 @@ def add_new_version(self, version: str) -> None:
         """
         Defines a new version in an existing application package.
         """
+        # Make the version a valid identifier, adding quotes if necessary
+        version = to_identifier(version)
         with self.use_role(self.package_role):
             cc.step(
                 f"Defining a new version {version} in application package {self.package_name}"
@@ -141,6 +142,8 @@ def add_new_patch_to_version(self, version: str, patch: Optional[int] = None):
         """
         Add a new patch, optionally a custom one, to an existing version in an application package.
         """
+        # Make the version a valid identifier, adding quotes if necessary
+        version = to_identifier(version)
         with self.use_role(self.package_role):
             cc.step(
                 f"Adding new patch to version {version} defined in application package {self.package_name}"
@@ -156,11 +159,7 @@ def add_new_patch_to_version(self, version: str, patch: Optional[int] = None):
                 add_version_query, cursor_class=DictCursor
             )
 
-            show_row = find_first_row(
-                result_cursor,
-                lambda row: row[VERSION_COL] == unquote_identifier(version),
-            )
-
+            show_row = result_cursor.fetchall()[0]
             new_patch = show_row["patch"]
             cc.message(
                 f"Patch {new_patch} created for version {version} defined in application package {self.package_name}."
@@ -328,6 +327,9 @@ def process(
                     "Manifest.yml file does not contain a value for the version field."
                 )
 
+        # Make the version a valid identifier, adding quotes if necessary
+        version = to_identifier(version)
+
         cc.step(
             dedent(
                 f"""\

tests/nativeapp/test_version_create_processor.py

@@ -105,8 +105,11 @@ def test_get_existing_release_direction_info(mock_execute, temp_dir, mock_cursor
 
 # Test add_new_version adds a new version to an app pkg correctly
 @mock.patch(NATIVEAPP_MANAGER_EXECUTE)
-def test_add_version(mock_execute, temp_dir, mock_cursor):
-    version = "V1"
+@pytest.mark.parametrize(
+    ["version", "version_identifier"],
+    [("V1", "V1"), ("1.0.0", '"1.0.0"'), ('"1.0.0"', '"1.0.0"')],
+)
+def test_add_version(mock_execute, temp_dir, mock_cursor, version, version_identifier):
     side_effects, expected = mock_execute_helper(
         [
             (
@@ -120,7 +123,7 @@ def test_add_version(mock_execute, temp_dir, mock_cursor):
                     dedent(
                         f"""\
                         alter application package app_pkg
-                            add version V1
+                            add version {version_identifier}
                             using @app_pkg.app_src.stage
                     """
                     ),
@@ -146,8 +149,13 @@ def test_add_version(mock_execute, temp_dir, mock_cursor):
 
 # Test add_new_patch_to_version adds an "auto-increment" patch to an existing version
 @mock.patch(NATIVEAPP_MANAGER_EXECUTE)
-def test_add_new_patch_auto(mock_execute, temp_dir, mock_cursor):
-    version = "V1"
+@pytest.mark.parametrize(
+    ["version", "version_identifier"],
+    [("V1", "V1"), ("1.0.0", '"1.0.0"'), ('"1.0.0"', '"1.0.0"')],
+)
+def test_add_new_patch_auto(
+    mock_execute, temp_dir, mock_cursor, version, version_identifier
+):
     side_effects, expected = mock_execute_helper(
         [
             (
@@ -161,7 +169,7 @@ def test_add_new_patch_auto(mock_execute, temp_dir, mock_cursor):
                     dedent(
                         f"""\
                         alter application package app_pkg
-                            add patch  for version V1
+                            add patch  for version {version_identifier}
                             using @app_pkg.app_src.stage
                     """
                     ),
@@ -187,8 +195,13 @@ def test_add_new_patch_auto(mock_execute, temp_dir, mock_cursor):
 
 # Test add_new_patch_to_version adds a custom patch to an existing version
 @mock.patch(NATIVEAPP_MANAGER_EXECUTE)
-def test_add_new_patch_custom(mock_execute, temp_dir, mock_cursor):
-    version = "V1"
+@pytest.mark.parametrize(
+    ["version", "version_identifier"],
+    [("V1", "V1"), ("1.0.0", '"1.0.0"'), ('"1.0.0"', '"1.0.0"')],
+)
+def test_add_new_patch_custom(
+    mock_execute, temp_dir, mock_cursor, version, version_identifier
+):
     side_effects, expected = mock_execute_helper(
         [
             (
@@ -202,7 +215,7 @@ def test_add_new_patch_custom(mock_execute, temp_dir, mock_cursor):
                     dedent(
                         f"""\
                         alter application package app_pkg
-                            add patch 12 for version V1
+                            add patch 12 for version {version_identifier}
                             using @app_pkg.app_src.stage
                     """
                     ),
@@ -222,7 +235,7 @@ def test_add_new_patch_custom(mock_execute, temp_dir, mock_cursor):
     )
 
     processor = _get_version_create_processor()
-    processor.add_new_patch_to_version(version, "12")
+    processor.add_new_patch_to_version(version, 12)
     assert mock_execute.mock_calls == expected
 
 
@@ -282,7 +295,7 @@ def test_process_no_version_exists_throws_bad_option_exception_one(
         processor.process(
             bundle_map=mock_bundle_map,
             version="v1",
-            patch="12",
+            patch=12,
             policy=policy_param,
             git_policy=policy_param,
             is_interactive=False,
@@ -315,7 +328,7 @@ def test_process_no_version_exists_throws_bad_option_exception_two(
         processor.process(
             bundle_map=mock_bundle_map,
             version="v1",
-            patch="12",
+            patch=12,
             policy=policy_param,
             git_policy=policy_param,
             is_interactive=False,

tests/nativeapp/test_version_drop_processor.py

@@ -96,13 +96,13 @@ def test_process_has_no_existing_app_pkg(mock_get_existing, policy_param, temp_d
 def test_process_no_version_from_user_no_version_in_manifest(
     mock_version_info_in_manifest,
     mock_build_bundle,
-    mock_mismatch,
+    mock_distribution,
     mock_get_existing,
     policy_param,
     temp_dir,
 ):
 
-    mock_mismatch.return_Value = "internal"
+    mock_distribution.return_value = "internal"
     current_working_directory = os.getcwd()
     create_named_file(
         file_name="snowflake.yml",
@@ -144,15 +144,15 @@ def test_process_drop_cannot_complete(
     mock_typer_confirm,
     mock_version_info_in_manifest,
     mock_build_bundle,
-    mock_mismatch,
+    mock_distribution,
     mock_get_existing,
     policy_param,
     is_interactive_param,
     expected_code,
     temp_dir,
 ):
 
-    mock_mismatch.return_value = "internal"
+    mock_distribution.return_value = "internal"
     current_working_directory = os.getcwd()
     create_named_file(
         file_name="snowflake.yml",
@@ -190,20 +190,20 @@ def test_process_drop_cannot_complete(
         (ask_always_policy, True),
     ],
 )
-def test_process_drop_success(
+def test_process_drop_from_manifest(
     mock_typer_confirm,
     mock_execute,
     mock_version_info_in_manifest,
     mock_build_bundle,
-    mock_mismatch,
+    mock_distribution,
     mock_get_existing,
     policy_param,
     is_interactive_param,
     temp_dir,
     mock_cursor,
 ):
 
-    mock_mismatch.return_value = "internal"
+    mock_distribution.return_value = "internal"
     side_effects, expected = mock_execute_helper(
         [
             (
@@ -234,3 +234,72 @@ def test_process_drop_success(
         version=None, policy=policy_param, is_interactive=is_interactive_param
     )
     assert mock_execute.mock_calls == expected
+
+
+@mock.patch(
+    f"{VERSION_MODULE}.{DROP_PROCESSOR}.get_existing_app_pkg_info",
+    return_value={"owner": "package_role"},
+)
+@mock_get_app_pkg_distribution_in_sf()
+@mock.patch(f"{VERSION_MODULE}.{DROP_PROCESSOR}.build_bundle", return_value=None)
+@mock.patch(NATIVEAPP_MANAGER_EXECUTE)
+@mock.patch(
+    f"snowflake.cli._plugins.nativeapp.policy.{TYPER_CONFIRM}", return_value=True
+)
+@pytest.mark.parametrize(
+    "policy_param, is_interactive_param",
+    [
+        (allow_always_policy, False),
+        (ask_always_policy, True),
+        (ask_always_policy, True),
+    ],
+)
+@pytest.mark.parametrize(
+    ["version", "version_identifier"],
+    [("V1", "V1"), ("1.0.0", '"1.0.0"'), ('"1.0.0"', '"1.0.0"')],
+)
+def test_process_drop_specific_version(
+    mock_typer_confirm,
+    mock_execute,
+    mock_build_bundle,
+    mock_distribution,
+    mock_get_existing,
+    policy_param,
+    is_interactive_param,
+    temp_dir,
+    mock_cursor,
+    version,
+    version_identifier,
+):
+
+    mock_distribution.return_value = "internal"
+    side_effects, expected = mock_execute_helper(
+        [
+            (
+                mock_cursor([{"CURRENT_ROLE()": "old_role"}], []),
+                mock.call("select current_role()", cursor_class=DictCursor),
+            ),
+            (None, mock.call("use role package_role")),
+            (
+                None,
+                mock.call(
+                    f"alter application package app_pkg drop version {version_identifier}"
+                ),
+            ),
+            (None, mock.call("use role old_role")),
+        ]
+    )
+    mock_execute.side_effect = side_effects
+
+    current_working_directory = os.getcwd()
+    create_named_file(
+        file_name="snowflake.yml",
+        dir_name=current_working_directory,
+        contents=[mock_snowflake_yml_file],
+    )
+
+    processor = _get_version_drop_processor()
+    processor.process(
+        version=version, policy=policy_param, is_interactive=is_interactive_param
+    )
+    assert mock_execute.mock_calls == expected

tests_integration/nativeapp/conftest.py

@@ -0,0 +1,63 @@
+from __future__ import annotations
+
+from contextlib import contextmanager
+from pathlib import Path
+from typing import Any
+
+import pytest
+
+from tests_integration.conftest import SnowCLIRunner
+
+
+@pytest.fixture
+def nativeapp_project_directory(project_directory, nativeapp_teardown):
+    """Wrapper around the project_directory fixture specific to Native App testing.
+
+    This fixture provides a context manager that does the following:
+    - Automatically calls `snow app teardown` before exiting
+
+    Parameters for the returned context manager:
+    :param name: The name of the directory in tests_integration/test_data/projects to use.
+    """
+
+    @contextmanager
+    def _nativeapp_project_directory(name):
+        with project_directory(name) as d:
+            with nativeapp_teardown(project_dir=d):
+                yield d
+
+    return _nativeapp_project_directory
+
+
+@pytest.fixture
+def nativeapp_teardown(runner: SnowCLIRunner):
+    """Runs `snow app teardown` before exiting.
+
+    This fixture provides a context manager that runs
+    `snow app teardown --force --cascade` before exiting,
+    regardless of any exceptions raised.
+
+    Parameters for the returned context manager:
+    :param project_dir: Path to the project directory (optional)
+    :param env: Environment variables to replace os.environ (optional)
+    """
+
+    @contextmanager
+    def _nativeapp_teardown(
+        *,
+        project_dir: Path | None = None,
+        env: dict | None = None,
+    ):
+        try:
+            yield
+        finally:
+            args = ["--force", "--cascade"]
+            if project_dir:
+                args += ["--project", str(project_dir)]
+            kwargs: dict[str, Any] = {}
+            if env:
+                kwargs["env"] = env
+            result = runner.invoke_with_connection(["app", "teardown", *args], **kwargs)
+            assert result.exit_code == 0
+
+    return _nativeapp_teardown

tests_integration/nativeapp/test_bundle.py

@@ -24,10 +24,10 @@
 
 
 @pytest.fixture(scope="function", params=["napp_init_v1", "napp_init_v2"])
-def template_setup(runner, project_directory, request):
+def template_setup(runner, nativeapp_project_directory, request):
     test_project = request.param
     with enable_definition_v2_feature_flag:
-        with project_directory(test_project) as project_root:
+        with nativeapp_project_directory(test_project) as project_root:
             # Vanilla bundle on the unmodified template
             result = runner.invoke_json(["app", "bundle"])
             assert result.exit_code == 0

tests_integration/nativeapp/test_debug_mode.py

@@ -90,6 +90,7 @@ def test_nativeapp_controlled_debug_mode(
     snowflake_session,
     default_username,
     resource_suffix,
+    nativeapp_teardown,
     project_definition_files: List[Path],
 ):
     project_name = "integration"
@@ -110,7 +111,7 @@ def test_nativeapp_controlled_debug_mode(
         result = runner.invoke_with_connection_json(["app", "run"])
         assert result.exit_code == 0
 
-        try:
+        with nativeapp_teardown():
             # debug mode should be true by default on first app deploy,
             # because snowflake.yml doesn't set it explicitly either way ("uncontrolled")
             assert is_debug_mode(snowflake_session, app_name)
@@ -135,12 +136,3 @@ def test_nativeapp_controlled_debug_mode(
             result = runner.invoke_with_connection_json(["app", "run"])
             assert result.exit_code == 0
             assert is_debug_mode(snowflake_session, app_name)
-
-            # make sure we always delete the app
-            result = runner.invoke_with_connection_json(["app", "teardown"])
-            assert result.exit_code == 0
-
-        finally:
-            # teardown is idempotent, so we can execute it again with no ill effects
-            result = runner.invoke_with_connection_json(["app", "teardown", "--force"])
-            assert result.exit_code == 0