-
Notifications
You must be signed in to change notification settings - Fork 0
feat: ✨ handle grouped errors under the enum constraint #183
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: main
Are you sure you want to change the base?
Conversation
| Path components are separated by '/'. | ||
| jsonpath (str): The JSON path to the field that violates the check. | ||
| instance (Any): The part of the object that failed the check. | ||
| schema_value (Optional[Any]): The part of the schema violated by this error. |
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.
E.g., for type checks, this would be the name of the type. I'm using this below to list the possible types for enum values in the error message.
lwjohnst86
left a 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.
Neat! Just some fairly small comments
| message = "All enum values must be the same type." | ||
| same_type = len(set(_map(parent_error.instance, lambda value: type(value)))) == 1 | ||
| if same_type: | ||
| allowed_types = set(_map(value_errors, lambda error: str(error.schema_value))) |
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.
What is the set() doing here? Why is it necessary?
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.
Because there can be more errors than allowed types.
E.g., if the allowed types are array and object and the enum array is [1, 2], then both enum items will cause errors for both allowed types. So 4 errors but only 2 allowed types.
Description
This PR handles grouped errors at
$.resources[x].schema.fields[x].constraints.enum. It's the worst one yet!This schema node is deeply nested, so handling errors here is a little more complex.
The situation is the following: for each field, you can list the allowed values for that field under
"constraints": {"enum": [...]}. Depending on the type of the field, the type of the values inenumcan be different. Sometimes it's straightforward (e.g. for string fields, enum values have to be strings), but sometimes the schema forenumis more permissive (e.g. for number fields, enum values can be either numbers or strings). However, you cannot mix types.If
enumhas mixed types or if some of the values are the wrong type, all sub-schemas underenumflag errors. These are usually contradictory, e.g., for a number field[1, "inf"]will alert both because 1 is not a string and because "inf" is not a number...The logic in the function:
enumproperty itself (not an array, duplicate items, etc.) -> keep only thesePart of #15
Needs an in-depth review.
Checklist
just run-all