Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Sep 25, 2020

Added support for the anyOf and oneOf JSON Schema keywords.

Trac ticket: https://core.trac.wordpress.org/ticket/51025

@TimothyBJacobs
Copy link
Member

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
Copy link
Member

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!

@ghost
Copy link
Author

ghost commented Sep 30, 2020

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
Copy link
Member

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.

@ghost
Copy link
Author

ghost commented Oct 8, 2020

@TimothyBJacobs, I've fixed the issues you noticed and improved the tests. Please let me know if I missed something. Thanks!

@TimothyBJacobs
Copy link
Member

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.

@ghost
Copy link
Author

ghost commented Oct 13, 2020

Thanks for your comments, @TimothyBJacobs! Just to be clear: everything previously mentioned has been fixed.

This is technically not as accurate, but I think the tradeoff for making the
message more friendly for users it worth it.
@TimothyBJacobs
Copy link
Member

Fixed in 54aa0bc.

Thanks again for your hard work on this @yakimun!

@ghost ghost deleted the 51025 branch March 29, 2021 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants