Page MenuHomePhabricator

REST: Make PARAM_ISMULTI require arrays in JSON
Closed, ResolvedPublic

Description

The ParamValidator::PARAM_ISMULTI setting causes strings to be split in arrays. This makes sense in the context where param seetings where originally used (query parameters and HTML form data) but it's confusing in a JSON request body. So in case we have a JSON request body, require a parameter that has PARAM_ISMULTI to be an array. To achieve this, we can use the same mechanism we already implemented for requiring the correct types for numeric parameters in JSON, throug the TypeDef::OPT_ENFORCE_JSON_TYPES option.

Originally Filed

Original title: REST: disallow PARAM_ISMULTI in JSON

The use of ParamValidator::PARAM_ISMULTI is confusing in a JSON request body. An ArrayDef should be used instead (T362108: Implement JSON schema validation in ArrayDef).

To implement this restriction, RestStructureTest::testBodyParameters should fail if ParamValidator::PARAM_ISMULTI is set.

Note that this will also prohibit the use of ParamValidator::PARAM_ISMULTI with form data requests. While this is a conceivable valid use case, we don't anticipate that we will actually encouonter it. If and when we do, we can implement a smarter solution as needed.

Event Timeline

BPirkle changed the task status from Open to In Progress.Jun 27 2024, 3:18 PM
BPirkle triaged this task as Medium priority.
BPirkle moved this task from Incoming (Needs Triage) to In Progress on the MW-Interfaces-Team board.

Change #1050677 had a related patch set uploaded (by Fgoodwin; author: Fgoodwin):

[mediawiki/core@master] Disallow PARAM_IS_MULTI in JSON

https://gerrit.wikimedia.org/r/1050677

Change #1052808 had a related patch set uploaded (by Fgoodwin; author: Fgoodwin):

[mediawiki/extensions/GrowthExperiments@master] Disallow PARAM_ISMULTI as JSON Body parameter

https://gerrit.wikimedia.org/r/1052808

I have been wondering if we shoudl take a different approch to this.
We now have the OPT_ENFORCE_JSON_TYPES option, that forces strict type checks on parameters comng from JSON.
We could use that to require an array when PARAM_ISMULTI is set. That way, we'd keep support for PARAM_ISMULTI for body params, without the awkwardness of automatically pslitting strings contained in a JSON structure. For form data, we'd still support that behavior, as before.

I made an experimental patch exploring this idea. It needds more tests, but it seems to wrok: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1052817

What do you think of that approach?

Restricting to an array seems less disruptive than disallowing PARAM_ISMULTI entirely. What additional tests are needed?

Restricting to an array seems less disruptive than disallowing PARAM_ISMULTI entirely. What additional tests are needed?

High level tests cases in HandlerTest, similar to the ones we have around "type coercion" for integers. PARAM_ISMULTI is basically a kind of type coercion for lists.

Let me make sure I understand the new idea.

If the value being validated is already an array by the time it gets to ParamValidator::validateValue(), then great, just use it as-is and continue validation.
If not, then if OPT_ENFORCE_JSON_TYPES is set, throw an exception.
Finally, if the value wasn't an array and we're not enforcing JSON types, then try to decode as a multivalue.

OPT_ENFORCE_JSON_TYPES will be set by default if the body is not form data. In other words, we will enforce JSON types on JSON bodies, unless a specific handler explicitly goes out of its way to override things to tell us not to.

The end result is that form data bodies can still happily and conveniently use PARAM_ISMULTI, while for JSON it is possible but only if a handler goes out of its way to say it wants to do that.

If I understood that correctly, then that seems reasonable and I have no objection.

If I understood that correctly, then that seems reasonable and I have no objection.

Yep, that's the idea.

daniel renamed this task from REST: disallow PARAM_IS_MULTI in JSON to REST: Make PARAM_IS_MULTI requrie arrays in JSON.Jul 11 2024, 2:49 PM
daniel updated the task description. (Show Details)

Change #1052817 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/core@master] EXPERIMENT: force multi-value to be an array in json.

https://gerrit.wikimedia.org/r/1052817

Change #1053985 had a related patch set uploaded (by Fgoodwin; author: Fgoodwin):

[mediawiki/core@master] WIP: Tests on forcing multi-value to be an array in JSON

https://gerrit.wikimedia.org/r/1053985

daniel renamed this task from REST: Make PARAM_IS_MULTI requrie arrays in JSON to REST: Make PARAM_ISMULTI requrie arrays in JSON.Jul 18 2024, 3:06 PM
daniel updated the task description. (Show Details)
Aklapper renamed this task from REST: Make PARAM_ISMULTI requrie arrays in JSON to REST: Make PARAM_ISMULTI require arrays in JSON.Jul 18 2024, 7:22 PM

Change #1052817 merged by jenkins-bot:

[mediawiki/core@master] REST: force multi-value to be an array in json.

https://gerrit.wikimedia.org/r/1052817

Change #1053985 merged by jenkins-bot:

[mediawiki/core@master] Tests on forcing multi-value to be an array in JSON

https://gerrit.wikimedia.org/r/1053985

Change #1050677 abandoned by Fgoodwin:

[mediawiki/core@master] Disallow PARAM_IS_MULTI in JSON

Reason:

A ticket has been merged to enforce array type rather than disallow PARAM_ISMULTI.

https://gerrit.wikimedia.org/r/1050677

Change #1052808 abandoned by Fgoodwin:

[mediawiki/extensions/GrowthExperiments@master] Disallow PARAM_ISMULTI as JSON Body parameter

Reason:

A ticket has been merged to enforce array type rather than disallow PARAM_ISMULTI.

https://gerrit.wikimedia.org/r/1052808