swagger-akka-http / swagger-scala-module

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

Regression: Option[Set[String]] schema renders with `type=string` instead of `type=array` #309

Open jgulotta opened 10 months ago

jgulotta commented 10 months ago

Given the class

case class OptSetString(strings: Option[Set[String]])

In 2.12.0 the schema incorrectly renders as

{"uniqueItems":true,"type":"string","items":{"type":"string"}}

In 2.7.2 the schema correctly renders as

{"uniqueItems":true,"type":"array","items":{"type":"string"}}

Moreover, the incorrect behavior cannot be overridden by adding an ArraySchema annotation

I suspect this is due to #174 and would apply recursively to any nested structure that eventually has a primitive as the first type parameter. Sorry about the late report on what is now an old change; we've only recently gone about updating our swagger codegen and encountered the problem

pjfanning commented 10 months ago

Hi @jgulotta - I can confirm that this is an issue. That PR you referenced is a bit of a mess. I need to work out whether it is best to abandon the ErasureHelper or try to keep digging a bigger hole for it.

In the mean time, I suggest that you stick with an older swagger-scala-module jar.

pjfanning commented 10 months ago

@jgulotta I added a fix - https://github.com/swagger-akka-http/swagger-scala-module/pull/310

This may not cover deeply nested strutures (eg option of seq of option of T) but it is hopefully enough for Option[Set[String]].

I don't use swagger any more - and not in a number of years. So, I would appreciate if you could try out the snapshot releases of swagger-scala-module. 2.12.0+7-ff3a38d1-SNAPSHOT has the #310 change in it.

You can work out the latest snapshot releases by looking at https://oss.sonatype.org/content/repositories/snapshots/com/github/swagger-akka-http/swagger-scala-module_2.13/ or checking the logs from the GitHub Action builds for this project.

jgulotta commented 10 months ago

Verified the snapshot solves the problem and all looks good, thanks!

jgulotta commented 10 months ago

Can I ask for a fix to the "incorrect behavior cannot be overridden by adding an ArraySchema annotation" part too? I think it would be making

val schemaOverride = propertyAnnotations.collectFirst { case s: SchemaAnnotation => s }

and subsequent code handle ArraySchema in addition to Schema

pjfanning commented 10 months ago

Can I ask for a fix to the "incorrect behavior cannot be overridden by adding an ArraySchema annotation" part too? I think it would be making

val schemaOverride = propertyAnnotations.collectFirst { case s: SchemaAnnotation => s }

and subsequent code handle ArraySchema in addition to Schema

As I stated earlier, I am not using swagger for anything. This lib is not a major priority for me. If you want to submit a PR, I will look at it but I have limited time and am focused on non-swagger work.

pjfanning commented 10 months ago

I have https://github.com/swagger-akka-http/swagger-scala-module/pull/312 but I still need to fix one of the tests

jgulotta commented 10 months ago

As I stated earlier, I am not using swagger for anything. This lib is not a major priority for me. If you want to submit a PR, I will look at it but I have limited time and am focused on non-swagger work.

Totally fair! I appreciate the speed and attention you've given so far