#51025 closed enhancement (fixed)
Add support for the anyOf and oneOf JSON Schema keywords.
Reported by: | TimothyBlynJacobs | Owned by: | TimothyBlynJacobs |
---|---|---|---|
Milestone: | 5.6 | Priority: | normal |
Severity: | normal | Version: | |
Component: | REST API | Keywords: | json-schema has-patch has-unit-tests commit |
Focuses: | Cc: |
Description
These would be particularly helpful for the Image Editor Batch API: https://github.com/WordPress/gutenberg/pull/23369
Reference: https://tools.ietf.org/html/draft-fge-json-schema-validation-00#section-5.5.4
One of the tricky things is how to handle error messages. For an example schema:
{ "type": "array", "items": { "oneOf": [ { "type": "object", "properties": { "operation": { "type": "string", "enum": [ "crop" ] }, "x": { "type": "integer" }, "y": { "type": "integer" } } }, { "type": "object", "properties": { "operation": { "type": "string", "enum": [ "rotate" ] }, "degrees": { "type": "integer", "minimum": 0, "maximum": 360 } } } ] } }
If the user submitted this data:
[ { "operation": "rotate", "degrees": 450 } ]
You want to be able to give an error like "degrees must be greater than 0 and less than 360". But the most simple validation approach would give an error like "did not match against exactly one schema".
Change History (20)
This ticket was mentioned in Slack in #core-restapi by mdmag. View the logs.
4 years ago
This ticket was mentioned in PR #555 on WordPress/wordpress-develop by yakimun.
4 years ago
#3
- Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed
Added support for the anyOf and oneOf JSON Schema keywords.
Trac ticket: https://core.trac.wordpress.org/ticket/51025
TimothyBJacobs commented on PR #555:
4 years ago
#4
Thanks for working on this @yakimun! We don't usually, but in this case since the error strings are so important, can we add the actual error message to the assertions?
TimothyBJacobs commented on PR #555:
4 years ago
#5
We'll also need to add support to rest_sanitize_value_from_schema
and ideally rest_filter_response_by_context
.
I've also left some inline feedback, let me know if anything is unclear. Thanks again for working on this!
4 years ago
#6
Thanks for the review!
I'm not sure what to do with rest_sanitize_value_from_schema
and rest_filter_response_by_context
.
For rest_sanitize_value_from_schema
I suggest putting the new keywords validation right after the allowed types check. If validation fails the error could be returned from rest_sanitize_value_from_schema
.
In rest_filter_response_by_context
, we can't return WP_Error
. Returning null
on anyOf
or oneOf
validation error also doesn't look good, since null
is one of the valid values. So I have no idea how to implement it.
@TimothyBJacobs, what do you think about it?
TimothyBJacobs commented on PR #555:
4 years ago
#7
For rest_sanitize_value_from_schema I suggest putting the new keywords validation right after the allowed types check. If validation fails the error could be returned from rest_sanitize_value_from_schema.
That's another thing we need to adjust. It should be allowed to use oneOf
or anyOf
without specifying a top-level type. So it would have to occur before the type check.
What we need to be able to do is figure out the schema used for validation, and then use it for sanitization. If there isn't a schema that matches we need to do the same procedure to get a WP_Error
.
In rest_filter_response_by_context, we can't return WP_Error. Returning null on anyOf or oneOf validation error also doesn't look good, since null is one of the valid values. So I have no idea how to implement it.
The idea for this is similar, find the oneOf
/anyOf
schema that matches the value and then use that as the schema to filter the response data by.
If none apply, we can skip trying to filter that property. For oneOf
I don't think we need to check that only one applies, we can just pick the first matching.
The JSON Schema Test Suite might also be helpful: https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/master/tests/draft4/anyOf.json Something to keep in mind is that the JSON Schema spec doesn't require that a type
is specified, but the WordPress implementation does.
4 years ago
#9
@TimothyBJacobs, I've fixed the issues you noticed and improved the tests. Please let me know if I missed something. Thanks!
TimothyBJacobs commented on PR #555:
4 years ago
#10
Thanks for your continued work on this @yakimun! I've left some more feedback. I'm going to take a deeper look at the tests later too.
4 years ago
#12
Thanks for your comments, @TimothyBJacobs! Just to be clear: everything previously mentioned has been fixed.
This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.
4 years ago
This ticket was mentioned in Slack in #core-restapi by hellofromtonya. View the logs.
4 years ago
#15
@
4 years ago
This is fundamentally a fairly straight forward change. It looks good, it has good test coverage, and it's immediately useful.
I have one concern about changing error codes that I noted in the PR @TimothyBlynJacobs .
Timothy also mentioned that he's not sure about some of the wording in the error messages that are exposed, and I agree. I think there are improvements that can and should be made there.
Let's:
- Check the back compat concern with renaming
rest_invalid_param
torest_invalid_type
- Commit
- Iterate on error message construction and phrasing
#16
@
4 years ago
Copying over my message from Github:
Will definitely call it out in the dev note, but I think we're ok. The rest_invalid_param error is the most generic error codes that we use. So someone wouldn't be able to use the error code alone to tell that it is a specific error. They'd have to do analysis on the error message itself.
We also don't currently expose the error codes to API consumers. It would only impact people using the REST validation function directly.
These are the direct usages: https://wpdirectory.net/search/01EN3HSK9PJ34VCY35631DD81T. Looking thru those usages, a chunk of them are people polyfilling the function, and the others don't appear to be looking at the specific error code used. Just checking whether an error is returned or not.
#17
@
4 years ago
- Keywords commit added
@TimothyBlynJacobs want to get this committed? In the meantime I'll make a list of improvements for error messages, docs, etc.
#18
@
4 years ago
- Owner set to TimothyBlynJacobs
- Status changed from new to accepted
@johnbillion Will do! That'd be great, and thanks for the review!
TimothyBJacobs commented on PR #555:
4 years ago
#20
Fixed in 54aa0bc7d1d6ca69de297d14785fe7e1b497e858.
Thanks again for your hard work on this @yakimun!
For schemas like the example that have a property in common that is used to disambiguate, we could solve this by introducing a keyword that would signify that property. But that would really only work for these
switch
type schemas.A more generic approach might be to do something like this:
We can also mandate that each schema in the
oneOf
array has atitle
set. That way we can at least give an error message like "x is not a valid Rotation or Crop".