swagger-akka-http / swagger-scala-module

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

Regression: Option[Map[String, CaseClass]] #208

Closed jgulotta closed 1 year ago

jgulotta commented 1 year ago

In version 2.6.0, having a field with type Option[Map[String, CaseClass]] would correctly generate a schema like

MapStringCaseClass {
  type: "object",
  additionalFields: { $ref: "#/components/schemas/CaseClass" }
}

In 2.7.7 the same type definition throws an exception like

com.fasterxml.jackson.databind.JsonMappingException: additionalProperties must be either a Boolean or a Schema instance
at [Source: (String)"{"type":"object","additionalProperties":{"$ref":"#/components/schemas/CaseClass"}}"; line: 1, column: 83] (through reference chain: io.swagger.v3.oas.models.media.StringSchema["additionalProperties"])
at com.fasterxml.jackson.databind.JsonMappingException.from(JsonMappingException.java:276)
at com.fasterxml.jackson.databind.deser.SettableBeanProperty._throwAsIOE(SettableBeanProperty.java:623)
at com.fasterxml.jackson.databind.deser.SettableBeanProperty._throwAsIOE(SettableBeanProperty.java:611)
at com.fasterxml.jackson.databind.deser.impl.MethodProperty.deserializeAndSet(MethodProperty.java:143)
at com.fasterxml.jackson.databind.deser.BeanDeserializer.vanillaDeserialize(BeanDeserializer.java:314)
at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:177)
at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:323)
at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4674)
at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3629)
at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3597)
at com.github.swagger.scala.converter.SwaggerScalaModelConverter.correctSchema(SwaggerScalaModelConverter.scala:299)
at com.github.swagger.scala.converter.SwaggerScalaModelConverter.$anonfun$scalaClassSchema$11(SwaggerScalaModelConverter.scala:212)

Some initial debugging suggests that ErasureHelper.erasedOptionalPrimitives is incorrectly resolving the Map as a String because it recursively looks at only the first type parameter. I expect this might apply to Option[Seq[String]] as well.

pjfanning commented 1 year ago

Thanks - 2.7.x is pretty experimental (mentioned in readme) - best to stick with v2.6.x. I can look at your example in a few days time.

pjfanning commented 1 year ago

Could you provide all the relevant classes so that I don't have to spend time reproducing the issue?

SamTheisens commented 1 year ago

@jgulotta thanks for the example. I ran into the same problem, but couldn't get a good reproduction scenario.

jgulotta commented 1 year ago

There's no special class - near as I can tell It's anything with the Option[Map[...]] form. This is as small a reproducer as I have right now

case class OptMap(optmap: Option[Map[String, Int]])

trait Route {
  @Path("/")
  @ApiResponse(content = Array(new Content(schema = new Schema(implementation = classOf[OptMap]))))
  def optmap: OptMap = ???
}

new Reader(new SwaggerConfiguration().openAPI(new OpenAPI())).read(classOf[Route])
SamTheisens commented 1 year ago

There's no special class - near as I can tell It's anything with the Option[Map[...]] form. This is as small a reproducer as I have right now

case class OptMap(optmap: Option[Map[String, Int]])

trait Route {
  @Path("/")
  @ApiResponse(content = Array(new Content(schema = new Schema(implementation = classOf[OptMap]))))
  def optmap: OptMap = ???
}

new Reader(new SwaggerConfiguration().openAPI(new OpenAPI())).read(classOf[Route])

I'm sorry, what I meant was that your example did help me create a reproduction test. It's in this PR, which should solve the problem https://github.com/swagger-akka-http/swagger-scala-module/pull/210

jgulotta commented 1 year ago

Oh! I was half responding to pjfanning, misinterpreted, and didn't notice you'd already filed the PR 😅

pjfanning commented 1 year ago

I'll sort out a 2.7.8 release - I'm travelling today so it might not be today

pjfanning commented 1 year ago

2.7.8 is published

jgulotta commented 1 year ago

Thanks for the quick fix!

Do you have any known issues that make the 2.7 series experimental? It'd be preferable for us due to not requiring annotations for handling of primitives + erasure.

pjfanning commented 1 year ago

Use v2.7 if you like but I can not guarantee that every scala class setup will operate as expected. The test coverage in this module is far from complete.