PetClinic - JPA, EntityManagerClinic.storeOwner need to use persist() rather than merge(), web.xml need to configure OpenEntityManagerInViewFilter for lazy load, [SPR-3045]

[spring-projects-issues]

Henry Lai opened SPR-3045 and commented

inside the org.springframework.samples.petclinic.webAddOwnerForm.onSubmit method
the owner.getId() is always null!

protected ModelAndView onSubmit(Object command) throws ServletException {
	Owner owner = (Owner) command;
	// delegate the insert to the Business layer
	getClinic().storeOwner(owner);
	return new ModelAndView(getSuccessView(), "ownerId", owner.getId());
}

in org.springframework.samples.petclinic.jpa.EntityManagerClinic

public void storeOwner(Owner owner) throws DataAccessException {
	em.merge(owner);
}

when the owner object is unmanaged, invoking entityManager.merge( owner) return a different manage instance of owner, the id is assigned to the manage instance not the unmanage instance.

changed to

public void storeOwner(Owner owner) throws DataAccessException {
    if ( owner.getId() == null ){
         em.persist(owner);
    } else {
         em.merge(owner);
    }

---

ClinicController

public ModelAndView ownerHandler(HttpServletRequest request, HttpServletResponse response) {
	Owner owner = this.clinic.loadOwner(ServletRequestUtils.getIntParameter(request, "ownerId", 0));
	if (owner == null) {
		return new ModelAndView("findOwnersRedirect");
	}
	return new ModelAndView().addObject(owner);
}

the clinic.loadOwner method only loads the owner, does not load the owner.pets and pet.visits
therefore when the view owner.jsp reference owner.pets, and pet.visits, they are not shown, even though they exist in the DB, but did not get lazy load exception.

after configuring the OpenEntityManagerInViewFilter in web.xml the problem was resolved. This should be documented in the petclinic jpa docs.

---

AddPetForm

protected Object formBackingObject(HttpServletRequest request) throws ServletException {
	Owner owner = getClinic().loadOwner(ServletRequestUtils.getRequiredIntParameter(request, "ownerId"));
	Pet pet = new Pet();
	owner.addPet(pet);
	return pet;
}

the formBackingObject method adds a new pet to the owner object, which cause the pet object to be inserted to the DB, the next time the persistence context is flush, at this point most of the fields in the pet object is null, the flush result in null contraint exception.
The flush was occur inside referenceData method, in getClinic().getPetTypes() resulting in null contraint exception.

changed the code as follows

protected Object formBackingObject(HttpServletRequest request) throws ServletException {
	Owner owner = getClinic().loadOwner(ServletRequestUtils.getRequiredIntParameter(request, "ownerId"));
	Pet pet = new Pet();
	pet.setOwner( owner );
	return pet;
}

---

Affects: 2.0.1

[spring-projects-issues]

Juergen Hoeller commented

The id issue itself is known from Hibernate3's merge operation, where we use an IdTransferringMergeEventListener to explicitly have the id exposed to the non-managed instance as well. This looks like it's simply the same in JPA...

However. I'm puzzled why this specific merge/persist problem wasn't noticed before... After all, we did test PetClinic against TopLink Essentials, Hibernate EntityManager and OpenJPA. Anyway, that should be straightforward to address properly.

With respect to lazy loading: We strived for making PetClinic use eager loading consistently, to avoid the need for an OpenEntityManagerInViewFilter. Maybe we simply missed some specific navigation path there...

Costin, can you please have a look at this?

Juergen

[spring-projects-issues]

Henry Lai commented

sorry, my orm.xml was changed to remove eager fetching at some point!
There is no lazy loading problem when using the original orm.xml

[spring-projects-issues]

Juergen Hoeller commented

OK, thanks for the clarification.

So this leaves the merge/persist problem. I guess we could alternatively simply copy the id over from the Owner instance that merge returns to the one passed into the storeOwner method. That should be safe in that the id is guaranteed to be there even if the persistence provider does not automatically copy it over as part of the merge operation.

The problem with and if-else check for merge/persist usage is the semantics for the passed-in Owner instance: Are change to that instance still monitored, i.e. will they be persisted on transaction commit - even if they are applied after the storeOwner call? With merge, this is not the case, whereas with persist, it is. Mixed merge/persist usage would create unclear semantics for storeOwner callers here. Hence the idea to always use merge and simply copy the id over...

Of course, PetClinic doesn't rely on those semantic subtleties; it would work even with mixed merge/persist usage. However, PetClinic's implementation should serve as a role model, so it's persistence semantics should still be tight.

Juergen

[spring-projects-issues]

Costin Leau commented

I'm going to do some tests this weekend trying to reproduce the problem - I'm not sure why the problem occurs really. I'm going to try more databases just to make sure it's not a jdbc driver.

[spring-projects-issues]

Juergen Hoeller commented

I doubt that this has much to do with the JDBC driver, Costin... I guess it's simple some persistence providers not applying a newly assigned id to the original object that was passed into the "merge" call. Technically, all they have to do is apply the id to the 'managed' instance that they register with the EntityManager, not to the 'non-managed' value object that the application passed in.

Hibernate3 has the same problem at the Hibernate API level: "merge" does not assign the id there either, which is why we introduced our IdTransferringMergeEventListener (which PetClinic's Hibernate3 version relies on).

So I guess TopLink maybe does such an id transfer automatically, but Hibernate EntityManager and OpenJPA may not do it. If that's the case, we could simply reimplement EntityManagerClinic like as follows, transferring the newly assigned id manually:

public void storeOwner(Owner owner) throws DataAccessException {
Owner persistedOwner = em.merge(owner);
owner.setId(persistedOwner.getId());
}

Juergen

[spring-projects-issues]

Costin Leau commented

The fix was to use persist instead of merge. We opted for this alternative to avoid changing the Clinic interface to return the persisted entity which contains the id assigned.
The option of manually flushing the entity manager to get the id didn't work either since depending on various use cases and calls, the first level cache/entity manger might postponed the call until all ends of the relationships were met - by forcing a flush, the db would trigger a FK constraint.