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

Configuration option to disable HTML escaping when using Gson #298

Merged
merged 2 commits into from
Jun 14, 2018
Merged

Configuration option to disable HTML escaping when using Gson #298

merged 2 commits into from
Jun 14, 2018

Conversation

stianlik
Copy link
Contributor

@stianlik stianlik commented Jun 13, 2018

Already merged into swagger-codegen, see 7966.

The default implementation of Gson will escape certain characters by default. This
includes the `=` character, which is used in base64 encoding and cause problems when
deserializing the value to a base64 encoded string in a service.

Adding an option for disabling this feature makes it easier to generate client code
with sane defaults.
@stianlik
Copy link
Contributor Author

PR to swagger-codegen was approved by @jeff9finger

@wing328 wing328 added this to the 3.0.2 milestone Jun 13, 2018
@wing328
Copy link
Member

wing328 commented Jun 13, 2018

cc @bbdouglas (2017/07) @JFCote (2017/08) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01) for review.

Copy link
Member

@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

@@ -64,7 +64,8 @@ public Class getClassForElement(JsonElement readElement) {
}
})
;
return fireBuilder.createGsonBuilder();
GsonBuilder builder = fireBuilder.createGsonBuilder();
Copy link
Member

Choose a reason for hiding this comment

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

did you forget to put the {{#disableHtmlEscaping}} ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I did not forget, I chose to always declare the variable and conditionally set disableHtmlEscaping:

{{#disableHtmlEscaping}}
builder.disableHtmlEscaping();
{{/disableHtmlEscaping}}

This keeps the complexity of the mustache code low, and makes it easy to accommodate more options in the future. For example:

{{#disableHtmlEscaping}}
builder.disableHtmlEscaping();
{{/disableHtmlEscaping}}
{{#someNewOption}}
builder.someNewOption();
{{/someNewOption}}

I can modify the template to handle the "no options" situation to avoid declaring the variable in generated Java-classes, if that is preferable?

Copy link
Member

Choose a reason for hiding this comment

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

It's my bad, since the path to the file was truncated by github UI, I did not realize it was a generated sample. Makes total sense! Keep it that way :)

@@ -64,7 +64,8 @@ public Class getClassForElement(JsonElement readElement) {
}
})
;
return fireBuilder.createGsonBuilder();
GsonBuilder builder = fireBuilder.createGsonBuilder();
Copy link
Member

Choose a reason for hiding this comment

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

same question here

Copy link
Member

Choose a reason for hiding this comment

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

all good

@@ -64,7 +64,8 @@ public Class getClassForElement(JsonElement readElement) {
}
})
;
return fireBuilder.createGsonBuilder();
GsonBuilder builder = fireBuilder.createGsonBuilder();
Copy link
Member

Choose a reason for hiding this comment

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

same question here

Copy link
Member

Choose a reason for hiding this comment

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

all good

Copy link
Member

@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.

LGTM, thanks for the PR!

Copy link
Member

@wing328 wing328 left a comment

Choose a reason for hiding this comment

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

Tested locally and the result is good 👍

@wing328 wing328 merged commit 680a2bc into OpenAPITools:master Jun 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants