WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 3 weeks ago

#48818 new defect (bug)

REST API does not check nested required properties

Reported by: TimothyBlynJacobs Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.9
Component: REST API Keywords:
Focuses: Cc:
PR Number:

Description

WP_REST_Server checks if a request is valid using WP_REST_Request::has_valid_params. That function first checks if all required parameters are supplied. A parameter is required if it has the required property specified. The REST API only checks for the existence of the top-level properties. Nested properties are not checked.

For example, given the following schema:

{
  "type": "object",
  "properties": {
    "my_obj": {
      "type": "object",
      "required": true,
      "properties": {
        "my_prop": {
          "type": "string",
          "required": true
        }
      }
    },
    "my_arr": {
      "type": "array",
      "required": true,
      "items": {
        "type": "object",
        "properties": {
          "my_prop": {
            "type": "string",
            "required": true
          }
        }
      }
    }
  }
}

The following data is valid.

{
  "my_obj": {
    "other_prop": "hi"
  },
  "my_arr": [
    {
      "other_prop": "hi"
    }
  ]
}

We could fix this in either WP_REST_Request::has_valid_params or rest_validate_value_from_schema.

Doing it in the request object would have the benefits of consistency with top-level required properties. It would also allow checking all required properties before checking for validity and returning it in one WP_Error object. However, there could be a BC issue if a user has specified a schema using required for child parameters, but then provided a custom validate_callback that didn't actually check that the parameter is required.

If it is in rest_validate_value_from_schema we wouldn't have the BC issue. It'd also be easier to implement since that function already recurses.

We'd also want to make sure that if a parent property isn't required, that child properties aren't required if the parent value is missing entirely. For instance given the following schema, an empty request object should be valid, but {my_prop: {}} wouldn't be.

{
  "type": "object",
  "properties": {
    "my_obj": {
      "type": "object",
      "properties": {
        "my_prop": {
          "type": "string",
          "required": true
        }
      }
    }
  }
}

Change History (3)

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


6 weeks ago

#2 @circuitbit
4 weeks ago

Just thought it would be important to mention that according to JSON-Schema spec it's also possible to have a required field as an array of strings containing the properties which are required for that object, so that would be, sort of, expected.

$schema = array(
   'type'       => 'object',
   'properties' => array(
      'first_name' => array( 'type' => 'string' ),
      'last_name'  => array( 'type' => 'string' ),
      'email'      => array( 'type' => 'string' ),
   ),
   'required'   => array( 'first_name', 'last_name', 'email' ),
);

#3 @TimothyBlynJacobs
3 weeks ago

@circuitbit this is a great point. In WordPress we use v4 of the JSON Schema except for required where we use the v3 syntax. This syntax is nice to work with for core because we do a lot of conditional adding/removing of properties as well as the addition of new properties through things like additional properties and metadata. The simpler boolean syntax allows the requiredness of the property to be entirely communicated through one array definition instead of having to modify the base schema.

That being said,. I do think supporting the array format would be quite nice, particularly for cases where you are documenting a single object property on a schema, as opposed to the schema for the entire resource. I think we could add support for that syntax in the patch for this feature.

Note: See TracTickets for help on using tickets.