Talk:EclipseLink/Bugs/309681

Discussion

Topics by package

org.eclipse.persistence.testing.tests.wdf.jpa1.entitymanager

TestFlush

testChangedEntityIgnoredByFlush

Tom: I'm not sure this is an error according to the spec, but there is likely an interesting enhancement related to this.

Adrian: This is not mandated by the spec.

testRelationshipToRemoved

Tom: What behavior are you actually seeing? Is the behavior any different when you assign both sides of the relationship? i.e. after emp1.setCubicle(cub1); add cub1.setEmployee(emp1)? At the moment, we expect both sides of bidirectional relationships to be set. It is important for our 2nd level cache. For the most part initial SQL will work without setting both sides, but the behavior after that could become unpredictable. We are considering implementing some behavior to help users that only want to set the owning side of bidirectional relationships, but it is not yet implemented. This is something to watch out for in all your tests.

Adrian: Relationships are assigned on both sides upon commit. The relationship from cubicle to employee is established in the constrcutor of the Cubicle entity. Later, the test deliberately removes the cubicle without nulling out the reference employee->cubicle. The task of the test is to assert that the reference to the removed entity is detected and rejected as mandated by § 3.2.4 <quote>For any entity Y referenced by a relationship from X, where the relationship to Y has not been annotated with the cascade element value cascade=PERSIST or cascade=ALL: If Y is new or removed, an IllegalStateException will be thrown by the flush operation (and the transaction marked for rollback) or the transaction commit will fail.</quote> In EclipseLink, neither flush nor commit fail."

testRelationshipToRemovedLazy

Tom: This test also does not set both sides of a bidirectional relationship. I'm not sure the change would solve the specific issue in the test, but nonetheless, technically an error in EclipseLink.

Adrian: From my point of view it is the same as with testRelationshipToRemoved. EclipseLink seems to validate relationships upon flush according to §3.2.4.

TestPersist

testNastyTimestampTwice

Tom: Spec issue, or Enhancement? If you persist a non-Entity subclass of an Entity, what do you expect to get when you read it back?

Adrian: This is a possible enhancement. The spec quite clearly rejects non-entity subclasses (§2.11.3 of JPA 2.0: <quote>Non-entity classes cannot be passed as arguments to methods of the EntityManager or Query interfaces[24] and cannot bear mapping information. Footnote [24] This includes instances of a non-entity class that extends an entity class.</qoute>.) However, there are implementations that use subclass proxies for lazy loading. We had a discussion on this in the EG and these subclass proxies are considered legal although they techincally a non-entity subclass. The test expects to be able to persist a subclass non-entity and to get back the closest superclass, which is an entity.

Adrian: For the test, it is not relevant that a non-entity subclass is persisted. Instead, the interesting feature of the class persisted is that it has a pre-persist method, which assignes a fixed id. The test makes sure that such an entity cannot be persisted twice and that this issue is detectd already in the persist method. EclipseLink however allows to persist two entities with the same persistent id but different object ids. Eventually, the INSERT will fail with a duprec. I'll rewrite the test so that it does without a non-entity subclass and that it accepts failure upon commit as well.

Adrian: same applies to TestMerge.testNastyTimestampTwice[NotInitial]

testPersistRemovedEntityWithIdModifiedInPrePersistFlushed

Tom: It is not clear to me from the description what the expected behavior is and how that is different from the actual behavior.

Adrian: The test atempts to remove and re-persist the entiy "Timestamp" in the same transaction:

Timestamp timestamp = em.find(Timestamp.class, timestamp.getId());
em.remove(timestamp);
if (removeExecuted) {
    em.flush();
}
try {
    em.persist(timestamp);
    flop("persist succeeded");
} catch (PersistenceException ex) {
    success();
}

The entity "Timestamp" is special as it assigns the id in a pre-persist method to the current time. So, the id will change upon the re-persist. This scenario is not supported in SAP JPA as it would confuse SAP JPA's state management. The test asserts that the scenario is rejected.

Adrian: Tried to turn the test in a positive test but it does not work. EclipseLink does not invoke the pre-persist method upon persist on the removed entity. Looks like a bug to me as pre-persist should be invoked synchronously with the persist operation.

TestReadOnly

testIllegalFieldModification, testQueryCachedEntity and testCacheQueriedEntity

Tom: Possibly a spec interpretation issue?

Adrian: read only entities are not (yet) covered by the spec. To me this appears to be a bug. Or I don't understand the intended behavior of EclipseLink's @ReadOnly annotation.

TestRefresh

testRefreshDeleted

Adrian: The 2.0 spec clearly states that refreshing an entity in the state removed should throw an IllegalArgumentException: §3.2.5 <quote>If X is a new, detached, or removed entity, the IllegalArgumentException is thrown.</quote>. The test needs to be fixed.

testRefreshManagedCheckContains

Tom: seems like a potential issue to me

testRefreshManagedNew

Tom: duprec?

Adrian: The test does the following:

* begin
* persist an entity (schedules it for INSERT)
* insert the same entity using SQL
* refresh // !!
* commit -> duprec''

The test assumes that the refresh operation detects that the entity already exists on the DB, refeshes its content from the DB and transits the entity's state from "scheduled for insert" to "managed". Basically, the asumption is that the explicit refresh should allow to avoid the duprec.

TestRemove

testRemoveFlushRemoveAgain

Tom: Lets discuss this

org.eclipse.persistence.testing.tests.wdf.jpa1.lock

TestLockMethod

testLockOldVersion

Tom: Seems like we should investigate including the causing entity

TestOptimistic

testIllegalVersionAccessNew

Tom: This is not something EclipseLink typically does, but is worth discussing

testNode

Tom: Needs investigating

org.eclipse.persistence.testing.tests.wdf.jpa1.relation

TestRelationshipsWithCache

testBicycleEmployee, ... testMotorVehicleEmployee

Tom: Needs investigating

org.eclipse.persistence.testing.tests.wdf.jpa1.simple

TestBasicPropertyTypes, TestBasicPropertyTypes

testNullsFAint, testNullsFAshort

Tom: Some of these issues likely require fixes. We have a persistence unit property called "eclipselink.allow-zero-id" that may affect the behavior here

Special topics

Persistent identity not assigned by persist?

Adrian: The WDF tests assume that the persistent id is available right after the persist operation. The reasoning for this is deduced from the specification as follows:

§3.2.2 Persisting an entity instance: 
A new entity instance becomes both managed and persistent by invoking the persist method on it.

§3.2 Entity Instance’s Life Cycle
A managed entity instance is an instance with a persistent identity that is currently 
associated with a persistence context</quote>.

In EclipseLink however, the interpretation seems to be that the persist operation ensures that an entity will eventually get a persistent identity upon the flush operation. This means that a generated is available only after the flush operation.

For auto ID columns, EclipseLink's approach has the advantage that the persist operation does not require a transaction.

I consider this a very noteworthy particularity of EclipseLink.

The affected tests should be adjusted, I am afraid.

Adrian: It turns out that if I use a sequence, the id is assigned upon persist.

Optional one-to-one relationship using @PrimaryKeyJoinColumn

Adrian: The one-to-one relationship Course<->Material is marked as optional and mapped using @PrimaryKeyJoinColumn. Although optional, the forward mapper generates a FK constraint on the PK column of Material. Consequently, I can't insert a Course without a Material.

Tests should be altered to always persist a Material with a course. The issue will be isolated in a dedicated test.

Forward mapper maps byte[] to BLOB even if not annotated with @Lob

The forward mapper maps byte arrays as BLOB even if they are not annotated with @Lob. See bug 317597. As a workaround, the affected columns are annotated with

columnDefinition="BINARY (<length>)"

The issue is considered solved as soon as the columnDefinition can be removed.