Skip to content
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

Conversation

Bluexin
Copy link
Contributor

@Bluexin Bluexin commented Jun 2, 2022

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.

@cowtowncoder
Copy link
Member

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.

@cowtowncoder
Copy link
Member

@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:

  1. Explicit polymorphic handling (via @JsonTypeInfo)
  2. "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.

@Bluexin
Copy link
Contributor Author

Bluexin commented Jun 3, 2022

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.

@cowtowncoder
Copy link
Member

@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.

@cowtowncoder
Copy link
Member

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.

@Bluexin
Copy link
Contributor Author

Bluexin commented Sep 30, 2022

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.

@Bluexin
Copy link
Contributor Author

Bluexin commented Oct 1, 2022

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 :)

@cowtowncoder
Copy link
Member

@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.

@Bluexin
Copy link
Contributor Author

Bluexin commented Oct 1, 2022

Alright, thanks !
Sorry for the delays, life's been rough around the edges.

@cowtowncoder
Copy link
Member

@Bluexin no problem, we all have other important things to do. I appreciate you getting this done whenever you could!

@cowtowncoder cowtowncoder merged commit a5c4632 into FasterXML:2.14 Oct 1, 2022
@cowtowncoder cowtowncoder changed the title Fix deduction deserializer with DefaultTypeResolverBuilder (pending #) Fix deduction deserializer with DefaultTypeResolverBuilder Oct 1, 2022
cowtowncoder added a commit that referenced this pull request Oct 1, 2022
@Bluexin
Copy link
Contributor Author

Bluexin commented Oct 1, 2022

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

@cowtowncoder
Copy link
Member

No problem @Bluexin I can just use PR id instead of issue; have done it in the past.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants