Project

General

Profile

bug #7874

TeamOrPersonBase entity can become unusable due to replacement of the title caches on using getters

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

Status:
Closed
Priority:
Highest
Category:
cdmlib
Target version:
Start date:
10/26/2018
Due date:
% Done:

100%

Severity:
critical
Found in Version:

Description

This issue is quite good explainable by the case examined in #7870#note-1 :

The initial problem is cause by the protectedNomenclaturalTitleCache flag of the Team being set to false. When binding the data to the UI the getNomenclaturalTitle() is called which causes the the nomenclaturalTitle being updated using the cache strategy. The value stored in the db is now different from the value contained in the nomenclaturalTitle-field:

nomenclaturalTitle in db:

Turland, Wiersema, Barrie, Greuter, D.Hawksw., Herend., S.Knapp, Kusber, D.Z.Li, Marhold, T.W.May, McNeill, A.M.Monro, J.Prado, M.J.Price & Gideon F.Sm.

nomenclaturalTitle in loaded entity:
Turland, Wiersema, Barrie, Greuter, D.Hawksw., Herend., S.Knapp, Kusber, D.Z.Li, Marhold, T.W.May, McNeill, Monro, A.M., J.Prado, M.J.Price & Gideon F.Sm.

The Team entity is loaded as part of the object graph which belongs to the reference. The submitter has no UPDATE permission for this Team. But saving the Reference will cause the Team being flushed to the data base. However the getter has changed the value of the nomenclaturalTitle-field and the user is required to have the UPDATE permission and a PermissionDeniedException is thrown.

One could now point out that the Team is in an inconsistent state since the nomenclaturalTitle differs from the value generated by the cache even if the protectedNomenclaturalTitleCache value is is false. But is seems as if this Team has been created by user henning manually using the Taxeditor (last updated on 2018-04-24 16:52:32). So this is not an import issue.

Either the Taxeditor must never leave Teams in such an inconsistent state or we need to find a way do deal with these situations in the entity class and/or during the flush operation.

This is really critical since it blocks underprivileged users from using object without having the UPDPATE permission.

agents.ods (13.4 KB) Andreas Kohlbecker, 10/26/2018 02:19 PM


Related issues

Copied from Edit - bug #7870: RegistrationWorkingsetEditor: Internal error on clicking "new name" Closed 10/26/2018
Copied to Edit - feature request #8030: Cache updater service methods externalized and base class specific Closed 01/29/2019

Associated revisions

Revision cfcf723b (diff)
Added by Andreas Kohlbecker 12 months ago

fix #7874 updating also the nomenclaturalTitle in updateTitleCacheForSingleEntity()

Revision a55e93b5 (diff)
Added by Andreas Kohlbecker 9 months ago

ref #7874 test for nomenclaturalTitle cache updater and fixing problem in IdentifiableServiceBase
which prevented from running the updater when titleCache was protected

Revision c497656b (diff)
Added by Andreas Müller 8 months ago

ref #7874 implement cache update for additional caches

Revision dfc93007 (diff)
Added by Andreas Müller 8 months ago

ref #7874 added reference to ticket to test

Revision b4be3c3e (diff)
Added by Andreas Kohlbecker 8 months ago

ref #7874 adding missing testcase related to Person nomenclatural title genration

Revision 40a73ef7 (diff)
Added by Andreas Müller 8 months ago

ref #7874 fix incorrect nomTitle test

Revision 694f0ef2 (diff)
Added by Katja Luther 7 months ago

fix #7874: updateCaches always returns true

Revision 2e1fd4da (diff)
Added by Katja Luther 6 months ago

fix #7874: updateCaches always returns true

History

#1 Updated by Andreas Kohlbecker 12 months ago

  • Copied from bug #7870: RegistrationWorkingsetEditor: Internal error on clicking "new name" added

#2 Updated by Andreas Kohlbecker 12 months ago

AM:

ganz so kritisch ist es glaube ich nicht. Letztlich muss man nur den cache updaten mit dem Cacheupdater. Allerdings sollte man sich dabei vorher ganz sicher sein, dass auch alle Daten, die NICHT protected sind richtig atomisiert vorliegen.
Das Problem kommt durch die Umstellung der TitleCaches auf Initials hinter dem Namen. Da die notTitles auch mit dem TitleCache gefüllt werden, wenn keine expliziten nomTitles vorliegen, werden die damit auch geändert.
Eigentlich soll auf allen DBs früher oder später der Cacheupdater laufen. ABER: zumindest von Palmweb weiß ich, dass es da noch Daten gibt, die nicht protected sind und auch nicht atomisiert. Deswegen habe ich das noch verschoben, bis ich Zeit habe, die Daten vorher kurz zu checken.
Wie gesagt, wenn ihr euch bei Phycobank sicher seid, dass alles stimmt, sollte es kein Problem sein.
Ansonsten müssen wir eben möglichst bals mal an das Cache Handling bzgl. Permissions ran wie kürzlich besprochen (mit System Rechten und so)

#3 Updated by Andreas Kohlbecker 12 months ago

  • Assignee changed from Andreas Müller to Andreas Kohlbecker

#4 Updated by Andreas Kohlbecker 12 months ago

The Problem comes from the Person named "A.M. Monroe" which has been entered in the Taxeditor by Henning on 2018-04-24.
He was sure that he set the protectedNomenclaturalTile to FALSE before saving the entity. But the protected flags in the DB are all NULL (see agents.ods). I would expect these field to be filled or is NULL the same as FALSE in the sense of the cdm protected-Flags?

#5 Updated by Andreas Müller 12 months ago

Andreas Kohlbecker wrote:

The Problem comes from the Person named "A.M. Monroe" which has been entered in the Taxeditor by Henning on 2018-04-24.
He was sure that he set the protectedNomenclaturalTile to FALSE before saving the entity. But the protected flags in the DB are all NULL (see agents.ods). I would expect these field to be filled or is NULL the same as FALSE in the sense of the cdm protected-Flags?

Persons do not have a protected flag on this field. Only teams.

#6 Updated by Andreas Kohlbecker 12 months ago

Running the CacheUpdater should fix the issue. But the method IdentifiableServiceBase.updateTitleCacheForSingleEntity() misses updating the nomenclaturalTitle.

#7 Updated by Andreas Kohlbecker 12 months ago

  • Status changed from New to Resolved
  • % Done changed from 0 to 50

#8 Updated by Andreas Kohlbecker 12 months ago

  • Assignee changed from Andreas Kohlbecker to Andreas Müller

please review

#9 Updated by Andreas Müller 9 months ago

  • Description updated (diff)

I don't understand why in line 414

if(!team.isProtectedTitleCache()){
    team.setProtectedNomenclaturalTitleCache(true);
    oldNomenclaturalTitle = team.getNomenclaturalTitle();
    team.setProtectedNomenclaturalTitleCache(false);
}

you test for protectedTitleCache but then you change the protected*Nomenclatural*TitleCache

#10 Updated by Andreas Müller 9 months ago

Generally at the moment I do not fully understand how far we need the lines after line 439. The call

entitiesToUpdate.add(entity);

is not really required I think as the entities all belong to the session already. If not, are the .getTitleCache or .getAbbrevTitleCace calls important here? Or can we completely delete this part. If not I wonder why the calls are alternative, e.g. if olcTitleCache <> newTitleCache the entity instanceof Team part is not called anymore. Is this on purpose?

Also the whole method is only called entity.isProtectedTitleCache() == false which might be unwanted e.g. for Teams. If the titleCache is protected but nomenclaturalTitleCache is not protected we may still want to update it.

Generally we need to refactor this part and move away the specific class handling from IdentifiableServiceBase (e.g. we could move the update routines to the model classes as the knowledge about the model classes is best here)

#11 Updated by Andreas Müller 9 months ago

The above issue might be moved to a new ticket.

#12 Updated by Andreas Müller 9 months ago

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

Also a test might be nice to have to explain a bit more the problem and to make sure that even after reimplementation the given problem will be solved.

There is a very short test for the method in IdentifiableServiceBaseTest already for general testing if the method works basically.

#13 Updated by Andreas Kohlbecker 9 months ago

I implemented a test in AgentServiceImplTest and found a bug in IdentifiableServiceBase which prevented from running the updater for TemOrPersonBase when protectedTitleCache == true, this is fixed, see a55e93b5

All other improvements should go into another issue ....

#14 Updated by Andreas Kohlbecker 9 months ago

Andreas Müller wrote:

I don't understand why in line 414

if(!team.isProtectedTitleCache()){
    team.setProtectedNomenclaturalTitleCache(true);
    oldNomenclaturalTitle = team.getNomenclaturalTitle();
    team.setProtectedNomenclaturalTitleCache(false);
}

you test for protectedTitleCache but then you change the protected*Nomenclatural*TitleCache

fixed in a55e93b5

#15 Updated by Andreas Kohlbecker 9 months ago

  • Copied to feature request #8030: Cache updater service methods externalized and base class specific added

#16 Updated by Andreas Kohlbecker 9 months ago

i suggest to close this issue now

#17 Updated by Andreas Kohlbecker 9 months ago

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

#18 Updated by Andreas Kohlbecker 9 months ago

  • Tags changed from phycobank to phycobank, permission

#19 Updated by Andreas Müller 8 months ago

  • Assignee changed from Andreas Müller to Andreas Kohlbecker

a55e93b5 was a bit to restricted only to Teams. Also it handled Persons not correct (they do not have an additional cache). I updated the code to also be valid for other classes having additional caches. Please review.

#20 Updated by Andreas Kohlbecker 8 months ago

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

would't it be necessary to add the following to TeamOrPersonBase?

    @Override
    public boolean hasUnprotectedCache(){
        return super.hasUnprotectedCache()
                || !isBlank(nomenclaturalTitle);
    }

to make it work in all cases.

The getNomenclaturalTitle() method treats the nomenclaturalTitles if it was unprotected in case it is blank!

#21 Updated by Andreas Kohlbecker 8 months ago

  • Category changed from cdm-vaadin to cdmlib

all commits went into cdmlib-model and cdmlib-service, adapting the category.

#22 Updated by Andreas Kohlbecker 8 months ago

Adding testcase which reproduces the bug mentioned in comment 20 b4be3c3e

IdentifiableServiceBase.updateTitleCacheImpl() fails with this assertion.

#23 Updated by Andreas Müller 8 months ago

I do not understand this line in user tests:

        assertEquals("Expecting nomenclaturalTitle to be set since it was NULL", "Ehrenberg, C.G.", nomenclaturalTitleField.get(ehrenberg));

NomTitle should be "Ehrenb." (same as titleCache) then as if null it is always computed from the titleCache

#24 Updated by Andreas Kohlbecker 8 months ago

You are right! Sorry for this bug in the assertion. Please correct it to :

assertEquals(
  "Expecting nomenclaturalTitle to be set since it was NULL",
  "Ehrenb.", 
   nomenclaturalTitleField.get(ehrenberg));

#25 Updated by Andreas Müller 8 months ago

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

This should be fully fixed now with fixing #8030

#26 Updated by Andreas Kohlbecker 8 months ago

  • Status changed from Resolved to Closed
  • Assignee changed from Andreas Kohlbecker to Andreas Müller
  • % Done changed from 60 to 100

looks like fixed by 100%

#27 Updated by Katja Luther 7 months ago

fixed a small bug in updateCaches of Team.java which results in the result was always true. cdmlib|694f0ef28f3f91c41d59def17430069c806980ec.

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 40 MB)