Read through the google doc, have some thoughts. Most of them are critical. That's not because I think module designations are a bad idea, just to poke at it to see how solid it is. Some of them are questions that you've probably already thought through, so "that's not a problem" is a great response. My comments below are mostly based on the linked google doc (which talks about module designations in a general sense), not the task description (which is more focused on private modules).

Additional thought: we have at least two http DELETE endpoints that do not use tokens (ReadingLists DELETE lists and DELETE lists/{id}/entries) that intentionally do not use csrf protection, and for compatibility reasons cannot. (The MW REST endpoints were created to be compatible with the original RESTBase endpoints, which did not require csrf tokens for these endpoints. Existing mobile apps will therefore call these endpoints without a csrf token, so requiring it now would be a user-facing breaking change, as users may have older version of the mobile apps on their devices.)

Some quick thoughts:

Given that this has a patch against it, moving it to In Progress

Given that this has a patch against it, I'm moving it to In Progress

Tagging serviceops (hopefully I picked the right tag).

In T365535#9820086, @daniel wrote:

I'm not saying we would (or wouldn't) do that, just confirming what the behavior would be.

It would certainly makes things a lot simpler. Adding fields is trivial. Replacing error handling would be more work.

Per discussion, changes in error responses are problematic, as the apps care about specific fields in the error JSON. The MW REST endpoints need to be adjusted for compatibility.

@Dbrant , question that I didn't think to ask during our synchronous meeting: are extraneous fields problematic for the app? In other words, if the error response json had the values from both the RESTBase error AND the MW REST error, would the app be happy? So for example:

The following endpoints were exposed with trailing slashes under RESTBase:

In T340105#9805519, @daniel wrote:

We had a meeting about this yesterday, with Bill, Thomas, Chris, Andrew, Daniel, Timo, Roan, Subbu

There seems to be a lot of support for the idea.

Below is a summary of our discussion, paraphrased based on my memory:

From a quick look, the error leading to the 400 is "rest-json-body-parse-error". I see that rMW020c7f5e7578: REST: introduce getSupportedRequestTypes() causes RequestData::hasBody() to return true if post params are present (previously the presence of post parameters did not cause this). And RequestData::hasBody() returning true causes Handler::processRequestBody() to be invoked, which tries to parse the body as json, fails and throws the above error.

The attached patch has some issues, mostly with how tests were modified. I'll look at improving that. But hopefully it at least illustrates what would be needed to deprecate this.

This was for the prototype service, which has been superseded by a permanent solution. This task is therefore no longer relevant.

I think this is done. If T362480: Introduces the notion of modules into the REST API framework continues the direction it is currently trending and is merged, then it changes some things with the MW REST API swagger spec that we'll want to address. But that would be more properly done under a new task.

Move the logic into a REST endpoint, and turn opensearch_desc.php into a redirect
Deprecate and eventually remove opensearch_desc.php

As @Tgr points out, the term "post" is confusing because it could be taken to mean either the HTTP POST method or $_POST.

1. Risky Patch! 🚂🔥

Patches merged, this is done. Thanks for thoughts and review.

I'm unsure whether to comment on the patch or here, as there is discussion in both places and it is a different set of people participating in each. I chose here because, from what I've seen, Phab tends to be more useful as a decision record, and also tends to be used by both engineers and non-engineers (EMs, PMs, etc.).

I've marked the core change as ready for review.

Looking through the potentially affected code, to see what would need to be changed if we go with the approach from my previous comment.

Daniel and I discussed this a bit more in Slack, and agreed to modify the plan somewhat. We will still be disallowing optional path parameters in routes. But we will be allowing them in handlers (more specifically, in GetParamSettings()).

In T359652#9669843, @daniel wrote:

I think it's useful to have suport for optional parameters in the parameter validator, so we can register the same handler for multiple routes, some of which may be missing some parameters.

The above patch just adds a few more test cases for PathMatcher. It does not change any existing PathMatcher behaviors.

In T359652#9674146, @Krinkle wrote:

• We have no known uses of, or use cases for, optional path components — other than the current Reading Lists API migration.

In T359652#9669843, @daniel wrote:

I think it's useful to have suport for optional parameters in the parameter validator, so we can register the same handler for multiple routes, some of which may be missing some parameters.

There might be a sixth option. If /myextension/foo/ and /myextension/foo/{id} (where ID can be optional) are considered conflicting routes today, does that mean there is also the option of implementing what you're looking for, by registering the latter and having the route handler respond with what you want from a conditional !id branch?

Hearing no objections, I'm planning to work on patches for disallowing non-required path parameters in the REST API. If anyone thinks that's a bad idea, please say so.

To elaborate a bit on the OpenAPI/Swagger issue this section of the swagger.io params documentation page reinforces that path parameters cannot be non-required. Specifically, this text: "path parameters must have required: true, because they are always required."

Tagging MW Engineering for visibility. Not promising this will be picked up, but we'll at least triage it.

REST API endpoints were created for (the duplicate of) this task. They were then determined to be unnecessary and will be removed as unused under https://gerrit.wikimedia.org/r/c/mediawiki/core/+/779917

REST API endpoints were created for this task. They were then determined to be unnecessary and will be removed as unused under https://gerrit.wikimedia.org/r/c/mediawiki/core/+/779917

Summary of related Slack discussion that occurred before this task was created. @daniel and @pmiazga , please correct anything that I misrepresented from your Slack comments.

Triaging this as Low priority, mostly because it has been around quite some time and it seems people are working around this for the moment. Feel to raise the priority if this is a blocker for anything you're doing.

Unable to reproduce at the moment. Which doesn't mean this isn't an issue, it just means it is challenging to diagnose.

Initial implementation of new handlers have been merged and is available on production. Nothing is routing to them, so there is no change to callers at this time. WMF Mobile apps are still hitting the RESTBase endpoints, with no changes. This will be the case until we proactively reroute calls to the new endpoints via the API Gateway. We do not plan to do this until callers have had the opportunity to review changes, and any necessary adjustments are made.

Added the following parameters that were previously exposed by the underlying Action API endpoint, but not by RESTBase: