Project

General

Profile

Actions

bug #7874

closed

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

Added by Andreas Kohlbecker over 4 years ago. Updated almost 4 years ago.

Status:
Closed
Priority:
Highest
Category:
cdmlib
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
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.


Files

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

Related issues

Related to EDIT - bug #9683: Auto-initialize root beanClosedAndreas Müller

Actions
Related to EDIT - bug #9684: OccurrenceServiceImpl.listRootUnitDTOsByAssociatedTaxon() - [UPDATE] not permitted for 'anonymousUser'ClosedAndreas Kohlbecker

Actions
Related to EDIT - feature request #9664: Add nomenclaturalTitleCache to TeamOrPersonBaseResolvedAndreas Müller

Actions
Copied from EDIT - bug #7870: RegistrationWorkingsetEditor: Internal error on clicking "new name"ClosedAndreas Kohlbecker

Actions
Copied to EDIT - feature request #8030: Cache updater service methods externalized and base class specificClosedAndreas Müller

Actions
Actions #1

Updated by Andreas Kohlbecker over 4 years ago

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

Updated by Andreas Kohlbecker over 4 years 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)

Actions #3

Updated by Andreas Kohlbecker over 4 years ago

  • Assignee changed from Andreas Müller to Andreas Kohlbecker
Actions #4

Updated by Andreas Kohlbecker over 4 years 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?

Actions #5

Updated by Andreas Müller over 4 years 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.

Actions #6

Updated by Andreas Kohlbecker over 4 years ago

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

Actions #7

Updated by Andreas Kohlbecker over 4 years ago

  • Status changed from New to Resolved
  • % Done changed from 0 to 50
Actions #8

Updated by Andreas Kohlbecker over 4 years ago

  • Assignee changed from Andreas Kohlbecker to Andreas Müller

please review

Actions #9

Updated by Andreas Müller about 4 years 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

Actions #10

Updated by Andreas Müller about 4 years 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)

Actions #11

Updated by Andreas Müller about 4 years ago

The above issue might be moved to a new ticket.

Actions #12

Updated by Andreas Müller about 4 years 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.

Actions #13

Updated by Andreas Kohlbecker about 4 years 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 ....

Actions #14

Updated by Andreas Kohlbecker about 4 years 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

Actions #15

Updated by Andreas Kohlbecker about 4 years ago

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

Updated by Andreas Kohlbecker about 4 years ago

i suggest to close this issue now

Actions #17

Updated by Andreas Kohlbecker about 4 years ago

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

Updated by Andreas Kohlbecker about 4 years ago

  • Tags changed from phycobank to phycobank, permission
Actions #19

Updated by Andreas Müller about 4 years 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.

Actions #20

Updated by Andreas Kohlbecker about 4 years 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!

Actions #21

Updated by Andreas Kohlbecker almost 4 years ago

  • Category changed from cdm-vaadin to cdmlib

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

Actions #22

Updated by Andreas Kohlbecker almost 4 years ago

Adding testcase which reproduces the bug mentioned in comment 20 b4be3c3e

IdentifiableServiceBase.updateTitleCacheImpl() fails with this assertion.

Actions #23

Updated by Andreas Müller almost 4 years 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

Actions #24

Updated by Andreas Kohlbecker almost 4 years 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));
Actions #25

Updated by Andreas Müller almost 4 years 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

Actions #26

Updated by Andreas Kohlbecker almost 4 years 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%

Actions #27

Updated by Katja Luther almost 4 years ago

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

Actions #28

Updated by Andreas Müller over 1 year ago

  • Related to bug #9683: Auto-initialize root bean added
Actions #29

Updated by Andreas Müller over 1 year ago

  • Related to bug #9684: OccurrenceServiceImpl.listRootUnitDTOsByAssociatedTaxon() - [UPDATE] not permitted for 'anonymousUser' added
Actions #30

Updated by Andreas Müller over 1 year ago

Actions

Also available in: Atom PDF