paulcwarren/spring-content

LockingAndVersioningRepositoryImpl seems to be shared between multiple repositories but caches EntityInformation

mdond opened this issue · 2 comments

mdond commented

Describe the bug
We have some problems with multiple repositories that implement the LockingAndVersioningRepository (v2.8.0):
It seems that the implementation LockingAndVersioningRepositoryImpl is shared between the different repositories. However EntityInformation is cached in the save-Method (LockingAndVersioningRepositoryImpl:135):

     @Override
     @Transactional
     public <S extends T> S save(S entity) {
         if (entityInformation == null) {
             this.entityInformation = this.entityInfo.getEntityInformation(entity.getClass(), em);
         }
         ...
     }

This results in problems when using a repository of a different entity. Could this be a bug?

Some more info: We use 2 repositories - one with an entity using JPA-optimistic locking - and one with an entity without JPA-optimistic locking. The cached entity information assumes JPA-optimistic locking and throws an error when working with the entity without JPA-optimistic locking.

To Reproduce
We have an automatic test which reproduces the problem but it uses to much of our infrastructure to hand it over. However it should be possible to reproduce the problem by using locking/versioning-repositories for 2 different entities, one with JPA-optimistic locking (@Version), one without. Save the JPA-optimistic locking entity first and then the other entity.

Expected behavior
Both entity-classes should be able to be persisted with their corresponding LockingAndVersioning-repositories.

Additional context
At the moment we use a patched version of the LockingAndVersioningRepositoryImpl which enables our automatic tests:

public class LockingAndVersioningRepositoryImpl<T, ID extends Serializable>
		implements LockingAndVersioningRepository<T, ID> {
        ...
	private Map<Class<?>, EntityInformation<T, ?>> entityInformationMap = new ConcurrentHashMap<>();
        ...
	@Override
	@Transactional
	public <S extends T> S save(S entity) {
		EntityInformation<T, ?> entityInformation = getEntityInformation(entity);
        ...
	protected EntityInformation<T, ?> getEntityInformation(T entity) {
		EntityInformation<T, ?> entityInformation = this.entityInformationMap.get(entity.getClass());
		if (entityInformation == null) {
			entityInformation = this.entityInfo.getEntityInformation(entity.getClass(), em);
			this.entityInformationMap.put(entity.getClass(), entityInformation);
		}
		return entityInformation;
	}
}

The method findAllVersionsLatest() seems not to be implementable as the entity context isn't known:

	@Override
	public <S extends T> List<S> findAllVersionsLatest() {
		logger.warn("Unknown entity context.  Try findAllVersionsLatest(Class<S> entityClass)");
		return new ArrayList<>();
	}

Hi @mdond ,

Thanks for the bug report and suggested fix. I'll need to take a closer look but from what you describe of the behavior that looks like an appropriate fix at first glance.

The findAllVersionsLatest() method is actually deprecated in favor of findAllVersionsLatest(Class<?> entityClass). I really didn't want to add the overloaded variant but I was struggling with the lack of context in the implementation of the former (as you can see in the code). Given the framework has moved on since I implemented this its possible there is enough context provided by the fragment variant to re-instate the original. I can look. But anyways, atm, that original version is not called by the rest layer. You can safely ignore it in your tests for now. Unless there is a reason why you need it?

mdond commented

Hi @paulcwarren

indeed we don't use findAllVersionsLatest(). Just wanted to mention the method as I don't see a way to implement it. The problem we stumbled upon was the caching of entityInformation.