New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error while saving composite entity having another entity id as null #712

Closed
vtiwari2134 opened this Issue Sep 15, 2018 · 20 comments

Comments

Projects
None yet
2 participants
@vtiwari2134
Contributor

vtiwari2134 commented Sep 15, 2018

Following exception occurs while saving entity with javers auditing enabled in spring -
JaversException ENTITY_INSTANCE_WITH_NULL_ID: Found Entity instance 'com.test.entity.DepartmentEntity' with null idProperty 'id'

I think the issue is with JaversSpringDataAuditableRepositoryAspect.class having aspect on save (@AfterReturning("execution(public * save(..)) && this(org.springframework.data.repository.CrudRepository)")) method. In this the entity is audited on the argument passed to save method but auditing should be performed on return entity that is saved by hibernate.

Issue arises because whenever detached entity is saved to persist hibernate tries to merge the entity creating new entity in hibernate session. Hence the return entity is different from the entity passed to save.
That why the Aspect public void onSaveExecuted(JoinPoint pjp) {
this.onVersionEvent(pjp, this.saveHandler);
}
should evaluate the return entity from save method.

Sample spring project to replicate bug -
test.zip

To replicate issue run JaversExceptionTest test case. This test class contains 2 test one saves entity without having auditing enabled and one with javers auditing enabled.
In first case EmployeeEntity is saved successfully in db while second case throws error : -

JaversException ENTITY_INSTANCE_WITH_NULL_ID: Found Entity instance 'com.test.entity.DepartmentEntity' with null idProperty 'id'

at org.javers.core.metamodel.type.EntityType.getIdOf(EntityType.java:88)
at org.javers.core.metamodel.object.InstanceId.createFromInstance(InstanceId.java:28)
at org.javers.core.metamodel.object.GlobalIdFactory.createId(GlobalIdFactory.java:49)
at org.javers.core.graph.LiveCdoFactory.create(LiveCdoFactory.java:25)
at org.javers.core.graph.LiveCdoFactory.create(LiveCdoFactory.java:10)
at org.javers.core.graph.EdgeBuilder.buildSingleEdge(EdgeBuilder.java:30)</code>
@bartoszwalacik

This comment has been minimized.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Sep 15, 2018

btw, in your test, employeeEntity is a new object not detached one (before hibernate generates Id for it), right?

@vtiwari2134

This comment has been minimized.

Contributor

vtiwari2134 commented Sep 15, 2018

@vtiwari2134 could you please push this test case to github? see https://github.com/javers/javers/tree/master/javers-spring-boot-starter-sql/src/test/groovy/org/javers/spring/sql
Sure, I will push the test cases. Do you want me to push it to above repository or any new repository would work ?

btw, in your test, employeeEntity is a new object not detached one (before hibernate generates Id for it), right?
Yes employeeEntity is a new object for which i am generating a random id and try to save it.

And thanks for quick reply.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Sep 16, 2018

Pull request from your fork of javers repo to this repo would be perfect. Then, I only have to switch to your branch.

@vtiwari2134

This comment has been minimized.

Contributor

vtiwari2134 commented Sep 16, 2018

Created pull request - #713 for this issue.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Sep 16, 2018

thnx

bartoszwalacik added a commit that referenced this issue Sep 17, 2018

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Sep 17, 2018

ok, so the problem is that you have different mapping style for hibernate (fields) and for JaVers (beans).
I have modified your PR to provide better isolation of this issue, see https://github.com/javers/javers/pull/716/files

bartoszwalacik added a commit that referenced this issue Sep 17, 2018

#712
missing @id

bartoszwalacik added a commit that referenced this issue Sep 17, 2018

#712
issue reproduction
@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Sep 17, 2018

After some digging, mapping style is not a problem. I dont konw the reason yet.

@vtiwari2134

This comment has been minimized.

Contributor

vtiwari2134 commented Sep 17, 2018

I triaged a bit and this is what i think -
When we pass entity with id generated manually to save method of JPARepository, it checks whether the entity is new or not.

public <S extends T> S save(S entity) { 
   /*** Check if id is null then persist else merge it */
    if (this.entityInformation.isNew(entity)) {  
      this.em.persist(entity);
      return entity;
    } else {
      return this.em.merge(entity);
    }
  }

Since we are generating id it will try to merge it as per the above code. Now the above entity it will treat as detached from session so it will create new entity in session and will save that with the ids.
So, the entity that we sent in argument for save would be different from the return object.

That why the Aspect public void onSaveExecuted(JoinPoint pjp) { this.onVersionEvent(pjp, this.saveHandler); } should evaluate the return entity from save method not the one that is passed in pjp argument list.

It should more like -

@AfterReturning("execution(public * save(..)) && this(org.springframework.data.repository.CrudRepository)", returning="returnObject")
  public void onSaveExecuted(JoinPoint pjp, Object returnObject) {
   // Check should be performed on returnObject
    this.onVersionEvent(pjp, this.saveHandler);
  }
@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Sep 18, 2018

Thanks for the insight! Would you like to create second PR with the fix?

@vtiwari2134

This comment has been minimized.

Contributor

vtiwari2134 commented Sep 18, 2018

Sure i will create a PR with the fix.

@vtiwari2134

This comment has been minimized.

Contributor

vtiwari2134 commented Sep 19, 2018

PR - #718

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Sep 19, 2018

ok, but tests are failing :) looks like commit isn't called now

@vtiwari2134

This comment has been minimized.

Contributor

vtiwari2134 commented Sep 20, 2018

Sorry, I just tested test cases for employee entity object. I will check this tomorrow, busy with some work. :)

@vtiwari2134

This comment has been minimized.

Contributor

vtiwari2134 commented Sep 23, 2018

@bartoszwalacik
Hi, I have made changes to the aspect, fix all the test cases and raised PR.
PR - #719
Please have a look whether it makes sense.

I have added inline description in test-cases where i felt it is required.
Do tell if more description is required for the changes.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Sep 26, 2018

ok, thanks, I will do the code review

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Sep 29, 2018

@vtiwari2134 thanks for your excellent contribution, release is on it's way to Central

@vtiwari2134

This comment has been minimized.

Contributor

vtiwari2134 commented Sep 29, 2018

@bartoszwalacik Thats great! Happy to contribute. :) Thanks!

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Sep 29, 2018

released in 3.11.6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment