[Swfit4] better support for type=string, format=number #3910

siilats · 2019-09-18T18:15:13Z

PR checklist

[ x] Read the contribution guidelines.
[ x] 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, ./bin/openapi3/{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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
[x ] Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
[x ] Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

(details of the change, additional tests that have been done, reference to the issue for tracking, etc)

…tring format=number

…rator

siilats · 2019-09-18T18:16:15Z

@jgavris @ehyche @Edubits @jaz-ah @d-date

lemoinem · 2019-09-19T14:14:26Z

Great idea!

But before supporting decimal properly in one generator, maybe we should add something so the base Codegen supports them properly.
Currently, there is a long list of warning when using a OpenAPI spec with decimal in it.

You should add a DECIMAL_FORMAT in SchemaTypeUtils to return "decimal" as a primitive type. This would allow us to avoid the warning at https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java#L1561-L1570 for decimals.

The default typeMapping associated with decimal should be the same as for number. This would allow us to avoid the warnings and properly support the decimal type in other generators as well.

…rator

siilats · 2019-09-19T15:14:43Z

It does not return error but BigDecimal, its a bit of hack calling it a string but that hack is currently in master, so this pull just uses the hack to make swift4 work. There is some reason calling decimals strings because its backward compatible like that https://github.com/OpenAPITools/openapi-generator/blob/5610610d444ca30fbc2a6396877c26496ef2bcb4/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java#L1547 } else if (ModelUtils.isStringSchema(schema) && "number".equals(schema.getFormat())) { // special handle of type: string, format: number return "BigDecimal";

…

On Thu, Sep 19, 2019 at 10:14 AM Mathieu Lemoine ***@***.***> wrote: Great idea! But before supporting decimal properly in one generator, maybe we should add something so the base Codegen supports them properly. Currently, there is a long list of warning when using a OpenAPI spec with decimal in it. You should add a DECIMAL_FORMAT in SchemaTypeUtils to return "decimal" as a primitive type. This would allow us to avoid the warning at https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java#L1561-L1570 for decimals. The default typeMapping associated with decimal should be the same as for number. This would allow us to avoid the warnings and properly support the decimal type in other generators as well. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3910?email_source=notifications&email_token=AAI2ZY46BQXGLP3LIVUAZCDQKOCMRA5CNFSM4IYCDLDKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7DTUUQ#issuecomment-533150290>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAI2ZY3ABHUP3DMDWDD3PD3QKOCMRANCNFSM4IYCDLDA> .

-- Keith Siilats [email protected] www.bytelogics.com Fax: 646-352-4705 Phone: +1 (646) 881-4378

wing328 · 2019-09-20T09:06:48Z

It does not return error but BigDecimal, its a bit of hack calling it a
string but that hack is currently in master, so

Right, agreed it's a bit of hack as only string format supports any value according to the spec. In other words, we can't do "type: number, format: decimal" as it's invalid according to the spec.

Ref: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#data-types

lemoinem · 2019-09-20T16:46:40Z

It does not return error but BigDecimal, its a bit of hack calling it a
string but that hack is currently in master, so

Right, agreed it's a bit of hack as only string format supports any value according to the spec. In other words, we can't do "type: number, format: decimal" as it's invalid according to the spec.

Ref: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#data-types

From your reference:

OAS uses several known formats to define in fine detail the data type being used. However, to support documentation needs, the format property is an open string-valued property, and can have any value.
And
Tools that do not recognize a specific format MAY default back to the type alone, as if the format is not specified.

So, according to the spec, any type support arbitrary format. There is no restriction. The list below the text, is only the set of format that should be supported by all implementations.

siilats · 2019-09-20T16:58:44Z

What is your recommendation? I like using string:decimal and alias of BigDecimal to Decimal in swift4.

…

On Fri, Sep 20, 2019 at 12:46 PM Mathieu Lemoine ***@***.***> wrote: It does not return error but BigDecimal, its a bit of hack calling it a string but that hack is currently in master, so Right, agreed it's a bit of hack as only string format supports any value according to the spec. In other words, we can't do "type: number, format: decimal" as it's invalid according to the spec. Ref: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#data-types From your reference: OAS uses several known formats to define in fine detail the data type being used. However, to support documentation needs, the format property is an open string-valued property, and can have any value. And Tools that do not recognize a specific format MAY default back to the type alone, as if the format is not specified. So, according to the spec, any type support arbitrary format. There is no restriction. The list below the text, is only the set of format that should be supported by all implementations. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3910?email_source=notifications&email_token=AAI2ZY3KLVCCLS2VA2JLPQLQKT47NA5CNFSM4IYCDLDKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7HIP5I#issuecomment-533628917>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAI2ZY42ZQ7SPPYJGH2H5DTQKT47NANCNFSM4IYCDLDA> .

-- Keith Siilats [email protected] www.bytelogics.com Fax: 646-352-4705 Phone: +1 (646) 881-4378

wing328 · 2019-09-20T17:03:23Z

So, according to the spec, any type support arbitrary format. There is no restriction. The list below the text, is only the set of format that should be supported by all implementations.

Good point. I re-read it and you're right. My answer is based on the discussion for OpenAPI/Swagger spec 2.0 a few years ago. Don't recall why we used "string" for type instead of number back then.

What is your recommendation? I like using string:decimal and alias of
BigDecimal to Decimal in swift4.

I suggest we stick with this implementation as this is what other generators are using at the moment.

v5.x is a better candidate to revisit the mapping and whether we should add other format to better support "number", "integer", etc

siilats · 2019-09-20T17:08:18Z

Great, I dont have rights to approve the pull req and its my first one to this project, so please advise whats next

…

On Fri, Sep 20, 2019 at 1:03 PM William Cheng ***@***.***> wrote: So, according to the spec, any type support arbitrary format. There is no restriction. The list below the text, is only the set of format that should be supported by all implementations. Good point. I re-read it and you're right. My answer is based on the discussion for OpenAPI/Swagger spec 2.0 a few years ago. Don't recall why we used "string" for type instead of number back then. What is your recommendation? I like using string:decimal and alias of BigDecimal to Decimal in swift4. I suggest we stick with this implementation as this is what other generators are using at the moment. v5.x is a better candidate to revisit the mapping and whether we should add other format to better support "number", "integer", etc — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3910?email_source=notifications&email_token=AAI2ZY6CS2QDU5HURAMTV5DQKT66FA5CNFSM4IYCDLDKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7HJ3XA#issuecomment-533634524>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAI2ZY76EI5TLEWMFNWXFADQKT66FANCNFSM4IYCDLDA> .

-- Keith Siilats [email protected] www.bytelogics.com Fax: 646-352-4705 Phone: +1 (646) 881-4378

wing328 · 2019-09-21T09:22:16Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

@@ -1068,7 +1068,7 @@ public DefaultCodegen() {
        typeMapping.put("file", "File");
        typeMapping.put("UUID", "UUID");
        typeMapping.put("URI", "URI");
-        //typeMapping.put("BigDecimal", "BigDecimal"); //TODO need the mapping?
+        typeMapping.put("BigDecimal", "BigDecimal"); //TODO need the mapping?


@siilats why do you need to uncomment the mapping in the default codegen as it's already done in Swfit4Codegen:

openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/Swift4Codegen.java

Line 203 in 7e58f03

typeMapping.put("BigDecimal", "Decimal");

as part of this PR?

If you remove this, I'm happy to merge the PR and discuss the mapping of BigDecimal in the default codegen if needed.

siilats · 2019-09-23T15:17:15Z

Not sure why it needs to be there, but if its not there, then it fails.

…

On Sat, Sep 21, 2019 at 5:22 AM William Cheng ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java <#3910 (comment)> : > @@ -1068,7 +1068,7 @@ public DefaultCodegen() { typeMapping.put("file", "File"); typeMapping.put("UUID", "UUID"); typeMapping.put("URI", "URI"); - //typeMapping.put("BigDecimal", "BigDecimal"); //TODO need the mapping? + typeMapping.put("BigDecimal", "BigDecimal"); //TODO need the mapping? @siilats <https://github.com/siilats> why do you need to uncomment the mapping in the default codegen as it's already done in Swfit4Codegen: https://github.com/OpenAPITools/openapi-generator/blob/7e58f0325bfdafda2edad1af7603440808fc763a/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/Swift4Codegen.java#L203 ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3910?email_source=notifications&email_token=AAI2ZYYJKMZWUTKIRJR6UFTQKXRU7A5CNFSM4IYCDLDKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCFPU2AI#pullrequestreview-291458305>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAI2ZY2QNHBVE7HLOLFPTBLQKXRU7ANCNFSM4IYCDLDA> .

wing328 · 2019-09-23T16:10:50Z

Not sure why it needs to be there, but if its not there, then it fails.

OK. I'll take another look later this week to see if I can figure out why.

wing328 · 2019-09-25T15:18:37Z

That line is needed for Java generators. I'll add a test case in a separate PR to better test type=string, format=number

* master: (207 commits) Add missing enum processing in C++ codegen, already present for Qt5 (#3986) [C++] [Pistache] Removed deprecated warnings (#3985) [C++][Pistache] Simplified model template (#3417) add go oas3 petstore to ensure up-to-date (#3979) replace gitter with slack in the doc (#3977) Fix wrong variable name in LessThan and LessThanOrEqual asserts (#3971) #3957 - Removed hardcoded baseUrl (#3964) Regenerate go openapi3 samples (#3975) [rust] Make it easier to test rust client generator (#3543) Fix issue3635 (#3948) add gradle repository (#3867) [java] allow to use setArtifactVersion() programmatically (#3907) Add a link to DevRelCon SF 2019 (#3961) Add a link to a medium blog post (#3960) update maven-compiler-plugin version (#3956) fix generateAliasAsModels in default generator (#3951) Implement BigDecimal to Decimal in swift4 for currency data as type=string format=number (#3910) Add F# Functions server generator (#3933) [python-experimental] generate model if type != object if enums/validations exist (#2757) [scala] add [date-time] field to codegen unit test (#3939) ...

…tring format=number (OpenAPITools#3910)

* master: (110 commits) [golang] Regenerate all go samples (#3988) Better tests for string (number) (#3953) Add missing enum processing in C++ codegen, already present for Qt5 (#3986) [C++] [Pistache] Removed deprecated warnings (#3985) [C++][Pistache] Simplified model template (#3417) add go oas3 petstore to ensure up-to-date (#3979) replace gitter with slack in the doc (#3977) Fix wrong variable name in LessThan and LessThanOrEqual asserts (#3971) #3957 - Removed hardcoded baseUrl (#3964) Regenerate go openapi3 samples (#3975) [rust] Make it easier to test rust client generator (#3543) Fix issue3635 (#3948) add gradle repository (#3867) [java] allow to use setArtifactVersion() programmatically (#3907) Add a link to DevRelCon SF 2019 (#3961) Add a link to a medium blog post (#3960) update maven-compiler-plugin version (#3956) fix generateAliasAsModels in default generator (#3951) Implement BigDecimal to Decimal in swift4 for currency data as type=string format=number (#3910) Add F# Functions server generator (#3933) ...

wing328 · 2019-10-04T14:19:38Z

@siilats thanks for the PR, which has been included in the v4.1.3 release: https://twitter.com/oas_generator/status/1180123829626003456

siilats added 2 commits September 18, 2019 14:06

Implement BigDecimal to Decimal in swift4 for currency data as type=s…

455c09b

…tring format=number

Merge branch 'master' of https://github.com/OpenAPITools/openapi-gene…

6734503

…rator

siilats mentioned this pull request Sep 18, 2019

[REQ] "decimal" format is not supported for "number" #2534

Open

Merge branch 'master' of https://github.com/OpenAPITools/openapi-gene…

7e58f03

…rator

wing328 added the Client: Swift label Sep 20, 2019

wing328 added this to the 4.1.3 milestone Sep 20, 2019

wing328 added the Enhancement: Compatibility label Sep 20, 2019

wing328 reviewed Sep 21, 2019

View reviewed changes

wing328 merged commit e20af77 into OpenAPITools:master Sep 25, 2019

wing328 changed the title ~~Decimal in swift4~~ [Swfit4] better support for type=string, format=number Sep 25, 2019

Jesse0Michael pushed a commit to Jesse0Michael/openapi-generator that referenced this pull request Oct 3, 2019

Implement BigDecimal to Decimal in swift4 for currency data as type=s…

6971483

…tring format=number (OpenAPITools#3910)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Swfit4] better support for type=string, format=number #3910

[Swfit4] better support for type=string, format=number #3910

siilats commented Sep 18, 2019

siilats commented Sep 18, 2019

lemoinem commented Sep 19, 2019

siilats commented Sep 19, 2019 via email

wing328 commented Sep 20, 2019

lemoinem commented Sep 20, 2019

siilats commented Sep 20, 2019 via email

wing328 commented Sep 20, 2019

siilats commented Sep 20, 2019 via email

wing328 Sep 21, 2019 •

edited

Loading

wing328 Sep 23, 2019

siilats commented Sep 23, 2019 via email

wing328 commented Sep 23, 2019

wing328 commented Sep 25, 2019

wing328 commented Oct 4, 2019

[Swfit4] better support for type=string, format=number #3910

[Swfit4] better support for type=string, format=number #3910

Conversation

siilats commented Sep 18, 2019

PR checklist

Description of the PR

siilats commented Sep 18, 2019

lemoinem commented Sep 19, 2019

siilats commented Sep 19, 2019 via email

wing328 commented Sep 20, 2019

lemoinem commented Sep 20, 2019

siilats commented Sep 20, 2019 via email

wing328 commented Sep 20, 2019

siilats commented Sep 20, 2019 via email

wing328 Sep 21, 2019 • edited Loading

Choose a reason for hiding this comment

wing328 Sep 23, 2019

Choose a reason for hiding this comment

siilats commented Sep 23, 2019 via email

wing328 commented Sep 23, 2019

wing328 commented Sep 25, 2019

wing328 commented Oct 4, 2019

wing328 Sep 21, 2019 •

edited

Loading