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.

Sign up for GitHub

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

Wrong generation of @JacksonXmlElementWrapper annotation #10333

Merged
merged 7 commits into from
Aug 31, 2020
Merged

Wrong generation of @JacksonXmlElementWrapper annotation #10333

merged 7 commits into from
Aug 31, 2020

Conversation

sanjeevgiri
Copy link
Contributor

@sanjeevgiri sanjeevgiri commented Jun 19, 2020 •
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\. NOTE: Are these still applicable. I did not see any file name java-petstore.sh.
  • 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. NOTE: Did not see any members specified for java.

Description of the PR

Language: Java
Base version: 2.4.15-SNAPSHOT

Problem summary:
The generated code uses incorrect values for @JacksonXmlElementWrapper and skips inclusion of @JacksonElement for collection/array type properties.

Fixes #9845

sanjeevgiri
sanjeevgiri commented Jun 19, 2020
View reviewed changes
modules/swagger-codegen/src/main/resources/Java/pojo.mustache
@@ -33,7 +33,8 @@ public class {{classname}} {{#parent}}extends {{{parent}}} {{/parent}}{{#parcela
{{#isContainer}}
{{#isXmlWrapped}}
// items.xmlName={{items.xmlName}}
@JacksonXmlElementWrapper(useWrapping = {{isXmlWrapped}}, {{#xmlNamespace}}namespace="{{xmlNamespace}}", {{/xmlNamespace}}localName = "{{#items.xmlName}}{{items.xmlName}}{{/items.xmlName}}{{^items.xmlName}}{{items.baseName}}{{/items.xmlName}}")
@JacksonXmlElementWrapper(useWrapping = {{isXmlWrapped}}, {{#xmlNamespace}}namespace="{{xmlNamespace}}", {{/xmlNamespace}}localName = "{{#xmlName}}{{xmlName}}{{/xmlName}}{{^xmlName}}{{baseName}}{{/xmlName}}")
Copy link
Contributor Author

@sanjeevgiri sanjeevgiri Jun 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

JacksonXmlElementWrapper should use the swaggerArrayProperty.xml.name as its local name instead of swaggerArrayProperty.items.xml.name

In order to customize the name for the wrapped xml element, JacksonXmlProperty annotation also must be included.

@HugoMario
Copy link
Contributor

hey @sanjeevgiri let me know when this is ready and i'll help you with merge

@sanjeevgiri
Copy link
Contributor Author

sanjeevgiri commented Jun 22, 2020 •
edited
Loading

Hi @HugoMario what are the steps needed to get this merged? Its a pretty minor template change and I have been able to see expected results for me. I have not been able to get feedback from the author of this issue so far though. I guess I can wait for her feedback?

@sanjeevgiri
Copy link
Contributor Author

@gracekarina @mobreza
The changes made in this PR introduces changes to the lines of codes added/modified by you. This is a fix for #9845 . Would you be able to chime in and perhaps approve the changes in this PR by any chance. I am also not sure what the process is for getting these merged to the master. Do I need to tag a maintainer for this merge?

@HugoMario HugoMario self-assigned this Aug 14, 2020
@HugoMario
Copy link
Contributor

hey @sanjeevgiri , sorry for delay, would you please update a sample to see how these change affect output code?

you could update samples running spring-*.sh scripts found on bin folder.

@sanjeevgiri
Copy link
Contributor Author

hey @sanjeevgiri , sorry for delay, would you please update a sample to see how these change affect output code?

you could update samples running spring-*.sh scripts found on bin folder.

Hi Hugo, thanks and sorry for late response myself. I will try to get to this by end of this week or hopefully early next week. I will get in touch once I get a chance to look back into this :).

sanjeevgiri
sanjeevgiri commented Aug 26, 2020
View reviewed changes
...les/client/petstore/java/resttemplate-withXml/src/main/java/io/swagger/client/model/Pet.java
@@ -52,7 +52,8 @@

@JsonProperty("photoUrls")
// items.xmlName=
@JacksonXmlElementWrapper(useWrapping = true, localName = "photoUrls")
@JacksonXmlElementWrapper(useWrapping = true, localName = "photoUrl")
@JacksonXmlProperty(localName = "photoUrls")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

...
  <photoUrls>
    <photoUrl>
      ...
    </photoUrl>
     <photoUrl>
        ...
     </photoUrl>
  <photoUrls>

@sanjeevgiri
Copy link
Contributor Author

sanjeevgiri commented Aug 26, 2020 •
edited
Loading

@HugoMario
I executed ./bin/java-petstore-resttemplate-withxml.sh to highlight the differences in xml annotations generation for objects with list properties. This really applies to all java pojos that require xml annotations to be generated.

@HugoMario
Copy link
Contributor

please build

@sanjeevgiri
Copy link
Contributor Author

sanjeevgiri commented Aug 30, 2020 •
edited
Loading

please build

I ran mvn clean compile

image

Did you want me to run something more specific. Sorry if I missed something that is routine. I am a first time contributer to this specific project. Let me know if I missed something and I will include those steps too.

I ran mvn clean test too. And all tests passed also.

@HugoMario
Copy link
Contributor

@sanjeevgiri , oh sorry for confuse. "please build" it's a comment tthat i made to trigger CI tools. it seems there is an issue with appveyor. Need to fix that in order to merge your request

@sanjeevgiri
Copy link
Contributor Author

Is there something I need to fix the tests?

@HugoMario
Copy link
Contributor

no really your PR looks good to me, this is a configuration issue. one of the CI tool is not being triggered, i already request for help about this. let's hope for tomorrow this is solved and your PR merged.

@sanjeevgiri
Copy link
Contributor Author

Thanks a lot Hugo. I appreciate your time and help regarding this PR.

@HugoMario HugoMario merged commit 99a4e2e into swagger-api:master Aug 31, 2020
@volvicoasis
Copy link

volvicoasis commented Sep 10, 2020 •
edited
Loading

@sanjeevgiri do you think the pr is on branch 3.0.0 too ? Master is only for 2 ? I use 3.0.22-SNAPSHOT swagger codegen and dont see improvement ...

@sanjeevgiri
Copy link
Contributor Author

@volvicoasis This fix was for 2.0 specific. I was using swagger 2.0. I believe I had briefly looked at 3.0 branch and noticed that the templates were being imported from elsewhere.

@volvicoasis
Copy link

@sanjeevgiri do you know an easy way to solve the xml wrapped = false in 3.0 ?

This was referenced Mar 11, 2021
Open
Closed
Open
Open
This was referenced Mar 11, 2021
Open
Merged
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Merged
Open
Open
Open
Open
Open
Closed
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@HugoMario HugoMario

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[JavaSpring] Wrong generation of @JacksonXmlElementWrapper annotation
3 participants
@sanjeevgiri @HugoMario @volvicoasis