WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 4 weeks ago

Last modified 4 weeks ago

#46191 closed defect (bug) (fixed)

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

Reported by: tmfespresso Owned by: TimothyBlynJacobs
Milestone: 5.7 Priority: normal
Severity: normal Version: 4.4
Component: REST API Keywords: has-patch has-unit-tests
Focuses: rest-api Cc:

Description

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;
    }
}

Change History (13)

This ticket was mentioned in Slack in #core by tmfespresso. View the logs.


2 years ago

#2 @pento
2 years ago

  • Version changed from trunk to 4.4

#3 @TimothyBlynJacobs
4 months ago

  • Milestone changed from Awaiting Review to 5.7

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

This ticket was mentioned in PR #862 on WordPress/wordpress-develop by TimothyBJacobs.


7 weeks ago

  • Keywords has-patch has-unit-tests added; needs-patch removed

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


5 weeks ago

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


5 weeks ago

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


4 weeks ago

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


4 weeks ago

#9 @xkon
4 weeks ago

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
                }
            ]
        }
    ]
}

#10 @TimothyBlynJacobs
4 weeks ago

  • Milestone changed from 5.7 to 5.8
  • Summary changed from WP_REST_Request::has_valid_params() should utilize get_error_messages() and not get_error_message(). Unexpected behavior occurs when you try to WP_Error->add() in the REST API to REST API parameter validation should handle multiple error messages/codes.

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.

#11 @TimothyBlynJacobs
4 weeks ago

  • Milestone changed from 5.8 to 5.7

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.

#12 @TimothyBlynJacobs
4 weeks ago

  • Owner set to TimothyBlynJacobs
  • Resolution set to fixed
  • Status changed from new to closed

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.

Note: See TracTickets for help on using tickets.