How to properly document status codes and data structures in endpoints?

[provokateurin]

We want to document what status codes and data types are returned from endpoints. This needs to be done in order to be able to generate OpenAPI specs.
None to very little code changes should be required, so updating comments/type annotations is the way we want to do this.
The comments should also help static analysis to validate the returned data.

My current approach:

/**
* receive notification about existing share
*
* @NoCSRFRequired
* @PublicPage
* @BruteForceProtection(action=receiveFederatedShareNotification)
*
* @param string $notificationType notification type, e.g. SHARE_ACCEPTED
* @param string $resourceType calendar, file, contact,...
* @param string|null $providerId id of the share
* @param array{}|null $notification the actual payload of the notification
*
* @psalm-import-type CloudFederationValidationError from ResponseDefinitions
* @psalm-import-type CloudFederationError from ResponseDefinitions
* @return JSONResponse<array{}> 201 The notification was successfully received
* @return JSONResponse<CloudFederationValidationError> 400 Bad request due to invalid parameters, e.g. when `type` is invalid or missing.
* @return JSONResponse<CloudFederationError> 501 The resource type is not supported.
*/
public function receiveNotification(string $notificationType, string $resourceType, ?string $providerId, ?array $notification): JSONResponse {
...
}

This way it is clear what data is returned (see the arrow brackets of DataResponse) under which status code. It also makes it possible to have a description, which is not required, but very useful.
The problem is that multiple @return annotations are not allowed, so we can't go with this.

Our best idea right now is something like this:

/**
* @return JSONResponse<array{}|CloudFederationValidationError|CloudFederationError>
* - 201: JSONResponse<array{}> The notification was successfully received
* - 400: JSONResponse<CloudFederationValidationError> Bad request due to invalid parameters, e.g. when `type` is invalid or missing.
* - 501: JSONResponse<CloudFederationError> The resource type is not supported.
*/

This is valid, but has the problem that the status codes are not validated against the code (just like in the first snippet) and also that people might forget to update the lower comments.

Another idea was to have more response classes like JSONResponseXXX for every status code. Then the annotations could look something like this:

/**
* @return JSONResponse201<array{}>|JSONResponse400<CloudFederationValidationError>|JSONResponse501<CloudFederationError>
* - The notification was successfully received
* - Bad request due to invalid parameters, e.g. when `type` is invalid or missing.
* - The resource type is not supported.
*/

It makes static analysis stronger, but also requires code changes which we really want to avoid. It also requires some more changes than just replacing the response class, because some pieces of code set the status code dynamically (e.g. from a service response).

Neither of those solutions is great, so we really want to have something better.

[provokateurin]

Based on @come-nc's idea I created the following: https://psalm.dev/r/681f33c842. I like it a lot since it helps the static analysis and is clear for the generator.

The two downsides I see are:

1. The description has to be set inside the response object (although it won't be used) in order to make the static analysis happy.
2. The @return lines could get very long with many status codes or long descriptions. It's not possible to split it over multiple lines AFAIK.

[ChristophWurst]

None to very little code changes should be required, so updating comments/type annotations is the way we want to do this.

I disagree on this. I believe that a separation of code and comment (API spec) is not ideal, because someone has to ensure these two stay in sync. That is why I would prefer to only have one way of expressing our API design, and make that as idiomatic as possible.

I support the idea of #36355 and typed responses. They allow us to express the types of a controller method and its returned data structure with PHP/PHPDoc. There is no new markup or syntax required.

The tricky parts are the errors, like you've shown above. PHPDoc doesn't allow multiple return types. What if we go the PHP native path and use exceptions for those? E.g. a fully generic HttpError class that is throwable and takes the response data, or more specific ClientException/ServerException and similar. Those can by typed as well, like the responses.
Those thrown exceptions can be documented in PHPDoc, and we can add as many as we like. And they can have a description for when a certain error is thrown. The only thing missing would be the status code.
Keeping the code that throws exceptions and PHPDoc in sync is easy, because our IDEs and static analysis already warn us about unhandled exceptions and exceptions that are never thrown.
We can add the HTTP errors/exceptions to the app framework, catch them and convert the result to a response identical to return values.

Writte as an example

/**
 * @param string $bar
 * @throws ClientException<IFoo> If the …
 * @return JsonResponse<IUser>
 */
public function foo(string $bar): Response {
  if ($bar === 'err') {
    throw new ClientException($this->foo, 200);
  }
  return new JsonResponse($this->service->getUser();
}

[provokateurin]

I disagree on this

I agree with you, I meant that we don't want to refactor a lot of code. Adding extra support code e.g. for typed responses is of course very good.

The tricky parts are the errors

I already handle all the @throws ..., so that works fine.

What do you think about the code snippet I sent above?

[ChristophWurst]

Do you handle how the thrown exceptions are converted into HTTP responses? Is that even possible at the moment?

I don't like mixing types with data and documentation to be honest. Status code is not a type, it's a value. Putting that into a template parameter feels wrong. And the documentation is also duplicated between PHPDoc and method body, which means they have to be kept in sync.

I think having alternative types in @return is fine because typically there shouldn't be many different types to return, right? And if you move the errors into @throw tags then the return spec stays simple.

[provokateurin]

Do you handle how the thrown exceptions are converted into HTTP responses? Is that even possible at the moment?

This is kind of hardcoded. Only exceptions that extend OCSException can have a status code. All other ones return 500. Then I match the status code based on the name of the exception. The returned data is always a plain text string.

I don't like mixing types with data and documentation to be honest. Status code is not a type, it's a value.

Technically yes, the status code is an integer, but it also indicates the type of the response, so it's some kind of type, right?

And the documentation is also duplicated between PHPDoc and method body, which means they have to be kept in sync.

I also dislike this, as I said above, but I haven't found a solution for that yet (probably not possible because of more/less specific data types in psalm).

I think having alternative types in @return is fine because typically there shouldn't be many different types to return, right? And if you move the errors into @throw tags then the return spec stays simple.

Agreed, I just had to do this for the cloud_federation_api which doesn't use exceptions because it's more complicated and returns complex data structures instead of a simple error string.

[ChristophWurst]

https://github.com/domaindrivendev/Swashbuckle.AspNetCore/blob/master/README.md#swashbuckleaspnetcoreannotations might be worth checking what other languages with comparable type systems do.

[provokateurin]

Not sure what I can take from that doc. [SwaggerResponse(201, "The product was created", typeof(Product))] looks interesting and doing something similar using php attributes for example is possible, but it won't help static analysis/validating the code correctness.

[provokateurin]

I had some serious issues with psalm and type resolving. In theory it looked very good, but psalm just handles stuff differently than I want.
It collapsed something like JSONResponse<array{test1: string}, 200>|JSONResponse<array{test2: string}, 201> into JSONResponse<array{test1: ?string, test2: ?string}, 200|201> which made it really useless/not possible to implement (because all nullable fields still need to be initialized).
I'm now going with having different classes for each response code like JSONResponseXXX that all extend JSONResponse which prevents the type collapsing and works really well. This should also keep everything compatible with the existing classes, since the new classes only override the data type and the status code (please correct me if I'm wrong).

I decided for the descriptions we will just have a comment line like XXX: Some description. They usually don't change since the endpoints should always do the same thing and it's not super catastrophic if they don't match. This also removes the need to copy the description to every return statement which was awful.

[provokateurin]

#36666 implements this approach. Psalm finally passes 🎉

[ChristophWurst]

I'm now going with having different classes for each response code like JSONResponseXXX that all extend JSONResponse which prevents the type collapsing and works really well. This should also keep everything compatible with the existing classes, since the new classes only override the data type and the status code (please correct me if I'm wrong).

There's gonna be a lot of classes needed to cover any possible status code: https://en.wikipedia.org/wiki/List_of_HTTP_status_codes. This doesn't scale.

Let's rather use the status as template parameter. I am still not fan of it but it's better than one class per status code.

[provokateurin]

As I explained above that doesn't work because psalm collapses the types, but I agree that this solution is not the best.

[ChristophWurst]

https://psalm.dev/r/4039c1c0d9 https://psalm.dev/r/e70598c116 is how far I got with templates and status codes and happy psalm

[provokateurin]

With classes as the returned data it works, but with type aliases the types just appear as array{...} to psalm and it merges them which makes it useless.

I had a stupid idea that could also work: Just have a few dummy classes like JSONResponse1, JSONResponse2 and so on. Then put the data type and status code inside the template parameters. This way we don't need to have one class for every status code, only as many classes as the endpoint with the most different data types returns. It will probably look ugly, but definitely scale much better.

[provokateurin]

To me this is a problem with psalm and type aliases, but I don't think we can change much about it.

[ChristophWurst]

It collapsed something like JSONResponse<array{test1: string}, 200>|JSONResponse<array{test2: string}, 201> into JSONResponse<array{test1: ?string, test2: ?string}, 200|201> which made it really useless/not possible to implement (because all nullable fields still need to be initialized).

If I read https://psalm.dev/docs/annotating_code/typing_in_psalm/#docblock-type-syntax correctly this is unexpected because you used union types, not intersection types 🤔

What about https://psalm.dev/r/8d4547dd06?

[ChristophWurst]

OTOH I'm not using type aliases, I spec the types directly.

[provokateurin]

What about https://psalm.dev/r/8d4547dd06?

Uh that looks like the expected behaviour?!

Maybe it is really something with type aliases or imported type aliases. The handling around those is definitely not great as I learned from reading many psalm issues.

[provokateurin]

I got a different error than I described above, but I've also seen this one already: https://psalm.dev/r/ac772774e8
It is definitely a problem of imported type aliases since it worked with just having the type alias on the method or class set.
So can we say this is a psalm bug?

[ChristophWurst]

Yeah, looks a like a Psalm bug with type imports :s

If there is no existing ticket it might be worth a report. The Psalm devs are fairly responsive to reported issues.

[provokateurin]

Will do :)

[provokateurin]

How does it work with updating psalm after this is fixed? We are currently one major version behind...

[ChristophWurst]

Someone has to do the update 😬

[provokateurin]

Let's see: vimeo/psalm#9305

[provokateurin]

Branch/PR is updated, except for the psalm bug everything should be working.