From 7ed0a381d6e7323b95381261818086c08f754527 Mon Sep 17 00:00:00 2001 From: Jorge Arturo Vasquez Rojas Date: Wed, 14 Aug 2024 09:06:32 -0600 Subject: [PATCH] Common error for mutually exclusive flags (#1375) * Add exception and OverridableParamete * Add incompatible error message to app events command * Update error message to be more understandable * Add ignore type flag * Update test name * Remove unused imports --- RELEASE-NOTES.md | 2 + src/snowflake/cli/_plugins/cortex/commands.py | 81 +++++----- .../cli/_plugins/nativeapp/commands.py | 12 +- src/snowflake/cli/_plugins/object/commands.py | 5 +- src/snowflake/cli/_plugins/sql/commands.py | 30 ++-- src/snowflake/cli/api/commands/flags.py | 118 +-------------- .../api/commands/overrideable_parameter.py | 143 ++++++++++++++++++ src/snowflake/cli/api/exceptions.py | 11 +- tests/__snapshots__/test_help_messages.ambr | 4 +- tests/__snapshots__/test_sql.ambr | 4 +- .../commands/__snapshots__/test_flags.ambr | 10 +- tests/api/commands/test_flags.py | 4 +- tests/cortex/test_cortex_commands.py | 44 +++++- tests/object/test_object.py | 16 +- .../__snapshots__/test_image_repository.ambr | 5 +- tests/spcs/test_image_repository.py | 2 +- tests/test_sql.py | 21 ++- tests/testing_utils/result_assertions.py | 4 +- tests_integration/nativeapp/test_deploy.py | 8 +- tests_integration/nativeapp/test_events.py | 15 +- .../assertions/test_result_assertions.py | 9 ++ 21 files changed, 342 insertions(+), 206 deletions(-) create mode 100644 src/snowflake/cli/api/commands/overrideable_parameter.py diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md index d12d6e81d3..d0e8416f99 100644 --- a/RELEASE-NOTES.md +++ b/RELEASE-NOTES.md @@ -36,6 +36,8 @@ * Fixed problem with whitespaces in `snow connection add` command * Added check for the correctness of token file and private key paths when addind a connection * Fix the typo in spcs service name argument description. It is the identifier of the **service** instead of the **service pool**. +* Improved error message for incompatible parameters. + # v2.7.0 diff --git a/src/snowflake/cli/_plugins/cortex/commands.py b/src/snowflake/cli/_plugins/cortex/commands.py index d49d0a77a5..6d64c9ac2e 100644 --- a/src/snowflake/cli/_plugins/cortex/commands.py +++ b/src/snowflake/cli/_plugins/cortex/commands.py @@ -31,7 +31,10 @@ Text, ) from snowflake.cli.api.cli_global_context import get_cli_context -from snowflake.cli.api.commands.flags import readable_file_option +from snowflake.cli.api.commands.overrideable_parameter import ( + OverrideableArgument, + OverrideableOption, +) from snowflake.cli.api.commands.snow_typer import SnowTyperFactory from snowflake.cli.api.constants import PYTHON_3_12 from snowflake.cli.api.output.types import ( @@ -48,6 +51,25 @@ SEARCH_COMMAND_ENABLED = sys.version_info < PYTHON_3_12 +SOURCE_EXCLUSIVE_OPTION_NAMES = ["text", "file", "source_document_text"] + +# Creates a Typer option and verifies if the mutually exclusive options are set in the command. +ExclusiveReadableFileOption = OverrideableOption( + None, + "--file", + mutually_exclusive=SOURCE_EXCLUSIVE_OPTION_NAMES, + exists=True, + file_okay=True, + dir_okay=False, + readable=True, + show_default=False, +) + +# Creates a Typer argument and verifies if the mutually exclusive options are set in the command. +ExclusiveTextSourceArgument = OverrideableArgument( + mutually_exclusive=SOURCE_EXCLUSIVE_OPTION_NAMES, +) + @app.command( requires_connection=True, @@ -100,8 +122,8 @@ def search( requires_connection=True, ) def complete( - text: Optional[str] = typer.Argument( - None, + text: Optional[str] = ExclusiveTextSourceArgument( + default=None, help="Prompt to be used to generate a completion. Cannot be combined with --file option.", show_default=False, ), @@ -110,9 +132,8 @@ def complete( "--model", help="String specifying the model to be used.", ), - file: Optional[Path] = readable_file_option( - param_name="--file", - help_str="JSON file containing conversation history to be used to generate a completion. Cannot be combined with TEXT argument.", + file: Optional[Path] = ExclusiveReadableFileOption( + help="JSON file containing conversation history to be used to generate a completion. Cannot be combined with TEXT argument.", ), **options, ) -> CommandResult: @@ -124,8 +145,6 @@ def complete( manager = CortexManager() - if text and file: - raise UsageError("--file option cannot be used together with TEXT argument.") if text: result_text = manager.complete_for_prompt( text=Text(text), @@ -152,14 +171,13 @@ def extract_answer( help="String containing the question to be answered.", show_default=False, ), - source_document_text: Optional[str] = typer.Argument( - None, + source_document_text: Optional[str] = ExclusiveTextSourceArgument( + default=None, help="String containing the plain-text or JSON document that contains the answer to the question. Cannot be combined with --file option.", show_default=False, ), - file: Optional[Path] = readable_file_option( - param_name="--file", - help_str="File containing the plain-text or JSON document that contains the answer to the question. Cannot be combined with SOURCE_DOCUMENT_TEXT argument.", + file: Optional[Path] = ExclusiveReadableFileOption( + help="File containing the plain-text or JSON document that contains the answer to the question. Cannot be combined with SOURCE_DOCUMENT_TEXT argument.", ), **options, ) -> CommandResult: @@ -170,10 +188,6 @@ def extract_answer( manager = CortexManager() - if source_document_text and file: - raise UsageError( - "--file option cannot be used together with SOURCE_DOCUMENT_TEXT argument." - ) if source_document_text: result_text = manager.extract_answer_from_source_document( source_document=SourceDocument(source_document_text), @@ -197,14 +211,13 @@ def extract_answer( requires_connection=True, ) def sentiment( - text: Optional[str] = typer.Argument( - None, + text: Optional[str] = ExclusiveTextSourceArgument( + default=None, help="String containing the text for which a sentiment score should be calculated. Cannot be combined with --file option.", show_default=False, ), - file: Optional[Path] = readable_file_option( - param_name="--file", - help_str="File containing the text for which a sentiment score should be calculated. Cannot be combined with TEXT argument.", + file: Optional[Path] = ExclusiveReadableFileOption( + help="File containing the text for which a sentiment score should be calculated. Cannot be combined with TEXT argument.", ), **options, ) -> CommandResult: @@ -216,8 +229,6 @@ def sentiment( manager = CortexManager() - if text and file: - raise UsageError("--file option cannot be used together with TEXT argument.") if text: result_text = manager.calculate_sentiment_for_text( text=Text(text), @@ -237,14 +248,13 @@ def sentiment( requires_connection=True, ) def summarize( - text: Optional[str] = typer.Argument( - None, + text: Optional[str] = ExclusiveTextSourceArgument( + default=None, help="String containing the English text from which a summary should be generated. Cannot be combined with --file option.", show_default=False, ), - file: Optional[Path] = readable_file_option( - param_name="--file", - help_str="File containing the English text from which a summary should be generated. Cannot be combined with TEXT argument.", + file: Optional[Path] = ExclusiveReadableFileOption( + help="File containing the English text from which a summary should be generated. Cannot be combined with TEXT argument.", ), **options, ) -> CommandResult: @@ -254,8 +264,6 @@ def summarize( manager = CortexManager() - if text and file: - raise UsageError("--file option cannot be used together with TEXT argument.") if text: result_text = manager.summarize_text( text=Text(text), @@ -275,8 +283,8 @@ def summarize( requires_connection=True, ) def translate( - text: Optional[str] = typer.Argument( - None, + text: Optional[str] = ExclusiveTextSourceArgument( + default=None, help="String containing the text to be translated. Cannot be combined with --file option.", show_default=False, ), @@ -292,9 +300,8 @@ def translate( help="String specifying the language code into which the text should be translated. See Snowflake Cortex documentation for a list of supported language codes.", show_default=False, ), - file: Optional[Path] = readable_file_option( - param_name="--file", - help_str="File containing the text to be translated. Cannot be combined with TEXT argument.", + file: Optional[Path] = ExclusiveReadableFileOption( + help="File containing the text to be translated. Cannot be combined with TEXT argument.", ), **options, ) -> CommandResult: @@ -307,8 +314,6 @@ def translate( source_language = None if from_language is None else Language(from_language) target_language = Language(to_language) - if text and file: - raise UsageError("--file option cannot be used together with TEXT argument.") if text: result_text = manager.translate_text( text=Text(text), diff --git a/src/snowflake/cli/_plugins/nativeapp/commands.py b/src/snowflake/cli/_plugins/nativeapp/commands.py index eebd5779ed..bc87f09f6a 100644 --- a/src/snowflake/cli/_plugins/nativeapp/commands.py +++ b/src/snowflake/cli/_plugins/nativeapp/commands.py @@ -22,7 +22,6 @@ from typing import Generator, Iterable, List, Optional, cast import typer -from click import ClickException, UsageError from snowflake.cli._plugins.nativeapp.common_flags import ( ForceOption, InteractiveOption, @@ -60,6 +59,7 @@ with_project_definition, ) from snowflake.cli.api.commands.snow_typer import SnowTyperFactory +from snowflake.cli.api.exceptions import IncompatibleParametersError from snowflake.cli.api.output.formats import OutputFormat from snowflake.cli.api.output.types import ( CollectionResult, @@ -371,7 +371,7 @@ def app_deploy( recursive = False if has_paths and prune: - raise ClickException("--prune cannot be used when paths are also specified") + raise IncompatibleParametersError(["paths", "--prune"]) cli_context = get_cli_context() manager = NativeAppManager( @@ -495,18 +495,18 @@ def app_events( https://docs.snowflake.com/en/developer-guide/native-apps/setting-up-logging-and-events """ if first >= 0 and last >= 0: - raise UsageError("--first and --last cannot be used together.") + raise IncompatibleParametersError(["--first", "--last"]) if (consumer_org and not consumer_account) or ( consumer_account and not consumer_org ): - raise UsageError("--consumer-org and --consumer-account must be used together.") + raise IncompatibleParametersError(["--consumer-org", "--consumer-account"]) if follow: if until: - raise UsageError("--follow and --until cannot be used together.") + raise IncompatibleParametersError(["--follow", "--until"]) if first >= 0: - raise UsageError("--follow and --first cannot be used together.") + raise IncompatibleParametersError(["--follow", "--first"]) assert_project_type("native_app") diff --git a/src/snowflake/cli/_plugins/object/commands.py b/src/snowflake/cli/_plugins/object/commands.py index 2477cdfec6..cb6271f538 100644 --- a/src/snowflake/cli/_plugins/object/commands.py +++ b/src/snowflake/cli/_plugins/object/commands.py @@ -26,6 +26,7 @@ ) from snowflake.cli.api.commands.snow_typer import SnowTyperFactory from snowflake.cli.api.constants import SUPPORTED_OBJECTS, VALID_SCOPES +from snowflake.cli.api.exceptions import IncompatibleParametersError from snowflake.cli.api.identifiers import FQN from snowflake.cli.api.output.types import MessageResult, QueryResult from snowflake.cli.api.project.util import is_valid_identifier @@ -153,9 +154,7 @@ def create( import json if object_attributes and object_json: - raise ClickException( - "Conflict: both object attributes and JSON definition are provided" - ) + raise IncompatibleParametersError(["object_attributes", "--json"]) if object_json: object_data = json.loads(object_json) diff --git a/src/snowflake/cli/_plugins/sql/commands.py b/src/snowflake/cli/_plugins/sql/commands.py index 78e910e3c5..ab7dd25cd3 100644 --- a/src/snowflake/cli/_plugins/sql/commands.py +++ b/src/snowflake/cli/_plugins/sql/commands.py @@ -17,43 +17,46 @@ from pathlib import Path from typing import List, Optional -import typer from snowflake.cli._plugins.sql.manager import SqlManager from snowflake.cli.api.commands.decorators import with_project_definition from snowflake.cli.api.commands.flags import ( parse_key_value_variables, variables_option, ) +from snowflake.cli.api.commands.overrideable_parameter import OverrideableOption from snowflake.cli.api.commands.snow_typer import SnowTyperFactory from snowflake.cli.api.output.types import CommandResult, MultipleResults, QueryResult # simple Typer with defaults because it won't become a command group as it contains only one command app = SnowTyperFactory() +SOURCE_EXCLUSIVE_OPTIONS_NAMES = ["query", "files", "std_in"] + +SourceOption = OverrideableOption( + mutually_exclusive=SOURCE_EXCLUSIVE_OPTIONS_NAMES, show_default=False +) + @app.command(name="sql", requires_connection=True, no_args_is_help=True) @with_project_definition(is_optional=True) def execute_sql( - query: Optional[str] = typer.Option( - None, - "--query", - "-q", + query: Optional[str] = SourceOption( + default=None, + param_decls=["--query", "-q"], help="Query to execute.", ), - files: Optional[List[Path]] = typer.Option( - None, - "--filename", - "-f", + files: Optional[List[Path]] = SourceOption( + default=[], + param_decls=["--filename", "-f"], exists=True, file_okay=True, dir_okay=False, readable=True, help="File to execute.", ), - std_in: Optional[bool] = typer.Option( - False, - "--stdin", - "-i", + std_in: Optional[bool] = SourceOption( + default=False, + param_decls=["--stdin", "-i"], help="Read the query from standard input. Use it when piping input to this command.", ), data_override: List[str] = variables_option( @@ -73,6 +76,7 @@ def execute_sql( The command supports variable substitution that happens on client-side. Both &VARIABLE or &{ VARIABLE } syntax are supported. """ + data = {} if data_override: data = {v.key: v.value for v in parse_key_value_variables(data_override)} diff --git a/src/snowflake/cli/api/commands/flags.py b/src/snowflake/cli/api/commands/flags.py index a7e87ffb84..40ba465ad2 100644 --- a/src/snowflake/cli/api/commands/flags.py +++ b/src/snowflake/cli/api/commands/flags.py @@ -17,15 +17,16 @@ import tempfile from dataclasses import dataclass from enum import Enum -from inspect import signature from pathlib import Path -from typing import Any, Callable, List, Optional, Tuple +from typing import Any, Callable, List, Optional import click import typer from click import ClickException from snowflake.cli.api.cli_global_context import get_cli_context_manager +from snowflake.cli.api.commands.overrideable_parameter import OverrideableOption from snowflake.cli.api.commands.typer_pre_execute import register_pre_execute_command +from snowflake.cli.api.config import get_all_connections from snowflake.cli.api.console import cli_console from snowflake.cli.api.exceptions import MissingConfiguration from snowflake.cli.api.identifiers import FQN @@ -44,106 +45,6 @@ class OnErrorType(Enum): CONTINUE = "continue" -class OverrideableOption: - """ - Class that allows you to generate instances of typer.models.OptionInfo with some default properties while allowing - specific values to be overridden. - - Custom parameters: - - mutually_exclusive (Tuple[str]|List[str]): A list of parameter names that this Option is not compatible with. If this Option has - a truthy value and any of the other parameters in the mutually_exclusive list has a truthy value, a - ClickException will be thrown. Note that mutually_exclusive can contain an option's own name but does not require - it. - """ - - def __init__( - self, - default: Any, - *param_decls: str, - mutually_exclusive: Optional[List[str] | Tuple[str]] = None, - **kwargs, - ): - self.default = default - self.param_decls = param_decls - self.mutually_exclusive = mutually_exclusive - self.kwargs = kwargs - - def __call__(self, **kwargs) -> typer.models.OptionInfo: - """ - Returns a typer.models.OptionInfo instance initialized with the specified default values along with any overrides - from kwargs. Note that if you are overriding param_decls, you must pass an iterable of strings, you cannot use - positional arguments like you can with typer.Option. Does not modify the original instance. - """ - default = kwargs.get("default", self.default) - param_decls = kwargs.get("param_decls", self.param_decls) - mutually_exclusive = kwargs.get("mutually_exclusive", self.mutually_exclusive) - if not isinstance(param_decls, list) and not isinstance(param_decls, tuple): - raise TypeError("param_decls must be a list or tuple") - passed_kwargs = self.kwargs.copy() - passed_kwargs.update(kwargs) - if passed_kwargs.get("callback", None) or mutually_exclusive: - passed_kwargs["callback"] = self._callback_factory( - passed_kwargs.get("callback", None), mutually_exclusive - ) - for non_kwarg in ["default", "param_decls", "mutually_exclusive"]: - passed_kwargs.pop(non_kwarg, None) - return typer.Option(default, *param_decls, **passed_kwargs) - - class InvalidCallbackSignature(ClickException): - def __init__(self, callback): - super().__init__( - f"Signature {signature(callback)} is not valid for an OverrideableOption callback function. Must have at most one parameter with each of the following types: (typer.Context, typer.CallbackParam, Any Other Type)" - ) - - def _callback_factory( - self, callback, mutually_exclusive: Optional[List[str] | Tuple[str]] - ): - callback = callback if callback else lambda x: x - - # inspect existing_callback to make sure signature is valid - existing_params = signature(callback).parameters - # at most one parameter with each type in [typer.Context, typer.CallbackParam, any other type] - limits = [ - lambda x: x == typer.Context, - lambda x: x == typer.CallbackParam, - lambda x: x != typer.Context and x != typer.CallbackParam, - ] - for limit in limits: - if len([v for v in existing_params.values() if limit(v.annotation)]) > 1: - raise self.InvalidCallbackSignature(callback) - - def generated_callback(ctx: typer.Context, param: typer.CallbackParam, value): - if mutually_exclusive: - for name in mutually_exclusive: - if value and ctx.params.get( - name, False - ): # if the current parameter is set to True and a previous parameter is also Truthy - curr_opt = param.opts[0] - other_opt = [x for x in ctx.command.params if x.name == name][ - 0 - ].opts[0] - raise click.ClickException( - f"Options '{curr_opt}' and '{other_opt}' are incompatible." - ) - - # pass args to existing callback based on its signature (this is how Typer infers callback args) - passed_params = {} - for existing_param in existing_params: - annotation = existing_params[existing_param].annotation - if annotation == typer.Context: - passed_params[existing_param] = ctx - elif annotation == typer.CallbackParam: - passed_params[existing_param] = param - else: - passed_params[existing_param] = value - return callback(**passed_params) - - return generated_callback - - -from snowflake.cli.api.config import get_all_connections - - def _callback(provide_setter: Callable[[], Callable[[Any], Any]]): def callback(value): set_value = provide_setter() @@ -612,19 +513,6 @@ def project_env_overrides_callback(env_overrides_args_list: list[str]) -> None: ) -def readable_file_option(param_name: str, help_str: str) -> typer.Option: - return typer.Option( - None, - param_name, - exists=True, - file_okay=True, - dir_okay=False, - readable=True, - help=help_str, - show_default=False, - ) - - def deprecated_flag_callback(msg: str): def _warning_callback(ctx: click.Context, param: click.Parameter, value: Any): if ctx.get_parameter_source(param.name) != click.core.ParameterSource.DEFAULT: # type: ignore[attr-defined] diff --git a/src/snowflake/cli/api/commands/overrideable_parameter.py b/src/snowflake/cli/api/commands/overrideable_parameter.py new file mode 100644 index 0000000000..521875221e --- /dev/null +++ b/src/snowflake/cli/api/commands/overrideable_parameter.py @@ -0,0 +1,143 @@ +# 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. + +from __future__ import annotations + +from abc import ABC, abstractmethod +from inspect import signature +from typing import Any, List, Optional, Tuple + +import typer +from click import ClickException +from snowflake.cli.api.exceptions import IncompatibleParametersError + + +class _OverrideableParameter(ABC): + """ + Class that allows you to generate instances of typer.models.OptionInfo with some default properties while allowing + specific values to be overridden. + + Custom parameters: + - mutually_exclusive (Tuple[str]|List[str]): A list of parameter names that this Option is not compatible with. If this Option has + a truthy value and any of the other parameters in the mutually_exclusive list has a truthy value, a + ClickException will be thrown. Note that mutually_exclusive can contain an option's own name but does not require + it. + """ + + def __init__( + self, + default: Any = ..., + *param_decls: str, + mutually_exclusive: Optional[List[str] | Tuple[str]] = None, + **kwargs, + ): + self.default = default + self.param_decls = param_decls + self.mutually_exclusive = mutually_exclusive + self.kwargs = kwargs + + def __call__(self, **kwargs) -> typer.models.ParameterInfo: + """ + Returns a typer.models.OptionInfo instance initialized with the specified default values along with any overrides + from kwargs. Note that if you are overriding param_decls, you must pass an iterable of strings, you cannot use + positional arguments like you can with typer.Option. Does not modify the original instance. + """ + default = kwargs.get("default", self.default) + param_decls = kwargs.get("param_decls", self.param_decls) + mutually_exclusive = kwargs.get("mutually_exclusive", self.mutually_exclusive) + if not isinstance(param_decls, list) and not isinstance(param_decls, tuple): + raise TypeError("param_decls must be a list or tuple") + passed_kwargs = self.kwargs.copy() + passed_kwargs.update(kwargs) + if passed_kwargs.get("callback", None) or mutually_exclusive: + passed_kwargs["callback"] = self._callback_factory( + passed_kwargs.get("callback", None), mutually_exclusive + ) + for non_kwarg in ["default", "param_decls", "mutually_exclusive"]: + passed_kwargs.pop(non_kwarg, None) + + return self.get_parameter(default, *param_decls, **passed_kwargs) + + @abstractmethod + def get_parameter( + self, default: Any = None, *param_decls: str, **kwargs + ) -> typer.models.ParameterInfo: + pass + + class InvalidCallbackSignature(ClickException): + def __init__(self, callback): + super().__init__( + f"Signature {signature(callback)} is not valid for an OverrideableOption callback function. Must have " + f"at most one parameter with each of the following types: (typer.Context, typer.CallbackParam, " + f"Any Other Type)" + ) + + def _callback_factory( + self, callback, mutually_exclusive: Optional[List[str] | Tuple[str]] + ): + callback = callback if callback else lambda x: x + + # inspect existing_callback to make sure signature is valid + existing_params = signature(callback).parameters + # at most one parameter with each type in [typer.Context, typer.CallbackParam, any other type] + limits = [ + lambda x: x == typer.Context, + lambda x: x == typer.CallbackParam, + lambda x: x != typer.Context and x != typer.CallbackParam, + ] + for limit in limits: + if len([v for v in existing_params.values() if limit(v.annotation)]) > 1: + raise self.InvalidCallbackSignature(callback) + + def generated_callback(ctx: typer.Context, param: typer.CallbackParam, value): + if mutually_exclusive: + for name in mutually_exclusive: + if value and ctx.params.get( + name, False + ): # if the current parameter is set to True and a previous parameter is also Truthy + curr_opt = param.opts[0] + other_opt = [x for x in ctx.command.params if x.name == name][ + 0 + ].opts[0] + raise IncompatibleParametersError([curr_opt, other_opt]) + + # pass args to existing callback based on its signature (this is how Typer infers callback args) + passed_params = {} + for existing_param in existing_params: + annotation = existing_params[existing_param].annotation + if annotation == typer.Context: + passed_params[existing_param] = ctx + elif annotation == typer.CallbackParam: + passed_params[existing_param] = param + else: + passed_params[existing_param] = value + return callback(**passed_params) + + return generated_callback + + +class OverrideableArgument(_OverrideableParameter): + def get_parameter( + self, default: Any = ..., *param_decls: str, **kwargs + ) -> typer.models.ArgumentInfo: + return typer.Argument(default, *param_decls, **kwargs) + + +# OverrideableOption doesn't work with flags with type List[Any] and default None, because typer executes the callback +# function which converts the default value iterating over it, but None is not iterable. +class OverrideableOption(_OverrideableParameter): + def get_parameter( + self, default: Any = ..., *param_decls: str, **kwargs + ) -> typer.models.OptionInfo: + return typer.Option(default, *param_decls, **kwargs) diff --git a/src/snowflake/cli/api/exceptions.py b/src/snowflake/cli/api/exceptions.py index 4e23328cb6..17745a2c3f 100644 --- a/src/snowflake/cli/api/exceptions.py +++ b/src/snowflake/cli/api/exceptions.py @@ -17,7 +17,7 @@ from pathlib import Path from typing import Optional -from click.exceptions import ClickException +from click.exceptions import ClickException, UsageError from snowflake.cli.api.constants import ObjectType @@ -169,3 +169,12 @@ def __init__(self, part: str, name: str): super().__init__( f"{part.capitalize()} provided but name '{name}' is fully qualified name." ) + + +class IncompatibleParametersError(UsageError): + def __init__(self, options: list[str]): + options_with_quotes = [f"'{option}'" for option in options] + comma_separated_options = ", ".join(options_with_quotes[:-1]) + super().__init__( + f"Parameters {comma_separated_options} and {options_with_quotes[-1]} are incompatible and cannot be used simultaneously." + ) diff --git a/tests/__snapshots__/test_help_messages.ambr b/tests/__snapshots__/test_help_messages.ambr index 229ff86823..2f29c104db 100644 --- a/tests/__snapshots__/test_help_messages.ambr +++ b/tests/__snapshots__/test_help_messages.ambr @@ -6605,8 +6605,8 @@ &VARIABLE or &{ VARIABLE } syntax are supported. +- Options --------------------------------------------------------------------+ - | --query -q TEXT Query to execute. [default: None] | - | --filename -f FILE File to execute. [default: None] | + | --query -q TEXT Query to execute. | + | --filename -f FILE File to execute. | | --stdin -i Read the query from standard input. Use it when | | piping input to this command. | | --variable -D TEXT String in format of key=value. If provided the SQL | diff --git a/tests/__snapshots__/test_sql.ambr b/tests/__snapshots__/test_sql.ambr index 96eadf5696..96431fd13e 100644 --- a/tests/__snapshots__/test_sql.ambr +++ b/tests/__snapshots__/test_sql.ambr @@ -13,8 +13,8 @@ &VARIABLE or &{ VARIABLE } syntax are supported. +- Options --------------------------------------------------------------------+ - | --query -q TEXT Query to execute. [default: None] | - | --filename -f FILE File to execute. [default: None] | + | --query -q TEXT Query to execute. | + | --filename -f FILE File to execute. | | --stdin -i Read the query from standard input. Use it when | | piping input to this command. | | --variable -D TEXT String in format of key=value. If provided the SQL | diff --git a/tests/api/commands/__snapshots__/test_flags.ambr b/tests/api/commands/__snapshots__/test_flags.ambr index 083d76dd0f..3b25b998f3 100644 --- a/tests/api/commands/__snapshots__/test_flags.ambr +++ b/tests/api/commands/__snapshots__/test_flags.ambr @@ -12,16 +12,22 @@ # --- # name: test_mutually_exclusive_options_error ''' + Usage: - [OPTIONS] + Try '- --help' for help. +- Error ----------------------------------------------------------------------+ - | Options '--option2' and '--option1' are incompatible. | + | Parameters '--option2' and '--option1' are incompatible and cannot be used | + | simultaneously. | +------------------------------------------------------------------------------+ ''' # --- # name: test_overrideable_option_callback_with_mutually_exclusive ''' + Usage: - [OPTIONS] + Try '- --help' for help. +- Error ----------------------------------------------------------------------+ - | Options '--option2' and '--option1' are incompatible. | + | Parameters '--option2' and '--option1' are incompatible and cannot be used | + | simultaneously. | +------------------------------------------------------------------------------+ ''' diff --git a/tests/api/commands/test_flags.py b/tests/api/commands/test_flags.py index a8944e5e18..51b9b68a13 100644 --- a/tests/api/commands/test_flags.py +++ b/tests/api/commands/test_flags.py @@ -111,7 +111,7 @@ def _(option_1: bool = _MUTEX_OPTION_1(), option_2: bool = _MUTEX_OPTION_2()): command = ["--option1", "--option2"] runner = CliRunner() result = runner.invoke(app, command) - assert result.exit_code == 1 + assert result.exit_code == 2 assert result.output == os_agnostic_snapshot @@ -212,5 +212,5 @@ def _( # test that we can't provide both options as non-falsey values without throwing error result = runner.invoke(app, ["--option1", "1", "--option2", "2"]) - assert result.exit_code == 1 + assert result.exit_code == 2 assert result.output == os_agnostic_snapshot diff --git a/tests/cortex/test_cortex_commands.py b/tests/cortex/test_cortex_commands.py index 50bf2984af..0f0d6dfe62 100644 --- a/tests/cortex/test_cortex_commands.py +++ b/tests/cortex/test_cortex_commands.py @@ -16,13 +16,17 @@ import re from contextlib import contextmanager +from tempfile import NamedTemporaryFile from typing import Any, Optional from unittest import mock import pytest from tests.testing_utils.fixtures import TEST_DIR -from tests.testing_utils.result_assertions import assert_successful_result_message +from tests.testing_utils.result_assertions import ( + assert_successful_result_message, + assert_that_result_is_usage_error, +) @pytest.fixture @@ -59,6 +63,44 @@ def test_cortex_complete_for_prompt_with_default_model(_mock_cortex_result, runn assert_successful_result_message(result, expected_msg="Yes") +@pytest.mark.parametrize("command", ["complete", "summarize", "sentiment", "translate"]) +def test_cortex_commands_with_text_and_file(runner, command): + with NamedTemporaryFile("r") as tmp_file: + result = runner.invoke( + [ + "cortex", + f"{command}", + "Is 5 more than 4? Please answer using one word without a period.", + "--file", + tmp_file.name, + ] + ) + + assert_that_result_is_usage_error( + result, + "Parameters 'text' and '--file' are incompatible and cannot be used simultaneously.", + ) + + +def test_cortex_extract_answer_with_text_and_file(runner): + with NamedTemporaryFile("r") as tmp_file: + result = runner.invoke( + [ + "cortex", + "extract-answer", + "What's the color of John's car?", + "Is 5 more than 4? Please answer using one word without a period.", + "--file", + tmp_file.name, + ] + ) + + assert_that_result_is_usage_error( + result, + "Parameters 'source_document_text' and '--file' are incompatible and cannot be used simultaneously.", + ) + + def test_cortex_complete_for_prompt_with_chosen_model(_mock_cortex_result, runner): with _mock_cortex_result( raw_result="Yes", diff --git a/tests/object/test_object.py b/tests/object/test_object.py index 85ed38c139..51c2bb2826 100644 --- a/tests/object/test_object.py +++ b/tests/object/test_object.py @@ -11,7 +11,7 @@ # 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. - +from tempfile import NamedTemporaryFile from unittest import mock import pytest @@ -19,6 +19,8 @@ from snowflake.cli._plugins.object.commands import _scope_validate from snowflake.cli.api.constants import OBJECT_TO_NAMES, SUPPORTED_OBJECTS +from tests.testing_utils.result_assertions import assert_that_result_is_usage_error + @mock.patch("snowflake.connector.connect") @pytest.mark.parametrize( @@ -133,7 +135,6 @@ def test_show_with_scope( def test_show_with_invalid_scope( mock_connector, object_type, input_scope, input_name, expected, runner ): - result = runner.invoke( ["object", "list", object_type, "--in", input_scope, input_name] ) @@ -253,3 +254,14 @@ def test_throw_exception_because_of_missing_arguments( assert result.output.__contains__( f"Missing argument '{expect_argument_exception}'." ) + + +def test_object_create_with_multiple_json_sources(runner): + with NamedTemporaryFile("r") as tmp_file: + result = runner.invoke( + ["object", "create", "schema", "name=schema_name", "--json", "json_data"] + ) + assert_that_result_is_usage_error( + result, + f"Parameters 'object_attributes' and '--json' are incompatible and cannot be used simultaneously.", + ) diff --git a/tests/spcs/__snapshots__/test_image_repository.ambr b/tests/spcs/__snapshots__/test_image_repository.ambr index 6a09c55153..121871c5a9 100644 --- a/tests/spcs/__snapshots__/test_image_repository.ambr +++ b/tests/spcs/__snapshots__/test_image_repository.ambr @@ -11,8 +11,11 @@ # --- # name: test_create_cli_replace_and_if_not_exists_fails ''' + Usage: default spcs image-repository create [OPTIONS] NAME + Try 'default spcs image-repository create --help' for help. +- Error ----------------------------------------------------------------------+ - | Options '--if-not-exists' and '--replace' are incompatible. | + | Parameters '--if-not-exists' and '--replace' are incompatible and cannot be | + | used simultaneously. | +------------------------------------------------------------------------------+ ''' diff --git a/tests/spcs/test_image_repository.py b/tests/spcs/test_image_repository.py index 8f01d4d28b..ef1885ae60 100644 --- a/tests/spcs/test_image_repository.py +++ b/tests/spcs/test_image_repository.py @@ -118,7 +118,7 @@ def test_create_cli_replace_and_if_not_exists_fails(runner, os_agnostic_snapshot "--if-not-exists", ] result = runner.invoke(command) - assert result.exit_code == 1 + assert result.exit_code == 2 assert result.output == os_agnostic_snapshot diff --git a/tests/test_sql.py b/tests/test_sql.py index 755f153ab1..a1ba6cd26b 100644 --- a/tests/test_sql.py +++ b/tests/test_sql.py @@ -91,19 +91,30 @@ def test_sql_help_if_no_query_file_or_stdin(runner, os_agnostic_snapshot): assert result.output == os_agnostic_snapshot -@pytest.mark.parametrize("inputs", [("-i", "-q", "foo"), ("-i",), ("-q", "foo")]) -def test_sql_fails_if_other_inputs_and_file_provided(runner, inputs): +def test_sql_fails_if_query_and_stdin_and_file_provided(runner): with NamedTemporaryFile("r") as tmp_file: - result = runner.invoke(["sql", *inputs, "-f", tmp_file.name]) + result = runner.invoke(["sql", "-i", "-q", "foo", "-f", tmp_file.name]) assert_that_result_is_usage_error( - result, "Multiple input sources specified. Please specify only one. " + result, + f"Parameters '--query' and '--stdin' are incompatible and cannot be used simultaneously.", + ) + + +@pytest.mark.parametrize("inputs", [(("-i",), "stdin"), (("-q", "foo"), "query")]) +def test_sql_fails_if_other_input_and_file_provided(runner, inputs): + with NamedTemporaryFile("r") as tmp_file: + result = runner.invoke(["sql", *(inputs[0]), "-f", tmp_file.name]) + assert_that_result_is_usage_error( + result, + f"Parameters '--filename' and '--{inputs[1]}' are incompatible and cannot be used simultaneously.", ) def test_sql_fails_if_query_and_stdin_provided(runner): result = runner.invoke(["sql", "-q", "fooo", "-i"]) assert_that_result_is_usage_error( - result, "Multiple input sources specified. Please specify only one. " + result, + "Parameters '--stdin' and '--query' are incompatible and cannot be used simultaneously. ", ) diff --git a/tests/testing_utils/result_assertions.py b/tests/testing_utils/result_assertions.py index 09439f9367..f65f08e86e 100644 --- a/tests/testing_utils/result_assertions.py +++ b/tests/testing_utils/result_assertions.py @@ -11,6 +11,7 @@ # 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 re from click.testing import Result @@ -23,7 +24,8 @@ def assert_successful_result_message(result: Result, expected_msg: str): def assert_that_result_is_usage_error( result: Result, expected_error_message: str ) -> None: + result_output = re.sub(r"\s*\|\|\s*", " ", result.output.replace("\n", "")) assert result.exit_code == 2, result.output - assert expected_error_message in result.output, result.output + assert expected_error_message in result_output, result.output assert isinstance(result.exception, SystemExit) assert "traceback" not in result.output.lower() diff --git a/tests_integration/nativeapp/test_deploy.py b/tests_integration/nativeapp/test_deploy.py index aa4cbcae29..44787578b6 100644 --- a/tests_integration/nativeapp/test_deploy.py +++ b/tests_integration/nativeapp/test_deploy.py @@ -29,6 +29,7 @@ ) from tests_integration.testing_utils import ( assert_that_result_failed_with_message_containing, + assert_that_result_is_usage_error, ) USER_NAME = f"user_{uuid.uuid4().hex}" @@ -442,9 +443,10 @@ def test_nativeapp_deploy_rejects_pruning_when_path_is_specified( ["app", "deploy", "app/README.md", "--prune"], env=TEST_ENV, ) - assert result.exit_code == 1 - assert ( - "--prune cannot be used when paths are also specified" in result.output + + assert_that_result_is_usage_error( + result, + "Parameters 'paths' and '--prune' are incompatible and cannot be used simultaneously.", ) finally: diff --git a/tests_integration/nativeapp/test_events.py b/tests_integration/nativeapp/test_events.py index 123e21e147..b0038e2a46 100644 --- a/tests_integration/nativeapp/test_events.py +++ b/tests_integration/nativeapp/test_events.py @@ -17,6 +17,7 @@ from snowflake.cli.api.project.util import generate_user_env from tests.project.fixtures import * from tests_integration.test_utils import enable_definition_v2_feature_flag +from tests_integration.testing_utils import assert_that_result_is_usage_error USER_NAME = f"user_{uuid.uuid4().hex}" TEST_ENV = generate_user_env(USER_NAME) @@ -53,10 +54,9 @@ def test_app_events_mutually_exclusive_options( ["app", "events", *command], env=TEST_ENV, ) - assert result.exit_code == 2, result.output - assert ( - f"{flag_names[0]} and {flag_names[1]} cannot be used together." - in result.output + assert_that_result_is_usage_error( + result, + f"Parameters '{flag_names[0]}' and '{flag_names[1]}' are incompatible and cannot be used simultaneously.", ) @@ -87,10 +87,9 @@ def test_app_events_paired_options( ["app", "events", *command], env=TEST_ENV, ) - assert result.exit_code == 2, result.output - assert ( - f"{flag_names[0]} and {flag_names[1]} must be used together." - in result.output + assert_that_result_is_usage_error( + result, + f"Parameters '{flag_names[0]}' and '{flag_names[1]}' are incompatible and cannot be used simultaneously.", ) diff --git a/tests_integration/testing_utils/assertions/test_result_assertions.py b/tests_integration/testing_utils/assertions/test_result_assertions.py index 3e4f5ccdfd..27e41444b4 100644 --- a/tests_integration/testing_utils/assertions/test_result_assertions.py +++ b/tests_integration/testing_utils/assertions/test_result_assertions.py @@ -60,6 +60,15 @@ def assert_successful_result_message(result: CommandResult, expected_msg: str) - assert result.output == expected_msg + "\n" +def assert_that_result_is_usage_error( + result: CommandResult, expected_error_message: str +) -> None: + assert result.output is not None + result_output = re.sub("\s*││\s*", " ", result.output.replace("\n", "")) # type: ignore + assert result.exit_code == 2, result.output + assert expected_error_message in result_output, result.output + + def assert_that_result_is_successful_and_output_json_contains( result: CommandResult, expected_output: Dict,