Page MenuHomePhabricator

Provide mechanism to validate API modules input before execution
Open, LowestPublic

Description

Problem
While I was working on T216888 I noticed that there isn't a good way to determine if a API module's parameters are valid before executing the module. I've added a crazy amount of logic just to deal with apierror-revisions-singlepage which can be seen here:
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GraphQL/+/491015/3/src/Source/Api.php

Solution
Provide a method to take in the parameter array and throw (or return) an error (if there is one). Then this method can be used during execution to validate the parameters. In essence, it's an abstraction of the validation code we already have.

Adding this method, would allow the MediaWiki-extensions-GraphQL to attempt to merge multiple requests, test the parameters, if they fail the validation, then we know the requests cannot be merged. This provides a quick way to check the merge-ability of requests.

Work Around
Implement the logic in the MediaWiki-extensions-GraphQL (which is what we are doing now).
Alternatively, we could remove the request merging until this is resolved, but that is a significant performance regression.

Event Timeline

dbarratt created this task.Feb 23 2019, 4:38 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 23 2019, 4:38 PM
dbarratt updated the task description. (Show Details)Feb 23 2019, 4:42 PM
Anomie triaged this task as Lowest priority.Feb 25 2019, 3:28 PM
Anomie moved this task from Unsorted to Needs details or plan on the MediaWiki-API board.
Anomie added a subscriber: Anomie.

Provide a method to take in the parameter array and return the error (if there is one).

"the error" should be a StatusValue, which could potentially contain multiple error messages.

The "and return the error" bit would be a significant change to the behavior of several ApiBase methods used for parameter checking; likely you'd have to add parallel versions that don't throw and deprecate the old ones.

On the other hand, most of the calls to this added verification method would be fine with the method simply throwing the error directly instead of requiring the caller to call $this->dieStatus() if the StatusValue isn't ok. MediaWiki-extensions-GraphQL could as well catch the ApiUsageException in that case, and the existing ApiBase methods wouldn't need changing.

It can also be used to expose this to the user before execution (for instance, the params could be validated in the sandbox before the user has executed the request).

That seems pretty unlikely to be all that useful, as it would still require an API call. You may as well just wait until the user is ready to make the actual call.

dbarratt added a comment.EditedFeb 25 2019, 3:34 PM

The "and return the error" bit would be a significant change to the behavior of several ApiBase methods used for parameter checking; likely you'd have to add parallel versions that don't throw and deprecate the old ones.

Oh it's totally fine if it throws, as long as there is a way to determine (either return or try...catch or something else?) that the params are valid before executing the request. It's asking the question: "If I make a request with these params, will this return a result or an error?" I know it's impossible to know if the request will actually return an error or not, but it seems like we should be able to answer that question (as best we can) with the params.

That seems pretty unlikely to be all that useful, as it would still require an API call. You may as well just wait until the user is ready to make the actual call.

That's fair, I was just thinking of other potential use cases.

dbarratt updated the task description. (Show Details)Feb 25 2019, 3:36 PM

@Anomie when you have a moment (no rush) would you mind giving an outline of how you would like this done? I might take a look at it and see if I can do it.

Off the top of my head, there are two areas of concern: the declaration and the call. I don't know which might be best off the top of my head.

For the declaration, should it be (1) abstract, (2) do-nothing, or (3) throw a BadMethodCallException? If #1, you'd have to submit patches to update all extensions in Gerrit to implement the method and have the core patch depend on at least the WMF-deployed extensions (yes, I've done that before; see also T151089). #2 allows for a more gradual updating of extensions, at the cost of callers not knowing whether it succeeded because the parameters are good or because it wasn't updated yet. #3 lets the caller know whether the extension has been updated, but depending on the calling method it could have the same update-everything requirement as #1.

Also, besides the normal declaration in ApiBase you may also want a version in ApiQueryGeneratorBase to parallel ApiQueryGeneratorBase::executeGenerator().

For the call, do you (1) update anything that calls ->execute() to first call the new method, (2) have every module call the new method at the top of execute(), (3) call it from ApiBase::extractRequestParams() just before the return, or (4) something else. For #1, you break the case of using an updated extension with an older caller since the validation won't be called, so that's probably a non-starter. #2 is pretty straightforward, but does mean every caller has to include that boilerplate. #3 should ensure it gets called for every module without having to update non-core callers (Flow), but still breaks using an updated extension with older core and could also break some code paths that call extractRequestParams() without wanting that validation (yet) (e.g. ApiQuery::isReadMode(), or ApiMain::setupModule(), or ApiPageSet::executeInternal() especially when $isDryRun is true, or ApiModuleManager::__construct()), although you could avoid that by having those callers pass a no-validate flag in $options. For #3 you could avoid the "updated extension with older core" problem by having the updated extension do like #2 just in case, with a note to remove that when the extension drops support for MW<1.33 (or whichever version).