play-swagger
play-swagger copied to clipboard
iheartradio
Default values for Option parameters aren't parsed correctly
play-swagger should be smarter when parsing defaults for Option[A].
While a workaround here is to leave defaults out of the routes file, this is less desirable because you lose the automatic documentation. In addition, reverse routes now have to pass in values for what should have been default parameters.
For Option[Boolean] ?= None, this results in complete failure because the SwaggerParameterMapper tries to parse the Boolean with "None".toBoolean resulting in an exception:
play.api.http.HttpErrorHandlerExceptions$$anon$1: Execution exception[[IllegalArgumentException: For input string: "None"]]
...
Caused by: java.lang.IllegalArgumentException: For input string: "None"
at scala.collection.immutable.StringLike$class.parseBoolean(StringLike.scala:290)
at scala.collection.immutable.StringLike$class.toBoolean(StringLike.scala:260)
at scala.collection.immutable.StringOps.toBoolean(StringOps.scala:29)
at com.iheart.playSwagger.SwaggerParameterMapper$$anonfun$7.apply(SwaggerParameterMapper.scala:36)
at com.iheart.playSwagger.SwaggerParameterMapper$$anonfun$7.apply(SwaggerParameterMapper.scala:32)
at scala.Option.map(Option.scala:146)
at com.iheart.playSwagger.SwaggerParameterMapper$.mapParam(SwaggerParameterMapper.scala:32)
at com.iheart.playSwagger.SwaggerParameterMapper$.com$iheart$playSwagger$SwaggerParameterMapper$$optionalParam$1(SwaggerParameterMapper.scala:70)
at com.iheart.playSwagger.SwaggerParameterMapper$$anonfun$3.applyOrElse(SwaggerParameterMapper.scala:85)
at com.iheart.playSwagger.SwaggerParameterMapper$$anonfun$3.applyOrElse(SwaggerParameterMapper.scala:83)
The above example is a showstopper for us due to the exception. The use case is a flag that can be forced to true or false, but it has automatic behavior if it's left out. For now, my workaround is to pass None for all Reverse routes since I have to leave ?= None out of routes.
For Option[String] ?= None, you might end up with:
"in": "query",
"name": "version",
"type": "string",
"required": false,
"default": "None"
Instead, there should just be no default.
For Some("v1"), you'd get the same except the default would be
"default": "Some(\"v1\")"
Since the documentation says this is just a string from the caller's perspective, the default can simply be "v1".
This is related to #72, but is more than just a UI issue.
I believe that you don't need to explicitly set None as the default value, it is already the default value. Play-swagger will treat augment of Option type as optional arguments. For Some, have you tried just put the string value there? Not sure if that would work though, if it doesn't, you may have to manually write the default value in the yaml swagger comment.
You're right in that you don't have to set none for routing. The example I mentioned above was the tradeoff:
reverse routes now have to pass in values for what should have been default parameters.
This isn't the end of the world of course.
For Some("v1") as a default, I had the same thought, but you can't use "v1" with an Option because Play parses and compiles all this and the resulting code won't compile. Setting the default explicitly for documentation is a good workaround there.
I'm not sure if this is related or if I should open another issue for this, but the swagger-custom-mappings.yml suffers similar issues when dealing with Option[A] wrapped types.
We make heavy use of case classes that wrap strings for type safety (eg: final case class PostalCode(value: String) but in JSON they're all just strings. The custom mapping approach works fine until they're optional, then they pull in the "$ref" definition anyway.
@pettyjamesm I think that is a separate issue. Would adding a duplicated custom mapping entry for Option\[YourType\] work for you as a work around?
@kailuowang Ok, I'll open another issue. I tried using Option\[MyType\] and scala\.Option\[MyType\] and neither successfully overrode the property type.
@wuservices is this still an issue after https://github.com/iheartradio/play-swagger/pull/141 was merged into version https://github.com/iheartradio/play-swagger/releases/tag/v0.5.4 ?
@pettyjamesm looks like it's happening on 0.5.4 for me. I changed w: Option[Boolean] to w: Option[Boolean] ?= None in one of my routes and got the same error as I was getting before.
On the other had, I think we weren't seeing Option[SomeEnumeration.Value] show up before, and now it does so I think that was improved!
Yeah, after thinking about this further- I think using Option and default query parameter values is a bad pattern without a clear implementation strategy. If a value is purely optional, then wrapping it in Option should be sufficient. If the value is always necessary but has a reasonable default (eg: GET /blog/posts/:page @controllers.BlogPosts.index(page: Int ?= 1)) then the code should not actually wrap an option type at all.
I suggest this issue be closed unless there's a clear definition of what the correct behavior should be.
@kailuowang a little awkwardness in reverse routing is probably to be expected here. I believe this is the router / reverse router abstraction leaking through. Anyway, I guess a special case for the Option type isn't the end of the world, but the implementation feels uncomfortably arbitrary to me personally.
@pettyjamesm I agree with you that we don't need a special case for the Option type. Just want to point that we might not want to close this issue since it is a legitimate, thought might be minor, issue for some users.
While agree that special cases can be ugly, this seems like a pretty typical one. Since having an Option with a default parameter works without using play-swagger, it's unfortunate to have to change potentially existing routes to avoid an exception.
While non-None defaults for an Optional param, may be more implementation specific, the whole purpose of Option in the route is to say that it's optional so implicitly, the default is None when you're making the request - so it's nice to have the same behavior internally as well.
Still, a fairly minor issue, but it doesn't seem too esoteric.
The main problem here is that the value for a default parameter is a text for the routes compiler in play and they never need to use it as a typed object. This means that any combination of types used as default value on a parameter needs to be parsed from string by play-swagger and I don't think this is a very good idea.
And as a side note when you put a default value on a route your parameter is marked as not required on the routes compiler, maybe it's a bug on play's reverse router to have this parameter without a default value.
I tried to simulate the reverse routes problem but it seems to behave correctly.
For example when you have a route like this:
GET /test/:id/defaultParam controllers.HomeController.idParam(id: String, strFlag ?= "defaultValue")
The generate reverse route looks like this:
def idParam(id:String, strFlag:String = "defaultValue"): Call
Which for me is correct and I have to agree with @pettyjamesm that using Option on a default value should be outside of the scope of the library. If it's really necessary to present this on the swagger file you can still override the route endpoint with your desired value.
I too had this problem yesterday.
variable: option[T] ? = None is useful when you want to omit arguments for redirects, URL references, etc.
? = None, would you consider merging a PR that ignores it?
#458
