Skip to content

[JAVA] equals and hashCode for models with byte[] and binary strings#7341

Merged
wing328 merged 3 commits intoswagger-api:2.4.0from
bmordue:fix-3731
Jan 14, 2018
Merged

[JAVA] equals and hashCode for models with byte[] and binary strings#7341
wing328 merged 3 commits intoswagger-api:2.4.0from
bmordue:fix-3731

Conversation

@bmordue
Copy link
Copy Markdown
Contributor

@bmordue bmordue commented Jan 8, 2018

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: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

For models with string fields in "byte" or "binary" format, the generated equals and hashCode methods are not correct. This change modifies the template to use Array.equals and Arrays.hash for byte array and binary variables.

Fixes #3731
This PR replaces/supersedes #6760

cc @bbdouglas @JFCote @sreeshas @jfiala @lukoyanov @cbornet
cc @brentryan (reporter of #3731)

}{{#hasVars}}
{{classname}} {{classVarName}} = ({{classname}}) o;
return {{#vars}}Objects.equals(this.{{name}}, {{classVarName}}.{{name}}){{#hasMore}} &&
return {{#vars}}{{#isByteArray}}Arrays{{/isByteArray}}{{#isBinary}}Arrays{{/isBinary}}{{^isByteArray}}{{^isBinary}}Objects{{/isBinary}}{{/isByteArray}}.equals(this.{{name}}, {{classVarName}}.{{name}}){{#hasMore}} &&
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.

@bmordue One suggestion: What about using {{#isBinary}}java.util.Arrays{{/isBinary}} to avoid unused import when no property is defined as byte array or binary?

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.

Maybe I've misunderstood, but I thought isBinary and isByteArray apply to a single CodegenProperty and would only be relevant inside models.model.vars in the template?

Copy link
Copy Markdown
Contributor

@wing328 wing328 Jan 8, 2018

Choose a reason for hiding this comment

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

My point is in https://github.com/swagger-api/swagger-codegen/pull/7341/files#diff-cc3e8eb9a2b26248932102a87608fc60R7, you're hard-coding "import java.util.Arrays;".

My suggestion using fully-qualified name is to avoid hard-coding the import

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.

@cbornet what's your take on this?

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.

Yes there's a risk of having an unused import. But note that it's the case in a lot of other places of our templates.
Managing imports is really something not easy with code generators.

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.

Yup, we'll try to take care of the unused import in the future so this PR is good to merge.

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