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

JsonBackReference is being set too late; breaks hashCode() #390

Closed
zAlbee opened this issue Jan 17, 2014 · 4 comments
Closed

JsonBackReference is being set too late; breaks hashCode() #390

zAlbee opened this issue Jan 17, 2014 · 4 comments
Milestone

Comments

@zAlbee
Copy link

zAlbee commented Jan 17, 2014

I've mapped a parent-children relationship in Jackson like so:

class Parent {
    @JsonManagedReference
    Set<Child> children; // defaults to HashSet
}

class Child {
    @JsonBackReference
    Parent parent;
}

and I am expecting the Jackson deserializer to properly set the value of Child.parent automatically.

I'm seeing an issue after deserialization of a parent (i.e. JSON to POJO), where set membership tests (children.contains(...)) don't work. This is a HashSet and the Child.hashCode() is implemented such that it uses all properties (including the value of Child.parent). I see that Child.parent is getting set for each child, but only AFTER hashCode() has been called already. Stepping through with a debugger the parent property is null at this point when Jackson calls set.add():

    Child.hashCode() line: 133  
    HashMap<K,V>.hash(Object) line: 351 
    HashMap<K,V>.put(K, V) line: 471    
    HashSet<E>.add(E) line: 217 
    CollectionDeserializer.deserialize(JsonParser, DeserializationContext, Collection<Object>) line: 230    
    CollectionDeserializer.deserialize(JsonParser, DeserializationContext) line: 203    
    CollectionDeserializer.deserialize(JsonParser, DeserializationContext) line: 23 
    MethodProperty(SettableBeanProperty).deserialize(JsonParser, DeserializationContext) line: 375  
    ManagedReferenceProperty.deserializeAndSet(JsonParser, DeserializationContext, Object) line: 101    
    BeanDeserializer.deserializeFromObject(JsonParser, DeserializationContext) line: 308    
    BeanDeserializer.deserialize(JsonParser, DeserializationContext) line: 121  

This is with Jackson 2.1.4.

@cowtowncoder
Copy link
Member

Ok. So the problem here is that the forward reference is set before back ref(s); and for your use case this causes issues. Implementation can be changed, but I would really want the unit test to verify this.
If it resolves the problem and does not break any of regression tests, this can be changed for 2.4 (just feel it bit risky to include in a 2.3.x patch release).

@cowtowncoder
Copy link
Member

Thinking about this bit more, I think that the hashCode() implementation isn't good -- it should not rely on parent; it is asking for trouble.

Regardless, I will change the order of setting property values for 2.4, assuming no test failures occur. But if there are regressions this may need to be revisited, and possibly even reverted.
There is also the possibility of adding a dynamic feature to determine ordering, if we really must.
But let's start with a simple change.

@cowtowncoder cowtowncoder added this to the 2.4.0 milestone Mar 15, 2014
@zAlbee
Copy link
Author

zAlbee commented Mar 17, 2014

You could be right. Basically the scenario is that the child cannot exist without the parent, and we would never move the child to a different parent. But I see your point; if the parent did change, then the hash code would also change, which is bad.

Since filing this bug, we actually changed the way we do hashCode() for that object for other reasons (it's also used by Hibernate, which prefers to calculate hashCode and equals based on a unique id field). So regrettably, I don't have a use for this fix anymore, but thanks anyway. Please don't hesitate to revert if it causes trouble.

@cowtowncoder
Copy link
Member

@zAlbee Thank you for the update. I was guessing fix might not be needed; but in a way it may be slightly better order for things anyway. But it is good to know that reverting would not have significant effects at this point.

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