Task #13237

Reconsider nested set implementation in some models

Added by José Raddaoui Marín 9 months ago. Updated 5 months ago.

Status:VerifiedStart date:01/13/2020
Priority:MediumDue date:
Assignee:-% Done:

100%

Category:Data model / ORM
Target version:Release 2.6.0
Google Code Legacy ID: Tested version:
Sponsored:No Requires documentation:No

Description

The following models have parent_id relations and nested set:

- actor
- contact_information (inherited from actor)
- repository (inherited from actor)
- rights_holder (inherited from actor)
- donor (inherited from actor)
- user (inherited from actor)
- digital_object (nested set partially removed)
- function_object
- information_object
- menu
- physical_object
- taxonomy
- term
- acl_group

We should investigate in which models the nested set is actually needed and remove it if it's not. Without being a major performance improvement, it could speed up the deletion of those models and simplify the AtoM code.

burlington-acl.png (68.6 KB) Dan Gillean, 05/29/2020 02:23 PM


Related issues

Related to Access to Memory (AtoM) - Task #13224: Improve hierarchy management queries Verified 12/09/2019
Related to Access to Memory (AtoM) - Task #10306: Improve foreign key declarations in database schema Verified 09/14/2016

History

#1 Updated by José Raddaoui Marín 9 months ago

  • Related to Task #13224: Improve hierarchy management queries added

#2 Updated by José Raddaoui Marín 6 months ago

  • Related to Task #10306: Improve foreign key declarations in database schema added

#3 Updated by José Raddaoui Marín 6 months ago

  • Description updated (diff)

My initial suggestion would be to:

Remove parent_id and nested set from:

- actor
- contact_information
- repository
- rights_holder
- donor
- user
- function_object
- physical_object
- taxonomy

Use only parent_id in:

- digital_object
- acl_group

Use both in:

- information_object
- menu
- term

#4 Updated by José Raddaoui Marín 6 months ago

  • Description updated (diff)

#5 Updated by David Juhasz 6 months ago

One caveat is the ACL module currently sets access permissions for all actors or repositories by setting the access rule on the "root" object. If the actor hierarchy is removed altogether, the ACL code will need to be changed as well.

#6 Updated by José Raddaoui Marín 6 months ago

That's a great point David, thanks!

I'd move them to the "Use only parent_id" category then, with the CTE implementation in the ancestors queries, we could still benefit from the lft/rgt removal in deletions and maintain the ACL code.

#7 Updated by José Raddaoui Marín 5 months ago

  • Status changed from New to Code Review
  • Assignee set to Steve Breker
  • Target version set to Release 2.6.0

Addressed in https://github.com/artefactual/atom/pull/1095 and ready for code review.

Current changes included in that PR in relation to this issue:

- Remove parent_id, lft and rgt columns from:
  - QubitFunctionObject.
  - QubitPhysicalObject.
- Remove lft and rgt columns from:
  - QubitActor (and other models extending this one).
  - QubitAclGroup.
  - QubitTaxonomy.

With this changes, the only models with a full nested set implementation
are QubitInformationObject, QubitTerm and QubitMenu as they require a
method to order siblings. While QubitActor (and related models),
QubitAclGroup and QubitTaxonomy only have a parent_id column, used for
the ACL system alongside CTE in the ancestors queries. Additionally,
the QubitDigitalObject model has only a parent_id column, in this case
to relate the derivatives with their master.

- Modify schema.
- Build SQL.
- Add migration.
- Bump version.
- Add addGetAncestorsAndSelfForAcl to QubitObjectBuilder:
  - Based on the self-relationship using parent_id.
  - Needed for the ACL system to get ancestors in those models that
    don't have a full nested set implementation.
  - Also used for models with nested set as it's more efficient and
    simplifies the code.
  - Uses a join with a CTE and level order to replicate the ordered
    results from `ancestors->andSelf()->orderBy('lft')`.
- Build models.
- Remove calls to non existing methods.
- Remove not needed uses and references of lft and rgt columns.
- Replace QubitActor by QubitMenu in the build nested set task.

#8 Updated by José Raddaoui Marín 5 months ago

  • Status changed from Code Review to QA/Review
  • Assignee changed from Steve Breker to Dan Gillean

Merged in qa/2.6.x.

For testing, I'd suggest to do it alongside #10306. Check the upgrade and the install processes and CRUD operations over the entities mentioned in the previous update.

Additionally, take special attention to the ACL system and if it works properly when setting permissions for all entity types and for specific ones.

#9 Updated by José Raddaoui Marín 5 months ago

  • Requires documentation set to Yes

We should test the build nested set task too, as it's now executed over IOs, terms and menus (instead of actors). Marked as requires documentation as that may require some tweaks in the docs.

#10 Updated by José Raddaoui Marín 5 months ago

  • Requires documentation changed from Yes to No

#11 Updated by Dan Gillean 5 months ago

  • File burlington-acl.png added
  • Status changed from QA/Review to Feedback
  • Assignee changed from Dan Gillean to José Raddaoui Marín

For the most part, things seem to be working well!

I can't tell if this is a pre-existing issue or not, because of all the known problems with the ACL in AtoM. However, I have found the following:

  • Navigate to a user
  • Go to description permissions
  • Add custom permissions for one particular institution - deny most permission (see attached screenshot)
  • Save, then go to browse > archival descriptions

Resulting error

  • Multiple descriptions belonging to other institutions are also missing.
  • In the demo data, when I applied this to Burlington (6 holdings), all of Sudbury (17 holdings) and several others disappeared as well.

Expected result

  • When applying custom permissions per institution, only descriptions linked to that institution are affected.

#12 Updated by José Raddaoui Marín 5 months ago

  • Assignee changed from José Raddaoui Marín to Dan Gillean

Thanks Dan!

I could reproduce this in 2.5.4 (and 2.5.3) with the "Editor" user. After adding the same permissions you mention over the same institution, only 3 top-level descriptions are shown in the browse page. I think this deserves its own ticket.

#13 Updated by Dan Gillean 5 months ago

  • Status changed from Feedback to Verified
  • Assignee deleted (Dan Gillean)
  • % Done changed from 0 to 100

Okay, thanks Radda. I've filed a new issue in #13345 for this.

In that case, I think we can verify this issue!

Also available in: Atom PDF