Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adopt a more idiomatic style #1

Open
wants to merge 1 commit into
base: round-5-handling-errors
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 4 additions & 18 deletions src/main/scala/io/scalac/lab/api/endpoints4s/ApartmentsApi.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import io.scalac.lab.api.storage.InMemoryNarrowerApartmentsStorage

import scala.concurrent.{ExecutionContext, Future}
import scala.io.StdIn
Expand All @@ -24,7 +23,7 @@ object ApartmentsApi extends App {

private object DocumentedEndpoints
extends ApartmentsEndpointsDefinition
with openapi.Endpoints
with openapi.EndpointsWithCustomErrors
with openapi.JsonEntitiesFromSchemas
with openapi.JsonSchemas {

Expand All @@ -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)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

This looks like the following in the Swagger UI:

Screenshot from 2020-10-14 21-53-16

.withSecurity(SecurityRequirement("apiKeyAuth", SecurityScheme("apiKey", None, Some("api-key"), Some("header"), None, None)))

override def withStatusCodes[A](responses: List[DocumentedResponse], codes: Int*): List[DocumentedResponse] = {
responses :++ codes.flatMap {
case 400 => badRequest
case 401 => unauthorized
case 404 => notFound
}
}

// We are going to return empty responses here for errors,
// because we want to customize status codes for each endpoint independently
override lazy val clientErrorsResponse: List[DocumentedResponse] = List.empty
override lazy val serverErrorResponse: List[DocumentedResponse] = List.empty

}

private object DocumentationServer extends server.Endpoints with server.JsonEntitiesFromEncodersAndDecoders {
Expand All @@ -82,7 +68,7 @@ object ApartmentsApi extends App {
)
}

private val apiStorage = new InMemoryApartmentsStorage()
private val apiStorage = new InMemoryNarrowerApartmentsStorage()
private val apiSecurity = new SecurityService {
override def authenticate(token: Option[String]): Future[Either[ApiError, ApiKey]] =
token match {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
package io.scalac.lab.api.endpoints4s

import endpoints4s.algebra.{Endpoints, JsonEntitiesFromSchemas}
import endpoints4s.algebra.{EndpointsWithCustomErrors, JsonEntitiesFromSchemas}
import endpoints4s.generic.JsonSchemas
import io.scalac.lab.api.model.{Address, Apartment, ApiError, Paging}
import io.scalac.lab.api.model.ApiError.{BadRequestError, NotFoundError}
import io.scalac.lab.api.model.{Address, Apartment, Paging}
import io.scalac.lab.api.security.Security.ApiKey

/**
Expand All @@ -16,7 +17,7 @@ import io.scalac.lab.api.security.Security.ApiKey
* - fifth for deleting apartment by id.
*/
trait ApartmentsEndpointsDefinition
extends Endpoints
extends EndpointsWithCustomErrors
with JsonEntitiesFromSchemas
with JsonSchemas
with SecuritySupport
Expand All @@ -26,41 +27,38 @@ trait ApartmentsEndpointsDefinition
private implicit val addressSchema: JsonSchema[Address] = genericJsonSchema
private implicit val apartmentSchema: JsonSchema[Apartment] = genericJsonSchema

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"))),
Comment on lines -29 to +33
Copy link
Author

@julienrf julienrf Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

docs = EndpointDocs().withDescription(Some("An endpoint responsible for listing all available apartments"))
)

val getApartment: Endpoint[(Int, ApiKey), Either[ApiError, Apartment]] =
val getApartment: Endpoint[(Int, ApiKey), Either[NotFoundError, Apartment]] =
authenticatedEndpoint(
request = get(path / "v1" / "data" / "apartments" / segment[Int]("id", Some("The identifier of the apartment to be found"))),
response = withStatusCodes(ok(jsonResponse[Apartment], Some("An apartment found for given id")), Unauthorized, NotFound),
response = notFound.orElse(ok(jsonResponse[Apartment], Some("An apartment found for given id"))),
docs = EndpointDocs().withDescription(Some("An endpoint responsible for getting specific apartment by id"))
)

val findApartment: Endpoint[(Address, ApiKey), Either[ApiError, Apartment]] =
val findApartment: Endpoint[(Address, ApiKey), Either[Either[BadRequestError, NotFoundError], Apartment]] =
authenticatedEndpoint(
request = get(path / "v1" / "data" / "apartments" / "search" /? addressQueryString),
response = withStatusCodes(ok(jsonResponse[Apartment], Some("An apartment found for query string parameters")),
Unauthorized,
BadRequest,
NotFound),
response = badRequest.orElse(notFound).orElse(ok(jsonResponse[Apartment], Some("An apartment found for query string parameters"))),
docs = EndpointDocs().withDescription(Some("An endpoint responsible for finding specific apartment by search predicates"))
)

val addApartment: Endpoint[(Apartment, ApiKey), Either[ApiError, Apartment]] =
val addApartment: Endpoint[(Apartment, ApiKey), Either[BadRequestError, Apartment]] =
authenticatedEndpoint(
request = post(path / "v1" / "data" / "apartments", jsonRequest[Apartment], Some("An apartment to be added in the storage")),
response = withStatusCodes(ok(jsonResponse[Apartment], Some("An apartment saved in the storage")), Unauthorized, BadRequest),
response = badRequest.orElse(ok(jsonResponse[Apartment], Some("An apartment saved in the storage"))),
docs = EndpointDocs().withDescription(Some("An endpoint responsible for adding new apartment"))
)

val deleteApartment: Endpoint[(Int, ApiKey), Either[ApiError, Apartment]] =
val deleteApartment: Endpoint[(Int, ApiKey), Either[NotFoundError, Apartment]] =
authenticatedEndpoint(
request = delete(path / "v1" / "data" / "apartments" / segment[Int]("id", Some("The identifier of the apartment to be deleted"))),
response = withStatusCodes(ok(jsonResponse[Apartment], Some("An apartment deleted")), Unauthorized, NotFound),
response = notFound.orElse(ok(jsonResponse[Apartment], Some("An apartment deleted"))),
docs = EndpointDocs().withDescription(Some("An endpoint responsible for deleting apartment by id"))
)

Expand Down
Original file line number Diff line number Diff line change
@@ -1,24 +1,22 @@
package io.scalac.lab.api.endpoints4s

import akka.http.scaladsl.server.Directives._
import akka.http.scaladsl.server.{Directive1, Route}
import akka.http.scaladsl.server.{Directive1, Route, StandardRoute}
import de.heikoseeberger.akkahttpcirce.FailFastCirceSupport
import endpoints4s.Tupler
import endpoints4s.akkahttp.server.{BuiltInErrors, Endpoints, JsonEntitiesFromSchemas}
import io.circe.generic.auto._
import io.scalac.lab.api.model.ApiError
import endpoints4s.akkahttp.server.JsonEntitiesFromSchemas
import endpoints4s.algebra.EndpointsWithCustomErrors
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.ApartmentsStorage
import io.scalac.lab.api.storage.NarrowerApartmentsStorage

import scala.util.Success

class ApartmentsEndpointsServer(storage: ApartmentsStorage, security: SecurityService)
class ApartmentsEndpointsServer(storage: NarrowerApartmentsStorage, security: SecurityService)
extends ApartmentsEndpointsDefinition
with Endpoints
with BuiltInErrors
with JsonEntitiesFromSchemas
with FailFastCirceSupport {
with EndpointsWithCustomErrors
with JsonEntitiesFromSchemas {

val listApartmentsRoute: Route = listApartments.implementedByAsync { case (paging, _) => storage.list(paging.from, paging.limit) }

Expand All @@ -33,18 +31,16 @@ class ApartmentsEndpointsServer(storage: ApartmentsStorage, security: SecuritySe
val deleteApartmentRoute: Route = deleteApartment.implementedByAsync { case (id, _) => storage.delete(id) }

override def authenticatedRequest[A, B](request: Directive1[A])(implicit tupler: Tupler.Aux[A, ApiKey, B]): Directive1[B] = {
optionalHeaderValueByName("api-key").flatMap { authToken: Option[String] =>
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 =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

optionalHeaderValueByName("api-key").flatMap { authToken: Option[String] =>
onComplete(security.authenticate(authToken)).flatMap {
case Success(Right(apiKey)) => provide(tupler(entity, apiKey))
case _ =>
val error = UnauthorizedError(authToken.fold("Missing API key")(_ => "Incorrect API key"))
StandardRoute(unauthorized(error))
}
}
}
}

override def withStatusCodes[A](response: A => Route, codes: StatusCode*): Either[ApiError, A] => Route = {
case Left(x @ ApiError.UnauthorizedError(_)) => complete(Unauthorized, x)
case Left(x @ ApiError.NotFoundError(_)) => complete(NotFound, x)
case Left(x @ ApiError.BadRequestError(_)) => complete(BadRequest, x)
case Right(value) => response(value)
}
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
package io.scalac.lab.api.endpoints4s

import endpoints4s.algebra.{Endpoints, JsonEntitiesFromSchemas}
import endpoints4s.Invalid
import endpoints4s.algebra.{EndpointsWithCustomErrors, JsonEntitiesFromSchemas}
import endpoints4s.generic.JsonSchemas
import io.scalac.lab.api.model.ApiError
import io.scalac.lab.api.model.ApiError._

/**
Expand All @@ -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 {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


private implicit val badRequestSchema: JsonSchema[BadRequestError] = genericJsonSchema
private implicit val unauthorizedSchema: JsonSchema[UnauthorizedError] = genericJsonSchema
private implicit val notFoundSchema: JsonSchema[NotFoundError] = genericJsonSchema

val badRequest: Response[BadRequestError] =
response(BadRequest, jsonResponse[BadRequestError], Some("Api error returned when request could not be processed properly"))
// Tell endpoints4s that client errors are modeled with our custom type, BadRequestError
type ClientErrors = BadRequestError

// Conversion from endpoints4s internal representation of client errors to our custom type
def invalidToClientErrors(invalid: Invalid): BadRequestError =
BadRequestError(invalid.errors.mkString(". "))
// Conversion from our custom type to endpoints4s internal representation of client errors
def clientErrorsToInvalid(clientErrors: BadRequestError): Invalid =
Invalid(clientErrors.reason)

def clientErrorsResponseEntity: ResponseEntity[BadRequestError] = jsonResponse[BadRequestError]

// Override the documentation of the response for client errors
override lazy val clientErrorsResponse: Response[BadRequestError] =
badRequest(docs = Some("Api error returned when request could not be processed properly"))

// We define three shorthands for our API responses, `badRequest`, `unauthorized`, and `notFound`
val badRequest: Response[BadRequestError] = clientErrorsResponse

val unauthorized: Response[UnauthorizedError] =
response(Unauthorized, jsonResponse[UnauthorizedError], Some("Api error returned when request could not be authenticated"))

val notFound: Response[NotFoundError] =
response(NotFound, jsonResponse[NotFoundError], Some("Api error returned when entity could not be found"))

/**
* 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]]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

withStatusCodes becomes unnecessary.

// We use the same representation as endpoints4s for server errors
type ServerError = Throwable
def throwableToServerError(throwable: Throwable): Throwable = throwable
def serverErrorToThrowable(serverError: Throwable): Throwable = serverError
def serverErrorResponseEntity: ResponseEntity[Throwable] =
jsonResponse(field[String]("reason")
.xmap[Throwable](new Exception(_))(_.toString))

}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package io.scalac.lab.api.endpoints4s

import endpoints4s.Validated
import endpoints4s.algebra.Endpoints
import endpoints4s.algebra.{Endpoints, EndpointsWithCustomErrors}
import io.scalac.lab.api.model.{Address, Paging}

/**
Expand All @@ -19,7 +19,7 @@ import io.scalac.lab.api.model.{Address, Paging}
* - address.street cannot be empty
* - address.number cannot be empty and needs to contain at least one digit
**/
trait QueryStringParams extends Endpoints {
trait QueryStringParams extends EndpointsWithCustomErrors {

private val pagingFrom = qs[Int]("from", Some("Indicates where we should start returning data from"))
private val pagingLimit = qs[Option[Int]]("limit", Some("An optional number of rows to be returned"))
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
package io.scalac.lab.api.endpoints4s

import endpoints4s.Tupler
import endpoints4s.algebra.Endpoints
import endpoints4s.algebra.{Endpoints, EndpointsWithCustomErrors}
import io.scalac.lab.api.security.Security.ApiKey

trait SecuritySupport extends Endpoints {
trait SecuritySupport extends EndpointsWithCustomErrors {

/** A request which is rejected by the server if it does not contain a valid authentication token */
def authenticatedRequest[I, O](request: Request[I])(implicit tupler: Tupler.Aux[I, ApiKey, O]): Request[O]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package io.scalac.lab.api.storage
import java.util.concurrent.atomic.AtomicLong

import io.scalac.lab.api.model.Apartment
import io.scalac.lab.api.model.ApiError.{BadRequestError, NotFoundError}

import scala.concurrent.Future

// Exactly the same implementation as `InMemoryApartmentsStorage`, except it
// extends `NarrowerApartmentsStorage`.
class InMemoryNarrowerApartmentsStorage(initState: () => List[Apartment] = () => List.empty) extends NarrowerApartmentsStorage {
private val storageId = new AtomicLong()
private var storage: List[Apartment] = initState()

override def list(from: Int, limit: Option[Int]): Future[Either[BadRequestError, List[Apartment]]] =
Future.successful(Right(storage.slice(from, from + limit.getOrElse(storage.length))))

override def get(id: Int): Future[Either[NotFoundError, Apartment]] =
storage.find(_.id.contains(id)) match {
case Some(value) => Future.successful(Right(value))
case None => Future.successful(Left(NotFoundError(s"Apartment with id: $id not found!")))
}

override def find(city: String, street: String, number: String): Future[Either[Either[BadRequestError, NotFoundError], Apartment]] = {
storage.find { s =>
s.address.city.equalsIgnoreCase(city) &&
s.address.street.equalsIgnoreCase(street) &&
s.address.number.equalsIgnoreCase(number)
} match {
case Some(value) => Future.successful(Right(value))
case None =>
Future.successful(Left(Right(NotFoundError(s"Apartment for city: $city, street: $street, number: $number not found!"))))
}
}

override def save(apartment: Apartment): Future[Either[BadRequestError, Apartment]] = {
val apartmentWithId = apartment.copy(id = Some(storageId.getAndIncrement()))

storage = storage :+ apartmentWithId
Future.successful(Right(apartmentWithId))
}

override def delete(id: Int): Future[Either[NotFoundError, Apartment]] = {
storage.find(_.id.contains(id)) match {
case Some(value) =>
storage = storage.filterNot(_.id.contains(id))
Future.successful(Right(value))
case None =>
Future.successful(Left(NotFoundError(s"Apartment with id: $id not found!")))
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package io.scalac.lab.api.storage

import io.scalac.lab.api.model.Apartment
import io.scalac.lab.api.model.ApiError.{BadRequestError, NotFoundError}

import scala.concurrent.Future

// Similar to `ApartmentsStorage`, but uses precise return
// types instead of `ApiError` everywhere
trait NarrowerApartmentsStorage {
def list(from: Int, limit: Option[Int]): Future[Either[BadRequestError, List[Apartment]]]
def get(id: Int): Future[Either[NotFoundError, Apartment]]
def find(city: String, street: String, number: String): Future[Either[Either[BadRequestError, NotFoundError], Apartment]]
def save(a: Apartment): Future[Either[BadRequestError, Apartment]]
def delete(id: Int): Future[Either[NotFoundError, Apartment]]
}