Adopt a more idiomatic style #1

julienrf · 2020-10-14T19:50:29Z

Use precise API error responses in endpoints descriptions, rather than the generic type ApiError
Make sure responses and documentation are consistent together
Quick fix to the authentication directive

- Use precise API error responses in endpoints descriptions, rather than the generic type `ApiError` - Make sure responses and documentation are consistent together - Quick fix to the authentication directive

julienrf

I found some inconsistencies between what was shown by the documentation (via Swagger UI) and what was actually implemented on the server.

For instance, the documentation says that the endpoint /v1/data/apartments can return a 400 response with a JSON entity having the following schema:

However, if I try the endpoint with an invalid request, I get a text response:

$ curl -v http://localhost:8080/v1/data/apartments
*   Trying ::1...
* TCP_NODELAY set
* connect to ::1 port 8080 failed: Connection refused
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8080 (#0)
> GET /v1/data/apartments HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.58.0
> Accept: */*
> 
< HTTP/1.1 400 Bad Request
< Server: akka-http/10.2.0
< Date: Wed, 14 Oct 2020 18:17:48 GMT
< Content-Type: text/plain; charset=UTF-8
< Content-Length: 39
< 
* Connection #0 to host localhost left intact
Invalid value for: query parameter from

The reason for this inconsistency comes from the way withStatusCodes is implemented. However, the approach based on withStatusCodes is not recommended in the first place. In this pull request, I propose a different approach that results in consistent documentation and server implementation. Furthermore, I have also modified the signature of the endpoint descriptions in ApartmentsEndpointsDefinition to provide more precise result type to possible clients of the API (although this part was not exercised in your comparison).

The approach I propose here is not yet documented in the website of endpoints4s, so I am the culprit here :)

After my changes, the documentation looks like this:

And invalid requests get rejected by the server with a response conform to the documentation:

$ curl -v -H 'api-key: admin' http://localhost:8080/v1/data/apartments
*   Trying ::1...
* TCP_NODELAY set
* connect to ::1 port 8080 failed: Connection refused
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8080 (#0)
> GET /v1/data/apartments HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.58.0
> Accept: */*
> 
< HTTP/1.1 400 Bad Request
< Server: akka-http/10.2.0
< Date: Wed, 14 Oct 2020 19:15:32 GMT
< Content-Type: application/json
< Content-Length: 53
< 
* Connection #0 to host localhost left intact
{"reason":"Missing value for query parameter 'from'"}

By the way, I’ve also noticed that the implementation based on tapir suffers from the same issue: the documentation and the server implementation are not consistent together, but I didn’t investigate the reason for that.

julienrf · 2020-10-14T19:51:08Z

src/main/scala/io/scalac/lab/api/endpoints4s/ApartmentsApi.scala

@@ -11,8 +11,7 @@ import io.scalac.lab.api.model.ApiError
 import io.scalac.lab.api.model.ApiError.UnauthorizedError
 import io.scalac.lab.api.security.Security.ApiKey
 import io.scalac.lab.api.security.SecurityService
-import io.scalac.lab.api.storage.InMemoryApartmentsStorage
-import io.scalac.lab.api.tapir.ApartmentsEndpointsServer


I’ve noticed that you were wiring the tapir server implementation :)

julienrf · 2020-10-14T20:04:22Z

src/main/scala/io/scalac/lab/api/endpoints4s/CustomStatusCodes.scala

@@ -13,25 +13,43 @@ import io.scalac.lab.api.model.ApiError._
  * - 401 `Unauthorized` with `UnauthorizedError` as json entity
  * - 404 `NotFound` with `NotFoundError` as json entity
  * */
-trait CustomStatusCodes extends Endpoints with JsonEntitiesFromSchemas with JsonSchemas {
+trait CustomStatusCodes extends EndpointsWithCustomErrors with JsonEntitiesFromSchemas with JsonSchemas {


The supported way to define custom error responses is to extend EndpointsWithCustomErrors instead of Endpoints. This is not yet documented because I was not sure this would be useful to everyone. I explained a bit how it works here, though.

julienrf · 2020-10-14T20:04:27Z

src/main/scala/io/scalac/lab/api/endpoints4s/CustomStatusCodes.scala

-    * Uses given status codes to handle or create documentation based on that.
-    * Note that single response may have more than one status code.
-   **/
-  def withStatusCodes[A](response: Response[A], codes: StatusCode*): Response[Either[ApiError, A]]


withStatusCodes becomes unnecessary.

julienrf · 2020-10-14T20:05:04Z

src/main/scala/io/scalac/lab/api/endpoints4s/ApartmentsEndpointsDefinition.scala

-  val listApartments: Endpoint[(Paging, ApiKey), Either[ApiError, List[Apartment]]] =
+  val listApartments: Endpoint[(Paging, ApiKey), Either[BadRequestError, List[Apartment]]] =
    authenticatedEndpoint(
      request = get(path / "v1" / "data" / "apartments" /? pagingQueryString),
-      response = withStatusCodes(ok(jsonResponse[List[Apartment]], Some("A list of apartments")), Unauthorized, BadRequest),
+      response = badRequest.orElse(ok(jsonResponse[List[Apartment]], Some("A list of apartments"))),


I switched to using orElse.

The issue with withStatusCodes was that it returned a ResponseEither[[ApiError, A]]. However, the type ApiError is too wide. For instance, NotFoundError is a subtype of ApiError, but the endpoint listApartments never returns a Not Found response.

This is not a problem in your specific case, but endpoints4s has been designed to generate clients in addition to servers and documentation. From the point of view of a client, calling listApartments was previously returning Either[ApiError, List[Apartment]], which means that, in case of ApiError, the user would have to deal with the (impossible) case NotFoundError.

Another change that I made was to remove the mention of the possible response status Unauthorized: this one is automatically handled by the authenticatedEndpoint method.

Oke, I see your point and it makes perfect sense to change the type to be more specific one.

julienrf · 2020-10-14T20:05:12Z

src/main/scala/io/scalac/lab/api/endpoints4s/ApartmentsApi.scala

@@ -45,22 +44,9 @@ object ApartmentsApi extends App {
        implicit
        tupler: Tupler.Aux[U, ApiKey, I]): DocumentedEndpoint =
      super
-        .authenticatedEndpoint(request, response, docs)
+        .authenticatedEndpoint(request, response ++ unauthorized, docs)


Here, I add the unauthorized response to the documentation of all the authenticated endpoints.

This looks like the following in the Swagger UI:

julienrf · 2020-10-15T18:16:00Z

cc @ajuszczak

ajuszczak · 2020-11-05T15:35:52Z

src/main/scala/io/scalac/lab/api/endpoints4s/ApartmentsEndpointsServer.scala

-      onComplete(security.authenticate(authToken)).flatMap {
-        case Success(Right(apiKey)) => request.map(r => tupler(r, apiKey))
-        case _                      => complete((Unauthorized, authToken.fold("Missing API key")(_ => "Incorrect API key")))
+    request.flatMap { entity =>


What is an advantage of making request.flatMap{ entity => ... }, instead of mapping the request later on?

IIRC, the previous version was returning Unauthorized responses even for “non-authenticated” requests (ie, public endpoints).
It boils down to the way Akka-HTTP works when we combine directives with ~ (maybe you have already experienced that: you need to carefully seal some directives otherwise all your endpoints may be affected by one directive).
This quick fix makes sure the request directive did succeed before we even try to check the Authorization header. This is not perfect though because a malicious (unauthenticated) user could flood your server with high volume of data and you would happily accept its request entities before eventually rejecting them when you finally check the Authorization header.
A better solution would be to still perform the authentication check before decoding the request entity, but I think I couldn’t do this without changing the signature of authenticatedRequest, which I didn’t want to do in this PR.

ajuszczak · 2020-11-05T15:41:56Z

@julienrf Thanks for the pull-request you did along with the explanation of each step. It looks better than what I originally proposed and the advantage of having specific types instead of the wider one makes huge difference. I understand that the changes in the InMemoryStorage are only about the types, so maybe in the future it will be also changed.

Adopt a more idiomatic style

1d9ea9f

- Use precise API error responses in endpoints descriptions, rather than the generic type `ApiError` - Make sure responses and documentation are consistent together - Quick fix to the authentication directive

julienrf force-pushed the idiomaticity branch from 93e94c7 to 1d9ea9f Compare October 14, 2020 19:52

julienrf commented Oct 14, 2020

View reviewed changes

ajuszczak reviewed Nov 5, 2020

View reviewed changes

ajuszczak approved these changes Nov 5, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt a more idiomatic style #1

Adopt a more idiomatic style #1

julienrf commented Oct 14, 2020

julienrf left a comment •

edited

Loading

julienrf Oct 14, 2020

julienrf Oct 14, 2020

julienrf Oct 14, 2020

julienrf Oct 14, 2020 •

edited

Loading

ajuszczak Nov 5, 2020

julienrf Oct 14, 2020

julienrf commented Oct 15, 2020

ajuszczak Nov 5, 2020

julienrf Nov 5, 2020

ajuszczak commented Nov 5, 2020

Adopt a more idiomatic style #1

Are you sure you want to change the base?

Adopt a more idiomatic style #1

Conversation

julienrf commented Oct 14, 2020

julienrf left a comment • edited Loading

Choose a reason for hiding this comment

julienrf Oct 14, 2020

Choose a reason for hiding this comment

julienrf Oct 14, 2020

Choose a reason for hiding this comment

julienrf Oct 14, 2020

Choose a reason for hiding this comment

julienrf Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

ajuszczak Nov 5, 2020

Choose a reason for hiding this comment

julienrf Oct 14, 2020

Choose a reason for hiding this comment

julienrf commented Oct 15, 2020

ajuszczak Nov 5, 2020

Choose a reason for hiding this comment

julienrf Nov 5, 2020

Choose a reason for hiding this comment

ajuszczak commented Nov 5, 2020

julienrf left a comment •

edited

Loading

julienrf Oct 14, 2020 •

edited

Loading