EntityManager.clear() should not implicitly invoke the flush operation

• Key: OPENJPA-119
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Kevin Sutter
• Reporter: Kevin Sutter
• Component/s: jpa
• Fix Version/s: 0.9.7

Description

From the dev mailing list...

=======================================
We've noticed that when EntityManager.clear() is invoked, an implicit flush() is performed. Although the spec is cloudy in this area, I don't think this processing is correct. The javadoc is as follows for clear():

/**
* Clear the persistence context, causing all managed
* entities to become detached. Changes made to entities that
* have not been flushed to the database will not be
* persisted.
*/
public void clear();

This indicates that Entities that have not been flushed will not be persisted. Thus, I would say this implies that we should not be doing an implicit flush. If the application wanted their Entities to be flushed before the clear, then they can call the flush() method before calling clear(). We shouldn't be doing this for them because then they have no choice.

The Pro EJB3 Java Persistence API book has similar wording on pages 138-139:

"..In many respects [clear] is semantically equivalent to a transaction rollback. All entity instances managed by the persistence context become detached with their state left exactly as it was when the clear() operation was invoked..."

Our current processing for clear() eventually gets to this code:

public void detachAll(OpCallbacks call) {
beginOperation(true);
try {
if ((_flags & FLAG_FLUSH_REQUIRED) != 0)
flush();
detachAllInternal(call);
} catch (OpenJPAException ke) {
throw ke;
} catch (RuntimeException re) {
throw new GeneralException(re);
} finally {
endOperation();
}
}

Basically, if we have dirtied the Persistence Context, then do a flush() followed by the detachAllInternal(). I don't think the clear() should be doing this flush() operation. Any disagreement?
=======================================

There was no disagreement, thus this JIRA issue.

Comments

[Kevin Sutter]

Contrary to Abe's comments in the dev mailing list, this problem may easily be resolved by just removing the conditional and flush() invocation from the detachAll() method.

After analyzing this further, the detachAllInternal() invocation eventually creates a DetachManager passing in "true" as the second parameter on the constructor. This "true" parameter indicates that any flushing has already been processed, as the following javadoc indicates:

     * @param full whether the entire broker cache is being detached; if
     * this is the case, we assume the broker has already
     * flushed if needed, and that we're detaching in-place

Thus, when we get to the detachInternal method on the DetachManager, the _flushed flag is already set to true and no more flushing is performed.

So, by removing the flush() processing in the detachAll() method on BrokerImpl, it seems to resolve the problem.

Abe had indicated that we still might need this flush processing for JDO. But, since I don't know how JDO is using the kernel, it's kind of difficult for me to determine how to make this work for both cases -- since I don't know the code paths that JDO follows. So, my take is that I will fix the problem for the JPA clear() processing. If this causes a problem with other usages of the kernel, then somebody more familiar with that usage may need to further adjust the code.

Fair? Or, am I missing something?

Kevin

[Abe White]

"But, since I don't know how JDO is using the kernel, it's kind of difficult for me to determine how to make this work for both cases"

JDO needs to flush when detachAll() is called. JPA doesn't. Rather than changing the code out from under JDO, how about adding a boolean to the method. At least then JDO code (and theoretically any other code using the method) will fail to compile. That will allow us (and theoretically anyone else) to see the incompatibility and update our code, rather than having our detach behavior suddenly become incorrect. If we later discover a better way to do it, we can remove the boolean and fix the compile-time problems that will again result -- I'd much rather be getting compile time errors that I'm forced to fix than to have to debug why an obscure unit test or user case suddenly stops working at a later date.

In fact given the new (and correct) clear() behavior, I think we should add an OpenJPAEntityManager.detachAll() method that retains the old behavior, because it is actually useful in some cases. So the EntityManagerImpl would end up using Broker.detachAll with both "true" and "false" flush flags.

[Kevin Sutter]

Sure, I can do the boolean parameter. I just thought a compile failure might be more harsh. But, I can see your point that detecting this change in behavior would be more difficult to detect.

As far as adding methods to OpenJPA public interfaces... Do we need any consensus to do this? Would we break any existing users? Or, isn't that a concern with this young project at this point?

Kevin

[Patrick Linskey]

Generally-speaking, adding new methods should not be a problem for users, although it may be a problem for people writing products that extend OpenJPA.

In general, I think that adding new methods to the OpenJPA published interfaces is something that we should do deliberately. Whether or not that means a vote on the dev list is a good question.

[Kevin Sutter]

Here's what I decided to do... I introduced a new boolean on the detachAll(OpCallbacks, boolean) method, leaving the original detachAll(OpCallbacks) as is. The original method now calls the new method with a value of "true" so that the original processing takes place. I have changed the EM.clear path so that it now calls the new method with a value of "false" so that no flushing takes place. This seemed to be the least amount of churn and still allow both types of flush/clear processing to take place.

I have decided not to introduce a new detachAll method on the OpenJPAEntityManager interface at this time. I personally like to wait for a need for modifying public interfaces, even if it's just for other products extending OpenJPA.

I'll be dropping the changes shortly (after the testing checks out). I have also added a testcase for this scenario.

Kevin

[Kevin Sutter]

Resolved.

[Abe White]

I'm not saying we need to add the new detachAll method, but I don't understand everyone's fear of breaking anything by adding new public methods. It's very hard to break anything by adding a new public method. The only thing you can possibly break is a non-dynamic proxy that implements the interface (OpenJPAEntityManager) without extending the impl (EntityManagerImpl). Anyone layering on top of OpenJPA in that fashion has to realize that their implementation will need updating fairly often.