Duplicate objects in UnitOfWorkImpl.primaryKeyToNewObjects

[TAregger]

When UnitOfWorkImpl#registerNotRegisteredNewObjectForPersist is called, the object is potentially added twice to the primaryKeyToNewObjects map.
First when the assignSequenceNumber method is called (assuming a sequence is used) and a second time when calling registerNewObjectClone (which calls addNewObjectToPrimaryKeyToNewObjects).

When the object is later unregistered, it is only removed once from the map (see removeObjectFromPrimaryKeyToNewObjects).

Subsequent calls on EntityManger.find will still find the remaining object, which should not be the case.

The following code snippet illustrates the behaviour:

  @Test
  public void test() {
    Entity entity = new Entity();
    this.entityManager.persist(entity);
    
    Entity savedEntity = this.entityManager.find(Entity.class, entity.getId());
    this.entityManager.remove(savedEntity);
    
    savedEntity = this.entityManager.find(Entity.class, entity.getId());
    assertThat(savedEntity, nullValue());
  }

The assertion fails, because the object is still in UnitOfWorkImpl.primaryKeyToNewObjects as it was added twice but only removed once.

Version affected: 2.7.13
Last version with expected behaviour: 2.7.12
Change introduced with #1843

[Zuplyx]

Thank you for the report.
I will provide a fix and a unit test for master and 2.7

[patric-r]

I find it both surprising and scary that this rather basic use case seems to be not covered in EclipseLink unit test suite so far
(especially when considering that eclipselink is still JPA's reference implementation).

[Zuplyx]

@rfelcman Could you please advise on where I should put the UnitTest for this?

I think the best place to add the UnitTest would be org.eclipse.persistence.testing.tests.unitofwork where the other UnitOfWork tests are located.
But surprisingly, none of the models used for these tests (package org.eclipse.persistence.testing.models) use sequences.
Should I create new models with sequences or should I add the test to another package?

Also I think we should reconsider whether it is wise to perform the UnitOfWork tests only with persistence units without sequences, when in reality most applications will be using sequences.
Ideally we should refactor these tests to use both persistence units with and without sequences.

[rfelcman]

As fix is related with UnitOfWork location should be https://github.com/eclipse-ee4j/eclipselink/tree/master/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/unitofwork org.eclipse.persistence.testing.tests.unitofwork or maybe in area where PR verification failed. It seems, that fix broke something.
https://github.com/eclipse-ee4j/eclipselink/actions/runs/6351075237/job/17251687925?pr=1951
org.eclipse.persistence.testing.tests.queries.inmemory.UnitOfWorkConformNewObjectTest
In JPA area should be org.eclipse.persistence.testing.tests.jpa.advanced.EntityManagerJUnitTestSuite.
But bug above. I'm not sure that code snipped is really bug as remove() is not synchronized against DB.
I think that Jakarta Persistence Specification 3.1 doesn't say, that Entity is null after removal. Just changed state into DETACHED see specification from 3.2.3. Removal and next chapters.

[Zuplyx]

@rfelcman You are right that the DB is not directly affected, but I still think this is a serious bug.

In the example above, the entity is removed from the persistence context before it is ever written to the DB. But the call to em.find will still return this entity, as it is still contained in the primaryKeyToNewObjects cache. Any subsequent operations based on the returned entity, e.g. performing some action only if em.find is not null, will therefore be wrong. This could then also indirectly lead to wrong data being written to the DB.
Additionally this bug created a difference in the behaviour of em.find between minor versions.

Therefore I think a new release with the fix should be published soon.

[TAregger]

One small addition to avoid any confusion: It seems the code snippet I originally posted is a bit too simple and actually shows the correct behaviour.
This is because UnitOfWorkImpl#getObjectFromNewObjects checks in the first few lines for the presence of new objects to possibly do an early return from that method. If this is not the case, the primaryKeyToNewObjects map is not even checked.
So it's necessary to have at least one other new object in that Unit Of Work. The following modified snippet should be sufficient to reproduce the issue:

  @Test
  public void test() {
    Entity entity = new Entity();
    this.entityManager.persist(entity);

    Entity entity2 = new Entity();
    this.entityManager.persist(entity2);
    
    Entity savedEntity = this.entityManager.find(Entity.class, entity.getId());
    this.entityManager.remove(savedEntity);
    
    savedEntity = this.entityManager.find(Entity.class, entity.getId());
    assertThat(savedEntity, nullValue());
  }

[Zuplyx]

@rfelcman is there a timeline for when the new versions with this fix will be published? Since this is a serious bug, I think fixed versions should be published soon.

[rfelcman]

@rfelcman is there a timeline for when the new versions with this fix will be published? Since this is a serious bug, I think fixed versions should be published soon.

Usually we try to make new release every 6 months, if our capacity will allow it.

[rfelcman]

Latest 2.7.13 was in July 2023, next one should be Dec-Jan.