[scala] updates for client default values, required attributes#7286
[scala] updates for client default values, required attributes#7286wing328 merged 14 commits intoswagger-api:masterfrom
Conversation
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
|
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.
| {{/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}} = { |
There was a problem hiding this comment.
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.
| * @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] = { |
There was a problem hiding this comment.
Notice all optional fields here and their respective default values.
…y conflicting names
|
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? |
|
@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 |
|
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 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 |
|
@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. |
|
@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. |
|
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 |
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. |
|
@jimschubert can you please have a look at the following errors when you've time? |
|
@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. |
|
@wing328 I'll have to provide a fix for the AbstractIntegrationTest class. The custom assertion does a naive comparison of directory listings: The javadocs for list state there is no guarantee about ordering of this array. 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
|
So, I fixed the issue with directory comparisons… On Travis, this results in a failure to compare file contents (see https://travis-ci.org/swagger-api/swagger-codegen/builds/326216751#L2550 ): Which shouldn't happen, considering I've added this line to the template, see And this is included in the generated test file, see I've run this on OS X and Ubuntu without issues using Java 8. When I run locally via 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 |
|
@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 ...
|
@wing328 I've skipped integration tests, since the runner has issues. This should be good. |
|
@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: Which produces the following in the spec: Which in turn use to result in |
|
@jimschubert I opened #7371 |
3 participants
PR checklist
./bin/to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.shand./bin/security/{LANG}-petstore.shif 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\.3.0.0branch for changes related to OpenAPI spec 3.0. Default:master.Description of the PR
cc @clasnake @wing328
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 thanOption(defaultValue).This PR fixes edge cases where
defaultValuemay benull, which would result in a value defined as null (Some(null)) rather than the intended scala-likeNone. This is fixed by defaulting usingOption(defaultValue)becauseOption(null)results inNone.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:
Where
path/to/scala-client-templateis your final desired location for templates.Then, when invoking swagger-codegen-cli, pass
-t path/to/scala-client-templateand this will use your customized templates.