Skip to content

Naive attempt at fixing #306 #754

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

Closed
wants to merge 10 commits into from
Closed

Naive attempt at fixing #306 #754

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c53bc27
change default property name for xml text from empty string to `<xml:…
duoduobingbing Apr 28, 2025
8989990
Merge branch '3.x' into feature/record-with-jacksonxmltext
cowtowncoder May 1, 2025
8d2e14c
address feedback
duoduobingbing May 2, 2025
c41f2dd
Merge branch '3.x' into feature/record-with-jacksonxmltext
cowtowncoder May 13, 2025
6ed7c4b
Minor rephrasing of Javadocs
cowtowncoder May 13, 2025
ff896b6
ws clean up
cowtowncoder May 13, 2025
5c68f0a
Merge branch '3.x' into feature/record-with-jacksonxmltext
cowtowncoder May 13, 2025
3146147
Merge branch '3.x' into feature/record-with-jacksonxmltext
cowtowncoder May 13, 2025
2e8e2b5
Merge branch '3.x' into feature/record-with-jacksonxmltext
cowtowncoder May 13, 2025
feaf2e8
Merge branch '3.x' into feature/record-with-jacksonxmltext
cowtowncoder May 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/main/java/tools/jackson/dataformat/xml/JacksonXmlAnnotationIntrospector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import tools.jackson.databind.cfg.MapperConfig;
import tools.jackson.databind.introspect.*;
import tools.jackson.dataformat.xml.annotation.*;
import tools.jackson.dataformat.xml.deser.FromXmlParser;

/**
* Extension of {@link JacksonAnnotationIntrospector} that is needed to support
Expand Down Expand Up @@ -208,6 +209,10 @@ public PropertyName findNameForDeserialization(MapperConfig<?> config, Annotated
PropertyName pn = PropertyName.merge(_findXmlName(a),
super.findNameForDeserialization(config, a));
if (pn == null) {
if(_hasAnnotation(a, JacksonXmlText.class)){
return PropertyName.construct(FromXmlParser.DEFAULT_TEXT_PROPERTY);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

if (_hasOneOf(a, ANNOTATIONS_TO_INFER_XML_PROP)) {
Copy link
Member

Choose a reason for hiding this comment

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

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").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

return PropertyName.USE_DEFAULT;
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/tools/jackson/dataformat/xml/XmlMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public Builder(XmlFactory f) {
// String into `null` (where it otherwise is an error) is very useful.
enable(DeserializationFeature.ACCEPT_EMPTY_STRING_AS_NULL_OBJECT);
_defaultUseWrapper = JacksonXmlAnnotationIntrospector.DEFAULT_USE_WRAPPER;
_nameForTextElement = FromXmlParser.DEFAULT_UNNAMED_TEXT_PROPERTY;
_nameForTextElement = FromXmlParser.DEFAULT_TEXT_PROPERTY;

// as well as AnnotationIntrospector: note, however, that "use wrapper" may well
// change later on
Expand Down
8 changes: 4 additions & 4 deletions src/main/java/tools/jackson/dataformat/xml/deser/FromXmlParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@ public class FromXmlParser
implements ElementWrappable
{
/**
* The default name placeholder for XML text segments is empty
* String ("").
* The default name placeholder for XML text segments is &lt;xml:text&gt;
* @since 3.0.0 Is now &lt;xml:text&gt; - was empty String ("") before.
*/
public final static String DEFAULT_UNNAMED_TEXT_PROPERTY = "";
public final static String DEFAULT_TEXT_PROPERTY = "<xml:text>";

/**
* XML format has some peculiarities, indicated via capability
Expand All @@ -61,7 +61,7 @@ public class FromXmlParser
* may be changed for inter-operability reasons: JAXB, for example, uses
* "value" as name.
*/
protected String _cfgNameForTextElement = DEFAULT_UNNAMED_TEXT_PROPERTY;
protected String _cfgNameForTextElement = DEFAULT_TEXT_PROPERTY;

/*
/**********************************************************************
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/tools/jackson/dataformat/xml/deser/XmlDeserializationContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public String extractScalarFromObject(JsonParser p, ValueDeserializer<?> deser,
final String propName = p.currentName();
JsonToken t = p.nextToken();
if (t == JsonToken.VALUE_STRING) {
if (propName.equals("")) {
if (FromXmlParser.DEFAULT_TEXT_PROPERTY.equals(propName)) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

text = p.getString();
}
} else {
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/tools/jackson/dataformat/xml/deser/MapDeserializationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public void testMapWithAttr() throws Exception
assertEquals(1, map.size());
Map<String,Object> inner = new LinkedHashMap<>();
inner.put("lang", "en");
inner.put("", "John Smith");
inner.put(FromXmlParser.DEFAULT_TEXT_PROPERTY, "John Smith");
assertEquals(Collections.singletonMap("person", inner), map);
}
}
2 changes: 1 addition & 1 deletion src/test/java/tools/jackson/dataformat/xml/deser/UntypedObjectDeserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ public void testMixedContent() throws Exception
final String XML = "<root>first<a>123</a>second<b>abc</b>last</root>";
final JsonNode fromXml = XML_MAPPER.valueToTree(XML_MAPPER.readValue(XML, Object.class));
final ObjectNode exp = XML_MAPPER.createObjectNode();
exp.putArray("")
exp.putArray(FromXmlParser.DEFAULT_TEXT_PROPERTY)
.add("first")
.add("second")
.add("last");
Expand Down
3 changes: 2 additions & 1 deletion src/test/java/tools/jackson/dataformat/xml/node/JsonNodeBasicDeserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import tools.jackson.databind.node.JsonNodeType;
import tools.jackson.databind.node.ObjectNode;
import tools.jackson.dataformat.xml.XmlTestUtil;
import tools.jackson.dataformat.xml.deser.FromXmlParser;

import static org.junit.jupiter.api.Assertions.*;

Expand Down Expand Up @@ -43,7 +44,7 @@ public void testMixedContent() throws Exception
{
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)
Copy link
Member

@cowtowncoder cowtowncoder May 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@duoduobingbing duoduobingbing May 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. Thank you for your help here @duoduobingbing .

.add("first")
.add("second")
.add("last");
Expand Down
11 changes: 6 additions & 5 deletions src/test/java/tools/jackson/dataformat/xml/node/JsonNodeMixedContent403Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import tools.jackson.databind.ObjectMapper;
import tools.jackson.databind.json.JsonMapper;
import tools.jackson.dataformat.xml.XmlTestUtil;
import tools.jackson.dataformat.xml.deser.FromXmlParser;

import static org.junit.jupiter.api.Assertions.assertEquals;

Expand All @@ -19,23 +20,23 @@ public class JsonNodeMixedContent403Test extends XmlTestUtil
public void testMixedContentBefore() throws Exception
{
// 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))),
Copy link
Member

Choose a reason for hiding this comment

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

and same here obviously.

XML_MAPPER.readTree("<root>before<a>1</a><b>2</b></root>"));
}

@Test
public void testMixedContentBetween() throws Exception
{
// Second, between
assertEquals(JSON_MAPPER.readTree(a2q("{'a':'1','':'between','b':'2'}")),
assertEquals(JSON_MAPPER.readTree(a2q(String.format("{'a':'1','%s':'between','b':'2'}", FromXmlParser.DEFAULT_TEXT_PROPERTY))),
XML_MAPPER.readTree("<root><a>1</a>between<b>2</b></root>"));
}

@Test
public void testMixedContentAfter() throws Exception
{
// and then after
assertEquals(JSON_MAPPER.readTree(a2q("{'a':'1','b':'2','':'after'}")),
assertEquals(JSON_MAPPER.readTree(a2q(String.format("{'a':'1','b':'2','%s':'after'}", FromXmlParser.DEFAULT_TEXT_PROPERTY))),
XML_MAPPER.readTree("<root><a>1</a><b>2</b>after</root>"));
}

Expand All @@ -44,7 +45,7 @@ public void testMultipleMixedContent() throws Exception
{
// and then after
assertEquals(JSON_MAPPER.readTree(
a2q("{'':['first','second','third'],'a':'1','b':'2'}")),
a2q(String.format("{'%s':['first','second','third'],'a':'1','b':'2'}", FromXmlParser.DEFAULT_TEXT_PROPERTY))),
XML_MAPPER.readTree("<root>first<a>1</a>second<b>2</b>third</root>"));
}

Expand All @@ -57,7 +58,7 @@ public void testMixed226() throws Exception
+" mixed2</a>\n"
+"</root>";
JsonNode fromJson = JSON_MAPPER.readTree(
a2q("{'a':{'':['mixed1 ',' mixed2'],'b':'leaf'}}"));
a2q(String.format("{'a':{'%s':['mixed1 ',' mixed2'],'b':'leaf'}}", FromXmlParser.DEFAULT_TEXT_PROPERTY)));
assertEquals(fromJson, XML_MAPPER.readTree(XML));
}
}
2 changes: 1 addition & 1 deletion src/test/java/tools/jackson/dataformat/xml/stream/XmlParser442Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public void testMixedContentBeforeElement442() throws Exception
// Here's what we are missing:
assertToken(JsonToken.START_OBJECT, xp.nextToken());
assertToken(JsonToken.PROPERTY_NAME, xp.nextToken());
assertEquals("", xp.currentName());
assertEquals(FromXmlParser.DEFAULT_TEXT_PROPERTY, xp.currentName());

assertToken(JsonToken.VALUE_STRING, xp.nextToken());
assertEquals("text", xp.getString().trim());
Expand Down
8 changes: 4 additions & 4 deletions src/test/java/tools/jackson/dataformat/xml/stream/XmlParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public void testRootScalar() throws Exception
try (JsonParser p = _xmlMapper.createParser(XML)) {
assertToken(JsonToken.START_OBJECT, p.nextToken());
assertToken(JsonToken.PROPERTY_NAME, p.nextToken());
assertEquals("", p.currentName());
assertEquals(FromXmlParser.DEFAULT_TEXT_PROPERTY, p.currentName());
assertToken(JsonToken.VALUE_STRING, p.nextToken());
assertEquals("value", p.getString());
assertToken(JsonToken.END_OBJECT, p.nextToken());
Expand All @@ -118,7 +118,7 @@ public void testRootMixed() throws Exception
assertToken(JsonToken.START_OBJECT, p.nextToken());

assertToken(JsonToken.PROPERTY_NAME, p.nextToken());
assertEquals("", p.currentName());
assertEquals(FromXmlParser.DEFAULT_TEXT_PROPERTY, p.currentName());
assertToken(JsonToken.VALUE_STRING, p.nextToken());
assertEquals("value", p.getString());

Expand Down Expand Up @@ -343,7 +343,7 @@ public void testXmlAttributes() throws Exception
@Test
public void testMixedContent() throws Exception
{
String exp = a2q("{'':'first','a':'123','':'second','b':'456','':'last'}");
String exp = a2q(String.format("{'%1$s':'first','a':'123','%1$s':'second','b':'456','%1$s':'last'}", FromXmlParser.DEFAULT_TEXT_PROPERTY));
String result = _readXmlWriteJson("<root>first<a>123</a>second<b>456</b>last</root>");

//System.err.println("result = \n"+result);
Expand Down Expand Up @@ -373,7 +373,7 @@ public void testInferredNumbers() throws Exception
assertEquals(42, xp.getIntValue());

assertToken(JsonToken.PROPERTY_NAME, xp.nextToken()); // implicit for text
assertEquals("", xp.currentName());
assertEquals(FromXmlParser.DEFAULT_TEXT_PROPERTY, xp.currentName());

assertToken(JsonToken.VALUE_STRING, xp.nextToken());
assertTrue(xp.isExpectedNumberIntToken());
Expand Down
1 change: 0 additions & 1 deletion src/test/java/tools/jackson/dataformat/xml/tofix/XmlTextViaCreator306Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,6 @@ public void testIssue306NoCtor() throws Exception
}

// [dataformat-xml#423]
@JacksonTestFailureExpected
@Test
public void testXmlTextViaCtor423() throws Exception
{
Expand Down
1 change: 0 additions & 1 deletion src/test/java/tools/jackson/dataformat/xml/tofix/records/XmlRecordDeser734Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ record Amount(@JacksonXmlText String value,
private final String XML =
a2q("<Amt Ccy='EUR'>1</Amt>");

@JacksonTestFailureExpected
@Test
public void testDeser() throws Exception {
XmlMapper mapper = new XmlMapper();
Expand Down