Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jgulotta opened this issue Feb 1, 2024 · 7 comments

Comments

@jgulotta
Copy link

jgulotta commented Feb 1, 2024

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

@jgulotta jgulotta changed the title Regression: Option[Seq[String]] schema renders with type=string instead of type=array Regression: Option[Set[String]] schema renders with type=string instead of type=array Feb 1, 2024
@pjfanning
Copy link
Contributor

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
Copy link
Contributor

@jgulotta I added a fix - #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
Copy link
Author

jgulotta commented Feb 1, 2024

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

@jgulotta
Copy link
Author

jgulotta commented Feb 2, 2024

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
Copy link
Contributor

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
Copy link
Contributor

I have #312 but I still need to fix one of the tests

@jgulotta
Copy link
Author

jgulotta commented Feb 2, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants