diff --git a/lib/galaxy/util/config_templates.py b/lib/galaxy/util/config_templates.py index 8053667d4e3..183d14786e3 100644 --- a/lib/galaxy/util/config_templates.py +++ b/lib/galaxy/util/config_templates.py @@ -115,7 +115,6 @@ class TemplateSecret(StrictModel): label: Optional[str] = None help: Optional[MarkdownContent] = None optional: Optional[bool] = None - default: Optional[str] = None # If set, secret is optional class TemplateEnvironmentSecret(StrictModel): @@ -179,7 +178,8 @@ def populate_default_variables(variables: Optional[List[TemplateVariable]], vari if variables: for variable in variables: name = variable.name - if name not in variable_values and variable.default is not None: + # Apply defaults only for explicitly optional variables + if variable.optional and name not in variable_values and variable.default is not None: variable_values[name] = variable.default @@ -365,8 +365,8 @@ def validate_defines_all_required_secrets(instance: InstanceDefinition, template secrets = instance.secrets for template_secret in template.secrets or []: name = template_secret.name - has_default = template_secret.default is not None - if name not in secrets and not has_default: + is_optional = bool(template_secret.optional) + if name not in secrets and not is_optional: raise RequestParameterMissingException(f"Must define secret '{name}'") @@ -374,8 +374,8 @@ def validate_defines_all_required_variables(instance: InstanceDefinition, templa variables = instance.variables for template_variable in template.variables or []: name = template_variable.name - has_default = template_variable.default is not None - if name not in variables and not has_default: + is_optional = bool(template_variable.optional) + if name not in variables and not is_optional: raise RequestParameterMissingException(f"Must define variable '{name}'") @@ -391,7 +391,18 @@ def validate_specified_datatypes(instance: InstanceDefinition, template: Templat def validate_specified_datatypes_variables(variables: Dict[str, Any], template: Template): for template_variable in template.variables or []: name = template_variable.name - variable_value = variables.get(name, template_variable.default) + # Only fall back to default for optional variables + if name in variables: + variable_value = variables[name] + elif template_variable.optional: + variable_value = template_variable.default + else: + variable_value = None + + # Skip validation only if variable is optional, not provided, and has no default applied + if name not in variables and template_variable.optional and template_variable.default is None: + continue + template_type = template_variable.type if template_type in ["string", "path_component"]: if not isinstance(variable_value, str): @@ -412,8 +423,8 @@ def validate_specified_datatypes_variables(variables: Dict[str, Any], template: if not _is_of_exact_type(variable_value, bool): raise RequestParameterInvalidException(f"Variable value for variable '{name}' must be of type bool") - # Run custom validators if present and value is not None or empty - if template_variable.validators and variable_value is not None and variable_value != "": + # Run custom validators if present. + if template_variable.validators: for validator in template_variable.validators: _run_variable_validator(validator, variable_value, name) diff --git a/test/unit/util/test_config_template_validation.py b/test/unit/util/test_config_template_validation.py index 90a1d4a459c..32678389238 100644 --- a/test/unit/util/test_config_template_validation.py +++ b/test/unit/util/test_config_template_validation.py @@ -346,103 +346,34 @@ def test_range_validator(): assert isinstance(e, RequestParameterInvalidException) -def test_validators_skip_empty_optional_values(): - """Test that validators skip empty optional values.""" - validator = LengthParameterValidatorModel( - min=3, - message="Value must be at least 3 characters", - ) - template = _template_with_variable( - TemplateVariableString(name="optional_field", help=None, type="string", default="", validators=[validator]) - ) +def test_variable_default_does_not_imply_optional(): + """A default value (including empty string) must not make a variable optional.""" + template = _template_with_variable(TemplateVariableString(name="string_var", help=None, type="string", default="")) - # Empty value should not trigger validator - instance = _test_instance_with_variables({"optional_field": ""}) - validate_secrets_and_variables(instance, template) - - -def test_validators_skip_null_values(): - """Test that validators skip empty values for optional fields with defaults.""" - validator = LengthParameterValidatorModel( - min=3, - message="Value must be at least 3 characters", - ) - template = _template_with_variable( - TemplateVariableString(name="optional_field", help=None, type="string", default="", validators=[validator]) - ) - - # Empty value should not trigger validator when field has empty default - instance = _test_instance_with_variables({}) - validate_secrets_and_variables(instance, template) - - -def test_secret_with_default_is_optional(): - """Test that secrets with default values are optional.""" - secret = TemplateSecret(name="mysecret", help="Help for secret.", default="default_value") - template = TestTemplate( - id=TEST_TEMPLATE_ID, - version=TEST_TEMPLATE_VERSION, - variables=[], - secrets=[secret], - environment=None, - ) - - # Should not require the secret when it has a default - instance = _test_instance_with_secrets({}) - validate_secrets_and_variables(instance, template) - - -def test_validators_run_on_optional_fields_with_values(): - """Test that validators run on optional fields when they have values.""" - validator = LengthParameterValidatorModel( - min=3, - message="Value must be at least 3 characters", - ) - template = _template_with_variable( - TemplateVariableString(name="optional_field", help=None, type="string", default="ok", validators=[validator]) - ) - - # Should run validator when optional field has a value - instance = _test_instance_with_variables({"optional_field": "ab"}) # Too short - e = assert_validation_throws(instance, template) - assert isinstance(e, RequestParameterInvalidException) - assert "at least 3 characters" in str(e) - - # Should pass validation with valid value - instance = _test_instance_with_variables({"optional_field": "valid"}) - validate_secrets_and_variables(instance, template) - - -def test_variable_with_null_default(): - """Test variables with null defaults are still required (None is not a valid default).""" - template = _template_with_variable( - TemplateVariableString(name="optional_var", help=None, type="string", default=None) - ) - - # Should still require the variable when default is None (None is not a valid default) + # Default alone does not make the variable optional instance = _test_instance_with_variables({}) e = assert_validation_throws(instance, template) assert isinstance(e, RequestParameterMissingException) + +def test_variable_optional_flag_allows_omission_without_default(): + """optional=True alone allows a variable to be omitted even without a default.""" + template = _template_with_variable( + TemplateVariableString(name="optional_var", help=None, type="string", optional=True) + ) + + # Should not require the variable when optional=True + instance = _test_instance_with_variables({}) + validate_secrets_and_variables(instance, template) + # Should pass when variable is provided instance = _test_instance_with_variables({"optional_var": "some_value"}) validate_secrets_and_variables(instance, template) -def test_variable_with_empty_string_default(): - """Test variables with empty string defaults are optional.""" - template = _template_with_variable( - TemplateVariableString(name="optional_var", help=None, type="string", default="") - ) - - # Should not require the variable when it has an empty string default - instance = _test_instance_with_variables({}) - validate_secrets_and_variables(instance, template) - - -def test_secret_without_default_is_required(): - """Test that secrets without defaults are required.""" - secret = TemplateSecret(name="requiredsecret", help="Help for secret.") +def test_secret_optional_flag_allows_omission_without_default(): + """optional=True alone allows a secret to be omitted even without a default.""" + secret = TemplateSecret(name="optional_secret", help="Help for secret.", optional=True) template = TestTemplate( id=TEST_TEMPLATE_ID, version=TEST_TEMPLATE_VERSION, @@ -451,12 +382,94 @@ def test_secret_without_default_is_required(): environment=None, ) - # Should require the secret when it has no default + # Should not require the secret when optional=True instance = _test_instance_with_secrets({}) - e = assert_validation_throws(instance, template) - assert isinstance(e, RequestParameterMissingException) - assert "requiredsecret" in str(e) + validate_secrets_and_variables(instance, template) # Should pass when secret is provided - instance = _test_instance_with_secrets({"requiredsecret": "myvalue"}) + instance = _test_instance_with_secrets({"optional_secret": "myvalue"}) + validate_secrets_and_variables(instance, template) + + +def test_optional_variable_validators_run_only_when_value_is_provided(): + """Optional variables are not required, but validators run when a value is supplied.""" + validator = LengthParameterValidatorModel( + min=3, + message="Value must be at least 3 characters", + ) + template = _template_with_variable( + TemplateVariableString(name="optional_var", help=None, type="string", optional=True, validators=[validator]) + ) + + # Should not require the variable when optional=True + instance = _test_instance_with_variables({}) + validate_secrets_and_variables(instance, template) + + # Should run validator when value is provided but invalid + instance = _test_instance_with_variables({"optional_var": "ab"}) # Too short + e = assert_validation_throws(instance, template) + assert isinstance(e, RequestParameterInvalidException) + assert "at least 3 characters" in str(e) + + # Should pass with valid value + instance = _test_instance_with_variables({"optional_var": "valid"}) + validate_secrets_and_variables(instance, template) + + # Defaults for optional fields are validated when they are applied + # Invalid default should fail validation + template_with_bad_default = _template_with_variable( + TemplateVariableString( + name="optional_var", + help=None, + type="string", + optional=True, + default="ab", # too short + validators=[validator], + ) + ) + instance = _test_instance_with_variables({}) + e = assert_validation_throws(instance, template_with_bad_default) + assert isinstance(e, RequestParameterInvalidException) + + # Valid default should pass validation + template_with_good_default = _template_with_variable( + TemplateVariableString( + name="optional_var", + help=None, + type="string", + optional=True, + default="good", + validators=[validator], + ) + ) + instance = _test_instance_with_variables({}) + validate_secrets_and_variables(instance, template_with_good_default) + + +def test_variable_optional_with_valid_default(): + """Test that optional variables with valid defaults work correctly.""" + # Optional string variable with default + template = _template_with_variable( + TemplateVariableString(name="optional_string", help=None, type="string", optional=True, default="default_value") + ) + # Should use default when not provided + instance = _test_instance_with_variables({}) + validate_secrets_and_variables(instance, template) + + # Should accept override + instance = _test_instance_with_variables({"optional_string": "override"}) + validate_secrets_and_variables(instance, template) + + # Optional integer variable with default + template = _template_with_variable( + TemplateVariableInteger(name="optional_int", help=None, type="integer", optional=True, default=100) + ) + instance = _test_instance_with_variables({}) + validate_secrets_and_variables(instance, template) + + # Optional boolean variable with default + template = _template_with_variable( + TemplateVariableBoolean(name="optional_bool", help=None, type="boolean", optional=True, default=False) + ) + instance = _test_instance_with_variables({}) validate_secrets_and_variables(instance, template)