OpenAPITools / openapi-generator

OpenAPI Generator allows generation of API client libraries (SDK generation), server stubs, documentation and configuration automatically given an OpenAPI Spec (v2, v3)
https://openapi-generator.tech
Apache License 2.0
21.35k stars 6.46k forks source link

[BUG] scala-sttp doesn't appear to be compatible with sttp 2.2.3 #7181

Open resinten opened 4 years ago

resinten commented 4 years ago

Bug Report Checklist

Description

As an educational effort to learn the openapi spec, I was working on writing one by hand. I need to use scala sttp version 2.2.3 because the sttp ZIO backend for the default 2.2.0 isn't compatible with the 1.0.0 release of ZIO as far as I can tell. However, there's a breaking change in the ResponseError type in sttp between 2.2.0 & 2.2.3. Previously it was a class with a body: String field. In 2.2.3 it does not have any fields but the two subclasses have the body field.

Generated ApiInvoker.scala (note: ex.body - ex is a ResponseError type)

    def result(implicit backend: SttpBackend[R, Nothing, Nothing]): R[T] = {
      val responseT = request.send()
      val ME: MonadError[R] = backend.responseMonad
      ME.flatMap(responseT) {
        response =>
          response.body match {
            case Left(ex) => ME.error[T](new HttpException(response.code, response.statusText, ex.body))
            case Right(value) => ME.unit(value)
          }
      }
    }

You can see the differences in the ResponseError type between the two versions here:

In sttp 2.2.0

sealed abstract class ResponseError[+T] extends Exception {
  def body: String
}
case class HttpError(body: String) extends ResponseError[Nothing]
case class DeserializationError[T](body: String, error: T) extends ResponseError[T]

In sttp 2.2.3

sealed abstract class ResponseError[+T](error: String) extends Exception(error)
case class HttpError(body: String, statusCode: StatusCode)
    extends ResponseError[Nothing](s"statusCode: $statusCode, response: $body")
case class DeserializationError[T: ShowError](body: String, error: T)
    extends ResponseError[T](implicitly[ShowError[T]].show(error))

I originally tried this with 4.3.1. It was suggested that I tried 5.0.0-beta to see if this was resolved. I tried it and specified the sttpClientVersion=2.2.3 property (see below) and noticed that the resulting build.sbt file still shows the dependency version for sttp as 2.2.0. I played around more by setting the mainPackage property and saw that the output package didn't change. This makes me wonder if the scala-sttp generator is properly reading the config properties.

openapi-generator version

Occurs with both 4.3.1 and 5.0.0-beta

OpenAPI declaration file content or url
# openapi.yaml
openapi: "3.0.3"
info:
  title: Twitch API Test
  version: "1.0"
servers:
  - url: https://api.twitch.tv/
    description: Primary
paths:
  /oauth2/token:
    servers:
      - url: https://id.twitch.tv/
        description: ID Server
    post:
      operationId: authenticate
      responses:
        "200":
          description: Success
          content:
            application/json:
              schema: { $ref: "#/components/schemas/AuthenticateResponse" }
      parameters:
        - name: client_id
          in: query
          required: true
          schema: { type: string }
        - name: client_secret
          in: query
          required: true
          schema: { type: string }
        - name: grant_type
          in: query
          required: true
          schema:
            type: string
            default: client_credentials
        - name: scope
          in: query
          required: false
          schema: { type: string }
  /helix/games:
    get:
      operationId: fetchGames
      responses:
        "200":
          description: Success
          content:
            application/json:
              schema: { $ref: "#/components/schemas/FetchGamesResponse" }
      parameters:
        - name: id
          in: query
          required: true
          schema: { type: string }
  /helix/streams:
    get:
      operationId: fetchStreams
      responses:
        "200":
          description: Success
          content:
            application/json:
              schema: { $ref: "#/components/schemas/FetchStreamsResponse" }
      parameters:
        - name: after
          in: query
          schema: { type:  string }
        - name: before
          in: query
          schema: { type: string }
        - name: first
          in: query
          schema:
            type: integer
            format: int33
        - name: game_id
          in: query
          schema: { type: string }
        - name: language
          in: query
          schema: { type: string }
        - name: user_id
          in: query
          schema: { type: string }
        - name: user_login
          in: query
          schema: { type: string }
      security:
        - twitch: []
components:
  securitySchemes:
    twitch:
      type: http
      scheme: bearer
  schemas:
    AuthenticateResponse:
      type: object
      required:
        - access_token
        - expires_in
        - scope
        - token_type
      properties:
        access_token: { type: string }
        expires_in:
          type: integer
          format: int64
        scope: { type: string }
        token_type: { type: string }
    FetchGamesResponse:
      type: object
      required:
        - data
        - pagination
      properties:
        data:
          type: array
          items:
            type: object
            required:
              - box_art_url
              - id
              - name
            properties:
              box_art_url: { type: string }
              id: { type: string }
              name: { type: string }
        pagination: { $ref: "#/components/schemas/Pagination" }
    FetchStreamsResponse:
      type: object
      required:
        - data
        - pagination
      properties:
        data:
          type: array
          items:
            type: object
            required:
              - id
              - user_id
              - user_name
              - game_id
              - type
              - title
              - viewer_count
              - started_at
              - language
              - thumbail_url
            properties:
              id: { type: string }
              user_id: { type: string }
              user_name: { type: string }
              game_id: { type: string }
              type: { type: string }
              title: { type: string }
              viewer_count: { type: string }
              started_at:
                type: string
                format: date-time
              language: { type: string }
              thumbnail_url: { type: string }
    Pagination:
      type: object
      properties:
        cursor: { type: string }
Generation Details
openapi-generator-cli generate \
  -i openapi.yaml \
  -g scala-sttp \
  --additional-properties sttpClientVersion=2.2.3,mainPackage=tv.twitch.api \
  -o twitch-api
Steps to reproduce

It was sufficient for me to just run the above command with the provided yaml file. After running, I checked the generated build.sbt and saw that it still references sttp 2.2.0 instead of 2.2.3. There is a breaking change between these two versions - the ResponseError type is subclassed in 2.2.3 and the body field does not exist on the root ResponseError type, but is referenced in the generated ApiInvoker.scala file. It also appears to ignore the mainPackage property as set above and still uses the default value.

Suggest a fix

Not sure what the fix is as I don't know how deep the issue goes. I'm hoping that it's just a simple fix of not reading the correct config keys that are described in the generator docs. It may also require some work to make sure that the generator is compatible with newer versions of the sttp client.

auto-labeler[bot] commented 4 years ago

👍 Thanks for opening this issue! 🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

wing328 commented 4 years ago

Can you please try the latest master instead? You can find links to download the snapshot version in the README.

resinten commented 4 years ago

@wing328

Looks like regarding the mainPackage property, I was wrong and it did pick it up, but I didn't see the new directory because I apparently can't read. but v4 and 5-beta were still not using the sttp version config correctly.

I just tried the latest snapshot version and it seems to work correctly with the sttpClientVersion property! Success! Thanks for the help! During that process, though, I did find a couple of additional bugs with the generation that are non-blocking (I can work around them for the time being).

First is that the servers property on a path item is ignored from what I can tell. In the above openapi.yaml, the authenticate operation uses id.twitch.tv rather than api.twitch.tv. The generated code puts everything under DefaultApi class and all operations share the class's baseUrl, so to use that operation, I have to create a separate DefaultApi instance and hardcode the url to what I already know it to be. Not hard since I wrote the yaml manually, but if someone were generating a large client library, that might be confusing. Maybe an alternative there would be for DefaultApi to have a overrideUrl: Option[String] field instead of base url. then the operations would do something like overrideUrl.getOrElse(<some default for the operation>)?

Second, modelPropertyNaming has to be set to original to deserialize correctly. In the yaml above, the fields are snake cased. The default naming for the generator using camel case, but it doesn't configure the deserializer to look for the original field names on the payload response, so the AuthenticateResponse has accessToken: String and looks for accessToken on the response body rather than access_token. To work around this, I have to set the property naming to original and be content to use snake case fields in my own code. This appears to be the case using both circe and json4s as the underyling jsonLibrary

Should those be wrapped up in this same issue, or should this one be closed in favor of a new issue? And should those be two separate issues or the same?