Fix XML annotations on model properties (JavaSpring)

[chschu]

This PR fixes the annotations generated by the JavaSpring generator when using withXml: true. It is related to #2417, #5078 and #5764.

• If the property is not a container:
  • If the property is configured as an XML attribute, the generator will produce @XmlAttribute and @JacksonXmlProperty using the property's XML configuration.
  • If the property is not configured as an XML attribute, the generator will produce @XmlElement and @JacksonXmlProperty using the property's XML configuration.
• If the property is a container:
  • The generator will produce @XmlElement and @JacksonXmlProperty using the container items' XML configuration.
  • If the property has wrapped: true in its XML configuration, the generator will additionally produce @XmlElementWrapper and @JacksonXmlElementWrapper using the container's XML configuration.
  • If the property doesn't have wrapped: true in its XML configuration, the generator will additionally produce @JacksonXmlElementWrapper(useWrapping = false). IIRC, different Jackson versions behave differently if the annotation is not present

PR checklist

• Read the contribution guidelines.
• Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
• Run the following to build the project and update samples:

  ./mvnw clean package 
  ./bin/generate-samples.sh ./bin/configs/*.yaml
  ./bin/utils/export_docs_generators.sh
  

  (For Windows users, please run the script in Git BASH)
  Commit all changed files.
  This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
  These must match the expectations made by your contribution.
  You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
  IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
• File the PR against the correct branch: master (upcoming 7.1.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
• If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

[Philzen]

Shouldn't the changes be done to modules/openapi-generator/src/main/resources/JavaSpring/xmlAnnotation.mustache?

Looking at that file, i believe it would also be cleaner to include it as well at line 217 ... or even better, remove it from there completely – in my tests with Jackson it was totally sufficient to have the annotation on the field rather than the getter.

[chschu]

@Philzen: Thanks for having a look.

The xmlAnnotation.mustache is responsible for the class-level annotations, so it cannot be used for the annotations on the getter.

Having the annotations on either the field or the getter doesn't make much of a difference in my experience as well. I had some issues with conflicting annotations on getters and fields in the past, so they should probably be all in one place.

As the @JacksonXmlProperty annotation has recently been moved to the getter, I just added the missing ones there.

[Philzen]

The xmlAnnotation.mustache is responsible for the class-level annotations, so it cannot be used for the annotations on the getter.

Of course, you're right, i missed that bit.

Having the annotations on either the field or the getter doesn't make much of a difference in my experience.

I know of at least one edge case that can feel like kind of a footshot: when you have a public variable that has a different name than its getter (e.g. boolean _public with boolean getPublic() getter), then you'll end up with Jackson producing both public and _public on serialization. However i understand that's a completely different discussion given that all of the annotations (not only XML related ones) were basically moved away from fields to getters somewhere between version 4 and 7, so i'm fine with keeping it at getter for the moment.

As the @JacksonXmlProperty annotation has recently been moved to the getter, I just added the missing ones there.

Thanks looking into the file history and providing this commit as context! However the link does not work this way in markdown, leaving the working one here for reference: 69a4a65

[Philzen]

@chschu Happy to report i was able to whip up a basic test: Philzen@df55a9d

I would like to add some refactoring, then also fix the default Java generators and finally add some more tests that also cover the JAXB-related annotation aspects before assembling everything into a joint PR. Kindly allow me some more time to finalize that.

If you have any suggestions which aspects should be covered as part of the test as well i'd be more than happy to hear them in the meanwhile.

[Philzen]

Note to self: Also add dedicated tests whether xml.name from $ref'd schemas is correctly applied (when it does, we have solved #5989 as well).

[Philzen]

Looks like this PR could also close #9371

[chschu]

@Philzen Nice, thank you for the test! I'm really looking forward to the joint PR.

I think the test could also check if the getTags methods has the following annotations (hope I got them right):

@JacksonXmlProperty(localName = "Tag")
@JacksonXmlElementWrapper(localName = "tag")

The property has an explicit XML wrapper name of "tag" (line 91 in the yaml), and an an XML element name of "Tag" (line 60 in the yaml). The wrapper could also be changed to something like "TagList" to distinguish it from the item.

Maybe Pet.name could use xml.attribute: true to cover the attribute branch in the template. This should generate the following annotation on getName:

@JacksonXmlProperty(isAttribute = true, localName = "name")

Are the JAXB annotations (@XmlAttribute, @XmlElement and @XmlElementWrapper) already checked elsewhere, or should the test also cover them?

[Philzen]

@chschu Thanks for your valuable feedback!

I think the test could also check if the getTags methods has the following annotations (hope I got them right):

@JacksonXmlProperty(localName = "Tag")
@JacksonXmlElementWrapper(localName = "tag")

The property has an explicit XML wrapper name of "tag" (line 91 in the yaml), and an an XML element name of "Tag" (line 60 in the yaml). The wrapper could also be changed to something like "TagList" to distinguish it from the item.

Done: Philzen@837a0a7

Maybe Pet.name could use xml.attribute: true to cover the attribute branch in the template. This should generate the following annotation on getName:

@JacksonXmlProperty(isAttribute = true, localName = "name")

Done: Philzen@2db2b7b

Are the JAXB annotations (@XmlAttribute, @XmlElement and @XmlElementWrapper) already checked elsewhere, or should the test also cover them?

Done: Philzen@a0afca5

If OK with you, i shall open a new PR when as soon as i'm happy with the branch, as i cannot push to your repo. Before that, i still have a couple of things to review:

• Go through all use cases listed at https://swagger.io/docs/specification/data-models/representing-xml/ and ensure the test spec covers each of them, namespaces in particular
• Refactor mustache template so that Jackson and JAXB annotations are grouped next to each other (currently mixed)
• Nice-to-have (but maybe too tedious to implement):
  • Skip rendering of @JacksonXmlElement and @XmlElement when the field name is equal to localName/name
• Also explicitly render useWrapping = true in @JacksonXmlElementWrapper (inspired by @jzrebiec via always use @JacksonXmlElementWrapper and read xml data from reference… #5371)
  • will guarantee compatibility with Jackson <2.1
  • ensures correct Jackson deserialization in cases where XmlWrapper is configured with useXmlWrapper(false)
• Check if [BUG] [Java] [withXml] xml name not used for items wrapped in an array #9371 could also easily be fixed here, i was able to reproduce it by using the TagList in our test spec
• Finally, copy the tests over non-spring-specific Java Generator to ensure the mustache templates in src/main/resources/Java also behave correctly, fixing them if necessary

[chschu]

@Philzen That's more than OK with me. Thank you for the effort you put into fixing this!

Once your PR is ready, I'll close this one.

[Philzen]

Closing this in favor of #18870 (which includes your commit)