Project

General

Profile

bug #7709

CdmTransientEntityCacher cannot handle multiple unpersisted entities of the same type

Added by Andreas Kohlbecker 12 months ago. Updated 6 months ago.

Status:
In Progress
Priority:
Highest
Category:
cdmlib
Start date:
08/30/2018
Due date:
% Done:

0%

Severity:
normal
Found in Version:

Description

the following code would fail:


newP1 = Person.NewInstance();
newP2 = Person.NewInstance();

pf1 = new PersonField(newP1);
cache.load(pf1);
pf2 = new PersonField(newP2);
cache.load(pf2);

assert pf1.getValue() != pf2.getValue()

The cacher replaces pf1 with pf2!!!

This is the cause for the problem described in issue #7641


Related issues

Related to Edit - bug #7641: Reference editor: Error when incerting several new authors into a new author team Closed 08/08/2018
Related to Edit - task #7785: CdmUserHelper caches permission information Closed 09/21/2018

Associated revisions

Revision c5eb9820 (diff)
Added by Andreas Müller 12 months ago

ref #7709 use more generics for CdmCacher (not completed yet)

Revision 67ff6af5 (diff)
Added by Andreas Müller 12 months ago

fix #7709 use uuid instead of id for CdmEntityCacheKey

Revision 7b2e142b (diff)
Added by Andreas Müller 12 months ago

ref #7709 use CdmBase constructor instead of class and id

Revision 3c5c2395 (diff)
Added by Andreas Müller 12 months ago

ref #7709 revert using uuid in CdmEntityCacheKey as

Revision 57e761cd (diff)
Added by Andreas Müller 12 months ago

ref #7709 revert using uuid in CdmEntityCacheKey as key part

Revision 7ad7b0fa (diff)
Added by Andreas Müller 12 months ago

ref #7709 cleanup CdmTransientEntityCacher

Revision 7b74ba3d (diff)
Added by Andreas Müller 12 months ago

ref #7709 use id instead of uuid for CdmEntityCacheKey in ConversationalTransientEntityCacher

Revision df75b75c (diff)
Added by Andreas Müller 12 months ago

ref #7709 remove NPE

Revision 9672401c (diff)
Added by Andreas Kohlbecker 10 months ago

ref #7709 JUnit testsuite to reproduce the test failure when running RegistrationIdentifierMinterTest and TaxonGraphHibernateListenerTest sequentially

Revision 8a790929 (diff)
Added by Andreas Kohlbecker 10 months ago

ref #7709 adding REGISTRATION to ClearDBDataSet.xml to solve test problems

History

#1 Updated by Andreas Kohlbecker 12 months ago

  • Related to bug #7641: Reference editor: Error when incerting several new authors into a new author team added

#2 Updated by Andreas Müller 12 months ago

  • Status changed from New to In Progress
  • Priority changed from New to Highest
  • Target version changed from Unassigned CDM tickets to Release 5.3

Is this on purpose assigned to me?

If yes, please give more info what is meant by PersonField. PersonField is not a CdmBase subclass but cacher.load(T) is only defined for T instanceOf CdmBase, therefore I ask.

However, I think the critical issue is that eu.etaxonomy.cdm.cache.CdmEntityCacheKey uses (int)id as identifier, not uuid. While uuid is unchangeable and unique no matter if a CdmEntity is persisted or not the id is neither unchangable (changes when an object is persisted) nor unique (multiple volatile objects with id=0 may exist for the same class). Only when merging data from different sources the uuid may not be unique but this is not necessarily something to be handled by this chacher.

Using the id not the uuid might be the reason for certain Multiple Representation Exceptions in the past.

Disadvantage of using UUID is that it requires more memory but this is probably acceptable as the caches should not become too big usually.

#3 Updated by Andreas Müller 12 months ago

With commit c5eb9820 I introduced more generics to the cacher (not yet 100% finished). Maybe we need to adapt external apps (TaxEditor, Vaadin) afterwards.

#4 Updated by Andreas Müller 12 months ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 50

#5 Updated by Andreas Müller 12 months ago

  • Status changed from Resolved to Feedback
  • Assignee changed from Andreas Müller to Andreas Kohlbecker
  • % Done changed from 50 to 0

Can you please check if this implementation solves your problems and if you generally agree with the implementation?

Before closing this ticket should be reviewed by all developers I guess as it is very core functionality.

#6 Updated by Andreas Kohlbecker 12 months ago

Andreas Müller wrote:

Is this on purpose assigned to me?

You are the default assignee for cdmlib, as this ticket was in "Unassigned CDM" is was not by purpose but automatically. You may want to reconsider being the default assignee.

Using the id not the uuid might be the reason for certain Multiple Representation Exceptions in the past.

There where some other problems in cacher which where definitely causing these issues. Multiple Representation Exceptions will not occur with un-persisted entities. However, there could be situations in which the id has changed which lead to Multiple Representation Exceptions.

Your suggestion to use the uuid instead of the id as key seems like an excellent idea but might need more investigantion and testing

Using the id not the uuid might be the reason for certain Multiple Representation Exceptions in the past.

Disadvantage of using UUID is that it requires more memory but this is probably acceptable as the caches should not become too big usually.

#7 Updated by Andreas Kohlbecker 12 months ago

Andreas Müller wrote:

Can you please check if this implementation solves your problems

My problem has been solved in cdm-vaadin using the cacher for un-persited entities makes not much sense as it can cause problems.
I not 100% sure but I think that the cacher should ignore un-persisted entities.

I am very busy with other tasks and will postpone this discussion for now. But I will come back to this for sure since I am the assignee of this issue now.

#8 Updated by Andreas Kohlbecker 12 months ago

I forgot one thing:

I am a bit frightend about the change you made in the cacher as this is touching an essential core functionality and we need to have a new release next week. It is not very likely but the switch from id to uuid can break existing functionality.

#9 Updated by Andreas Müller 12 months ago

I discussed with Katja the handling of un-persisted objects. She also thinks that unpersisted objects should/are not handled by the cacher. The problem is that the expected behavior and usage of the cacher is not well documented.

The main question is if unintended wrong usage of the cacher may lead to difficult to find errors. This could be prevented by throwing an exception whenever someone tries to add an unpersisted object to cacher. This way it is much easier to find wrong implementations as the error is thrown immediately.

We will need to investigate more on how volatile client side objects that get saved are afterwards replaced by their persistent counterpart. Somehow this happens by Cache.load(recursive=true). But in case this works not 100% save the above described exceptions may appear.

#10 Updated by Andreas Müller 12 months ago

Andreas Kohlbecker wrote:

I forgot one thing:

I am a bit frightend about the change you made in the cacher as this is touching an essential core functionality and we need to have a new release next week. It is not very likely but the switch from id to uuid can break existing functionality.

I agree that we should revert the commit (easy to revert) in case we can't test sufficiently. However, for now I would like to keep it for a moment as I do want to do some tests with the new implementations.
Also I can't really imagine that it creates more problems than the old implementation (but of course you never know).

#11 Updated by Andreas Müller 12 months ago

  • Status changed from Feedback to In Progress
  • Assignee changed from Andreas Kohlbecker to Andreas Müller

I reverted using UUID as key in CdmEntityCacheKey because

  • the eu.etaxonomy.cdm.cache.CdmTransientEntityCacher should only be used for transient (not volatile) entities, these should always have id>0
  • the eu.etaxonomy.taxeditor.remoting.cache.ConversationalTransientEntityCacher transforms CdmEntityIdentifier, which are returned by UpdateResults into CdmEntityCacheKeys, this is not possible if CdmEntityCacheKey uses uuid instead of id (Note: CdmEntityIdentifier and CdmEntityCacheKey seem to do exactly the same and should be unified)
  • even if using UUID were the better choice it still needs intensive testing

#12 Updated by Andreas Müller 12 months ago

  • Subject changed from CdmTransientEntityCacher cannot handle multiple unpersieted entities of the same type to CdmTransientEntityCacher cannot handle multiple unpersisted entities of the same type

#13 Updated by Andreas Müller 11 months ago

  • Target version changed from Release 5.3 to Release 5.4
  • Severity changed from critical to normal

We should think about throwing an exception in the moment when the cacher is used incorrectly (e.g. for volatile objects, but not using the addNewEntity method). Therefore move it to next version and don't close yet.

#14 Updated by Andreas Kohlbecker 10 months ago

  • Blocks task #7785: CdmUserHelper caches permission information added

#15 Updated by Andreas Kohlbecker 10 months ago

Andreas Müller wrote:

  • the eu.etaxonomy.taxeditor.remoting.cache.ConversationalTransientEntityCacher transforms CdmEntityIdentifier, which are returned by UpdateResults into CdmEntityCacheKeys, this is not possible if CdmEntityCacheKey uses uuid instead of id (Note: CdmEntityIdentifier and CdmEntityCacheKey seem to do exactly the same and should be unified)

Note that the CdmTransientEntityCacher currently handles volatile entities, see :


    //map for volatile entities (id=0)
    private final Map<UUID, CdmBase> newEntitiesMap = new HashMap<>();

we would need to remove this map if we want to avoid the CdmTransientEntityCacher handling volatile entities. Before that we need to examine why Cherian added this and if it is needed for some reason!!!

#16 Updated by Andreas Müller 10 months ago

  • Target version changed from Release 5.4 to Release 5.5

#17 Updated by Andreas Kohlbecker 9 months ago

  • Blocks deleted (task #7785: CdmUserHelper caches permission information)

#18 Updated by Andreas Kohlbecker 9 months ago

  • Related to task #7785: CdmUserHelper caches permission information added

#19 Updated by Andreas Müller 6 months ago

  • Target version changed from Release 5.5 to Release 5.6

#20 Updated by Andreas Müller 6 months ago

  • Target version changed from Release 5.6 to Reviewed Next Major Release

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 40 MB)