-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Fix deduction deserializer with DefaultTypeResolverBuilder #3505
Fix deduction deserializer with DefaultTypeResolverBuilder #3505
Conversation
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 Once I get that I can merge this in for inclusion in 2.14 assuming tests pass. |
@Bluexin um, did you do the usual
In case (2) subtype information should not be needed; I suspect that might break handling. |
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 sent the CLA last night. |
@Bluexin I do not usually run all test from IDE; part of the reason is that But I don't think this fix can be used as is, after all. |
…ypeResolverBuilder
CI still reports tons of failures:
so I can't really proceed with this until those are better understood. |
Sorry I just got back to this. |
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. |
.../fasterxml/jackson/databind/deser/TestDeserializerFactoryWithDefaultTypeResolverBuilder.java
Outdated
Show resolved
Hide resolved
@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 ! |
@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 FasterXML/jackson-module-kotlin#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. |
Fix for FasterXML/jackson-module-kotlin#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.