WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 4 months ago

#50300 closed defect (bug) (fixed)

Multi type on args properties returning invalid error message when not validated

Reported by: pentatonicfunk Owned by: TimothyBlynJacobs
Milestone: 5.5 Priority: normal
Severity: normal Version: 5.3
Component: REST API Keywords: json-schema has-patch has-unit-tests
Focuses: rest-api Cc:

Description

I was able to reproduce it only on specific case when type being used are null AND integer, AND specify minimum & maximum. But its probably the case too when using exclusiveMinimum or exclusiveMaximum

Test Case

<?php

$request = new WP_REST_Request();
$request->set_attributes(
    array(
        'args' => array(
            'null_or_integer' => array(
                'type'    => array( 'null', 'integer' ),
                'minimum' => 10,
                'maximum' => 20,
            ),
        ),
    )
);

$param    = 'null_or_integer';
$value    = 1;
$is_valid = rest_validate_request_arg( $value, $request, $param );

print_r( $is_valid->get_error_message() );

Expected

null_or_integer must be between 10 (inclusive) and 20.

Actual

null_or_integer is not of type null,integer.
Which isn't true, since value passed was integer.

Change History (10)

#1 @TimothyBlynJacobs
5 months ago

  • Keywords json-schema needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 5.5
  • Version changed from 5.4.1 to 5.3

Thanks for the ticket @pentatonicfunk!

Yeah the error message when using multi types isn't very helpful. We should improve it by separately trying to find the best type for the input based on the possible types.

#2 @pentatonicfunk
5 months ago

This could be tricky to solve since rest_validate_value_from_schema have multiple responsibilities and multiple exits.
I wonder if this can be solved by break it down into smaller functions, also considering this function tend to get bigger once we add more validation args, such as new formats that recently added min/maxLength and pattern.

#3 @TimothyBlynJacobs
5 months ago

@pentatonicfunk Definitely. I'd like to introduce a class like WP_REST_Schema_Validator. Something that could also support the recommended JSON Schema Output Formats.

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


5 months ago

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

This gives us better error messages and makes sanitization more usable in different scenarios.
Trac ticket: https://core.trac.wordpress.org/ticket/50300

#5 @TimothyBlynJacobs
5 months ago

I've done a first pass at what this could look like. Essentially we match based solely on the type instead of validating each schema as a different type.

This changes a couple of sanitization tests, but I think the new output is more desirable.

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


5 months ago

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


4 months ago

#8 @TimothyBlynJacobs
4 months ago

@pentatonicfunk could you take a look at the attached PR and see if it solves your issue?

#9 @pentatonicfunk
4 months ago

@TimothyBlynJacobs yes it solves my issue

#10 @TimothyBlynJacobs
4 months ago

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

In 48306:

REST API: Make multi-typed schemas more robust.

A multi-type schema is a schema where the type keyword is an array of possible types instead of a single type. For instance, [ 'object', 'string' ] would allow objects or string values.

In [46249] basic support for these schemas was introduced. The validator would loop over each schema type trying to find a version that matched. This worked for valid values, but for invalid values it provided unhelpful error messages. The sanitizer also had its utility restricted.

In this commit, the validators and sanitizers will first determine the best type of the passed value and then apply the schema with that set type. In the case that a value could match multiple types, the schema of the first matching type will be used.

To maintain backward compatibility, if unsupported schema types are used, the value will always pass validation. A doing it wrong notice is issued in this case.

Fixes #50300.
Props pentatonicfunk, dlh, TimothyBlynJacobs.

Note: See TracTickets for help on using tickets.