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

Problem serializing JsonMappingException due to addition of non-ignored processor property #1368

Closed
jecaplan opened this issue Sep 11, 2016 · 6 comments
Milestone

Comments

@jecaplan
Copy link

jecaplan commented Sep 11, 2016

I notice that, e.g., JsonMappingException has several bidirectional links in internal fields like _processor.

Why not annotate those fields with @JsonIgnore? You've already got the Java serialization "transient" on them.

One symptom of not eating your own dog food here is that AWS SWF Flow Framework is broken. If it attempts to deserialize the result of an activity and that attempt fails with a JsonMappingException, the framework then attempts to serialize the resulting DataConverterException via Jackson...which fails with a different JsonMappingException (StackOverflowError) because of the embedded JsonMappingException.

I see no reason why a serde framework should not be able to serde itself.

@cowtowncoder
Copy link
Member

Since there is transient (and since field is marked as protected) it seems superfluous to also add @JsonIgnore: former alone should prevent usage. If this is not enough for some reason, I am not against adding annotations; and certainly exceptions produced should be Jackson-serializable.
In perfect world deserialization should also work, although that is bit trickier due to usually polymorphic nature of exception hierarchies.

If there is an actual problem (which sounds like there is), a reproducible test case would be helpful, especially to guard against future regressions. But also to make sure I understand the specific problem.

@jecaplan
Copy link
Author

jecaplan commented Sep 12, 2016

This example is with Jackson 2.8.0 I believe (whatever the latest AWS SDK is using).

public class TestJackson {
    public static class NoSerdeConstructor {
        private String strVal;
        public String getVal() { return strVal; }
        public NoSerdeConstructor( String strVal ) {
            this.strVal = strVal;
        }
    }
    @Test
    public void test() throws IOException {
        ObjectMapper mpr = new ObjectMapper();

//      // fails: shouldn't this pass?
//      assertFalse( mpr.canDeserialize( mpr.constructType( NoSerdeConstructor.class ) ) );

        try {
            mpr.readValue( "{ \"val\": \"foo\" }", NoSerdeConstructor.class );
        } catch ( JsonMappingException excCannotConstructInstance ) {
            // throws a different JsonMappingException: Infinite recursion
            mpr.writeValueAsString( excCannotConstructInstance );
        }
    }
}

@cowtowncoder
Copy link
Member

cowtowncoder commented Sep 13, 2016

Thank you! I'll see if it occurs with 2.7 as well (most likely does); original description makes more sense with code now.

Note on canDeserialize: test is approximate and can not currently catch all problem cases. This particular case should be possible to detect (since there are no alternate means to construct), but JsonDeserializer API does not have means to expose the problem as is. The only cases that would report "can not deserialize" are the ones where either no deserializer is found for type, or construction of deserializer fails with exception. So in practice neither canSerialize() nor canDeserialize() is particular useful.

@cowtowncoder
Copy link
Member

Ah yes. You are right about need for @JsonIgnore: I forgot that transient keyword does not (by default) propagate to matching getter.

Quite odd that this wasn't reported earlier for 2.7, but better late than never.

@cowtowncoder cowtowncoder added this to the 2.7.8 milestone Sep 13, 2016
@cowtowncoder cowtowncoder changed the title Jackson exceptions should themselves be Jackson-serializable Problem serializing JsonMappingException due to addition of non-ignored processor property Sep 13, 2016
@jecaplan
Copy link
Author

I have a pull request with regression test open if that's of any use to you.

On Sep 12, 2016 10:46 PM, "Tatu Saloranta" notifications@github.com wrote:

Ah yes. You are right about need for @JsonIgnore: I forgot that transient
keyword does not (by default) propagate to matching getter.

Quite odd that this wasn't reported earlier for 2.7, but better late than
never.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1368 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAM9dCvdMG1K_Yv_wHr8sDINWogK58USks5qpjicgaJpZM4J6EKy
.

@cowtowncoder
Copy link
Member

@jecaplan it is indeed.

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