Closed henrik242 closed 3 years ago
@cowtowncoder Have you seen this? Looking forward to rc2 😄
@henrik242 To make sure this is categorized correctly, are you sure this should be here in jackson-dataformat-xml? Or should it go under jackson-module-kotlin?
dependencies {
implementation("com.fasterxml.jackson.dataformat:jackson-dataformat-xml:$jacksonVersion")
implementation**("com.fasterxml.jackson.module:jackson-module-kotlin:$jacksonVersion")**
testImplementation("org.junit.jupiter:junit-jupiter-api:$junitVersion")
testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine:$junitVersion")
}
@henrik242 Had not noticed it. As per @kupci's comment I suspect this might not Kotlin-specific, so I'll try to see if I can reproduce with XML and Java.
I would expect test to probably fail for different reason: instead of null
, should give empty String to constructor. But we'll see.
Actually, no, I would need a plain Java reproduction -- I am guessing this may well be due to problem with @JsonCreator
handling (properties vs delegating), and/or not using @JacksonXmlText
. Alternative would be to try to reproduce this with json and move to Kotlin but that I don't think would work well.
Test itself could be added under jackson-integration-tests
which can combine Kotlin and XML modules (neither of which is allowed to depend on each other, even for tests, since doing so would lead to cyclic dependency that's gnarly for builds).
So, the issue very likely has to do with the way Kotlin module's mapping of data classes to creators; in this case creator would have to be "delegating", not properties-based, looking at structure. I don't know how it might have worked before but I do not think there is a way to make that structure map as expected with properties-style constructors.
Example works with Java definitions like:
static class Stuff427 {
String str;
@JsonCreator(mode = JsonCreator.Mode.DELEGATING)
public Stuff427(String s) { str = s; }
}
static class Product427 {
Stuff427 stuff;
public Product427(@JsonProperty("stuff") Stuff427 s) { stuff = s; }
}
public void testEmptyIssue427() throws Exception
{
String xml = "<product><stuff></stuff></product>";
Product427 product = MAPPER.readValue(xml, Product427.class);
assertNotNull(product);
assertNotNull(product.stuff);
assertEquals("", product.stuff.str);
}
and the distinction is that the outer POJO requires properties-based creator (since there is propert "stuff"), whereas inner POJO requires delegate-based as it must map straight from String value.
So I don't see a bug in XML format module, and suspect that challenge is that of annotating Stuff
to delegate String
value.
@cowtowncoder Should I re-post this issue in the Kotlin module? Or add a PR to https://github.com/FasterXML/jackson-integration-tests ? Or maybe both?
@henrik242 I think I'll first transfer this to Kotlin module, for maintainers to see.
@cowtowncoder @dinomite The 2.12 release fails with this issue now.
What is the timeline for 2.12.1 release and can we add this to the test coverage to prevent regression
Typically first .1 patch follows in about 1 month of the .0, depending on accumulation of fixes. I am trying to take it easy over December anyway, but I think it is likely that 2.12.1 would be released around end of 2020, either last week of Dec or first of Jan 2021.
As to regression test: if the test requires both XML and Kotlin modules, it should to go in:
https://github.com/FasterXML/jackson-integration-tests
to avoid adding cross-component dependencies (beyond existing compile-time dependencies to core components). This assuming it is not possible to reproduce this without xml module (or kotlin module).
@cowtowncoder @michaelbrewer @dinomite I've added an integration test for this now
@cowtowncoder We actually already have the jackson-dataformat-xml
module in the Kotlin module's test dependencies, so I added @henrik242 's test to this module's test suite.
I haven't dug into what's actually going wrong, happy to take tips or PRs against that branch for a fix.
@dinomite Ok. I guess that's fine then -- means that I will need to make sure to always publish XML module before Kotlin one, but that is reasonable from ordering perspective.
Hmm, that's a good point so maybe it should be in the integration tests. Is the error only happening with the XML module in conjunction with the Kotlin module?
Is the error only happening with the XML module in conjunction with the Kotlin module?
Yes, AFAIK
Just dropping in to mention that the branch is open with the test case, awaiting a fix: https://github.com/FasterXML/jackson-module-kotlin/pull/401
@dinomite it should be possible to add currently failing tests under src/test/kotlin/com/fasterxml/jackson/module/kotlin/test/failing
-- that allows running manually without breaking build?
Good idea, I'll move that test into the failing tests group
Moved that test into the failing group on the 2.12 branch. Fixes welcome against 2.12.
This is still a problem in 2.12.1
Yes, still not working
No fix in 2.12.2 either
Edit: Works with JSON.
No fix in 2.12.3, 2.11.4 works. (I'm working with json although). Updating jackson-module-kotlin to 2.12.x doesn't break anything, so I suspect the issue is in jackson.core itself.
I hope this helps, I'm happy to provide a minimal example if needed.
It works with the following dependency configuration:
[INFO] | | +- (com.fasterxml.jackson.core:jackson-core:jar:2.11.4:compile - version managed from 2.10.1; scope updated from test; omitted for duplicate) [INFO] | | +- (com.fasterxml.jackson.core:jackson-annotations:jar:2.11.4:compile - version managed from 2.10.1; scope updated from test; omitted for duplicate) [INFO] | | +- (com.fasterxml.jackson.core:jackson-databind:jar:2.11.4:test - version managed from 2.12.3; omitted for duplicate) [INFO] | | | | +- com.github.java-json-tools:jackson-coreutils:jar:1.9:test [INFO] | | +- (com.fasterxml.jackson.core:jackson-databind:jar:2.11.4:compile - version managed from 2.12.3; omitted for duplicate) [INFO] | | +- com.fasterxml.jackson.datatype:jackson-datatype-jdk8:jar:2.12.3:compile [INFO] | | | +- (com.fasterxml.jackson.core:jackson-core:jar:2.11.4:compile - version managed from 2.12.3; omitted for duplicate) [INFO] | | | \- (com.fasterxml.jackson.core:jackson-databind:jar:2.11.4:compile - version managed from 2.12.3; omitted for duplicate) [INFO] | | +- com.fasterxml.jackson.datatype:jackson-datatype-jsr310:jar:2.12.3:compile [INFO] | | | +- (com.fasterxml.jackson.core:jackson-annotations:jar:2.11.4:compile - version managed from 2.12.3; omitted for duplicate) [INFO] | | | +- (com.fasterxml.jackson.core:jackson-core:jar:2.11.4:compile - version managed from 2.12.3; omitted for duplicate) [INFO] | | | \- (com.fasterxml.jackson.core:jackson-databind:jar:2.11.4:compile - version managed from 2.12.3; omitted for duplicate) [INFO] | | \- com.fasterxml.jackson.module:jackson-module-parameter-names:jar:2.12.3:compile [INFO] | | +- (com.fasterxml.jackson.core:jackson-core:jar:2.11.4:compile - version managed from 2.12.3; omitted for duplicate) [INFO] | | \- (com.fasterxml.jackson.core:jackson-databind:jar:2.11.4:compile - version managed from 2.12.3; omitted for duplicate) [INFO] | | +- (com.fasterxml.jackson.core:jackson-databind:jar:2.11.4:runtime - version managed from 2.12.3; omitted for duplicate) [INFO] | | \- (com.fasterxml.jackson.datatype:jackson-datatype-jsr310:jar:2.12.3:runtime - omitted for duplicate) [INFO] +- com.fasterxml.jackson.module:jackson-module-kotlin:jar:2.12.3:compile [INFO] | +- com.fasterxml.jackson.core:jackson-databind:jar:2.11.4:compile [INFO] | | +- (com.fasterxml.jackson.core:jackson-annotations:jar:2.11.4:compile - version managed from 2.12.3; omitted for duplicate) [INFO] | | \- com.fasterxml.jackson.core:jackson-core:jar:2.11.4:compile (version managed from 2.12.3) [INFO] | +- com.fasterxml.jackson.core:jackson-annotations:jar:2.11.4:compile [INFO] | \- (com.fasterxml.jackson.core:jackson-databind:jar:2.11.4:compile - version managed from 2.12.1; omitted for duplicate) [INFO] | | +- (com.fasterxml.jackson.datatype:jackson-datatype-jsr310:jar:2.12.3:compile - version managed from 2.11.2; omitted for duplicate) [INFO] | | +- com.fasterxml.jackson.dataformat:jackson-dataformat-xml:jar:2.11.4:compile (version managed from 2.11.2) [INFO] | | | +- (com.fasterxml.jackson.core:jackson-core:jar:2.11.4:compile - version managed from 2.12.3; omitted for duplicate) [INFO] | | | +- (com.fasterxml.jackson.core:jackson-annotations:jar:2.11.4:compile - version managed from 2.12.3; omitted for duplicate) [INFO] | | | +- (com.fasterxml.jackson.core:jackson-databind:jar:2.11.4:compile - version managed from 2.12.1; omitted for duplicate) [INFO] | | | +- com.fasterxml.jackson.module:jackson-module-jaxb-annotations:jar:2.12.3:compile (version managed from 2.11.4) [INFO] | | | | +- (com.fasterxml.jackson.core:jackson-annotations:jar:2.11.4:compile - version managed from 2.12.3; omitted for duplicate) [INFO] | | | | +- (com.fasterxml.jackson.core:jackson-core:jar:2.11.4:compile - version managed from 2.12.3; omitted for duplicate) [INFO] | | | | +- (com.fasterxml.jackson.core:jackson-databind:jar:2.11.4:compile - version managed from 2.12.3; omitted for duplicate)
@patrick-dedication A minimal example (or a failing test) is always good to have! Interesting that you have found the same issue with JSON.
@patrick-dedication We do have a failing test case for this issue, if you create a PR that adds an example for what you're seeing that would be helpful https://github.com/FasterXML/jackson-module-kotlin/blob/2.13/src/test/kotlin/com/fasterxml/jackson/module/kotlin/test/github/failing/Github396.kt
@henrik242 @dinomite Sure, I'll open a PR ASAP.
@dinomite I was unable to repro this with JSON. When I had a closer look at my application I saw it switched to returning XML instead of JSON, so totally my bad.
Ooh, that's good to know—more information is always good!
I don't think this is a Kotlin-specific problem. I have managed to reproduce the regression in the below Java snippet, which works in 2.11.4, but fails in 2.12.x:
package com.vlkan.jackson;
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonRootName;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.module.SimpleModule;
import com.fasterxml.jackson.dataformat.xml.XmlMapper;
import org.junit.jupiter.api.Test;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import static org.junit.jupiter.api.Assertions.assertEquals;
class MixinTest {
interface Problem {
String DEFAULT_TYPE = "about:blank";
int DEFAULT_STATUS = 500;
String getType();
int getStatus();
}
static class DefaultProblem implements Problem {
private final String type;
private final int status;
/**
* This is required to workaround Jackson's missing support for static
* {@link JsonCreator}s in mix-ins. That is, we need to define the
* creator on a constructor in the mix-in that is matching with a
* constructor here too.
*
* @see <a href="https://github.com/FasterXML/jackson-databind/issues/1820">jackson-databind issue 1820</a>
*/
DefaultProblem(String type, Integer status) {
this.type = type != null ? type : Problem.DEFAULT_TYPE;
this.status = status != null ? status : Problem.DEFAULT_STATUS;
}
@Override
public String getType() {
return type;
}
@Override
public int getStatus() {
return status;
}
}
@JsonTypeInfo(
use = JsonTypeInfo.Id.NAME,
include = JsonTypeInfo.As.EXISTING_PROPERTY,
property = "type",
defaultImpl = DefaultProblem.class,
visible = true)
@JsonInclude(JsonInclude.Include.NON_EMPTY)
@JsonRootName("problem")
interface ProblemMixIn extends Problem {
@Override
@JsonProperty("type")
String getType();
@Override
@JsonProperty("status")
int getStatus();
}
abstract static class DefaultProblemMixIn extends DefaultProblem {
@JsonCreator
DefaultProblemMixIn(
@JsonProperty("type") String type,
@JsonProperty("status") Integer status) {
super(type, status);
throw new IllegalStateException(
"mix-in constructor is there only for extracting the JSON mapping, " +
"it should not have been called");
}
}
static class ProblemModule extends SimpleModule {
@Override
public void setupModule(SetupContext context) {
super.setupModule(context);
registerMixIns(context);
}
private static void registerMixIns(SetupContext context) {
context.setMixInAnnotations(DefaultProblem.class, DefaultProblemMixIn.class);
context.setMixInAnnotations(Problem.class, ProblemMixIn.class);
}
}
private static final ProblemModule MODULE = new ProblemModule();
private static final ObjectMapper JSON_MAPPER = new ObjectMapper().registerModule(MODULE);
private static final XmlMapper XML_MAPPER = (XmlMapper) new XmlMapper().registerModule(MODULE);
@Test
void test_empty_Problem_JSON_deserialization() throws IOException {
byte[] problemJsonBytes = "{}".getBytes(StandardCharsets.UTF_8);
Problem problem = JSON_MAPPER.readValue(problemJsonBytes, Problem.class);
assertEquals(Problem.DEFAULT_TYPE, problem.getType());
assertEquals(Problem.DEFAULT_STATUS, problem.getStatus());
}
@Test
void test_empty_Problem_XML_deserialization() throws IOException {
byte[] problemXmlBytes = "<problem/>".getBytes(StandardCharsets.UTF_8);
Problem problem = XML_MAPPER.readValue(problemXmlBytes, Problem.class);
assertEquals(Problem.DEFAULT_TYPE, problem.getType());
assertEquals(Problem.DEFAULT_STATUS, problem.getStatus());
}
}
Both tests pass on 2.11.4, whereas, on 2.12.x, test_empty_Problem_JSON_deserialization()
passes and
test_empty_Problem_XML_deserialization()
fails with the following message:
com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot construct instance of `com.vlkan.jackson.MixinTest$DefaultProblem` (although at least one Creator exists): no default no-arguments constructor found
at [Source: (byte[])"<problem/>"; line: 1, column: 1]
at com.fasterxml.jackson.databind.exc.MismatchedInputException.from(MismatchedInputException.java:63)
at com.fasterxml.jackson.databind.DeserializationContext.reportInputMismatch(DeserializationContext.java:1588)
at com.fasterxml.jackson.databind.DeserializationContext.handleMissingInstantiator(DeserializationContext.java:1213)
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:1495)
at com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeOther(BeanDeserializer.java:207)
at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:197)
at com.fasterxml.jackson.databind.jsontype.impl.AsPropertyTypeDeserializer._deserializeTypedUsingDefaultImpl(AsPropertyTypeDeserializer.java:194)
at com.fasterxml.jackson.databind.jsontype.impl.AsPropertyTypeDeserializer.deserializeTypedFromObject(AsPropertyTypeDeserializer.java:96)
at com.fasterxml.jackson.databind.deser.AbstractDeserializer.deserializeWithType(AbstractDeserializer.java:263)
at com.fasterxml.jackson.databind.deser.impl.TypeWrappedDeserializer.deserialize(TypeWrappedDeserializer.java:74)
at com.fasterxml.jackson.dataformat.xml.deser.XmlDeserializationContext.readRootValue(XmlDeserializationContext.java:91)
at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4593)
at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3609)
at com.vlkan.jackson.MixinTest.test_empty_Problem_XML_deserialization(MixinTest.java:129)
...
In order to make test_empty_Problem_XML_deserialization()
pass on 2.12.x, one needs to add a no-arg ctor to DefaultProblem
.
@vy Well that is certainly interesting! You've clearly got pure Java code so I think this should be sent back to jackson-databind.
@cowtowncoder Given this seems to be a pure Java issues should the tests just go in databind or jackson-integration-tests?
@dinomite Since it is wrt XML (if I read things correctly), it would need to go in jackson-dataformat-xml
(if no additional deps needed). Otherwise jackson-integration-tests
is fine as well (if deps needed).
I am releasing 2.12.5 now, wondering if one fix in particular might help here.
@cowtowncoder The original test still fails with 2.13.0-rc2
. Crossing my fingers for 2.12.5
:)
@henrik242 Thanks for keeping an eye on this, please report back once you've had a chance to check out 2.12.5
2.12.5 is out, but if if 2.13.0-rc2 fails, I suspect the issue is still there.
For me the issue is still there with 2.12.5
No reason for everyone to run their own personal tests, just run the one in jackson-integration-tests:
$ git checkout 2.12
$ mvn test -Dtest=Jackson212MissingConstructorTest
...
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.657 s <<< FAILURE! - in com.fasterxml.jackson.failing.Jackson212MissingConstructorTest
[ERROR] testMissingConstructor(com.fasterxml.jackson.failing.Jackson212MissingConstructorTest) Time elapsed: 0.599 s <<< ERROR!
com.fasterxml.jackson.databind.exc.MismatchedInputException:
Cannot construct instance of `com.fasterxml.jackson.failing.Jackson212MissingConstructorTest$Stuff` (although at least one Creator exists): no default no-arguments constructor found
at [Source: (StringReader); line: 1, column: 17] (through reference chain: com.fasterxml.jackson.failing.Jackson212MissingConstructorTest$Product["stuff"])
at com.fasterxml.jackson.failing.Jackson212MissingConstructorTest.testMissingConstructor(Jackson212MissingConstructorTest.kt:22)
@vy I created a pull request with your test in jackson-integration-tests, hope that's ok! https://github.com/FasterXML/jackson-integration-tests/pull/12
EDIT: Never mind, I saw just noticed that you added it yourself in https://github.com/FasterXML/jackson-integration-tests/pull/10 Sorry! :)
Test added in jackson-module-xml https://github.com/FasterXML/jackson-dataformat-xml/pull/492
Just as a note to myself (and others following this), this bug is now being tracked in https://github.com/FasterXML/jackson-dataformat-xml/issues/491
It can be reproduced with mvn test -Dtest=Issue491NoArgCtorDeserRegressionTest
in the jackson-dataformat-xml repo.
@henrik242 Thanks for following up with that—such links of information are very helpful.
Fixed in 2.15.0:
https://github.com/FasterXML/jackson-dataformat-xml/issues/547
need to change "failing failing test" now
@cowtowncoder Amazing, thanks for the great work! Fingers crossed for an imminent 2.15.0 release 🤞😊
Here's another test that's probably fixed too, btw: https://github.com/FasterXML/jackson-integration-tests/blob/master/src/test/kotlin/com/fasterxml/jackson/failing/Jackson212MissingConstructorTest.kt
And maybe this is related? https://github.com/FasterXML/jackson-dataformat-xml/blob/master/src/test/java/tools/jackson/dataformat/xml/failing/NoArgCtorDeser491Test.java
@henrik242 I think release will be sooner than I originally planned, due to one particular CVE. But I think we are still out by at least a month since first release candidate. Still, lots of good fixes esp. for Java record types, for 2.15.
@cowtowncoder No problem :) I see that the jackson-integration-tests
test is still failing, by the way. Maybe I'm trying the wrong way? Here's the change: https://github.com/FasterXML/jackson-integration-tests/compare/master...henrik242:jackson-integration-tests:2.15-missing-constructor-fail
(I also had to do a sbt publishM2
in jackson-module-scala
to make the build run)
@henrik242 Hmmh. Ok, I got Scala module snapshot resolved (should now push snapshots automatically with fix @pjfanning did). But I have always hard time running Kotlin-tests from Eclipse... not sure how to get that to work.
@cowtowncoder Cool, I can probably make an issue of it then. Can't you just make Eclipse trigger mvn
to run the tests?
I can't get Eclipse to read Kotlin projects in a way that it recognizes Kotlin classes, unfortunately. Running test might work somehow but wouldn't help a lot. Well, I guess it would at least show the failure I guess.
Weird. It works out of the box in IntelliJ IDEA. Haven't used Eclipse in ages, so I can't help there :/
EDIT: I tried the latest Eclipse IDE, but couldn't get it to work. The kotlin plugin had problems that I was unable to resolve, and the scala plugin isn't supported anymore.
Understood. I can try with Idea, I have a free copy I think. The only issue I've had where Eclipse is better is ability to work with multiple modules (like breakpoints in jackson-databind as well as Kotlin module); never figured out how that'd work with Idea. Not a problem here tho.
Then again even if I can reproduce the failure not sure I can do much to fix it, but just to acknowledge. I guess I could at least re-open this issue & remove "fixed" entry from release notes. I suspect this has to do with Kotlin doing its own Creator discovery.
Forgot to update: had absolutely no problem running Kotlin test from Idea.
I added notes on integration test over at:
https://github.com/FasterXML/jackson-integration-tests/issues/14
but basically I think things actually work as expected from Jackson perspective (no more exception, outcomes as expected I think); so what need is:
@JsonCreator(mode =
), to make sure desired outcome@henrik242 Let me know what you think.
I have this Kotlin object which parses an XML into a
data class
:And this Java test:
Parsing this in Jackson 2.12.0 fails with
com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot construct instance of jackson.xml.XmlTool$Stuff (although at least one Creator exists): no default no-arguments constructor found
.Jackson 2.11.3 works just fine.
The full exception is:
Here's a test project: https://github.com/henrik242/jackson-xml-problem/tree/2-12-empty-constructor