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

Resolving forward references of collections might result in corrupt behaviour. #1546

Open
NielsDoucet opened this issue Mar 7, 2017 · 4 comments

Comments

@NielsDoucet
Copy link

NielsDoucet commented Mar 7, 2017

We have a curious case where removing items from a HashSet is no longer possible, if the set is deserialized by jackson using the bean deserializer (v2.8.6).

Testcase illustrating the issue:
DeserializationTest.java.txt
Note that testRemovalOfNestedSetElementsAfterDeserialization fails.

The main issue is the fact that we initialize the id field of the abstract base class with a random UUID through the constructor. This constructor is used by jackson to instantiate the objects, which is correct behaviour.
What I think goes wrong is related to the following code fragment:
https:/HeadOnAPlate/jackson-databind/blob/master/src/main/java/com/fasterxml/jackson/databind/deser/impl/ObjectIdValueProperty.java#L90-L95
I think it binds the item too soon, instead of waiting until the deserialization of the id value has fully completed, which in our case would overwrite the id on the current instance in the deserialization-context.
This bind then adds the instance to the collection being deserialized, using the hashcode of the random id value instead of the proper id value.
Any contains/remove check on the set will fail, as the calculated hashcode is different from the one used when adding the item to said set, and thus the bucket where the object should reside within the hashsets backing hashmap is missed (although it could randomly succeed based on pure luck putting both the old hashcode and the new hashcode inside the same bucket).

I did not test, but updating the idProp before trying to resolve it seems like the intuitive way to fix this.

I understand that initializing the id with a random value and relying on the setId later on to 'fix' the actual id value while also using the id as our hashcode, isn't clean
But as @JsonCreator doesn't work well within class hierarchies, this seems like the cleanest code for our model.

@EAnushan
Copy link

EAnushan commented Dec 6, 2017

I'm experiencing a similar issue with sets of elements that contain back references to their parents. The hashCode and equals methods of the child elements include the parent.

At first, I thought it should have been resolved by #390, but I still seem to be experiencing the issue. My issue goes away when I remove both the JsonBackReference and JsonManagedReference annotations.

@EAnushan
Copy link

EAnushan commented Dec 7, 2017

I took a look at ManagedReferenceProperty.setAndReturn(). I'm not sure if the fix there actually resolves the issue. The value parameter is already a collection, in my case, a HashSet. _backProperty.set(ob, instance) updates the instance while it is part of the collection, thereby possibly modifying its hashCode and breaking the HashSet contract.

Basically, this piece of code is very likely to break collection contracts, since we're iterating a collection and changing its values:

for (Object ob : (Collection<?>) value) {
    if (ob != null) { _backProperty.set(ob, instance); }
}

Perhaps, as per regular convention in these cases, we should be removing the element from the collection, updating it, then re-inserting it?

@MartinHaeusler
Copy link

MartinHaeusler commented Feb 28, 2018

I just ran into exactly this issue. I have a JSON that looks like this:

{
    orders: [
        {
            id: "first-order",
            items: ["1", "2", "3"]
        },
        {
            id: "second-order",
            items: ["3","2"]
        }
    ],
    products: [
        {
            id: "1",
            name: "Apple"
        },
        {
            id: "2",
            name: "Cherry"
         },
         {
             id: "3",
             name: "Strawberry"
         }
    ]
}

The corresponding classes (sketched):

public class Root {
   private HashSet<Order> orders;
    @JsonIdentityInfo(
        property = "id",
        generator = ObjectIdGenerators.PropertyGenerator.class
    )
    private HashSet<Product> products;
}

public class Order {
    private String id = UUID.randomUUID().toString(); // ensure we always have an ID to avoid NullPointerException
    @JsonIdentityReference(alwaysAsId = true)
    private HashSet<Product> items;   
    
    public int hashCode() { /* based on this.id */ }
    public boolean equals(Object other) { /* based on this.id and other.id */}
}

public class Product {
    private String id  = UUID.randomUUID().toString(); // ensure we always have an ID to avoid NullPointerException
    private String name;

    public int hashCode() { /* based on this.id */ }
    public boolean equals(Object other) { /* based on this.id and other.id */}
 }

The above example serializes and deserializes correctly (provided that I didn't make a mistake in anonymizing it...), except that the entries of the Order#items hash set are inserted with an invalid hash code. The hash code of the Product elements was calculated before the id field was assigned to the Product in question.

TL;DR I bumped into exactly this issue today, and I was scratching my head for quite a while why oder.items.contains(myProduct) returned false on the deserialized object, but order.items.stream().filter(e -> e.equals(myProduct).findAny().isPresent() returns true. A debugging session revealed that a Product was inserted into a HashSet before it's id field was set up. Afterwards, the id field was changed, which is why contains resulted in false.

@catta
Copy link

catta commented Mar 27, 2019

I've got into same issue: deserialize a parent with a HashSet of children. The child hashcode is based on parent and parent hashcode is not based on children.

As a context, this issue surfaced when we create integration tests for rest end point. For example, for an edit end-point we load a parent object, change some properties and than send it to edit end-point. The end point return the object changed. The test validate by reflection that all attributes of sent object are equals with the attributes of the returned object. The equals of HashSet use the containsAll method and contains method use the hashcode of the look-up value.

I isolated the issue in a unit test that is attached: DeserializeTest.java.txt. The test also prove that the child is added to parent HashSet before the parent attribute is set.

I test it with version 2.9.8.

@cowtowncoder cowtowncoder added 2.10 and removed 2.9 labels Sep 12, 2019
@cowtowncoder cowtowncoder added 2.12 and removed 2.10 labels Apr 12, 2020
@cowtowncoder cowtowncoder added 2.13 and removed 2.12 labels Oct 27, 2020
@cowtowncoder cowtowncoder added 2.14 and removed 2.13 labels Jul 15, 2021
@cowtowncoder cowtowncoder removed the 2.14 label Mar 2, 2024
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

5 participants