-
Notifications
You must be signed in to change notification settings - Fork 15
fix: improve parameter definition validation and error reporting #184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: mainline
Are you sure you want to change the base?
Conversation
This commit includes several improvements to parameter validation: 1. Add explicit validation for None values in allowedValues across all parameter types 2. Fix error location reporting in validation error messages 3. Correct documentation comments for maxValue parameters 4. Fix type casting in JobPathParameterDefinition 5. Add test cases for both implicit and explicit None handling The changes ensure that explicitly set None values for allowedValues are properly rejected with clear error messages, while implicitly omitted allowedValues continue to work correctly. Signed-off-by: Roman Yakobenchuk <[email protected]>
|
@@ -1438,7 +1446,7 @@ def _validate_allowed_values_item( | |||
errors.append( | |||
InitErrorDetails( | |||
type="value_error", | |||
loc=("allowedValues", i), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before, the error included allowedValues
twice for JobPathParameterDefinition
:
'loc': ('allowedValues', 'allowedValues', 2)
After:
('allowedValues', 2)
This is now also consistent with JobStringParameterDefinition
and other job parameter types as well.
"name": "Foo", | ||
"type": "STRING", | ||
}, | ||
id="allowedValues is implicitly None", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Omitting we intend to be legal which makes sense - but we consider that "implicit None" rather than "implicit Any" or similar? In our code we test for None as an error case, so None even implicitly seems odd that our tests are saying it should be valid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would renaming this id to allowedValues not provided
address your comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep that would work!
Fix parameter definition validation and error reporting
This PR improves parameter definition validation across all parameter types to properly handle
allowedValues
and provide clearer error messages.Fixes: #179
What was the problem/requirement? (What/Why)
There were several issues with parameter definition validation:
None
values forallowedValues
, without a clear error messagemaxValue
parameters incorrectly described them as "Minimum value"JobPathParameterDefinition
was using the wrong class nameWhat was the solution? (How)
None
values inallowedValues
across all parameter types (String, Path, Int, Float) with clear error messages("allowedValues", i)
formatmaxValue
parameters to say "Maximum value"JobPathParameterDefinition
to use the correct class nameNone
handlingWhat is the impact of this change?
This change ensures that:
None
values forallowedValues
are properly rejected with clear error messagesallowedValues
continue to work correctlyHow was this change tested?
Added test cases for both implicit and explicit
None
handling across all parameter types. All existing and new tests pass successfully.Yes, all unit tests pass.
Was this change documented?
Yes, fixed incorrect documentation comments for
maxValue
parameters.Is this a breaking change?
No, this is not a breaking change. It improves validation to reject invalid inputs that should have been rejected before, while maintaining compatibility with valid inputs. The error message itself has changed, which could be considered a breaking change but the benefit should be worth it. Compare:
to
Does this change impact security?
No.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.