fix(spaces): Correct hierarchy traversal #2134
No reviewers
Labels
No labels
Abandoned
Blocked
Bug
Changelog
Added
Changelog
Missing
Changelog
None
Cherry-picking
Database
Dependencies
Dependencies/Renovate
Difficulty
Easy
Difficulty
Hard
Difficulty
Medium
Documentation
Enhancement
Good first issue
Help wanted
Inherited
Matrix/Administration
Matrix/Appservices
Matrix/Auth
Matrix/Client
Matrix/Core
Matrix/E2EE
Matrix/Federation
Matrix/Hydra
Matrix/MSC
Matrix/Media
Matrix/T&S
Merge
Merge/Manual
Merge/Squash
Meta
Meta/CI
Meta/Packaging
Priority
Blocking
Priority
High
Priority
Low
Security
Status
Confirmed
Status
Duplicate
Status
Invalid
Status
Needs Investigation
Support
bob the builder
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
continuwuation/continuwuity!2134
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "eleboucher/continuwuity:fix/space-hierarchy"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Fixes space hierarchy pagination, depth limits, child ordering, and non-space child traversal. Results now conform to the stable space hierarchy API.
Pull request checklist:
mainbranch, and the branch is named something other thanmain.myself, if applicable. This includes ensuring code compiles.
@ -26,0 +23,4 @@let max_depth = body.max_depth.unwrap_or(max_max_depth).min(max_max_depth);let limit = body.limit.map_or(DEFAULT_LIMIT, usize_from_ruma);if limit == 0 {return Err!(Request(InvalidParam("limit must be greater than zero.")));I don't see a requirement in the spec that says it has to be greater than zero, only that it can't be negative:
@ -60,2 +84,4 @@impl crate::Service for Service {fn build(args: crate::Args<'_>) -> Result<Arc<Self>> {let config = &args.server.config;let cache_capacity = f64::from(config.roomid_spacehierarchy_cache_capacity)This is calculated wrong afaict (no parallelism scale), and that config field was supposed to be removed anyway
25f63330894f0969887c@ -71,6 +99,7 @@ impl crate::Service for Service {state_cache: args.depend::<rooms::state_cache::Service>("rooms::state_cache"),timeline: args.depend::<rooms::timeline::Service>("rooms::timeline"),},hierarchy_cache: LruCache::new(usize_from_f64(cache_capacity)?.max(1)).into(),When is this cache invalidated? If the space hierarchy changes, will cached pages be saved forever?
@ -293,0 +334,4 @@room_id: OwnedRoomId,max_depth: UInt,suggested_only: bool,from: Option<&str>,The lack of pagination support in the current logic is intentional. There's no sensible way to handle changes to the space hierarchy during pagination, and most spaces are not large enough for it to be beneficial regardless.
4f0969887ca3da271c77@ -169,2 +173,2 @@.map(|child| SpaceChild { room_id: child.state_key, via: child.content.via }).collect();.collect();children.sort();the double allocation to do a sort here bothers me but I guess it's fine