Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update DefaultCodeGen to allow additional primitive types #2799

Merged
merged 2 commits into from Jul 3, 2019

Conversation

Projects
None yet
4 participants
@steco
Copy link
Contributor

commented May 2, 2019

If a string field is specified with a format which is also defined using --typeMappings, it will be treated as a primitive type

For example, if you run the following command:

java -jar openapi-generator-cli.jar generate [options] --type-mappings dateorcutlabel=CutLabel

The generated code will assume there is a class CutLabel and use it for any parameter which has a specific similiar to this:

{
"name": "effectiveAt",
"in": "query",
"description": "Optional. The effective date of the data",
"required": false,
"type": "string",
"format": "dateorcutlabel"
}

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, ./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\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
  • 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)

@wing328

This comment has been minimized.

Copy link
Member

commented May 5, 2019

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

@steco

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

Fixed - thanks William

@@ -1452,6 +1452,10 @@ private static String getPrimitiveType(Schema schema) {
} else if (ModelUtils.isUUIDSchema(schema)) {
return "UUID";
} else if (ModelUtils.isStringSchema(schema)) {
if(typeMapping.containsKey(schema.getFormat()))
{

This comment has been minimized.

Copy link
@wing328

wing328 May 20, 2019

Member

Can this open curly bracket go to line 1455 instead to conform to Java code style?

Please also add a comment (1-liner) explaining this check or the use case here.

This comment has been minimized.

Copy link
@steco

steco May 20, 2019

Author Contributor

Fixed and updated

@@ -1,10 +1,7 @@
set executable=.\modules\openapi-generator-cli\target\openapi-generator-cli.jar

If Not Exist %executable% (
mvn clean package
)

This comment has been minimized.

Copy link
@wing328

wing328 May 20, 2019

Member

Why remove these? Are these lines not working as expected (to build the JAR if not present)?

This comment has been minimized.

Copy link
@steco

steco May 20, 2019

Author Contributor

These lines don't work on my Windows machine (I need to run mvnw and it is not on my path anyway) so I removed them, reinstated for people who have a more complete setup.

This comment has been minimized.

Copy link
@wing328

wing328 May 20, 2019

Member

Very good point. May be we should use mvnw instead of mvn to make it easier for devleopers using Windows machines to build the project.

@wing328

This comment has been minimized.

Copy link
Member

commented May 20, 2019

cc @openapitools-bot as the change covers default codegen

// then treat the format as a primitive type.
// This allows the typeMapping flag to add a new custom type which can then
// be used in the format field.
return schema.getFormat();

This comment has been minimized.

Copy link
@wing328

wing328 May 20, 2019

Member

@steco Previously there's a special case handled in the code above: https://github.com/OpenAPITools/openapi-generator/pull/2799/files#diff-7cb46fa53f89a458a7b7cb201d2214a8R1417 (type: string, format: number).

If we just return format, would it be an issue if schema.getFormat() happens to be one of the existing OAS type (e.g. number)?

Should we return something like string_number instead to make it distinguishable with the existing types? Would that work in your case?

This comment has been minimized.

Copy link
@steco

steco May 22, 2019

Author Contributor

I don't see the special case for type: string, format: number. I see the following specific formats for strings - "date", "date-time", "password", "byte", "binary", "uuid", email" (in SchemaTypeUtil).

If someone has a swagger.json file which contains fields which formats which are also primitive types, ie which appear in typeMapping (eg with type:string, format:number) then this change will mean that the field would change from being a string to being a double in the generated code. However, I think this would be unlikely.

Using string_number would work for me, but I think this adds unnecessary complexity and makes a custom field different to the existing special cases (listed above).

This comment has been minimized.

Copy link
@jimschubert

jimschubert Jun 8, 2019

Member

format could be anything. I don't think we should try to cover edge cases, as I think that would over-complicate things.

This comment has been minimized.

Copy link
@sigpwned

sigpwned Jun 16, 2019

I realize I'm not a contributor or member here, so I may not fully grasp everything in play here. However, I am a user who also needs this feature, and I implemented a very similar patch independently, so here's my opinion, for whatever it's worth.

@wing328's concern above appears to be that the edited code no longer handles a special case: string / number. However, that case still appears to be handled on (what is currently) line 1417:

// Line 1417
} else if (ModelUtils.isStringSchema(schema) && "number".equals(schema.getFormat())) {
    // special handle of type: string, format: number
    return "BigDecimal";
} else if (ModelUtils.isByteArraySchema(schema)) {

Because that code is still there, it looks (to me, at least) like we can be confident it still handles the string/number case properly. Even better, that code didn't change and all edits to the function come after that test, which increases our confidence further.

I ran just this code change through the build and all tests pass, which is further evidence that this particular change does The Right Thing. (Note: In retrospect, I only ran surefire tests.)

With this change in, I have also confirmed that I can use type mappings to generate references to custom types instead of String for specific string/format combinations only.

@wing328 wing328 modified the milestones: 4.0.1, 4.0.2 May 31, 2019

@sigpwned

This comment has been minimized.

Copy link

commented Jun 15, 2019

I have a similar requirement and independently generated what is (essentially) the same PR as above. It worked for my Java application as well. I would love to see this get merged and released soon! My team will likely have to maintain a patched version until then.

Thank you @steco for issuing the PR, and to the broader OpenAPI team for making such a great toolchain around OpenAPI!

@wing328 wing328 modified the milestones: 4.0.2, 4.0.3 Jun 20, 2019

@wing328

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

@steco as discussed over Gitter, the edge case rarely happens and if it does, we will instead use "string_{format}" as a bug fix (non-breaking change) to handle the edge case correctly.

steco added some commits May 2, 2019

Update DefaultCodeGen to allow additional primitive types
If a string field is specified with a format which is also defined using
--typeMappings, it will be treated as a primitive type

@steco steco force-pushed the finbourne:fbn/string-format branch from 75f795f to 38580fc Jul 2, 2019

@wing328

wing328 approved these changes Jul 3, 2019

Copy link
Member

left a comment

👍

@wing328 wing328 merged commit b44f6c3 into OpenAPITools:master Jul 3, 2019

4 checks passed

Shippable Run 8891 status is SUCCESS.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@steco steco deleted the finbourne:fbn/string-format branch Jul 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.