indiehd / web-api

GNU Affero General Public License v3.0
6 stars 4 forks source link

Consider making ApiController validation clearer #173

Closed mblarsen closed 4 years ago

mblarsen commented 4 years ago

(adopted from Discord chat)

Currently it is a bit unclear what happens when you read the action methods on the ApiController.

https://cdn.discordapp.com/attachments/474753428665663507/721227919602614312/Screenshot_from_2020-06-13_13-00-36.png

The a new update request is resolved and in doing so it will validate the incoming data. If the validation fails a ValidationException is thrown.

All of that is fine, however, it is unclear and it is a bit of an anti-pattern to rely only on the side-effect, which is what happens here. The side-effect of resolving is validation.

Also a FormRequst doesn't only have to validate. So not using the output from she resolved request and using the generic one instead is kind of not presenting the 100% quarantines that the specific request offer.

One way to improve the readability issue is to wrap the call in a function data tells you what happens:

protected function validateRequest($requestClass): array
{
    return resolve($requestClass)->all();
}

and then use it like this:

public function update($id)
{
    $data = $this->validateRequest($this->updateRequest());

    $this->repository->update($id, $data);

    ...
}

This achieves two things:

  1. documentation
  2. ensure that the request data is the validated (and possibly transformed) data.
cbj4074 commented 4 years ago

Thanks for pointing this out!

If you find the time to submit a PR to make this change, I'd be most appreciative! (Otherwise, I'm happy to do it.)