RepositoryEntityController.saveAndReturn calls AfterSaveEvent with wrong domain object [DATAREST-847]

[spring-projects-issues]

Marc Zampetti opened DATAREST-847 and commented

The saveAndReturn method has the following code to handle the Repository Event logic.

publisher.publishEvent(new BeforeSaveEvent(domainObject));
Object obj = invoker.invokeSave(domainObject);
publisher.publishEvent(new AfterSaveEvent(domainObject));

but I think this is wrong. It is using the provided domainObject for the AfterSaveEvent, instead of the obj returned by the invoker. From what I can tell, the expectation is that the After events should be supplied the object with its current state (i.e. after the change has been applied). This is further born out by the behavior of the createAndReturn method, which uses:

publisher.publishEvent(new BeforeCreateEvent(domainObject));
Object savedObject = invoker.invokeSave(domainObject);
publisher.publishEvent(new AfterCreateEvent(savedObject));

Here it is using the returned object (savedObject in this case). This gets even more confusing in the case of a REST PUT call, since that method first determines if the request should create a new item or update an existing item. Granted, since the contract of PUT is to always supply a complete object, the impact is less then a PATCH case, if one is using Generated fields or has EntityListeners that affect the object, those changes would be hidden to the AfterSaveEvent.

---

Affects: 2.3.2 (Fowler SR2), 2.4.4 (Gosling SR4), 2.5.2 (Hopper SR2)

Backported to: 2.5.3 (Hopper SR3), 2.4.5 (Gosling SR5)

[spring-projects-issues]

Oliver Drotbohm commented

Good catch. Did you just get puzzled looking at the code or did this cause any real issues? Asking because in theory the object returned from the method should be the one handed to it in the first place. A JPA persistence provider is allowed to return a completely different instance bu so far I haven't seen that ever being done.

That said, I'm definitely going ahead to fix this. Would just be interested personally :)

[spring-projects-issues]

Marc Zampetti commented

I discovered looking at the code when researching how to handle a different use case. I haven't identified a known issue caused by this yet. The way I read it, the most likely cause where this could case problems is if you have JPA-level EntityListeners or @Generated field values that would have been modified by the save() call (such as db triggers, etc.). We don't have much of that at the moment, but are planning to add some more soon

[spring-projects-issues]

Oliver Drotbohm commented

That's in place now