-
-
Notifications
You must be signed in to change notification settings - Fork 175
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
Update to 2.8.0-SNAPSHOT for jackson dependencies #31
Update to 2.8.0-SNAPSHOT for jackson dependencies #31
Conversation
Making the pom change by itself broke a few tests: Tests in error: TestCasesFromSlack1.testCzarSpringThing1:64 ▒ JsonMapping Argument #0 of const... TestCasesFromSlack2.testCzarSpringThing2:64 ▒ JsonMapping Argument #0 of const... TestGithub15.testEnumConstructorWithParm:9 ▒ JsonMapping Argument #0 of constr... TestGithub15.testNormEnumWithoutParam:16 ▒ JsonMapping Argument #0 of construc... Tests run: 36, Failures: 0, Errors: 4, Skipped: 1 They all failed to deserialize enums. Adding the line to KotlinModule that skips AnnotatedConstructors for enum classes fixes those tests. I don't entirely understand why the upgrade to 2.8.0-SNAPSHOT caused those failures, but the fix sounds safe. There's never a case where we would want to try invoking an enum constructor, right?
My first guess would be that this could be side effect of: FasterXML/jackson-databind#1082 in which factory method creators are allowed for enums now. Perhaps some of invariants that are true for Java are not so for Kotlin; or maybe none of java-side tests triggered the edge case. I'll wait for @apatrida to say this is (or isn't) ok first just to be sure; I assume fix/workaround is ok. |
I'll look at this, basically I don't think this branch of the code happened with enum types, and if Kotlin enums have the Kotlin class marker, then the wrong functionality would execute probably the plugin misguiding Jackson about what to do. I think it is ok, will check it and test soon. |
The enum change is on all branches now (2.5, 2.6, 2.8, master) The other change is on master due to other work (using Jackson 2.8.0-SNAPSHOT) |
@apatrida Ok excellent. I'll get to 2.8.0.rc1 Real Soon Now (possibly even over the weekend), and 2.7.5 should be released quite soon too. There will probably not be general releases from earlier branches (they are considered closed), although if you think it makes sense I can push micro-patches (2.6.6.1 etc) where applicable. |
It depends on the last version Spark uses for Jackson (for old branches), On Sat, Jun 4, 2016 at 12:54 AM, Tatu Saloranta notifications@github.com
|
@apatrida yeah Spark's class loading is really sucky: absolutely wrong NOT to use classloader to separate platform jars from jobs running on it. Flink is better in this respect (as well as many others), hoping that Spark 2.0 will bring improvements. Spark 2.0 does upgrade to Jackson 2.7(.4) so at least that should help a bit, even if isolation wasn't added. |
@apatrida One more I remembered -- what would the best way to get release notes at https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.7.5 updated? Could you edit the page, or is there a file in repo that I could look at to just copy? (ditto for other versions too as necessary). Also: I figured that since there are some other changes pending in 2.6 branch, I might as well do one extra full push to get them out. So Kotlin can go out as full 2.6.7. For earlier branches just let me know which ones would make sense -- I really prefer explicit need for such extra releases, since my experience is that adoption rates for latest patch versions are quite low; see for example 2.5.5 (last 2.5 patch): http://mvnrepository.com/artifact/com.fasterxml.jackson.core/jackson-databind |
I'll do edit on the page, 2.7.5 was smaller changes, the master 2.8.0 is Done, I updated the page. On Sat, Jun 4, 2016 at 3:38 PM, Tatu Saloranta notifications@github.com
|
I updated 2.8 notes as well. |
Thanks! |
Making the pom change by itself broke a few tests:
They all failed to deserialize enums. Adding the line to KotlinModule
that skips AnnotatedConstructors for enum classes fixes those tests.
I don't entirely understand why the upgrade to 2.8.0-SNAPSHOT caused
those failures, but the fix sounds safe. There's never a case where
we would want to try invoking an enum constructor, right?