REST API parameter validation should handle multiple error messages/codes.

[tmfespresso]

This is best illustrated via some code that attempts to extend the REST API, please see the comments above CoffeeApi->validateBeans().

<?php
class CoffeeApi {

    // CONTEXT ONLY: this is fine and provided for context 
    public function registerRoutes()
    {
        $version = '1';
        $namespace = 'coffee/v' . $version;
        $base = 'espresso';

        register_rest_route( $namespace, '/' . $base,
            [
                [
                    'methods'  => \WP_REST_Server::READABLE, // GET
                    'callback' => [$this, 'statusCheck'],
                    'permission_callback' => [$this, 'authorize'],
                ],
                [
                    'methods'  => \WP_REST_Server::CREATABLE, // POST
                    'callback' => [$this, 'createOrUpdate'],
                    'permission_callback' => [$this, 'authorize'],
                    'args' => array(
                        'beanId' => array(
                            'validate_callback' => [$this, 'validateBeans']
                        ),
                    ),
                ],
            ]
        );
    }

    // CONTEXT ONLY: this is fine and provided for context 
    public function createOrUpdate(\WP_REST_Request $request)
    {
        $parameters = $request->get_json_params();
        // TODO: pretend stuff happens here 
        return rest_ensure_response( 'SUCCESS: Record created.' );

    }

    /**
    * WARNING: this does not result in the expected behavior
    *
    * TEST CASE: the parameter tested here is non-numeric and fails the first condition 
    *
    * EXPLANATION OF PROBLEM: 
    * Because of https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/class-wp-rest-request.php#L878 
    * utilizing get_error_message() instead of get_error_messages(), only 'Bean data is invalid.' gets returned. 
    * E.g.
    *
    * {"code":"rest_invalid_param","message":"Invalid parameter(s): merchantAccountId","data":{"status":400,"params":
    * {"merchantAccountId":"User data is invalid."}}}
    *
    * If get_error_messages() is used, the error response looks like this:
    * {"code":"rest_invalid_param","message":"Invalid parameter(s): merchantAccountId","data":{"status":400,"params":
    * {"merchantAccountId":["User data is invalid.","Merchant Account ID must be numeric."]}}}
    ** / 
    public function validateBeans($rawBeanId){

        $userError = new \WP_Error( 'rest_invalid_validate_beans', esc_html__( 'Bean data is invalid.', 'coffee-api' ), array( 'status' => 400 ) );

        // this gets ignored in the REST API returned errors
        if(!is_numeric( $rawBeanId )){
            $userError->add('rest_invalid_validate_beans', esc_html__( 'Bean data must be numeric.', 'coffee-api' ));
            return $userError;
        }

        // if you get here, this also gets ignored in the REST API returned errors
        if(!$this->otherCondition()){
            $userError->add('rest_invalid_validate_beans', esc_html__( 'Fail for funsies.', 'coffee-api' ));
            return $userError;
        }
    
        return true;
    }
}

[TimothyBlynJacobs]

Now that we can merge multiple WP_Error objects I think this is something we can look at addressing. Milestoning for 5.7

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

[xkon]

PR looks good to me! A couple of minor things noticed, not blockers in any way but we could consider them for making the response a bit easier to read & follow:

One is the error statuses that are returned on all instances ( see following example code & responses ) we always have x2 status as an example with this test code. Ideally we should be able to show an 1:1 relationship of which status goes with which error as it might be a bit confusing at the moment. This might need to be dealt within WP_Error though.

Also maybe we can clean up the additional_errors a bit by not duplicating the "data" as well on each iteration.

---

For testing purposes I'm adding a simple snippet ( can be used as a mu-plugin ).

Can be tested with Postman via an ( example ) :

POST ​https://core.local/src/index.php?rest_route=/ticket/v1/test&id=1

<?php

add_action( 'rest_api_init', 'add_custom_users_api');
function add_custom_users_api() {
	register_rest_route(
		'ticket/v1',
		'/test',
		array(
			'methods' => 'POST',
			'callback' => 'my_ticket_test',
			'args' => array(
				'id' => array(
					'validate_callback' => 'test_validate'
				),
			),
		)
	);
}

function test_validate( $id ) {

	$error = new \WP_Error( 'rest_test_error', 'This is our first error.', array( 'status' => 410 ) );

	if ( is_numeric( $id ) ) {
		$error->add( 'rest_test_error', 'Well this is yet another error :D.', array( 'status' => 420 ) );

		return $error;
	}

	return true;
}

function my_ticket_test( \WP_REST_Request $request ) {
	return 'success';
}

---

Pre-patch the response would've been:

{
    "code": "rest_invalid_param",
    "message": "Invalid parameter(s): id",
    "data": {
        "status": 400,
        "params": {
            "id": "This is our first error."
        }
    }
}

After applying PR we get:

{
    "code": "rest_invalid_param",
    "message": "Invalid parameter(s): id",
    "data": {
        "status": 400,
        "params": {
            "id": "This is our first error. Well this is yet another error :D."
        }
    },
    "additional_errors": [
        {
            "code": "rest_test_error",
            "message": "This is our first error.",
            "data": {
                "param": "id"
            },
            "param": "id",
            "additional_data": [
                {
                    "status": 420
                },
                {
                    "status": 410
                }
            ]
        },
        {
            "code": "rest_test_error",
            "message": "Well this is yet another error :D.",
            "data": {
                "param": "id"
            },
            "param": "id",
            "additional_data": [
                {
                    "status": 420
                },
                {
                    "status": 410
                }
            ]
        }
    ]
}

[TimothyBlynJacobs]

Thanks for testing @xkon! I'm reiterating some of our conversation on Slack to note it for posterity. And this sort of turned into a stream of consciousness, so apologies in advance :D.

Yeah so one of the issues is that error data is attached to the error code, not the code and message together thru add(). Disambiguating this is I think essentially impossible because even if we did somehow make changes to add to keep track of this, a user can just call add_data at anytime.

I was thinking we might be able to only include data for the first time an error code appears. So for example:

{
  "code": "rest_invalid_param",
  "message": "Invalid parameter(s): id",
  "data": {
    "status": 400,
    "params": {
      "id": "This is our first error. Well this is yet another error :D."
    }
  },
  "additional_errors": [
    {
      "code": "rest_test_error",
      "message": "This is our first error.",
      "data": {
        "param": "id"
      },
      "param": "id",
      "additional_data": [
        {
          "status": 420
        },
        {
          "status": 410
        }
      ]
    },
    {
      "code": "rest_test_error",
      "message": "Well this is yet another error :D.",
      "param": "id"
    }
  ]
}

This, however, would be a breaking change. We would have to instead only omit the additional_data, and keep the data around.

{
  "code": "rest_invalid_param",
  "message": "Invalid parameter(s): id",
  "data": {
    "status": 400,
    "params": {
      "id": "This is our first error. Well this is yet another error :D."
    }
  },
  "additional_errors": [
    {
      "code": "rest_test_error",
      "message": "This is our first error.",
      "data": {
        "param": "id"
      },
      "param": "id",
      "additional_data": [
        {
          "status": 420
        },
        {
          "status": 410
        }
      ]
    },
    {
      "code": "rest_test_error",
      "message": "Well this is yet another error :D.",
      "data": {
        "param": "id"
      },
      "param": "id"
    }
  ]
}

This reduces the response size. But may make it harder for clients that need to operate on the error data more complicated. However, I suppose any operation on that would be wrong since they wouldn't be able to use the right data.

I'm not sure what the correct move is here. If I could see anyway to actually track error data to a code/message, I'd punt until we could figure that out. But I'm really not sure what that would look like.

For example, imagine two enum parameters enum_a and enum_b. If we wanted to get the enum for each, we'd run into an issue disambiguating.

{
  "code": "rest_invalid_param",
  "message": "Invalid parameter(s): enum_a, enum_b",
  "data": {
    "status": 400,
    "params": {
      "enum_a": "enum_a is not one of a, b, c.",
      "enum_b": "enum_b is not one of d, e, f."
    }
  },
  "additional_errors": [
    {
      "code": "rest_not_in_enum",
      "message": "enum_a is not one of a, b, c.",
      "data": {
        "param": "enum_a"
      },
      "param": "enum_a",
      "additional_data": [
        {
          "enum": ["a", "b", "c"]
        },
        {
          "enum": ["d", "e", "f"]
        }
      ]
    },
    {
      "code": "rest_not_in_enum",
      "message": "enum_b is not one of d, e, f.",
      "data": {
        "param": "enum_b"
      },
      "param": "enum_b",
      "additional_data": [
        {
          "enum": ["a", "b", "c"]
        },
        {
          "enum": ["d", "e", "f"]
        }
      ]
    }
  ]
}

Thinking on it more, it would actually be worse since the same thing would happen to the param data.

{
  "code": "rest_invalid_param",
  "message": "Invalid parameter(s): enum_a, enum_b",
  "data": {
    "status": 400,
    "params": {
      "enum_a": "enum_a is not one of a, b, c.",
      "enum_b": "enum_b is not one of d, e, f."
    }
  },
  "additional_errors": [
    {
      "code": "rest_not_in_enum",
      "message": "enum_a is not one of a, b, c.",
      "data": {
        "param": "enum_a"
      },
      "param": "enum_a",
      "additional_data": [
        {
          "param": "enum_b"
        },
        {
          "enum": ["a", "b", "c"]
        },
        {
          "enum": ["d", "e", "f"]
        }
      ]
    },
    {
      "code": "rest_not_in_enum",
      "message": "enum_b is not one of d, e, f.",
      "data": {
        "param": "enum_a"
      },
      "param": "enum_a",
      "additional_data": [
        {
          "param": "enum_b"
        },
        {
          "enum": ["a", "b", "c"]
        },
        {
          "enum": ["d", "e", "f"]
        }
      ]
    }
  ]
}

I wonder if @dlh or @johnbillion have any thoughts on this. It feels like an inescapable problem that data isn't attached to a code and message.

Given all that, I'm going to punt this. I'm not certain what the right solution is, or if there is one, but this definitely needs improvement.

[TimothyBlynJacobs]

After much brainstorming with @xkon, we came to the conclusion that combining errors like I was in WP_REST_Request is _doing_it_wrong. When you are manually constructing a WP_Error object, you wouldn't wind up in that scenario since each error check is distinct from each other.

The issue here is that the WP_REST_Request code is trying to be generic and combine all the errors from validation even though they are unconnected error instances and due to how schema validation works, likely to have duplicate error codes.

So the proper fix for this is to instead account for it when building the validation error. Now, the previous example will look like this.

{
  "code": "rest_invalid_param",
  "message": "Invalid parameter(s): enum_a, enum_b",
  "data": {
    "status": 400,
    "params": {
      "enum_a": "enum_a is not one of a, b, c.",
      "enum_b": "enum_b is not one of d, e, f."
    },
    "details": {
      "enum_a": {
        "code": "rest_not_in_enum",
        "message": "enum_a is not one of a, b, c.",
        "data": {
          "enum": ["a", "b", "c"]
        }
      },
      "enum_b": {
        "code": "rest_not_in_enum",
        "message": "enum_b is not one of d, e, f.",
        "data": {
          "enum": ["d", "e", "f"]
        }
      }
    }
  }
}

The details is built using rest_convert_error_to_response so it has a consistent error format.

[TimothyBlynJacobs]

In 50150:

REST API: Return detailed error information from request validation.

Previously, only the first error message for each parameter was made available. Now, all error messages for a parameter are concatenated. Additionally, the detailed error for each parameter is made available in a new details section of the validation error. Each error is formatted following the standard REST API error formatting.

The WP_REST_Server::error_to_response method has been abstracted out into a standalone function rest_convert_error_to_response to allow for reuse by WP_REST_Request. The formatted errors now also contain an additional_data property which contains the additional error data provided by WP_Error::get_all_error_data.

Props dlh, xkon, TimothyBlynJacobs.
Fixes #46191.

[prbot]

​TimothyBJacobs commented on ​PR #862:

Merged in ​https://github.com/WordPress/wordpress-develop/commit/7f049d3ac168dc0461f2322aa2af1a10fd1e734e.