Naive attempt at fixing #306 #754

duoduobingbing · 2025-04-27T15:27:17Z

This PR aims to fix #306 naively by just changing the default property name of XmlText from "" to <xml:text>. The new value was chosen, because it is an invalid property and element name in XML and thus should hopefully not collide with existing setups.

I was not sure if XmlText having the property name "" is considered an implementation detail. If it is not an implementation detail this is most likely to be considered as a breaking change.

Feel free to close this PR if e.g. something else is planned to fix #306 or if XmlText has to keep the property name empty string because of compatibility reasons.

cowtowncoder · 2025-04-28T00:54:30Z

Choice of "" to represent XmlText is not a simple implementation detail so I doubt we can change that for Jackson 2.x, unfortunately due to (like you correctly guessed) compatibility concerns.

But I think we might be able to do just that for 3.x, being major version change.

I haven't looked into PR yet (just description) to comment on changes, but wanted to point out that change as described likely can't be made in 2.x branch.

src/main/java/com/fasterxml/jackson/dataformat/xml/deser/XmlTextPropertyNameHolder.java

cowtowncoder · 2025-04-28T01:27:14Z

Ok so I do like the idea -- just need to re-create for 3.0.0 (branch 3.x).

…text>`

duoduobingbing · 2025-04-28T19:04:58Z

Ok so I do like the idea -- just need to re-create for 3.0.0 (branch 3.x).

Thanks for looking into this so swiftly.

I did rebase onto 3.x and just used FromXmlParser.DEFAULT_TEXT_PROPERTY and removed the newly introduced XmlTextPropertyNameHolder.PROPERTY_NAME completely.

cowtowncoder · 2025-05-01T16:49:15Z

src/main/java/tools/jackson/dataformat/xml/JacksonXmlAnnotationIntrospector.java

                super.findNameForDeserialization(config, a));
        if (pn == null) {
+            if(_hasAnnotation(a, JacksonXmlText.class)){
+                return PropertyName.construct(FromXmlParser.DEFAULT_TEXT_PROPERTY);


Quick notes: not just _hasAnnotation -- JacksonXmlText.value() must be true (specifying false can be used to disable annotation's effect).

But I have related concern: there now won't be a way to override name of logical property to use as it's effectively hard-coded. Should there be a way to re-configure that? And if so, how?
(MapperConfig passed won't have access to XML-specific config mapper has. But maybe this introspector should have its own config or something.

For the first note: This has been fixed in the latest commit of this pull request

For the second note: I've added a config on the introspector so that this can be configured.

src/main/java/tools/jackson/dataformat/xml/deser/FromXmlParser.java

cowtowncoder · 2025-05-13T02:20:35Z

Ok. I am not quite sure what the added configurability does, to be honest. I know I asked for it, but the combination of name and "infer" flag is bit unclear to me.

I realized that FromXmlParser already allows specifying different underlying name to use like so

FromXmlFactory f = FromXmlFactory.builder()
    .nameForTextElement("text-marker-element")
    .build();

So, f.ex input like:

<root attr="123">456</root>

would in 3.0 be exposed as:

{
  "root": {
    "attr": "123",
    "<xml:text>": "456"
  }
}

with default settings.

Given this, I wonder if annotation-level configurability is really needed? If it is, how does it control things?

cowtowncoder · 2025-05-13T02:38:02Z

src/main/java/tools/jackson/dataformat/xml/deser/XmlDeserializationContext.java

            JsonToken t = p.nextToken();
            if (t == JsonToken.VALUE_STRING) {
-                if (propName.equals("")) {
+                if (FromXmlParser.DEFAULT_TEXT_PROPERTY.equals(propName)) {


I think this needs to actually consider FromXmlParser._cfgNameForTextElement, since that may be configured different from default. Comment above hints at that.

Will change to do that.

cowtowncoder · 2025-05-13T02:43:41Z

src/main/java/tools/jackson/dataformat/xml/JacksonXmlAnnotationIntrospector.java

+                return _cfgIntrospectorConfig.xmlTextPropertyName();
+            }
+
            if (_hasOneOf(a, ANNOTATIONS_TO_INFER_XML_PROP)) {


This will look for @JacksonXmlText OR @JacksonXmlElementWrapper and handling will now differ -- former should use configured "text element name", latter should not (should return "use default").

I do not quite understand, the highlighted line only looks for @JacksonXmlText

Sorry, I meant line 225 right after (if (_hasOneOf(...)).

cowtowncoder · 2025-05-13T03:14:34Z

src/test/java/tools/jackson/dataformat/xml/node/JsonNodeBasicDeserTest.java

        JsonNode fromXml = XML_MAPPER.readTree("<root>first<a>123</a>second<b>abc</b>last</root>");
        final ObjectNode exp = XML_MAPPER.createObjectNode();
-        exp.putArray("")
+        exp.putArray(FromXmlParser.DEFAULT_TEXT_PROPERTY)


Ohhhhhhh. This is NOT good -- what used to be something like:

{ "":["first","second","last"], "a":"123", "b":"abc" }

now looks like:

{ "<xml:text>":["first","second","last"], "a":"123", "b":"abc" }

which I don't think is what anyone likes to see :-(

So for JsonNode at least exposing XML character data sections should be with nominal key of "".
And I don't think it is reasonable to expected those using XmlMapper.readTree() to have explicitly configure things to work this way.

We need to figure out another way to handle the issue, I think.

We need to figure out another way to handle the issue, I think.

I mean this PR was just a naive shot at solving #306 by changing the property name, if it has to be "" I'm fine with closing this PR - because I do not know how to make it so, that the property name differs from the nominal key.

Right. I'll leave this open because I'd really like to figure out a way and feel there probably is a way (despite not seeing it yet). :)

I appreciate your attempt; sorry it took me a while to sync up to what changes really mean.

No Problem, thanks for having a look at it. I will close this for now. Feel free to reopen or use the code in any way, shape or form you see fit.

Ok. Thank you for your help here @duoduobingbing .

cowtowncoder · 2025-05-13T03:16:57Z

src/test/java/tools/jackson/dataformat/xml/node/JsonNodeMixedContent403Test.java

    {
        // First, before elements:
-        assertEquals(JSON_MAPPER.readTree(a2q("{'':'before','a':'1','b':'2'}")),
+        assertEquals(JSON_MAPPER.readTree(a2q(String.format("{'%s':'before','a':'1','b':'2'}", FromXmlParser.DEFAULT_TEXT_PROPERTY))),


and same here obviously.

cowtowncoder reviewed Apr 28, 2025

View reviewed changes

src/main/java/com/fasterxml/jackson/dataformat/xml/deser/XmlTextPropertyNameHolder.java Outdated Show resolved Hide resolved

cowtowncoder reviewed Apr 28, 2025

View reviewed changes

src/main/java/com/fasterxml/jackson/dataformat/xml/deser/XmlTextPropertyNameHolder.java Outdated Show resolved Hide resolved

duoduobingbing changed the base branch from 2.x to 3.x April 28, 2025 17:46

change default property name for xml text from empty string to `<xml:…

c53bc27

…text>`

duoduobingbing force-pushed the feature/record-with-jacksonxmltext branch from 24b57f1 to c53bc27 Compare April 28, 2025 19:00

duoduobingbing requested a review from cowtowncoder April 30, 2025 10:15

Merge branch '3.x' into feature/record-with-jacksonxmltext

8989990

cowtowncoder reviewed May 1, 2025

View reviewed changes

src/main/java/tools/jackson/dataformat/xml/deser/FromXmlParser.java Outdated Show resolved Hide resolved

address feedback

8d2e14c

duoduobingbing requested a review from cowtowncoder May 6, 2025 13:02

cowtowncoder added the cla-received PR already covered by CLA (optional label) label May 10, 2025

Merge branch '3.x' into feature/record-with-jacksonxmltext

c41f2dd

cowtowncoder reviewed May 13, 2025

View reviewed changes

src/main/java/tools/jackson/dataformat/xml/deser/FromXmlParser.java Show resolved Hide resolved

cowtowncoder added 5 commits May 12, 2025 18:29

Minor rephrasing of Javadocs

6ed7c4b

ws clean up

ff896b6

Merge branch '3.x' into feature/record-with-jacksonxmltext

5c68f0a

Merge branch '3.x' into feature/record-with-jacksonxmltext

3146147

Merge branch '3.x' into feature/record-with-jacksonxmltext

2e8e2b5

cowtowncoder reviewed May 13, 2025

View reviewed changes

Merge branch '3.x' into feature/record-with-jacksonxmltext

feaf2e8

cowtowncoder reviewed May 13, 2025

View reviewed changes

duoduobingbing closed this May 14, 2025

duoduobingbing deleted the feature/record-with-jacksonxmltext branch May 14, 2025 06:50

duoduobingbing restored the feature/record-with-jacksonxmltext branch May 14, 2025 06:51

Uh oh!

Naive attempt at fixing #306 #754

Naive attempt at fixing #306 #754

Uh oh!

Conversation

duoduobingbing commented Apr 27, 2025

Uh oh!

cowtowncoder commented Apr 28, 2025

Uh oh!

Uh oh!

Uh oh!

cowtowncoder commented Apr 28, 2025

Uh oh!

duoduobingbing commented Apr 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cowtowncoder commented May 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cowtowncoder May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duoduobingbing May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cowtowncoder May 13, 2025 •

edited

Loading

duoduobingbing May 13, 2025 •

edited

Loading