Page MenuHomePhabricator

HTTP API typechecking method is very confusing and intimidating
Closed, ResolvedPublic

Description

Currently, all API parameters type checking is done in ApiBase::getParameterFromSettings AFAIK. This method is 238 lines-long. This is confusing and lacks the possibility for extension.

The first thing that could be done is move type-checking in separate type-checker classes responsible for everything regarding one type (from error reporting - e.g. for invalid values etc. - to value determining from default value, type etc.)

I propose something like:

abstract class ApiParamTypechecker {
    public abstract function __construct(array $data);
    public abstract function validate();
}

What do you think? I could go ahead and implement something if you think this would be a good idea!

Event Timeline

Anomie subscribed.

My first reaction was to decline this bug, since it comes across as basically "OMG! You have a method that's more than 10 lines long!" But I decided that discussion about making things clearer is probably a good thing, even if it begins from a bad start.

That said, I think that a plethora of tiny validation classes would be much worse than one not-that-long method for validating parameters. You'd either duplicate code, or spread a significant quantity of shared code across multiple files making the code flow harder to follow. And having to instantiate objects for each type would slow every API request and increase resource usage.

I thought I would be a good idea because adding a new type to the already existing code is pretty hard already. If you remember my task in Google Code-in, when I implemented a new parameter type, it was pretty ambiguous where error reporting, type checking etc. would come.

I thought of a classes approach because it would be cleaner (IMO) to implement new types and modify the existing validation. Eg. Look at the integer type. If I add a lower limit, I am required to add an upper limit as well

PS. What if type checking would be moved in separate callbacks registered in an array for example?

I think moving that function to a single class would be a good start for readability and maintainability probably. If we choose to add some abstraction, that should come afterwards.

Allowing extensions to add new types is interesting, but I'd like to see specific cases of where it would be useful...

Change 434717 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] API: Refactor parameter validation

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

Change 434718 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] API: Use MediaWiki\Api\ParamValidator

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

Change 515143 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] API: Abstract out parameter validation

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

Change 434717 abandoned by Anomie:
API: Refactor parameter validation

Reason:
I5c0cc3a8d686ace97596df5832c450a6a50f902c

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

Change 515143 merged by jenkins-bot:
[mediawiki/core@master] API: Abstract out parameter validation

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

Change 434718 merged by jenkins-bot:
[mediawiki/core@master] API: Use ParamValidator library

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

Anomie moved this task from Blocked to Done on the Platform Team Workboards (Green) board.