AnnotatedMethod.getValue()/setValue() doesn't have useful exception message

[henryptung]

https://github.com/FasterXML/jackson-databind/blob/d42d345/src/main/java/com/fasterxml/jackson/databind/introspect/AnnotatedMethod.java#L161

InvocationTargetException.getMessage() will always be null. Should probably be using InvocationTargetException.getCause().getMessage() instead.

[cowtowncoder]

Makes sense. Change itself would be easy to make of course, but I think what would be great is a unit test to verify that message from original exception message is retained.
Marking as "good first issue" as well (if no one else does I will eventually get to do it)

[dm20]

@cowtowncoder I'm looking for easy Hacktoberfest issues and may try to take this on - I don't see a specific test suite for AnnotatedMethod - should a new file be created?

[cowtowncoder]

@dm20 You could add tests for AnnotatedMethod itself, but I was more thinking of custom Bean with setter that throws Exception (and/or getter). Then attempt to deserialize would fail with specific wrapped exception (and/or serialization).
Mostly because fundamentally that is the interesting part, end to end handling.
I'll see which package might have similar tests...

[cowtowncoder]

Ok, so... could be under com.fasterxml.jackson.databind.deser, something like DeserizationErrorHandlingTest (for setValue() case); and ``com.fasterxml.jackson.databind.sermaybeSerializationErrorHandlingTest`.
I can refactor tests later on (I rearrange them over time anyway) -- so not too worried about naming.

LMK if this does not make sense or you think you are missing something.

Also -- yes, Hacktoberfest is a good reason!

[dm20]

@cowtowncoder I think I get the gist of what you want, thx for the clarification! I will open a PR shortly.

[cowtowncoder]

Cool!

[dm20]

I'm having some issues running tests, I'm using Intellij - the project is all set up for the most part (it appears I did not have to add any modules or mark directories manually?) but com.fasterxml.jackson.databind.cfg.PackageVersion is not found. do I need to generate this dependency with a script or something?

[cowtowncoder]

ah. Do mvn compile from shell first. For some reason IDEs do not seem to initialize that, it's done via maven plugin. So that's piece that injects version number from pom.xml into PackageVersion class to be exposed via Versioned (so one can programmatically find version of all Jackson components).

Also: can do either against 2.10 branch (it'll need to go there to get in 2.10.1), or master (in which case I can cherry-pick).

[dm20]

oh ok that makes sense - I had just tried mvn install and noticed it ran the tests, leading me to try mvn test..will compile now, thx. I will branch off of 2.10!

[dm20]

perhaps I am lacking the context necessary to write useful tests, but it appears that adding getCause() results in an NPE..but when I run the following "test" without getCause() it throws the expected "Failed to getValue() with method...." message.

public class DeserializationErrorHandlingTest extends BaseTest {

    static class Bean {
        public String a = "a";
        public String b = "b";

        public Bean() { }

        public String getA() {
            return this.a;
        }
    }

    public void testGetValue() throws Exception {
        AnnotatedMethod m = new AnnotatedMethod(null, Bean.class.getMethod("getA"), null, null);
        Bean b = new Bean();
        m.getValue(b); //throws NPE when getCause() is used in AnnotatedMethod, throws normally as is
    }
}

[cowtowncoder]

Maybe above example is incomplete, but it does not have exception triggered from within getA()?

[cowtowncoder]

@dm20 Looking at this I think it's... somewhat more complicated.

@henryptung At this point actual unit test to reproduce the issue would be useful, we have some trouble reproducing issue as reported -- I can see exception message of the original exception correctly.

[cowtowncoder]

@dm20 I think this is probably not as easy as I thought -- will remove tag. You can continue if you want, but I am not assuming you will work on this one.

[dm20]

@cowtowncoder ok, if I have time in the next few days I will continue to work on it. I did make a test case, but am not seeing any change in functionality on thrown exceptions when getCause is used

[cowtowncoder]

@dm20 Sounds good. All I need is something to actually trigger this behavior: number of distinct code paths is big and ones I tried do not actually trigger this handling. But without test I can not verify that fix is working as expected, nor guard against possible regression in future.

[cowtowncoder]

No reproduciton at this point, unfortunately, will close. May be reopened with some kind of reproduction (unit test).

[Stephan202]

Filed a PR that should resolve this issue: #3109.

[cowtowncoder]

@Stephan202 Excellent! Thank you for resolving this; I backported the fix in 2.13 for 2.13.0 release.