SNOW-1621834 Cast version to identifier when creating/dropping app ve…

…rsions (#1475)

When running `snow app version create` and `snow app version drop`, wrap the version in `to_identifier()` so users don't have to specify the quotes around version names that aren't valid identifiers. If the name is already quoted, `to_identifier()` doesn't do anything.

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