Add support for isUuid #6382

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

Jump to bottom

Merged

wing328 merged 5 commits into swagger-api:master from tzimisce012:uuid-issue

Sep 3, 2017

Merged

Add support for isUuid #6382

wing328 merged 5 commits into swagger-api:master from tzimisce012:uuid-issue

Sep 3, 2017

Conversation

@tzimisce012

Copy link

Contributor

@tzimisce012 tzimisce012 commented Aug 26, 2017 •

edited

Loading

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: master for non-breaking changes and 3.0.0 branch for breaking (non-backward compatible) changes.

Description of the PR

This will fix issue #6381

tzimisce012 added 2 commits

August 26, 2017 16:44


 Fix Uuid parse

3dfea8e


 Merge branch 'master' into uuid-issue

916117b

@tzimisce012

Copy link

Contributor Author

tzimisce012 commented Aug 28, 2017

What do you think @JFCote?

JFCote

JFCote suggested changes

Aug 28, 2017

View reviewed changes

Copy link

Contributor

@JFCote JFCote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments for suggested changes.
By the way, also check my comment in the issue itself. I'm not sure we want to implements all "custom format" available. Will need the stamp from people like @wing328

modules/swagger-codegen/src/main/java/io/swagger/codegen/CodegenParameter.java

public String example; // example value (x-example)

public String jsonSchema;

public boolean isString, isInteger, isLong, isFloat, isDouble, isByteArray, isBinary, isBoolean, isDate, isDateTime;

public boolean isString, isInteger, isLong, isFloat, isDouble, isByteArray, isBinary, isBoolean, isDate, isDateTime, isUuid;

Copy link

Contributor

@JFCote JFCote Aug 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a change in the very generic CodegenParameter, this will need the stamps from other collaborator since it can have effects on other generators.

modules/swagger-codegen/src/main/java/io/swagger/codegen/CodegenProperty.java

public boolean hasMoreNonReadOnly; // for model constructor, true if next properyt is not readonly

public boolean isPrimitiveType, isContainer, isNotContainer;

public boolean isString, isInteger, isLong, isFloat, isDouble, isByteArray, isBinary, isFile, isBoolean, isDate, isDateTime;

public boolean isString, isInteger, isLong, isFloat, isDouble, isByteArray, isBinary, isFile, isBoolean, isDate, isDateTime, isUuid;

Copy link

Contributor

@JFCote JFCote Aug 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here

modules/swagger-codegen/src/main/java/io/swagger/codegen/CodegenResponse.java

public String dataType, baseType, containerType;

public boolean hasHeaders;

public boolean isString, isInteger, isLong, isFloat, isDouble, isByteArray, isBoolean, isDate, isDateTime;

public boolean isString, isInteger, isLong, isFloat, isDouble, isByteArray, isBoolean, isDate, isDateTime, isUuid;

Copy link

Contributor

@JFCote JFCote Aug 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same thing here

modules/swagger-codegen/src/main/java/io/swagger/codegen/DefaultCodegen.java

}

if (p instanceof UUIDProperty) {

property.isString = true;

property.isUuid = true;

Copy link

Contributor

@JFCote JFCote Aug 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems dangerous to me. You should at least keep the isString = true and add your line just after. But, that would be strange because it would be the first case where the "is" field could be true for 2 variables. For example, right now you can't be isString and isDate at the same time.

Copy link

Contributor

@wing328 wing328 Aug 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also suggest keeping isString = true to make the change backward compatible as we don't have the resource to review all the templates due to this change.

modules/swagger-codegen/src/main/resources/JavaPlayFramework/conversionBegin.mustache Outdated

@@ -1 +1 @@

{{#isBoolean}}Boolean.valueOf({{/isBoolean}}{{#isInteger}}Integer.parseInt({{/isInteger}}{{#isDouble}}Double.parseDouble({{/isDouble}}{{#isLong}}Long.parseLong({{/isLong}}{{#isFloat}}Float.parseFloat({{/isFloat}}{{#isDateTime}}OffsetDateTime.parse({{/isDateTime}}

Copy link

Contributor

@JFCote JFCote Aug 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, you need to remove the "isString", it is not needed since it's always a string when the conversionBegin is called. If you need this for you case when isUuid and isString are true at the same time, please put the (String) in the isUUid section only.

modules/swagger-codegen/src/main/resources/JavaPlayFramework/conversionEnd.mustache Outdated

@@ -1 +1 @@

{{#isBoolean}}){{/isBoolean}}{{#isInteger}}){{/isInteger}}{{#isDouble}}){{/isDouble}}{{#isLong}}){{/isLong}}{{#isFloat}}){{/isFloat}}{{#isDateTime}}){{/isDateTime}} No newline at end of file

{{#isBoolean}}){{/isBoolean}}{{#isInteger}}){{/isInteger}}{{#isDouble}}){{/isDouble}}{{#isLong}}){{/isLong}}{{#isFloat}}){{/isFloat}}{{#isString}}{{/isString}}{{#isUuid}}){{/isUuid}}{{#isDateTime}}){{/isDateTime}} No newline at end of file

Copy link

Contributor

@JFCote JFCote Aug 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here, remove the isString section, not needed.

@tzimisce012

Copy link

Contributor Author

tzimisce012 commented Aug 28, 2017

Ok, i'll wait the second opinion. This looks like a deeper change on the code that involves the codegen in general, and not just the javaplayframework codegen.

@JFCote

Copy link

Contributor

JFCote commented Aug 28, 2017

@tzimisce012 Yeah, exactly. It involves all generators.

Daniel Ochoa Rodríguez added 3 commits

September 1, 2017 13:53


 Add isString = true when property is Uuid

0b68d0c

This will not break previous code


 Update conversionBegin.mustache

d5278d8


 Update conversionEnd.mustache

3dfa046

@JFCote

Copy link

Contributor

JFCote commented Sep 1, 2017

@wing328 The latest changes seems good to me. If the fact that we deal with "custom" format like guid is ok with you, then it's a go for the merge.

Thanks @tzimisce012 for the great work.

@wing328 wing328 added Enhancement: General Server: Java labels

Sep 1, 2017

@wing328 wing328 added this to the v2.3.0 milestone

Sep 1, 2017

@wing328 wing328 merged commit cfa2074 into swagger-api:master

Sep 3, 2017

@wing328

Copy link

Contributor

wing328 commented Sep 3, 2017

Thanks @tzimisce012 for the enhancement, which has been merged into master.

cc @cbornet @ePaul

@wing328 wing328 changed the title ~~(削除) Fix Uuid parse (削除ここまで)~~ (追記) Add support for isUuid (追記ここまで)

Sep 3, 2017

@tzimisce012 tzimisce012 deleted the uuid-issue branch

September 4, 2017 00:53

@wing328 wing328 mentioned this pull request

Sep 4, 2017

[Java-Playframework] UUID parameter is not well parsed #6381

Closed

@sparta15 sparta15 mentioned this pull request

Oct 3, 2017

[Java-Playframework] UUID parameter is not found #6609

Closed

Labels

Enhancement: General Server: Java

3 participants

@tzimisce012 @JFCote @wing328

Add support for isUuid #6382

Add support for isUuid #6382

Uh oh!

Conversation

@tzimisce012 tzimisce012 commented Aug 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Description of the PR

Uh oh!

tzimisce012 commented Aug 28, 2017

Uh oh!

@JFCote JFCote left a comment

Choose a reason for hiding this comment

Uh oh!

@JFCote JFCote Aug 28, 2017

Choose a reason for hiding this comment

Uh oh!

@JFCote JFCote Aug 28, 2017

Choose a reason for hiding this comment

Uh oh!

@JFCote JFCote Aug 28, 2017

Choose a reason for hiding this comment

Uh oh!

@JFCote JFCote Aug 28, 2017

Choose a reason for hiding this comment

Uh oh!

@wing328 wing328 Aug 31, 2017

Choose a reason for hiding this comment

Uh oh!

@JFCote JFCote Aug 28, 2017

Choose a reason for hiding this comment

Uh oh!

@JFCote JFCote Aug 28, 2017

Choose a reason for hiding this comment

Uh oh!

tzimisce012 commented Aug 28, 2017

Uh oh!

JFCote commented Aug 28, 2017

Uh oh!

JFCote commented Sep 1, 2017

Uh oh!

wing328 commented Sep 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

@tzimisce012 tzimisce012 commented Aug 26, 2017 •

edited

Loading