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

Hard coding for Indexed type item/wrapper name removes all possiblity of customization #356

Open
perilbrain opened this issue Aug 16, 2019 · 7 comments

Comments

@perilbrain
Copy link

In following function

protected void _startRootArray(ToXmlGenerator xgen, QName rootName) throws IOException
    {
        xgen.writeStartObject();
        // Could repeat root name, but what's the point? How to customize?
        xgen.writeFieldName("item");
    }  

Although rootName has been passed it is not being used. This makes it impossible to change array type wrapper name. If someone needs item as their wrapper they can set @JacksonXmlRootElement(localName="item") but otherwise is not possible.

Alternatively we can have another attribute in annotation like @JacksonXmlRootElement(localName="some_node", wrapperForIndexedType="some_node")
having interface declaration of JacksonXmlRootElement.java as

@Target({ElementType.TYPE})
@Retention(RetentionPolicy.RUNTIME)
public @interface JacksonXmlRootElement
{
    String namespace() default "";
    String localName() default "";
    String wrapperForIndexedType() default "item";
}

Problem:
Trying to generate output like

<ElementList>
  <Element>
    <p1>p1</p1>
    <p2>p2</p2>
  </Element>
  <Element>
    <p1>p1</p1>
    <p2>p2</p2>
  </Element>
</ElementList>

getting:

<ElementList>
  <item>
    <p1>p1</p1>
    <p2>p2</p2>
  </item>
  <item>
    <p1>p1</p1>
    <p2>p2</p2>
  </item>
</ElementList>

However if hard-coding is removed it would be easier to generate json and xml from a same function and entity definition with some varied level of alteration at XML layer. This avoids re-declaring everything to get two different representation of output i.e. json and xml.

For example in my case I have to port an old API which has response format for json:

[ 
  {"p1": "p1", "p2": "p2"},
  {"p1": "p1", "p2": "p2"}
]

for xml:

<ElementList>
  <Element>
    <p1>p1</p1>
    <p2>p2</p2>
  </Element>
  <Element>
    <p1>p1</p1>
    <p2>p2</p2>
  </Element>
</ElementList>

but I cannot achieve the xml format without rewriting all POJOs.

@cowtowncoder
Copy link
Member

I suspect that comment and hard-coding might predate time when root name was being passed (and perhaps assumption was that List types are from JDK... although even those could have annotations via mix-ins), so perhaps limitation may be removed now?

I hope I'll have time in medium-term future to look into this, but if anyone has time to see if a PR was possible, I would make time to review it at least. And timing would be right for inclusion in 2.10.0.pr2 (or final 2.10.0) if so.

@perilbrain
Copy link
Author

perilbrain commented Aug 18, 2019

@cowtowncoder I have submitted a pull request can you please review. Also please have a look on this line where the logic could be faulty else everything else I am confident of being right.

@cowtowncoder
Copy link
Member

Hi @perilbrain! Thank you for contribution -- I did notice it, but haven't had time to look. I hope to get to it some time this week. I'll probably have some questions, mostly regarding if use of existing annotations was not possible.

@perilbrain
Copy link
Author

I'll probably have some questions, mostly regarding if use of existing annotations was not possible.

Use of existing annotation is possible but will break backward compatibility. Anyone consuming will have to update their existing client application.

@cowtowncoder
Copy link
Member

@perilbrain Not really, actually: JDKs handling of annotations is somewhat more forgiving wrt finding unrecognized annotations, properties thereof. There is dependency across components, sure, but new annotations are regular added in Jackson minor releases and these are rarely problematic wrt versioning, compared to other changes.

In this case much depends on annotation being used: if one from this package is used (like is the case in your PR), there should be even fewer issues.

But it is certainly true that additions to annotation definitions are only possible in minor versions (and removal, changes, in major versions if ever).

@cowtowncoder
Copy link
Member

cowtowncoder commented Sep 6, 2019

Ok. I'll have to read this a few more times as I am not sure what to think of it, except for one concern: adding annotation in value type seems wrong to me, somehow. I can see why it is done, just not sure I like it. But maybe thinking it through again makes things more clear for me.

One other thing: testing is also needed for deserialization side, to ensure that works.
And this is why I am bit uneasy on logic that iterates over entries of Collection on serialization: this is not possible when deserializing.

cowtowncoder added a commit that referenced this issue Sep 6, 2019
@cowtowncoder
Copy link
Member

Ok. So, some more thoughts. I added couple of tests to remind myself on root name handling, and ignoring item part, at least:

  1. Root name is used for Collections too
  2. Namespace can be defined both via annotation and root name override (PropertyName does have local name and namespace)

Conceptually, it seems as if root-name info should, then, contain another fully-qualified name.
So for @JacksonXmlRootElement we'd want 2 properties, to allow local name and namespace be defined. That seems doable on its own.

Unfortunately part about root name override is trickier as ObjectWriter.withRootName() comes from jackson-databind, and modules can not easily sub-class ObjectWriter in a way that things would work. But one possibility could be to sub-class PropertyName, to make something like IndexedPropertyName, which would add the secondary name to use for elements.
That could then be cached... although requiring some changes to XmlRootNameLookup.

So I think that might be better way to go.

@cowtowncoder cowtowncoder removed the 2.10 label Apr 29, 2020
@cowtowncoder cowtowncoder removed the 2.12 label Nov 14, 2020
@cowtowncoder cowtowncoder changed the title Hard coding for Indexed type removes all possiblity of customization. Hard coding for Indexed type item/wrapper name removes all possiblity of customization Nov 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants