[scala] updates for client default values, required attributes #7286

jimschubert · 2018-01-01T19:50:42Z

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.

Description of the PR

PRs #7274 and #7275 have logic around parameter types marked with "required" and their default values. While the fix in #7274 is most likely fine considering body parameters should work the same whether missing from the request or null, the fix in #7275 will break support for default parameter values in the scala client. This appears to be related to a comment about an edge case from a previously merged PR (see #7273 references), which I assume refer to the use of Some(defaultValue) rather than Option(defaultValue).

This PR fixes edge cases where defaultValue may be null, which would result in a value defined as null (Some(null)) rather than the intended scala-like None. This is fixed by defaulting using Option(defaultValue) because Option(null) results in None.

This also undoes the workaround merged from #6538, which removed support for parameter default values in the scala client template.

The added integration test includes an optional model property without a default value (Person.age), an optional query string parameter without a default value (GET /people?age=???) and an optional query string parameter with a default value (POST /people?size=???). Please let me know if there are other conditions we'd like to test against.

This includes a fix to the samples, pertaining to a missing import that is also fixed in #7271.

This also adds support for better support of type=string and format={date,date-time,binary,byte}. Previously, binary and byte were inconsistently defined as strings rather than byte arrays, while date/date-time were parsing default values into formats that did not match OpenAPI/Swagger 2.0 specifications for full-date and date-time.

We may want to consider pulling in json4s-ext to support wider date formats and moving to date=LocalDate and date-time=ZonedDateTime.

This will have breaking changes for consumers expecting binary/byte to be strings rather than byte arrays.

Note

If your team requires a workaround similar to that from #6538, I recommend customizing the template locally. This can be done with the following steps:

mkdir -p ~/temp/swagger-codegen
git init
git remote add swagger-codegen https://github.com/swagger-api/swagger-codegen.git
git fetch swagger-codegen
git checkout swagger-codegen/master -- modules/swagger-codegen/src/main/resources/scala
mv modules/swagger-codegen/src/main/resources/scala path/to/scala-client-template
\rm -rf modules/
\rm -rf .git

Where path/to/scala-client-template is your final desired location for templates.

Then, when invoking swagger-codegen-cli, pass -t path/to/scala-client-template and this will use your customized templates.

This uses consistent logic for optional types with default values in the scala client. Also, uses Option(default) instead of Some(default) to guard against people defining defaultValue = null. Option(null) becomes None while Some(null) defines a null value explicitly and will break maplike operations.

…but allows current samples to generate

jimschubert · 2018-01-04T02:56:24Z

I'm fixing additional issues found with string types and format specified (date, date-time, binary, byte). Will push an update to this PR when I've tested.

This adds support for better support of type=string and format={date,date-time,binary,byte}. Previously, binary and byte were inconsistently defined as strings rather than byte arrays, while date/date-time were parsing default values into formats that did not match OpenAPI/Swagger 2.0 specifications for full-date and date-time. We may want to consider pulling in json4s-ext to support wider date formats and moving to date=LocalDate and date-time=ZonedDateTime. This will have breaking changes for consumers expecting binary/byte to be strings rather than byte arrays.

jimschubert · 2018-01-04T03:07:45Z

modules/swagger-codegen/src/main/resources/scala/api.mustache

@@ -66,7 +78,7 @@ class {{classname}}(
 {{#allParams}}   * @param {{paramName}} {{description}} {{^required}}(optional{{#defaultValue}}, default to {{{.}}}{{/defaultValue}}){{/required}}
 {{/allParams}}   * @return {{#returnType}}{{{returnType}}}{{/returnType}}{{^returnType}}void{{/returnType}}
   */
-  def {{operationId}}({{#allParams}}{{paramName}}: {{#required}}{{dataType}}{{#defaultValue}} = {{{defaultValue}}}{{/defaultValue}}{{/required}}{{^required}}Option[{{dataType}}]{{#defaultValue}} = Option({{{defaultValue}}}){{/defaultValue}}{{^defaultValue}} = None{{/defaultValue}}{{/required}}{{#hasMore}}, {{/hasMore}}{{/allParams}}){{#returnType}}: Option[{{returnType}}]{{/returnType}} = {
+  def {{operationId}}({{#allParams}}{{paramName}}: {{#required}}{{dataType}}{{#defaultValue}} = {{#isString}}"{{{defaultValue}}}"{{/isString}}{{^isString}}{{#isByteArray}}"{{/isByteArray}}{{#isDate}}dateFormatter.parse("{{/isDate}}{{#isDateTime}}dateTimeFormatter.parse("{{/isDateTime}}{{{defaultValue}}}{{#isDate}}"){{/isDate}}{{#isDateTime}}"){{/isDateTime}}{{#isByteArray}}".getBytes{{/isByteArray}}{{/isString}}{{/defaultValue}}{{/required}}{{^required}}Option[{{dataType}}]{{#defaultValue}} = Option({{#isString}}"{{{defaultValue}}}"{{/isString}}{{^isString}}{{#isByteArray}}"{{/isByteArray}}{{#isDate}}dateFormatter.parse("{{/isDate}}{{#isDateTime}}dateTimeFormatter.parse("{{/isDateTime}}{{{defaultValue}}}{{#isDate}}"){{/isDate}}{{#isDateTime}}"){{/isDateTime}}{{#isByteArray}}".getBytes{{/isByteArray}}{{/isString}}){{/defaultValue}}{{^defaultValue}} = None{{/defaultValue}}{{/required}}{{#hasMore}}, {{/hasMore}}{{/allParams}}){{#returnType}}: Option[{{returnType}}]{{/returnType}} = {


The templating for defaultValue cases is a little ugly. We may be able to abstract it out, but it relies on date formatters local to this file and I thought it may be odd to extract.

jimschubert · 2018-01-04T03:08:51Z

...la/client/required-attributes-expected/src/main/scala/io/swagger/client/api/HobbiesApi.scala

+   * @param b3 an octet string (optional, default to DEADBEEF)
+   * @return Future(Hobby)
+   */
+  def getHobbiesAsync(s: Option[String] = Option("some string"), i: Option[Integer] = Option(1), l: Option[Long] = Option(2), b: Option[Boolean] = Option(true), f: Option[Float] = Option(0.1), d: Option[Double] = Option(10.005), datetime: Option[Date] = Option(dateTimeFormatter.parse("2018-01-01T08:30:00Z-04:00")), date: Option[Date] = Option(dateFormatter.parse("2018-01-01")), b2: Option[ArrayByte] = Option("c3dhZ2dlciBjb2RlZ2Vu".getBytes), b3: Option[ArrayByte] = Option("DEADBEEF".getBytes)): Future[Hobby] = {


Notice all optional fields here and their respective default values.

…y conflicting names

jimschubert · 2018-01-04T03:16:52Z

Ok. I've pushed my changes and also updated the original PR description.

@gmarz would you mind also review this PR and the defaults in the integration tests?

gmarz · 2018-01-06T14:47:39Z

@jimschubert LGTM! 👍

@wing328 I noticed you added this to the 2.4.0 milestone...do you think 2.3.1 is more appropriate seeing that this is a bug fix? If we think the defaultValue change here is too big for a patch release then maybe we consider #7273 as the interim fix for 2.3.1?

jimschubert · 2018-01-06T15:42:13Z

Thanks for reviewing.

It's likely that anyone who requires default values has already created a custom template with fixes similar to these.

The only code changes are mapping binary and byte to byte arrays. In my experience, binary isn't too common. Byte as a byte array would likely have little negative/unwanted impact. So I'd suggest 2.3.1.

For instance, this change won't affect my team's Scala client generation, because I've customized our template to generate a finagle-http client which just relies on the generator class's file structure expectation. We don't use byte arrays, so the code change won't affect us. If it did, it would be a matter of adding .toByteArray wherever that code is touched, or including an implicit conversion.

A change I was going to make here, but considered too much of a breaking change, was to move the definition of the date formatters to a separate file as a utility object. This way, consumers can change format expectations in the file, and update .swagger-codegen-ignore to allow that file to persist across generations. If this gets merged to 2.3.1, I can make that change separately. If it targets 2.4.0, I can go ahead and add that to this PR.

wing328 · 2018-01-07T08:59:36Z

@jimschubert @gmarz if this fix is urgent, I can merge it into 2.3.1.

Currently, I've merged a few regression fixes for other generators into 2.3.1 and can include this one as well. Let me know.

jimschubert · 2018-01-07T13:22:26Z

@wing328 without this fix, Scala client doesn't support some default values that it did before, and sync/async methods have inconsistent APIs in the built in templates. I'm not sure it's "urgent", as users could use 2.2.0 with little or no changes to templates. But I also think that because the fix introduced support for all default values, that's a net I positive over any existing code that needs to update string to byte array.

The date string is configured to Open API 2.0 required format, which is a slight change from the previous format, which didn't parse Open API 2.0 specified format or ISO 8601 date strings consistently.

jimschubert · 2018-01-07T13:44:11Z

Sorry, can't seem to edit that comment on mobile. To clarify, older versions of the Scala client didn't support default values for type=string with a format defined. A recent pr changed everything to = None, which removed support for all defaults. This undoes that change, adding back support for default value that were previously supported. This also includes an update to add additional support. Sorry for the confusion opening sentence in my previous comment.

wing328 · 2018-01-07T13:44:32Z

without this fix, Scala client doesn't support some default values that it did before

OK. I think we need to include it in the patch release. Otherwise, users will need to use 2.2.3 or 2.4.0 later.

wing328 · 2018-01-07T13:45:52Z

@jimschubert can you please have a look at the following errors when you've time?

Results :

Failed tests: 
  ScalaClientRequiredAttributesIntegrationTest.test:52->AbstractIntegrationTest.generatesCorrectDirectoryStructure:54 Directory content of '/home/ubuntu/swagger-codegen/modules/swagger-codegen/target/test-classes/integrationtests/scala/client/required-attributes-expected/src/main/scala/io/swagger/client' and '/home/ubuntu/swagger-codegen/modules/swagger-codegen/target/test-classes/integrationtests/scala/client/required-attributes-result/src/main/scala/io/swagger/client' differ.: Lists differ at element [0]: model != api expected [model] but found [api]

Tests run: 545, Failures: 1, Errors: 0, Skipped: 3

jimschubert · 2018-01-07T14:01:38Z

@wing328 I'll try to take a look a little later. I didn't write the base integration test comparer, so I'll have to dig into that and check it out.

What version of Ubuntu and Java was that? I'll also setup a VM and test in the same OS to verify if it's an environment thing.

jimschubert · 2018-01-08T03:19:07Z

@wing328 I'll have to provide a fix for the AbstractIntegrationTest class.

The custom assertion does a naive comparison of directory listings:

assertEquals(expectedDir.toFile().list(),
    actualDir.toFile().list(),
    String.format("Directory content of '%s' and '%s' differ.", expectedDir, actualDir));

The javadocs for list state there is no guarantee about ordering of this array.

There is no guarantee that the name strings in the resulting array
will appear in any specific order; they are not, in particular,
guaranteed to appear in alphabetical order.

It should be a relatively quick fix, I'll try to get it pushed by tomorrow evening.

Per File#list() javadocs: There is no guarantee that the name strings in the resulting array will appear in any specific order; they are not, in particular, guaranteed to appear in alphabetical order. I'm unable to repro directory listing failures on OS X High Sierra or Ubuntu 16.04 under Parallels, so it's not clear to me if listing order is indeterminate per-platform or the behavior is just not defined and up to the platform's installed runtime. Sorting the array of strings prior to comparison should resolve this issue on every platform/runtime.

Script should match options in the integration test class

jimschubert · 2018-01-08T06:05:40Z

So, I fixed the issue with directory comparisons… File#list() doesn't guarantee a sort order.

On Travis, this results in a failure to compare file contents (see https://travis-ci.org/swagger-api/swagger-codegen/builds/326216751#L2550 ):

Results :
Failed tests: 
  ScalaClientRequiredAttributesIntegrationTest.test:52->AbstractIntegrationTest.generatesCorrectDirectoryStructure:54 files diff:
	file: '/home/travis/build/swagger-api/swagger-codegen/modules/swagger-codegen/target/test-classes/integrationtests/scala/client/required-attributes-expected/src/main/scala/io/swagger/client/api/HobbiesApi.scala' 
	file: '/home/travis/build/swagger-api/swagger-codegen/modules/swagger-codegen/target/test-classes/integrationtests/scala/client/required-attributes-result/src/main/scala/io/swagger/client/api/HobbiesApi.scala' 
	diffs:
[InsertDelta, position: 46, lines: [import org.json4s._, ]]

Which shouldn't happen, considering I've added this line to the template, see

https://github.com/jimschubert/swagger-codegen/blob/8052848703d3161eda77e9a2b8680769cc8470f6/modules/swagger-codegen/src/main/resources/scala/api.mustache#L40

And this is included in the generated test file, see

https://github.com/jimschubert/swagger-codegen/blob/8052848703d3161eda77e9a2b8680769cc8470f6/modules/swagger-codegen/src/test/resources/integrationtests/scala/client/required-attributes-expected/src/main/scala/io/swagger/client/api/HobbiesApi.scala#L51

I've run this on OS X and Ubuntu without issues using Java 8.

When I run locally via ./run-in-docker.sh mvn clean install (which uses a Java 7 image), I get a compilation error:

[INFO] -------------------------------------------------------------
[ERROR] COMPILATION ERROR :
[INFO] -------------------------------------------------------------
[ERROR] /gen/modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/JavaPKMSTServerCodegen.java:[85,35] method remove in interface java.util.Map<K,V> cannot be applied to given types;
  required: java.lang.Object
  found: java.lang.String,java.lang.String
  reason: actual and formal argument lists differ in length
[ERROR] /gen/modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/JavaPKMSTServerCodegen.java:[86,33] method remove in interface java.util.Map<K,V> cannot be applied to given types;
  required: java.lang.Object
  found: java.lang.String,java.lang.String
  reason: actual and formal argument lists differ in length

I'm not sure if this is a Java 7 issue (I'll investigate this in Ubuntu tomorrow), or a build cache issue. I'm not too familiar with TravisCI or CircleCI and how they handle caching. If we run mvn install and swagger-codegen is cached in the ~/.m2/repository directory, are those previous builds included on the classpath for the running build? If so, would that cause template conflicts? Sorry if those are silly questions, I've always only done local compile/publish of build artifacts and cached dependencies only.

wing328 · 2018-01-08T17:08:27Z

@jimschubert thanks for the update. I'll also take a look tomorrow.

CI doesn't seem to pick up template changes in integration tests. Disabling scala client integration tests, pending investigation of the issue.

* master: (26 commits) [Scala] Fix async helper methods when body is optional (swagger-api#7274) [Rust] Recommend style based on 'rustfmt' defaults (swagger-api#7335) [Java:vertx] Initialize router in init method and re-use router member to create S… (swagger-api#7234) [Scala] Fix missing json4s import (swagger-api#7271) deploy snapshot version 2.3.1 [Ada] Add Ada support for server code generator swagger-api#6680 (swagger-api#7256) add shijinkui to scala technical committee Generate swagger yaml for go client (swagger-api#7281) use openjdk7 in travis to ensure it works with jdk7 docs(readme): update link to contributing guid (swagger-api#7332) Fix a regression bug that was introduce in a recent commit. Removed the tabs that were causing error in Play Framework (swagger-api#7241) Fix issue swagger-api#7262 with the parameter name in the path. The problem was that camelCase naming was forced only in this part of the code when everywhere else it is configurable. (swagger-api#7313) Java8 fix (swagger-api#7260) update to 2.3.1-SNAPSHOT fix typo, update 2017 to 2018 [Doc] add huawei cloud to companies list swagger-api#7308 (swagger-api#7309) Adding Peatio opensource as reference project (swagger-api#7267) Update README.md (swagger-api#7298) Update README.md (swagger-api#7299) [all] sys props in CodegenConstants ...

jimschubert · 2018-01-10T04:31:11Z

@wing328 I've skipped integration tests, since the runner has issues. This should be good.

gmarz · 2018-01-10T23:43:41Z

@jimschubert I think I might have found one issue with this change while testing a snapshot of 2.3.1.

We have the following annotations for one of our responses:

@ApiResponses(Array(new ApiResponse(code = 200, message = "blah", response = classOf[Byte]))

Which produces the following in the spec:

"schema": {
  "type": "string",
  "format": "byte"
}

Which in turn use to result in Option[String] in the generated Scala client, but now results in Option[ArrayByte]...which isn't even a valid Scala type? Furthermore, since it's not a valid type, it thinks this is an API model and adds it as an import to the *Api classes.

gmarz · 2018-01-11T16:30:04Z

@jimschubert I opened #7371

jimschubert added 4 commits December 30, 2017 15:46

[scala] Regenerate client sample

6291eeb

[scala] Add missing json4s import, which will be added by another PR …

e2551fd

…but allows current samples to generate

[scala] Include integration tests for required attributes support

b6a11ab

jimschubert mentioned this pull request Jan 3, 2018

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

Closed

4 tasks

jimschubert commented Jan 4, 2018

View reviewed changes

[scala] Unique parameter names in integration test, to avoid seemingl…

075ce98

…y conflicting names

[scala] Regenerate client sample

f240ea3

wing328 added Client: Scala Issue: Bug labels Jan 5, 2018

wing328 added this to the v2.4.0 milestone Jan 5, 2018

jimschubert added 2 commits January 7, 2018 22:25

[scala] exclude api tests for integration test gen script

60510be

Script should match options in the integration test class

jimschubert added 2 commits January 9, 2018 22:24

[scala] Temporarily disable client integration tests

4375290

CI doesn't seem to pick up template changes in integration tests. Disabling scala client integration tests, pending investigation of the issue.

jimschubert added 3 commits January 9, 2018 22:39

[scala] Remove redundant json4s import

4921df4

[scala] Regenerate integration test

ba01341

[scala] Regenerate sample

1eb43a6

wing328 merged commit 65bda3e into swagger-api:master Jan 10, 2018

jimschubert deleted the scala-client-default-values branch January 10, 2018 05:47

gmarz mentioned this pull request Jan 11, 2018

[Scala] Invalid generated type for type=string, format=byte #7371

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[scala] updates for client default values, required attributes #7286

[scala] updates for client default values, required attributes #7286

jimschubert commented Jan 1, 2018 •

edited

Loading

jimschubert commented Jan 4, 2018

jimschubert Jan 4, 2018

jimschubert Jan 4, 2018

jimschubert commented Jan 4, 2018

gmarz commented Jan 6, 2018

jimschubert commented Jan 6, 2018

wing328 commented Jan 7, 2018

jimschubert commented Jan 7, 2018

jimschubert commented Jan 7, 2018

wing328 commented Jan 7, 2018

wing328 commented Jan 7, 2018

jimschubert commented Jan 7, 2018

jimschubert commented Jan 8, 2018

jimschubert commented Jan 8, 2018

wing328 commented Jan 8, 2018

jimschubert commented Jan 10, 2018

gmarz commented Jan 10, 2018

gmarz commented Jan 11, 2018

[scala] updates for client default values, required attributes #7286

[scala] updates for client default values, required attributes #7286

Conversation

jimschubert commented Jan 1, 2018 • edited Loading

PR checklist

Description of the PR

Note

jimschubert commented Jan 4, 2018

jimschubert Jan 4, 2018

Choose a reason for hiding this comment

jimschubert Jan 4, 2018

Choose a reason for hiding this comment

jimschubert commented Jan 4, 2018

gmarz commented Jan 6, 2018

jimschubert commented Jan 6, 2018

wing328 commented Jan 7, 2018

jimschubert commented Jan 7, 2018

jimschubert commented Jan 7, 2018

wing328 commented Jan 7, 2018

wing328 commented Jan 7, 2018

jimschubert commented Jan 7, 2018

jimschubert commented Jan 8, 2018

jimschubert commented Jan 8, 2018

wing328 commented Jan 8, 2018

jimschubert commented Jan 10, 2018

gmarz commented Jan 10, 2018

gmarz commented Jan 11, 2018

jimschubert commented Jan 1, 2018 •

edited

Loading