jackson-databind
jackson-databind copied to clipboard
FasterXML
Fix deduction deserializer with DefaultTypeResolverBuilder (pending #)
Fix for https://github.com/FasterXML/jackson-module-kotlin/issues/568, which will be transferred to this repo (if you prefer I can just create a new issue instead) and then I will update the comments in the test. I will also send you the signed CLA shortly.
Excellent, thank you @Bluexin! The only other thing I need now is CLA (unless you have sent one earlier, if so let me know). It's here:
https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf
and the usual way is to print it, fill & sign, scan/take pic, email to info at fasterxml dot com.
This is one-time thing so CLA is good for any and all future contributions.
Once I get that I can merge this in for inclusion in 2.14 assuming tests pass.
@Bluexin um, did you do the usual mvn clean test verification with this patch? Looks like this breaks a whole lot of test cases. I think the logic defined was there for a reason and looking at it, there are two cases:
- Explicit polymorphic handling (via
@JsonTypeInfo) - "Default typing" with no annotations, applied to type categories. Uses (always?) class name as Type Id
In case (2) subtype information should not be needed; I suspect that might break handling. I do not remember all details, nor did check failure cases yet.
I was having trouble with some of the test cases even before changing any code, and running a full maven test (or "all tests" in IDEA) showed many failing/erroring but then running the test classes one by one they all passed except for one expecting a class to be missing that wasn't actually missing. So I figured something might be up with the JDK version I was using or something else with my setup. I will have another look, but there was definitely something weird so it might take some time.
I sent the CLA last night.
@Bluexin I do not usually run all test from IDE; part of the reason is that mvn test will skip tests under ..../failing/ -- but IDE has no idea that these should not be run. They are runnable from IDE (not annotated to be excluded) to allow manual usage.
This might explain what you see.
But I don't think this fix can be used as is, after all.
CI still reports tons of failures:
Tests run: 3694, Failures: 51, Errors: 4, Skipped: 0
so I can't really proceed with this until those are better understood.
Sorry I just got back to this. After some more digging into this, it turns out the addition of the test is what is causing all these failures somehow : I tried without the changes but with the tests and saw all these failures, as well as with the changes but without the test and saw no failures. I'm trying to find out why this happens now.
Turns out the test I copied as structure was using a shared test mapper, which meant changing it's default typing would skew other tests results. Got all green locally, I hope the CI build will be happy as well :)
@Bluexin ah. That makes sense. Thank you for following up; I should have caught that. It is kind of obscure issue.
I wish this came up 1 week ago before 2.14.0-rc1 so we could get some more testing but... oh well. I am already thinking of releasing 2.14.0-rc2.
So I think I'll merge this later today or tomorrow, after thinking it through once more.
Alright, thanks ! Sorry for the delays, life's been rough around the edges.
@Bluexin no problem, we all have other important things to do. I appreciate you getting this done whenever you could!
Oh I just realised the issue never got moved from https://github.com/FasterXML/jackson-module-kotlin/issues/568 so the test doesn't mention any issue number, I hope that's alright
No problem @Bluexin I can just use PR id instead of issue; have done it in the past.