http4s generator: generated code results in very awkward error handling

[mchimirev]

For example, see saksdirect/mobile-middleware project, AccountService.scala

Let's say we have an endpoint like this:

          "method": "POST",
          "path": "/accounts",
          "description": "Sign the user up. (Register new account).",
          "body": {
            "type": "account_sign_up_request_data",
            "required": true
          },
          "parameters": [
            {
              "name": "sessionId",
              "type": "uuid",
              "location": "header"
            }
          ],
          "responses": {
            "200": {
              "type": "account_sign_up_response_wrapper"
            },
            "400": {
              "type": "account_sign_up_errors_wrapper"
            },
            "409": {
              "type": "account_sign_up_errors_wrapper"
            }
          }
        },

Then generated code looks like this:

          case r if r.status.code == 200 => _root_.com.hbc.mobile.saks.mobile.middleware.api.v1.Client.parseJson[F, com.hbc.mobile.saks.mobile.middleware.api.v1.models.AccountSignUpResponseWrapper]("com.hbc.mobile.saks.mobile.middleware.api.v1.models.AccountSignUpResponseWrapper", r)
          case r if r.status.code == 400 => Sync[F].raiseError(new com.hbc.mobile.saks.mobile.middleware.api.v1.errors.AccountSignUpErrorsWrapperResponse(r))
          case r if r.status.code == 409 => Sync[F].raiseError(new com.hbc.mobile.saks.mobile.middleware.api.v1.errors.AccountSignUpErrorsWrapperResponse(r))
          case r => Sync[F].raiseError(new com.hbc.mobile.saks.mobile.middleware.api.v1.errors.FailedRequest(r.status.code, s"Unsupported response code[${r.status.code}]. Expected: 200, 400, 409"))
        }

This results in very awkward client code (to handle 400 and 409):

        val response: IO[PostRegisterResponse] = client.accounts.postAccounts(toAccountSignUpRequestData(form), getSessionId(_req)).map { wrapper =>
          PostRegisterResponse.HTTP200(toAccount(wrapper))
        }
        response.recoverWith {
          case wrapperResponse: sakserrors.AccountSignUpErrorsWrapperResponse[IO] => {
            wrapperResponse.response.status.code match {
              case 400 | 409 => {
                wrapperResponse.accountSignUpErrorsWrapper.attempt.map(_.toOption).map(accountSignUpErrorsWrapper => {
                  PostRegisterResponse.HTTP422(ValidationErrors(cleanupValidationErrors(accountSignUpErrorsWrapper.get.errors.map(toValidationError))))
                })
              }
              case undefined =>
                throw new RuntimeException(s"undefined response code ${undefined}")
            }
          }
        }

1. response has to be typed, just doing .recoverWith is not enough
2. doing .attempt. seems wrong
3. AccountSignUpErrorsWrapperResponse has to be typed with [IO], this results in a compilation warning, because we cannot really guarantee the type at runtime

@fiadliel Can you add / correct my comment?

[amarrella]

Hello @mchimirev , i tried to minimize the use case for testing (I don't have access to the repo you are referring to).

Here is the api.json I tested:

{
  "name": "test",
  "models": {
    "okModel": {
      "fields": [
        {
          "name": "name",
          "type": "string"
        }
      ]
    },
    "errorModel": {
      "fields": [
        {
          "name": "description",
          "type": "string"
        }
      ]
    }
  },
  "resources": {
    "okModel": {
      "path": "/test",
      "operations": [
        {
          "method": "GET",
          "path": "/",
          "responses": {
            "200": {
              "type":"okModel"
            },
            "422": {
              "type":"errorModel"
            },
            "404": {
              "type":"errorModel"
            }
          }
        }
      ]
    }
  }
}

The generated *Client.scalais here: https://gist.github.com/amarrella/93b18bbe407da365f2efc56002375992)

It seems to me that you can handle your use case this way:

package com.example

import cats.effect.IO
import com.example.test.v0.Client
import com.example.test.v0.errors.ErrorModelResponse
import org.http4s.Uri
import org.http4s.client.blaze._

object Test extends App {

  for {
    httpClient <- Http1Client[IO]()
    apiClient = new Client[IO](Uri.unsafeFromString("https://www.example.com"), httpClient = httpClient)
    response <- apiClient.okModels.get().attempt
  } yield {
    response match {
      case Right(x) => x
      case Left(e: ErrorModelResponse[_]) =>
        e.response.status.code match {
          case 422 => e.errorModel // Do something with it
          case 404 => e.errorModel // Do something with it
        }
      case Left(_) => throw new Exception("Unhandled")
    }
  }

}

Notice that:

• attempt is called immediately after the get
• Pattern matching in the errors ignores the type of ErrorModelResponse (so no compiler warnings) but I can still find out the error and do something with it.

This works because ErrorModelResponse[_] extends Exception.

Did I understand the problem correctly? Does this solve it?

[fiadliel]

My point (in discussion with @mchimirev) was that ErrorModelResponse always has the same F as the client, but this is not encoded in the API. I suspect that the response error should be defined inside the client, so that it inherits the same F.

What is currently there can work, but it's inelegant, considering what we know about the invariants.

[fiadliel]

Alternatively, the error response could be altered so that it doesn't need an F. This is a questionable design anyway, in my opinion (e.g. how sure are we about the timescale under which this F can be evaluated).

[amarrella]

Ok I agree with that, we can move the error responses into the client class in the generated code. I'll open a PR