From 55f798a62049fc468f59f5180cee985856998c57 Mon Sep 17 00:00:00 2001 From: Jeremy Leon Date: Mon, 15 Apr 2024 17:15:23 -0400 Subject: [PATCH] refactor(api): more clear error messages for type validation when creating runtime parameters (#14903) --- .../protocols/parameters/validation.py | 73 +++++++++++++------ .../protocols/parameters/test_validation.py | 9 ++- 2 files changed, 55 insertions(+), 27 deletions(-) diff --git a/api/src/opentrons/protocols/parameters/validation.py b/api/src/opentrons/protocols/parameters/validation.py index 9410db294ed..2db343c71b6 100644 --- a/api/src/opentrons/protocols/parameters/validation.py +++ b/api/src/opentrons/protocols/parameters/validation.py @@ -29,15 +29,21 @@ def validate_variable_name_unique( def ensure_display_name(display_name: str) -> str: """Validate display name is within the character limit.""" + if not isinstance(display_name, str): + raise ParameterNameError( + f"Display name must be a string and at most {DISPLAY_NAME_MAX_LEN} characters." + ) if len(display_name) > DISPLAY_NAME_MAX_LEN: raise ParameterNameError( - f"Display name {display_name} greater than {DISPLAY_NAME_MAX_LEN} characters." + f'Display name "{display_name}" greater than {DISPLAY_NAME_MAX_LEN} characters.' ) return display_name def ensure_variable_name(variable_name: str) -> str: """Validate variable name is a valid python variable name.""" + if not isinstance(variable_name, str): + raise ParameterNameError("Variable name must be a string.") if not variable_name.isidentifier(): raise ParameterNameError( "Variable name must only contain alphanumeric characters, underscores, and cannot start with a digit." @@ -49,19 +55,29 @@ def ensure_variable_name(variable_name: str) -> str: def ensure_description(description: Optional[str]) -> Optional[str]: """Validate description is within the character limit.""" - if description is not None and len(description) > DESCRIPTION_MAX_LEN: - raise ParameterNameError( - f"Description {description} greater than {DESCRIPTION_MAX_LEN} characters." - ) + if description is not None: + if not isinstance(description, str): + raise ParameterNameError( + f"Description must be a string and at most {DESCRIPTION_MAX_LEN} characters." + ) + if len(description) > DESCRIPTION_MAX_LEN: + raise ParameterNameError( + f'Description "{description}" greater than {DESCRIPTION_MAX_LEN} characters.' + ) return description def ensure_unit_string_length(unit: Optional[str]) -> Optional[str]: """Validate unit is within the character limit.""" - if unit is not None and len(unit) > UNIT_MAX_LEN: - raise ParameterNameError( - f"Description {unit} greater than {UNIT_MAX_LEN} characters." - ) + if unit is not None: + if not isinstance(unit, str): + raise ParameterNameError( + f"Unit must be a string and at most {UNIT_MAX_LEN} characters." + ) + if len(unit) > UNIT_MAX_LEN: + raise ParameterNameError( + f'Unit "{unit}" greater than {UNIT_MAX_LEN} characters.' + ) return unit @@ -135,7 +151,7 @@ def convert_type_string_for_enum( return "str" else: raise ParameterValueError( - f"Cannot resolve parameter type {parameter_type} for an enumerated parameter." + f"Cannot resolve parameter type '{parameter_type.__name__}' for an enumerated parameter." ) @@ -147,7 +163,7 @@ def convert_type_string_for_num_param(parameter_type: type) -> Literal["int", "f return "float" else: raise ParameterValueError( - f"Cannot resolve parameter type {parameter_type} for a number parameter." + f"Cannot resolve parameter type '{parameter_type.__name__}' for a number parameter." ) @@ -173,7 +189,7 @@ def _validate_choices( ensure_display_name(display_name) if not isinstance(value, parameter_type): raise ParameterDefinitionError( - f"All choices provided must match type {type(parameter_type)}" + f"All choices provided must be of type '{parameter_type.__name__}'" ) @@ -192,21 +208,27 @@ def _validate_min_and_max( "If a maximum value is provided a minimum must also be provided." ) elif maximum is not None and minimum is not None: - if isinstance(maximum, (int, float)) and isinstance(minimum, (int, float)): - if maximum <= minimum: + if parameter_type is int or parameter_type is float: + if not isinstance(minimum, parameter_type): raise ParameterDefinitionError( - "Maximum must be greater than the minimum" + f"Minimum is type '{type(minimum).__name__}'," + f" but must be of parameter type '{parameter_type.__name__}'" ) - - if not isinstance(minimum, parameter_type) or not isinstance( - maximum, parameter_type - ): + if not isinstance(maximum, parameter_type): raise ParameterDefinitionError( - f"Minimum and maximum must match type {parameter_type}" + f"Maximum is type '{type(maximum).__name__}'," + f" but must be of parameter type '{parameter_type.__name__}'" + ) + # These asserts are for the type checker and should never actually be asserted false + assert isinstance(minimum, (int, float)) + assert isinstance(maximum, (int, float)) + if maximum <= minimum: + raise ParameterDefinitionError( + "Maximum must be greater than the minimum" ) else: raise ParameterDefinitionError( - "Only parameters of type float or int can have a minimum and maximum" + "Only parameters of type float or int can have a minimum and maximum." ) @@ -214,7 +236,8 @@ def validate_type(value: ParamType, parameter_type: type) -> None: """Validate parameter value is the correct type.""" if not isinstance(value, parameter_type): raise ParameterValueError( - f"Parameter value {value} has type {type(value)}, must match type {parameter_type}." + f"Parameter value {value} has type '{type(value).__name__}'," + f" but must be of type '{parameter_type.__name__}'." ) @@ -226,7 +249,11 @@ def validate_options( parameter_type: type, ) -> None: """Validate default values and all possible constraints for a valid parameter definition.""" - validate_type(default, parameter_type) + if not isinstance(default, parameter_type): + raise ParameterValueError( + f"Parameter default {default} has type '{type(default).__name__}'," + f" but must be of type '{parameter_type.__name__}'." + ) if choices is None and minimum is None and maximum is None: raise ParameterDefinitionError( diff --git a/api/tests/opentrons/protocols/parameters/test_validation.py b/api/tests/opentrons/protocols/parameters/test_validation.py index 4d3b2fc83b5..4206d3d3cd4 100644 --- a/api/tests/opentrons/protocols/parameters/test_validation.py +++ b/api/tests/opentrons/protocols/parameters/test_validation.py @@ -278,14 +278,15 @@ def test_convert_type_string_for_num_param_raises(param_type: type) -> None: None, [{"display_name": "abc", "value": "123"}], int, - "must match type", + "must be of type", ), (123, 1, None, None, int, "maximum must also"), (123, None, 100, None, int, "minimum must also"), (123, 100, 1, None, int, "Maximum must be greater"), - (123, 1.1, 100, None, int, "Minimum and maximum must match type"), - (123, 1, 100.5, None, int, "Minimum and maximum must match type"), - (123, "1", "100", None, int, "Only parameters of type float or int"), + (123, 1.1, 100, None, int, "Minimum is type"), + (123, 1, 100.5, None, int, "Maximum is type"), + (123.0, "1.0", 100.0, None, float, "Minimum is type"), + ("blah", 1, 100, None, str, "Only parameters of type float or int"), ], ) def test_validate_options_raise_definition_error(