swagger-akka-http / swagger-scala-module

Swagger support for scala
Apache License 2.0
10 stars 10 forks source link

Extraneous schema objects being generated for Scala Collection properties #63

Closed krnhotwings closed 3 years ago

krnhotwings commented 4 years ago

Not exactly sure what's going on, but I've tried narrowing it down.

Ever since I've moved from swagger-akka-http 1.2.0 to 2.2.0, I've noticed that the generated oas v3 doc adds extra schemas if a response schema case class contains a scala collection object.

For example, if I had case class:

case class Test(myProp: Seq[String])

and an akka http route annotated like:

  @Path("test")
  class Routes {
    @GET
    @Operation(
      summary="Retrieves test object",
      responses=Array(new ApiResponse(
        responseCode="200",
        description="Success",
        content=Array(new Content(
          mediaType="application/json",
          schema=new Schema(implementation=classOf[Test]))))))
    def route = ???

The generated yaml will contain the following under schemas:

SeqString:
  required:
  - traversableAgain
  type: array
  properties:
    traversableAgain:
      type: boolean
    empty:
      type: boolean
  items:
    type: string

As far as I can tell, this is coming from this swagger-scala-module. I've tried doing the following:

  1. Created a new sbt project with the following for build.sbt
    
    scalaVersion := "2.13.3"
    name := "example"
    libraryDependencies ++= List(
    "io.swagger.core.v3" % "swagger-annotations" % "2.1.4",
    "io.swagger.core.v3" % "swagger-jaxrs2" % "2.1.4",
    "javax.ws.rs" % "javax.ws.rs-api" % "2.1.1",
    // "com.github.swagger-akka-http" %% "swagger-akka-http" % "2.2.0",
    )
2. Added the following under `src/main/scala/Example.scala`
```scala
import io.swagger.v3.jaxrs2.Reader
import io.swagger.v3.oas.integration.SwaggerConfiguration
import io.swagger.v3.oas.models.OpenAPI

import io.swagger.v3.oas.annotations._
import io.swagger.v3.oas.annotations.media._
import io.swagger.v3.oas.annotations.responses._
import io.swagger.v3.oas.annotations.extensions._
import javax.ws.rs._

// import com.github.swagger.akka.SwaggerHttpService
// import scala.jdk.CollectionConverters._

object Example {
  case class Test(myProp: Seq[String])

  @Path("test")
  class Routes {
    @GET @Path("{testId}")
    @Operation(
      summary="Retrieves test object",
      responses=Array(new ApiResponse(
        responseCode="200",
        description="Success",
        content=Array(new Content(
          mediaType="application/json",
          schema=new Schema(implementation=classOf[Test]))))))
    def route = ???
  }

  def run(): Unit = {
    val readerConfig = new SwaggerConfiguration()
    val swaggerConfig = new OpenAPI()
    val reader = new Reader(readerConfig.openAPI(swaggerConfig))
    println(reader.read(classOf[Routes]))

    ()
  }
}
  1. Ran Example.run() in console and examined the output in both scenarios where the swagger-scala-module dependency is and is not commented out. The results are attached:

swagger-core-reader-output-without-scala-module.log swagger-core-reader-output-with-scala-module.log

If you look at "swagger-core-reader-output-with-scala-module" you can find "SeqString" being added in there, which I assume is how it ends up in the YAML.

pjfanning commented 4 years ago

I thought this might have been fixed by https://github.com/swagger-akka-http/swagger-scala-module/issues/48

pjfanning commented 4 years ago

There is a sample project (https://github.com/pjfanning/swagger-akka-http-sample) and it has an example case class called EchoList that is very similar to your class. I even modified the sample to just have case class EchoList(myProps: Seq[String]). The schema that is generated for that version of EchoList is:

      "EchoList" : {
        "required" : [ "myProps" ],
        "type" : "object",
        "properties" : {
          "myProps" : {
            "type" : "array",
            "properties" : { },
            "items" : {
              "type" : "string"
            }
          }
        }
      }
krnhotwings commented 4 years ago

Hmmm, I thought that the generation of SeqString schemas was a bug because the SeqX schemas seem to be unnecessary (they weren't generated in the 1.x version of swagger-akka-http/swagger-scala-module.)

If they can't be removed, then that changes the bug report a bit.

The (minor) problem is that the doc doesn't pass validation. SeqString has an empty array for property required, and the validators squawk about that. It seems that if you have a case class that has a collection of an object type that itself has required fields, the required array gets thrown into the generated SeqX schema.

For example...

case class Continent(countries: Seq[Country])
case class Country(name: String)

If we have an object (Continent) that contains a collection of another object (Country) that itself has a required field (name), the following will be generated:

Continent:
  required:
  - countries

Country:
  required:
  - name

SeqCountry:
  required: []

The extra SeqCountry adds that empty required property that breaks validation.

I saw #48 before I created this one, but I wasn't sure if it was related at the time. Maybe it will suffice to remove the required property as well?

pjfanning commented 4 years ago

That required: [] makes sense in swagger json. I'm not sure about how it should be represented in yaml. When you says breaks validation, what tool are you using. If yaml causes trouble, could you try json?

The Yaml serialization is done with a swagger-core class (and it uses jackson's yaml support) so if you don't like the yaml representation, maybe you go to those projects instead.

krnhotwings commented 4 years ago

I've used several validators:

They complain about the same fields, regardless of JSON or YAML. The issue isn't about the output format because the problem seems to manifest at the Reader level when the scala module is present in the classpath (as seen in the log files I originally attached, you can see that SeqString has a required: [] property present.)

I've done some reading this morning as to why the validators complain, and it turns out that OAS3 uses JSON Schema draft 00 to validate Schema objects. That particular draft version states the following regarding the required property:

The value of this keyword MUST be an array. This array MUST have at least one element. Elements of this array MUST be strings, and MUST be unique.

https://tools.ietf.org/html/draft-wright-json-schema-validation-00#section-5.15

Anywho, the issue is pretty inconsequential (at least to me,) but I figured I should report it since there was some kind of change in behavior between this module's v1.1 and 2.1.

pjfanning commented 3 years ago

I think this is fixed by https://github.com/swagger-akka-http/swagger-scala-module/pull/121 (released in v2.3.4)