Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REST API: Introduce batch controller #25096

Merged
merged 10 commits into from
Oct 7, 2020

Conversation

TimothyBJacobs
Copy link
Member

Description

This PR introduces a batch controller at /__experimental/batch. It includes support for a validation mode called require-all-validate that necessitates that each request pass validation before it is dispatched. This functionality requires the latest trunk. For users not on trunk, a simpler version of batching is implemented.

For example:

POST /__experimental/batch
{
	"validation": "require-all-validate",
	"requests": [
		{
			"path": "/__experimental/menu-items",
			"body": {
				"title": "Hello Moon",
				"content": "From the world",
				"type": "custom",
				"menus": 3,
				"url": "#"
			}
		},
		{
			"path": "/__experimental/menu-items",
			"body": {
				"title": "Hello World",
				"content": "From the moon",
				"type": "custom",
				"menus": 3,
				"url": "#",
				"target": "invalid"
			}
		}
	]
}
{
  "failed": "validation",
  "responses": [
    null,
    {
      "body": {
        "code": "rest_invalid_param",
        "message": "Invalid parameter(s): target",
        "data": {
          "status": 400,
          "params": {
            "target": "target is not one of _blank, ."
          }
        }
      },
      "status": 400,
      "headers": []
    }
  ]
}

See #24907, Trac#50244.

How has this been tested?

I've done manual testing of the endpoint and added some basic tests to start. I believe tests will be temporarily failing because of the changes required to the REST API server.

Types of changes

New feature

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. <!-- React Native mobile Gutenberg guidelines: https://github.com/WordPress/gutenberg/blob/master/docs/contributors/native-mobile.md -→

@TimothyBJacobs
Copy link
Member Author

PHPUnit is now passing. Before merge I'd like to expand allow_batch to indicate whether require-all-validate is supported.

Another thing to consider is a way to indicate that permission checks should be called before request validation happens. Right now Core calls permission checks after request validation which complicates things if verifying whether data is valid could wind up exposing sensitive data.

);
}

foreach ( $requests as $i => $batch_request ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: $batch_request -> $single_request

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, fixed in 47edcf6.

*
* @since 9.0.0
*/
public function register_routes() {
Copy link
Contributor

@adamziel adamziel Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand why this code had to be copied and pasted, still I would love to get rid of it (maybe not in this PR). Maybe by updating WP_REST_Posts_Controller::register_routes() to accept an optional $allow_batch=false argument? We could also add a filter to register_rest_route.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah its really not great. My current thinking is we'll just make all posts controllers opt in to batching when we do the Core merge.

/**
* Tests for the batch controller.
*/
class REST_Batch_Controller_Test extends WP_Test_REST_TestCase {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to test if allow_batch opt-in works - I don't think any test case actually sets it at the moment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that covered in test_batch_requires_allow_batch_opt_in?

Copy link
Contributor

@adamziel adamziel Oct 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like it tests the case where allow_batch is not set, but I don't see any test that actually sets allow_batch to true - maybe I'm missing something. Is it because of the core patch / versioning thing? @TimothyBJacobs

@adamziel
Copy link
Contributor

I left a few notes, it looks great overall @TimothyBJacobs ! I would still like to address this comment somehow, but it seems beyond the scope of this PR. I think it's pretty close, awesome work!

@TimothyBJacobs
Copy link
Member Author

Thanks for the review @adamziel!

Regarding the comment on the earlier PR, I've left some discussion about that on #23474. I agree we can address that in a separate PR.

Copy link
Contributor

@adamziel adamziel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update @since annotations to say 9.2.0, but that's a detail. It seems like a great first version. Let's get it in and iterate where needed. Great job @TimothyBJacobs !

@adamziel
Copy link
Contributor

adamziel commented Oct 7, 2020

E2E test failures seem unrelated to this PR and are similar to failures on recent commits. It seems like TravisCI is having a bad day :-)

@adamziel adamziel merged commit b6438f5 into WordPress:master Oct 7, 2020
@github-actions github-actions bot added this to the Gutenberg 9.2 milestone Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core REST API Task Task for Core REST API efforts REST API Interaction Related to REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants