[Scala] Fix inconsistency between *Api and *ApiAsyncHelper default parameters

[gmarz]

PR checklist

• Read the contribution guidelines.
• Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
• Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
• Copied the technical committee to review the pull request if your PR is targeting a particular programming language. (/cc @clasnake @jimschubert)

Description of the PR

Closes #7273

[jimschubert]

Unless I'm reading this incorrectly, the change removes support for default values for types that are optionally required and have a default defined.

That is… PathParameter are always required.

QueryParameters may be optional, meaning you can pass a query parameter with a value or omit the parameter and provide a default. Your change forces those defaults to always be Option[A] = None, which isn't correct. The way the code worked before is:

maybeOmitted = Option[A] = if(hasDefault) Some(defaultValue) else None

As an example, I can have a GET /orders/recent defined with an int query parameter count. This count can be defined with attributes:

• min=0
• max=100
• defaultValue=20
• required=false

If I call the endpoint with GET /orders/recent?count=50, the Option[Int] should have a value of Some(50).

If I call the endpoint without supplying the optional query parameter, that is GET /orders/recent, the Option[Int] should have a value of Some(20). With your change it will always be None with a comment in code of /* Some(20) */.

[gmarz]

Unless I'm reading this incorrectly, the change removes support for default values for types that are optionally required and have a default defined.

That's correct. However, I believe there are > 1 issues with defaulting to defaultValue at the moment (in addition to #6538 (comment), I also think defaultValue = "someString" ends up as Some(someString) which is also wrong...but I need to test that again to confirm).

More importantly, the *Api methods at the moment default to None so this change at least makes *ApiAsyncHelper consistent with that behavior until we fix the default values.

[jimschubert]

@gmarz please see my pr #7286, where I added integration tests referring to this PR and changing from the None default change from the other linked PR back to supporting default values.

I don't understand your comment that a spec defining defaultValue = "some string" resulting in the generated code ending up at Some(some string) is wrong. Are you saying that the string isn't quoted? If so, that's an easy enough fix, and I can make the change with an appropriate test modification in PR #7286. I think string is the only primitive value that would have this issue in Scala (if it's not be properly quoting).

Sorry, I meant to tag you in that other pr as it directly relates to and partially conflicts with this pr.

[gmarz]

Are you saying that the string isn't quoted?

Yep, sorry for not being clear. This is what I observed during my testing of 2.3.0, but would be great if you could confirm as part of #7286 and even better if you can add a fix!

I think string is the only primitive value that would have this issue in Scala (if it's not be properly quoting).

Sounds right.

[jimschubert]

@gmarz I'm working on a fix. I have the string issue fixed locally, but there were issues with string types having formats (date, date-time, byte, binary). I'll try to have those fixed and added to that PR within a day or two.

Thanks for raising that as an issue, because it was a pretty big one.

[jimschubert]

I updated #7286 to support all previously missing functionality, and this should allow for full support of default values. Please review, as I think that PR would supersede this one.

[wing328]

Closed via #7286 instead