Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#51025 closed enhancement (fixed)

Add support for the anyOf and oneOf JSON Schema keywords.

Reported by: timothyblynjacobs's profile TimothyBlynJacobs Owned by: timothyblynjacobs's profile 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)

#1 @TimothyBlynJacobs
4 years ago

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:

  1. Discard schemas that don't match the type.
  2. For objects, find the schema that has the most properties in common.
  3. Find the schema that has the most keywords that validate.

We can also mandate that each schema in the oneOf array has a title set. That way we can at least give an error message like "x is not a valid Rotation or Crop".

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!

yakimun commented on PR #555:


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.

#8 @hellofromTonya
4 years ago

  • Keywords needs-refresh added

Refresh needed as PR has merge conflicts.

yakimun commented on PR #555:


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.

#11 @hellofromTonya
4 years ago

  • Keywords needs-refresh removed

Merge conflict resolved.

yakimun commented on PR #555:


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 @johnbillion
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:

  1. Check the back compat concern with renaming rest_invalid_param to rest_invalid_type
  2. Commit
  3. Iterate on error message construction and phrasing

#16 @TimothyBlynJacobs
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 @johnbillion
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 @TimothyBlynJacobs
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!

#19 @TimothyBlynJacobs
4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 49246:

REST API: Add support for the oneOf and anyOf keywords.

This allows for REST API routes to define more complex validation requirements as JSON Schema instead of procedural validation.

The error code returned from rest_validate_value_from_schema for invalid parameter types has been changed from the generic rest_invalid_param to the more specific rest_invalid_type.

Props yakimun, johnbillion, TimothyBlynJacobs.
Fixes #51025.

TimothyBJacobs commented on PR #555:


4 years ago
#20

Fixed in 54aa0bc7d1d6ca69de297d14785fe7e1b497e858.

Thanks again for your hard work on this @yakimun!

Note: See TracTickets for help on using tickets.