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

Regression updating from 2.3.3 to 2.4.0: null won't deserialize in String[] #479

Closed
huxi opened this issue Jun 3, 2014 · 12 comments
Closed

Comments

@huxi
Copy link

huxi commented Jun 3, 2014

Steps to reproduce

  1. Clone the repository at https://github.com/huxi/sulky
  2. Execute the contained ./gradlew or gradlew.bat
  3. Clone the repository at https://github.com/huxi/lilith/
  4. Change jackson-version in the project.ext.versions map of dependencyDefinitions.gradle from '2.3.3' to '2.4.0'.
  5. Execute the contained ./gradlew or gradlew.bat

There will be six test-failures with 2.4.0 that won't happen with 2.3.3.

There are actually only 2 test-methods that fail 3 times each.

Those methods reside at full() and nullArgument().

I first suspected that AfterburnerModule might be the culprit but removing it from LoggingJsonDecoder/LoggingJsonEncoder didn't fix the problem.

Sorry for not narrowing down the problem further. I'll give this another look tomorrow but you may already be able to find the issue in the meantime.

The interesting thing is that several other test cases are working as intended...

@huxi
Copy link
Author

huxi commented Jun 3, 2014

Forgot to mention my system specs:

------------------------------------------------------------
Gradle 1.12
------------------------------------------------------------

Build time:   2014-04-29 09:24:31 UTC
Build number: none
Revision:     a831fa866d46cbee94e61a09af15f9dd95987421

Groovy:       1.8.6
Ant:          Apache Ant(TM) version 1.9.3 compiled on December 23 2013
Ivy:          2.2.0
JVM:          1.8.0_05 (Oracle Corporation 25.5-b02)
OS:           Mac OS X 10.9.3 x86_64

@cowtowncoder
Copy link
Member

And I assume JDK 1.8 has not been problematic with 2.3.3. I don't yet use Java8 except for building 2 modules that require it.

@cowtowncoder
Copy link
Member

Hmmh. Build of sulky fails (using JDK 1.7.0_09-b05)

de.huxhorn.sulky.formatting.SafeStringTest > date FAILED
    java.lang.AssertionError at SafeStringTest.java:226

@huxi
Copy link
Author

huxi commented Jun 3, 2014

Yes, 2.3.3 works with Java 8.

This might be a bit unusual but could you change the log-level to INFO and send me the output of the test? I suspect a timezone snafu. sigh

Otherwise, just @Ignore the test in question to get a result.

@huxi
Copy link
Author

huxi commented Jun 3, 2014

Fixed the problem you reported. Thanks for the info.

@cowtowncoder
Copy link
Member

Ok now I can build sulky just fine. Will see how far I get. :)

@cowtowncoder
Copy link
Member

Ok, looks like this stack trace:

Caused by: java.lang.NullPointerException
    at com.fasterxml.jackson.databind.deser.std.StringArrayDeserializer.deserialize(StringArrayDeserializer.java:66)
    at com.fasterxml.jackson.databind.deser.std.StringArrayDeserializer.deserialize(StringArrayDeserializer.java:18)
    at com.fasterxml.jackson.databind.deser.SettableBeanProperty.deserialize(SettableBeanProperty.java:538)
    at com.fasterxml.jackson.module.afterburner.deser.SettableObjectMethodProperty.deserializeAndSet(SettableObjectMethodProperty.java:53)
    at com.fasterxml.jackson.module.afterburner.deser.SuperSonicBeanDeserializer.deserializeFromObject(SuperSonicBeanDeserializer.java:226)
    ... 44 more

might be relevant here; not necessarily due to Afterburner (although it is in callpath), but possibly due to changes in StringArrayDeserializer (which I think I did indeed work on for 2.4.0, for some performance reason).

@cowtowncoder
Copy link
Member

Ok yes. I think I found the (regression) bug:

        while ((t = jp.nextToken()) != JsonToken.END_ARRAY) {
            // Ok: no need to convert Strings, but must recognize nulls
            String value;
            if (t == JsonToken.VALUE_STRING) {
                value = jp.getText();
            } else if (t == JsonToken.VALUE_NULL) {
                value = _elementDeserializer.getNullValue();
            } else { .... }

and what happens here is that _elementDeserializer is null (since it is only non-null for custom String
deserializer). I will write a unit test to verify, and fix this.

Depending on rate of bugs found, I may want to push an intermediate micro release (2.4.0-1) sooner, before there are enough bugs to do full 2.4.1 patch release. But first things first, will need to fix this bug and see if there are other problems.

cowtowncoder added a commit that referenced this issue Jun 4, 2014
@cowtowncoder cowtowncoder changed the title Regression updating from 2.3.3 to 2.4.0 Regression updating from 2.3.3 to 2.4.0: null won't deserialize in String[] Jun 4, 2014
@cowtowncoder
Copy link
Member

I fixed the first problem; looks like there may be others (only 2 out of 6 were NPEs), will create new bugs for those as needed.

@cowtowncoder
Copy link
Member

Actually test pass for me locally, after the fix. I checked in the fix -- is it possible for you to build things locally (2.4.1-SNAPSHOT), just to make sure there are no other problems?

Bug itself is unfortunate, but worse part is that neither unit tests caught it, nor various tests I have for benchmarking. I added a regression test for what that is worth, and added that in 2.3.3 as well (where it passes as expected).

@huxi
Copy link
Author

huxi commented Jun 4, 2014

Wow, fast response time!

Just gave the jackson-databind-2.4.1-SNAPSHOT a spin with Lilith and two other rather big closed-source company projects that both use Jackson extensively.

Everything looks fine.

@cowtowncoder
Copy link
Member

Ok good. Thank you for verifying this!

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

No branches or pull requests

2 participants