Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#51020 closed defect (bug) (fixed)

The public route args schema drops valid JSON Schema properties

Reported by: raubvogel's profile raubvogel Owned by: timothyblynjacobs's profile TimothyBlynJacobs
Milestone: 5.6 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: dev-feedback has-patch has-unit-tests
Focuses: rest-api Cc:

Description

The minItems and maxItems documented at https://developer.wordpress.org/rest-api/extending-the-rest-api/schema/#minitems-and-maxitems are not shown in the argument schema.

Add argument like (note minItems and maxItems)

<?php
$args = ['f_marker_author_args'    => [
                                'description' => __( 'Filter.', PVGW_TEXT_DOMAIN ),
                                'required'    => false,
                                'type'        => 'array',
                                'minItems'    => 1,
                                'maxItems'    => 2,
                                'items'       => [ 'type' => 'integer' ],
                        ]]

to

<?php
register_rest_route( $this->namespace, '/' . $this->rest_base,
                        array(
                                // register the readable endpoint for collections
                                [
                                        'methods'             => WP_REST_Server::READABLE,
                                        'callback'            => [ $this, 'get_items' ],
                                        'permission_callback' => [ $this, 'get_items_permissions_check' ],
                                        'args'                => $args,
                                ],
                                // register schema callback
                                'schema' => [ $this, 'get_public_item_schema' ],
                        )
                );

If I do something like OPTIONS http://localhost/wordpress-beta/wp-json/pvgw/v1/markers, in part I only get

"f_marker_author_args": {
                    "required": false,
                    "description": "Filter.",
                    "type": "array",
                    "items": {
                        "type": "integer"
                    }
                },

The keys minItems and maxItems are missing.

Attachments (2)

51020#.diff (3.4 KB) - added by raubvogel 4 years ago.
51020#.2.diff (3.6 KB) - added by raubvogel 4 years ago.
version 2

Download all attachments as: .zip

Change History (16)

#1 @TimothyBlynJacobs
4 years ago

  • Milestone changed from Awaiting Review to 5.6
  • Summary changed from minItems/maxItems not shown in argument schema to The public route args schema drops valid JSON Schema properties
  • Version changed from trunk to 4.7

Thanks for the ticket @raubvogel!

This is because the REST Server only adds a small number of the JSON Schema properties to the args: https://github.com/WordPress/wordpress-develop/blob/8c378606ab14c2e426c719de231bf63b05d45420/src/wp-includes/rest-api/class-wp-rest-server.php#L1300

The full list of what should be allowed is in WP_REST_Controller::get_endpoint_args_for_item_schema(): https://github.com/WordPress/wordpress-develop/blob/8c378606ab14c2e426c719de231bf63b05d45420/src/wp-includes/rest-api/endpoints/class-wp-rest-controller.php#L632

We should probably update WP_REST_Server::get_data_for_route() to include that same list of valid property names.

Do you want to work on a patch for this?

#2 @raubvogel
4 years ago

@TimothyBlynJacobs, thank you! Yes, I will give it a try …

@raubvogel
4 years ago

#3 @raubvogel
4 years ago

  • Keywords has-patch added; needs-patch removed

@TimothyBlynJacobs, can you review the patch, please? I did some testing (with my Plugin) and it seems to work.

Last edited 4 years ago by raubvogel (previous) (diff)

#4 @TimothyBlynJacobs
4 years ago

Thanks for the patch @raubvogel, this looks good!

Instead of making this a method on WP_REST_Controller though, let's make this a standalone function: rest_get_allowed_schema_keywords.

Do you want to try writing unit tests for this?

#5 @raubvogel
4 years ago

@TimothyBlynJacobs, ok, on my way …

@raubvogel
4 years ago

version 2

#6 @raubvogel
4 years ago

@TimothyBlynJacobs, next patch is ready for review.

I guess for the unit tests, I will need some help (phpunit is working already). I never have done this for WordPress before.

#7 @raubvogel
4 years ago

  • Keywords needs-testing added

#8 @TimothyBlynJacobs
4 years ago

  • Keywords needs-unit-tests added; needs-testing removed

That's looking good @raubvogel! I'd recommend opening a Pull Request in WordPress/wordpress-develop. That will run unit tests as well as linting automatically.

In terms of writing the test. You'll want to add a test case to the existing collection of tests covering WP_REST_Server. These are in the tests/phpunit/tests/rest-api/rest-server.php file. An example of one of these tests is test_get_index. I would add another method after that one following a pattern something like this.

<?php
public function test_get_index_for_route() {
    $server = new WP_REST_Server();
    $server->register_route(
        'test-ns/v1',
        '/test',
        array(
            array(
                'methods'  => WP_REST_Server::READABLE,
                'callback' => '__return_true',
                'args'     => array(
                    // The list of args you want to verify.
                    'test' => array( 'type' => 'array' ),
                ),
            ),
        )
    );

    $request = new WP_REST_Request( 'GET', '/test-ns/v1' );
    $index   = $server->dispatch( $request );
    $data    = $index->get_data();
    $args    = $data['routes']['/test']['args'];

    $this->assertArrayHasKey( 'schemaKeyword', $args['test'] );
}

In the route registration you'd add the arguments that use the different schema keywords. Then verify that they are present in the output.

I hope that helps, let me know if you run into issues. You can ping me on Slack timothybjacobs.


Something we'll might want to consider is whether we should traverse the schema's properties and items and apply the same sanitization as we do on the top-level. It was decided not to do that for items in #38420. I think we'd probably stick with that decision unless that are compelling reasons we shouldn't.

#9 follow-up: @TimothyBlynJacobs
3 years ago

Hi @raubvogel!

Just wanted to check in with you. Are you still interested in trying to write Unit Tests for this patch? No worries if not.

#10 in reply to: ↑ 9 @raubvogel
3 years ago

Replying to TimothyBlynJacobs:

Hi @TimothyBlynJacobs!

I had a short look at the Unit Tests, but for now it would be nice if you could write them – sorry for that!

Can I do something else? Is the patch fine?

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


3 years ago
#11

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

#12 follow-up: @TimothyBlynJacobs
3 years ago

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

In 49257:

REST API: Make sure all supported JSON Schema keywords are output in the index.

Previously, only a small subset of keywords were exposed which limited the utility of OPTIONS requests.

Props raubvogel, TimothyBlynJacobs.
Fixes #51020.

#13 in reply to: ↑ 12 @raubvogel
3 years ago

@TimothyBlynJacobs, thank your very much!

Note: See TracTickets for help on using tickets.