FasterXML / jackson-dataformat-xml

Extension for Jackson JSON processor that adds support for serializing POJOs as XML (and deserializing from XML) as an alternative to JSON
Apache License 2.0
567 stars 221 forks source link

Parsing empty tags without default no-arguments constructor fails in 2.14 #547

Closed henrik242 closed 1 year ago

henrik242 commented 2 years ago

As in https://github.com/FasterXML/jackson-dataformat-xml/issues/491, I've found an additional issue where parsing empty tags without default constructor fails. This worked in 2.11.3, but has failed since.

Check should_parse_empty_tag_without_default_constructor() in https://github.com/henrik242/jackson-xml-problem/blob/no-string-argument/src/test/java/jackson/xml/NoStringArgumentTest.java#L37

Test (java):

    @Test
    void  should_parse_empty_tag_without_default_constructor() {
        String xml = "<outer><inner></inner></outer>";
        Outer res = XmlTool.parseOuter(xml);
        assertEquals(res, new Outer(null)); // SUCCESS in Jackson 2.11.x, but FAIL in 2.14.x
    }

Code (kotlin):

object XmlTool {
    val xmlIn = XMLInputFactory.newInstance()
    val factory = XmlFactory(xmlIn)
    val xmlModule = JacksonXmlModule()
    val mapper = XmlMapper(factory, xmlModule).registerKotlinModule()

    @JvmStatic
    fun parseOuter(xml: String?): Outer = mapper.readValue(xml, Outer::class.java)

    data class Inner(val code: String?)
    data class Outer(val inner: Inner?)
}

Fails with:

com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot construct instance of `jackson.xml.XmlTool$Inner` (although at least one Creator exists): no default no-arguments constructor found
 at [Source: (StringReader); line: 1, column: 15] (through reference chain: jackson.xml.XmlTool$Outer["inner"])
    at app//com.fasterxml.jackson.databind.exc.MismatchedInputException.from(MismatchedInputException.java:63)
    at app//com.fasterxml.jackson.databind.DeserializationContext.reportInputMismatch(DeserializationContext.java:1733)
    at app//com.fasterxml.jackson.databind.DeserializationContext.handleMissingInstantiator(DeserializationContext.java:1358)
    at app//com.fasterxml.jackson.databind.deser.ValueInstantiator.createUsingDefault(ValueInstantiator.java:248)
    at app//com.fasterxml.jackson.databind.deser.std.StdValueInstantiator.createUsingDefault(StdValueInstantiator.java:275)
    at app//com.fasterxml.jackson.databind.deser.BeanDeserializerBase.getEmptyValue(BeanDeserializerBase.java:1042)
    at app//com.fasterxml.jackson.databind.deser.std.StdDeserializer._deserializeFromEmptyString(StdDeserializer.java:322)
    at app//com.fasterxml.jackson.databind.deser.std.StdDeserializer._deserializeFromString(StdDeserializer.java:270)
    at app//com.fasterxml.jackson.databind.deser.BeanDeserializerBase.deserializeFromString(BeanDeserializerBase.java:1500)
    at app//com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeOther(BeanDeserializer.java:197)
    at app//com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:187)
    at app//com.fasterxml.jackson.databind.deser.SettableBeanProperty.deserialize(SettableBeanProperty.java:542)
    at app//com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeWithErrorWrapping(BeanDeserializer.java:564)
    at app//com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeUsingPropertyBased(BeanDeserializer.java:439)
    at app//com.fasterxml.jackson.databind.deser.BeanDeserializerBase.deserializeFromObjectUsingNonDefault(BeanDeserializerBase.java:1405)
    at app//com.fasterxml.jackson.databind.deser.BeanDeserializer.deserializeFromObject(BeanDeserializer.java:352)
    at app//com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:185)
    at app//com.fasterxml.jackson.dataformat.xml.deser.XmlDeserializationContext.readRootValue(XmlDeserializationContext.java:91)
    at app//com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4697)
    at app//com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3652)
    at app//com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3620)
    at app//jackson.xml.XmlTool.parseOuter(XmlTool.kt:22)
    at app//jackson.xml.NoStringArgumentTest.should_parse_empty_tag_without_default_constructor(NoStringArgumentTest.java:39)
    at java.base@11.0.16.1/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base@11.0.16.1/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at java.base@11.0.16.1/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base@11.0.16.1/java.lang.reflect.Method.invoke(Method.java:566)
    at app//org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:675)
    at app//org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
    at ...

This is the exact same issue as mentioned in https://github.com/FasterXML/jackson-module-kotlin/issues/396, and is already covered a couple of places with existing failing tests:

cowtowncoder commented 2 years ago

@henrik242 Since we cannot use Kotlin in tests here (not allowed to add dependency), would it be possible to have Java version? It also rules out possibility the issue with Kotlin module, which is quite often the case.

henrik242 commented 2 years ago

I'm not sure if I'm able to reproduce it with plain java. Maybe move this issue to jackson-kotlin-module and rename Github396.kt to match? Or reopen issue 396 there?

cowtowncoder commented 2 years ago

It has to be in Kotlin if it uses Kotlin code. Or to avoid Kotlin module (test) dependency to XML, could also go into jackson-integration-tests. But we need to get into habit of running those tests somehow... maybe scheduled on daily basis. Plus update to refer to latest publish versions etc etc. Still, that test module is intended for cross-Module tests.

Looking at stack trace this does seem to call the wrong constructor via getEmptyValue(). I wonder if Kotlin module overrides something to prevent things from working as expected.

cowtowncoder commented 2 years ago

Oh. Or this might occur with Java too. The problem, I think, is that there is ambiguity on empty String value being found. There are 2 ways to consider it:

  1. Ignorable white space (-> empty content for Object/Element)
  2. String with white space (-> Needs to have @JacksonXmlText)

so perhaps this would occur with similar POJO. But I think Jackson 2.13 added "coerce Scalar from Object [in some cases]" which might solve this... unless it only works for not-all-white-space case (seems plausible).

henrik242 commented 2 years ago

Or to avoid Kotlin module (test) dependency to XML, could also go into jackson-integration-tests.

Yeah, it can easily go there too since it also has a failing test that cover this (as mentioned above).

  1. Ignorable white space ...
  2. String with white space ...

But in this case there's no white space at all, just no definition of the arg in the default constructor.

cowtowncoder commented 2 years ago

But in this case there's no white space at all, just no definition of the arg in the default constructor.

Ah. Yes, indeed.

magicaljellybeans commented 1 year ago

I'm getting a couple of test regressions going from 2.11.3 to 2.14.0 writing scala which I think are related to this.

One failing test I have seems to be a carbon copy of @henrik242's posted situation.

com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot construct instance of `uk.gov.hmrc.wco.dec.ResponseDateTimeElement` (although at least one Creator exists): no default no-arguments constructor found
 at [Source: (StringReader); line: 1, column: 240] (through reference chain: uk.gov.hmrc.wco.dec.MetaData["Response"]->com.fasterxml.jackson.module.scala.deser.GenericFactoryDeserializerResolver$BuilderWrapper[0]->uk.gov.hmrc.wco.dec.Response["IssueDateTime"])
    at com.fasterxml.jackson.databind.exc.MismatchedInputException.from(MismatchedInputException.java:63)
    at com.fasterxml.jackson.databind.DeserializationContext.reportInputMismatch(DeserializationContext.java:1733)
    at com.fasterxml.jackson.databind.DeserializationContext.handleMissingInstantiator(DeserializationContext.java:1358)
    at com.fasterxml.jackson.databind.deser.ValueInstantiator.createUsingDefault(ValueInstantiator.java:248)
    at com.fasterxml.jackson.databind.deser.std.StdValueInstantiator.createUsingDefault(StdValueInstantiator.java:275)
    at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.getEmptyValue(BeanDeserializerBase.java:1042)
    at com.fasterxml.jackson.databind.deser.std.StdDeserializer._deserializeFromEmptyString(StdDeserializer.java:322)
    at com.fasterxml.jackson.databind.deser.std.StdDeserializer._deserializeFromString(StdDeserializer.java:270)
    at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.deserializeFromString(BeanDeserializerBase.java:1500)
    at com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeOther(BeanDeserializer.java:197)
    at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:187)
    at com.fasterxml.jackson.module.scala.deser.OptionDeserializer.deserialize(OptionDeserializerModule.scala:62)
    at com.fasterxml.jackson.module.scala.deser.OptionDeserializer.deserialize(OptionDeserializerModule.scala:11)
    at com.fasterxml.jackson.databind.deser.SettableBeanProperty.deserialize(SettableBeanProperty.java:542)
    at com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeWithErrorWrapping(BeanDeserializer.java:564)
    at com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeUsingPropertyBased(BeanDeserializer.java:439)
    at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.deserializeFromObjectUsingNonDefault(BeanDeserializerBase.java:1405)
    at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserializeFromObject(BeanDeserializer.java:352)
    at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:185)
    at com.fasterxml.jackson.dataformat.xml.deser.WrapperHandlingDeserializer.deserialize(WrapperHandlingDeserializer.java:122)
    at com.fasterxml.jackson.databind.deser.std.CollectionDeserializer._deserializeFromArray(CollectionDeserializer.java:359)
    at com.fasterxml.jackson.databind.deser.std.CollectionDeserializer.deserialize(CollectionDeserializer.java:244)
    at com.fasterxml.jackson.module.scala.deser.GenericFactoryDeserializerResolver$Deserializer.deserialize(GenericFactoryDeserializerResolver.scala:125)
    at com.fasterxml.jackson.databind.deser.SettableBeanProperty.deserialize(SettableBeanProperty.java:542)
    at com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeWithErrorWrapping(BeanDeserializer.java:564)
    at com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeUsingPropertyBased(BeanDeserializer.java:439)
    at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.deserializeFromObjectUsingNonDefault(BeanDeserializerBase.java:1405)
    at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserializeFromObject(BeanDeserializer.java:352)
    at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:185)
    at com.fasterxml.jackson.dataformat.xml.deser.WrapperHandlingDeserializer.deserialize(WrapperHandlingDeserializer.java:122)
    at com.fasterxml.jackson.dataformat.xml.deser.XmlDeserializationContext.readRootValue(XmlDeserializationContext.java:91)
    at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4730)
    at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3677)
    at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3645)

On another test an empty tag returns Some("") instead of the expected None.

cowtowncoder commented 1 year ago

At this point what I would really need is a plain Java test. I'd love to have a look but I'd need to translate Data classes into their equivalent (from JVM perspective) counterparts.

The specific distinction has to do with EXACTLY how constructor for Outer and Inner is declared: problem being they both take 1 argument and may use mode of either Mode.DELEGATING of Mode.PROPERTIES -- and there are basically 4 combinations all of which may behave slightly differently.

I am guessing tho that Outer must have Mode.PROPERTIES so question really is about constructor setting for Inner. I will try it out a bit on Java side but definitive answer here would be helpful.

cowtowncoder commented 1 year ago

@henrik242 Ok, I think that what you can do is to add @JsonCreator(mode = JsonCreator.Mode.DELEGATING) for constructor of Inner. In this case String "" will be passed (not null).

I will try to see if the alternative (Inner declared to take PROPERTIES) could possibly be made to also work but thought I'll add a note of work-around; and you'd probably want to indicate mode in either since behavior does generally differ between two modes.

EDIT: there is actually another work-around that does work with PROPERTIES approach -- just add no-argument constructor. That will make deserializer happy.

cowtowncoder commented 1 year ago

Fixed in 2.15 branch (for 2.15.0) -- actual fix is in jackson-databind but I'll modify test to verify it. Fix cannot be backported in 2.14 due to requiring a small API change for ValueInstantiator. In the meantime 2 work-arounds I mentioned hopefully help.

henrik242 commented 1 year ago

@cowtowncoder Thanks! Is it a bit too early to ask for an 2.15 release date estimate? 😄

cowtowncoder commented 1 year ago

Yes it is. :)

It will take a while, most likely, looking at historic trends.

henrik242 commented 1 year ago

@cowtowncoder Okay, I kinda assumed as much :) We have a lot of data classes affected by this, so a per-class workaround will require a lot of work. There aren't any workarounds that can be put on the ObjectMapper, right?

cowtowncoder commented 1 year ago

I can't think of any based on specific fix, but that does not mean someone could not figure out a way around it.

However... I suspect that you could use AnnotationIntrospector to essentially force mode = Mode.DELEGATING for specific inner classes; detect cases where it is needed, return. I don't remember off-hand which method in AnnotationIntrospector but shouldn't be too hard to find.