[PR #5689] [MERGED] Better parallel permission gen handling

## 📋 Pull Request Information **Original PR:** https://github.com/BookStackApp/BookStack/pull/5689 **Author:** [@ssddanbrown](https://github.com/ssddanbrown) **Created:** 7/2/2025 **Status:** ✅ Merged **Merged:** 7/9/2025 **Merged by:** [@ssddanbrown](https://github.com/ssddanbrown) **Base:** `development` ← **Head:** `permission_table_locking` --- ### 📝 Commits (3) - [`add0913`](https://github.com/BookStackApp/BookStack/commit/add091305c0e23face6070f40f52c24e2d0f6df4) Perms: Removed entity perm regen on general update - [`47fd578`](https://github.com/BookStackApp/BookStack/commit/47fd578edb0df2cc1613d7a823da6becc98e1f1a) Perms: Added transactions around permission effecting actions - [`35a5119`](https://github.com/BookStackApp/BookStack/commit/35a51197cee66385f61a0f8616aac3ff232dc089) Perms: Fixed some issues made when adding transactions ### 📊 Changes **15 files changed** (+228 additions, -148 deletions) <details> <summary>View changed files</summary> 📝 `app/Entities/Controllers/BookController.php` (+4 -1) 📝 `app/Entities/Controllers/ChapterController.php` (+4 -1) 📝 `app/Entities/Repos/BaseRepo.php` (+1 -2) 📝 `app/Entities/Repos/BookRepo.php` (+15 -11) 📝 `app/Entities/Repos/BookshelfRepo.php` (+9 -7) 📝 `app/Entities/Repos/ChapterRepo.php` (+20 -15) 📝 `app/Entities/Repos/PageRepo.php` (+33 -25) 📝 `app/Entities/Tools/HierarchyTransformer.php` (+6 -11) 📝 `app/Entities/Tools/TrashCan.php` (+15 -13) 📝 `app/Permissions/JointPermissionBuilder.php` (+16 -20) 📝 `app/Permissions/PermissionsController.php` (+17 -5) 📝 `app/Permissions/PermissionsRepo.php` (+35 -28) 📝 `app/Sorting/BookSortController.php` (+11 -8) 📝 `app/Sorting/BookSorter.php` (+0 -1) ➕ `app/Util/DatabaseTransaction.php` (+42 -0) </details> ### 📄 Description Related to #4838. After review, the initial plan is to generally raise transaction handling up to higher levels so that it wraps the general changes being made which then impact permissions, so conflicting events can then be fully rolled back. ### Permission regen events - [x] Entity creation - [x] Entity copy - [x] Entity delete - [x] Page move - [x] Chapter move - [x] Book sort - [x] Entity Permission update - [x] Book promotion - [x] Chapter promotion - [x] Role creation - [x] Role update - [x] Role delete - [x] ZIP import - [x] API consideration ### Discovery - Pretty sure the default "REPEATABLE READ" transactions would cause issues since, if parallel updates were made, one would be blocked on permission table delete/update, but the latter process would not consider changes commited/made in the earlier process, so it mat not consider read-information that's changed which is used in permission gen. - "READ COMMITTED" sounds like it behaves as needed (new reads post commit from others) ~~but this needs to be confirmed~~. - Tested this in usage, appears to work as desired. - Does not seem to be an easy way to run this in runtime via Eloquent/DB methods. - Seems well supported across DB types but needs testing. - Maybe lower-level locking is better here? Need to assess. ### Testing required - MySQL ISAM - Can't really migrate via ISAM, but converting most tables (including pages/chapters) then testing worked find. - MySQL innodb - Works fine. - MariaDB innodb - Works fine. - ~~MariaDB other?~~ - Loads of others, we'll stick to InnoDB. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

OVERLORD commented

2026-02-05 10:35:15 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/BookStackApp/BookStack/pull/5689
Author: @ssddanbrown
Created: 7/2/2025
Status: ✅ Merged
Merged: 7/9/2025
Merged by: @ssddanbrown

Base: development ← Head: permission_table_locking

📝 Commits (3)

add0913 Perms: Removed entity perm regen on general update
47fd578 Perms: Added transactions around permission effecting actions
35a5119 Perms: Fixed some issues made when adding transactions

📊 Changes

15 files changed (+228 additions, -148 deletions)

View changed files

📝 app/Entities/Controllers/BookController.php (+4 -1)
📝 app/Entities/Controllers/ChapterController.php (+4 -1)
📝 app/Entities/Repos/BaseRepo.php (+1 -2)
📝 app/Entities/Repos/BookRepo.php (+15 -11)
📝 app/Entities/Repos/BookshelfRepo.php (+9 -7)
📝 app/Entities/Repos/ChapterRepo.php (+20 -15)
📝 app/Entities/Repos/PageRepo.php (+33 -25)
📝 app/Entities/Tools/HierarchyTransformer.php (+6 -11)
📝 app/Entities/Tools/TrashCan.php (+15 -13)
📝 app/Permissions/JointPermissionBuilder.php (+16 -20)
📝 app/Permissions/PermissionsController.php (+17 -5)
📝 app/Permissions/PermissionsRepo.php (+35 -28)
📝 app/Sorting/BookSortController.php (+11 -8)
📝 app/Sorting/BookSorter.php (+0 -1)
➕ app/Util/DatabaseTransaction.php (+42 -0)

📄 Description

Related to #4838.

After review, the initial plan is to generally raise transaction handling up to higher levels so that it wraps the general changes being made which then impact permissions, so conflicting events can then be fully rolled back.

Permission regen events

Entity creation
Entity copy
Entity delete
Page move
Chapter move
Book sort
Entity Permission update
Book promotion
Chapter promotion
Role creation
Role update
Role delete
ZIP import
API consideration

Discovery

Pretty sure the default "REPEATABLE READ" transactions would cause issues since, if parallel updates were made, one would be blocked on permission table delete/update, but the latter process would not consider changes commited/made in the earlier process, so it mat not consider read-information that's changed which is used in permission gen.
"READ COMMITTED" sounds like it behaves as needed (new reads post commit from others) ~~but this needs to be confirmed~~.
- Tested this in usage, appears to work as desired.
- Does not seem to be an easy way to run this in runtime via Eloquent/DB methods.
- Seems well supported across DB types but needs testing.
Maybe lower-level locking is better here? Need to assess.

Testing required

MySQL ISAM - Can't really migrate via ISAM, but converting most tables (including pages/chapters) then testing worked find.
MySQL innodb - Works fine.
MariaDB innodb - Works fine.
~~MariaDB other?~~ - Loads of others, we'll stick to InnoDB.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

OVERLORD added the pull-request label 2026-02-05 10:35:15 +03:00

OVERLORD closed this issue

2026-02-05 10:35:18 +03:00

[PR #5689] [MERGED] Better parallel permission gen handling #6547