Skip to content

Make a copy of vendorExtensions map instead of copying the reference#4093

Merged
wing328 merged 3 commits intoswagger-api:masterfrom
RaphC:master
Nov 18, 2016
Merged

Make a copy of vendorExtensions map instead of copying the reference#4093
wing328 merged 3 commits intoswagger-api:masterfrom
RaphC:master

Conversation

@RaphC
Copy link
Copy Markdown
Contributor

@RaphC RaphC commented Oct 27, 2016

PR checklist

  • Read the contribution guildelines.
  • Ran the shell/batch 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)
  • Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

In the method copy of the class CodegenParameter, we make a copy of vendorExtensions map instead of copying the reference to prevent unwanted instance modification of a copied instance.

}
output.vendorExtensions = this.vendorExtensions;
if(this.vendorExtensions != null){
output.vendorExtensions = new HashMap<String, Object>(this.vendorExtensions);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please use 4-space instead of tab.

Also what about vendorExtensions in other objects (e.g. CodegenProperty) ?

Copy link
Copy Markdown
Contributor Author

@RaphC RaphC Nov 4, 2016

Choose a reason for hiding this comment

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

4-space instead of tab done.

CodegenProperty implements Cloneable interface and use default cloning behavior. So members which are not primitive are copied by reference. We should rewrite the clone method to be prevent the copy by reference.

neither CodegenOperation nor CodegenModel doesn't support cloning feature and has no copy method.... but there's an InlineModelResolver which provides a copyVendorExtensions method.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@RaphC have you committed/pushed the change? I only see 1 commit in this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@wing328 Not yet. I'm waiting for your feedback about CodegenProperty

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should rewrite the clone method to be prevent the copy by reference.

Yes, please help contribute the enhancement.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right. I'll make a PR ASAP.

@wing328 wing328 added this to the v2.2.2 milestone Nov 1, 2016
@wing328 wing328 merged commit a260636 into swagger-api:master Nov 18, 2016
@wing328 wing328 changed the title #3908 make a copy of vendorExtensions map instead of copying the reference Make a copy of vendorExtensions map instead of copying the reference Feb 22, 2017
davidgri pushed a commit to davidgri/swagger-codegen that referenced this pull request May 11, 2017
…ng the reference (swagger-api#4093)

* swagger-api#3908 make a copy of vendorExtensions map instead of copying the
reference

* using 4-space instead of tab.

* make a copy of vendorExtensions map instead of copying the reference
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.

2 participants