Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

de-MSC-ifying space summaries (MSC2946) #3134

Merged
merged 22 commits into from
Jul 20, 2023
Merged

Conversation

swedgwood
Copy link
Contributor

@swedgwood swedgwood commented Jul 4, 2023

This PR moves and refactors the code for MSC2946 ('Space Summaries') to integrate it into the rest of the codebase.

Solves #3096

Pull Request Checklist

Signed-off-by: Sam Wedgwood sam@wedgwood.dev

Copy link
Collaborator

@devonh devonh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall things are looking really good and in the right place!

clientapi/routing/room_hierarchy.go Outdated Show resolved Hide resolved
clientapi/routing/room_hierarchy.go Outdated Show resolved Hide resolved
roomserver/internal/query/query_room_hierarchy.go Outdated Show resolved Hide resolved
clientapi/routing/room_hierarchy.go Outdated Show resolved Hide resolved
federationapi/api/api.go Outdated Show resolved Hide resolved
roomserver/internal/api.go Show resolved Hide resolved
swedgwood added a commit to matrix-org/gomatrixserverlib that referenced this pull request Jul 13, 2023
- Opt for referring to it as 'room hierarchies', reflecting the route
- Split msc2945.go across clientapi and roomserverapi
- Still TODO fed api
- Some other TODOs still to do
- still need to rename some identifiers from GMSL
- A few commits ago we moved `Queryer` to be init when fed API is set
- This causes Queryer to be nil before it is set
- Causes nil error in `userapi.NewInternalAPI`
- Rearrange to avoid this
@swedgwood
Copy link
Contributor Author

(rebase on main)

S7evinK pushed a commit to matrix-org/gomatrixserverlib that referenced this pull request Jul 13, 2023
Add some constants used in space summaries / room hierarchy (
matrix-org/dendrite#3134 )

### Pull Request Checklist

* [x] Pull request includes a [sign
off](https://github.com/matrix-org/dendrite/blob/master/docs/CONTRIBUTING.md#sign-off)

Signed-off-by: `Sam Wedgwood <sam@wedgwood.dev>`
@swedgwood swedgwood marked this pull request as ready for review July 14, 2023 13:54
@swedgwood swedgwood requested a review from a team as a code owner July 14, 2023 13:54
@swedgwood
Copy link
Contributor Author

swedgwood commented Jul 14, 2023

I've opened this PR up to review now, but just a few things to note:

  • I'm looking at adding tests for this PR to complement, so I haven't fully tested these changes yet (EDIT: the tests already exist, and they're failling, so going to fix that as well) Complement is now passing
  • Because I added room hierarchy querying to the Queryer struct in the roomserver API, and room hierarchy querying may require requests over federation, I moved around the initialisation order, such that Queryer is initialised when SetFederationAPI is called (and gives Queryer a handle to the federation API). However, as creating a new user API with userapi.NewInternalAPI depends on some methods implemented by Queryer, this meant I had to move around some lines of code to make this work. Though, in most of the tests, there is no fsAPI to set, so I added a lot of rsAPI.SetFederationAPI(nil, nil) to make this work. This feels like a bit of a hack, but not sure how else to do it. Do give me your thoughts when reviewing :)
  • I also need to update some names in GMSL, to remove mentions of MSC2946 from some identifiers. However I'm not sure how I should do this without breaking existing versions of dendrite.
  • Also, I've mostly opted for the term 'room hierarchy' instead of 'space summary' to reflect the URL route, but if you think 'space hierarchy' or 'space summary' or some other term would be better, let me know

@codecov
Copy link

codecov bot commented Jul 14, 2023

Codecov Report

Patch coverage: 3.83% and project coverage change: -0.73 ⚠️

Comparison is base (297479e) 65.28% compared to head (31f86e6) 64.55%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3134      +/-   ##
==========================================
- Coverage   65.28%   64.55%   -0.73%     
==========================================
  Files         504      506       +2     
  Lines       56389    56992     +603     
==========================================
- Hits        36814    36794      -20     
- Misses      15770    16390     +620     
- Partials     3805     3808       +3     
Flag Coverage Δ
unittests 49.17% <3.83%> (-0.54%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/dendrite/main.go 61.42% <ø> (-0.81%) ⬇️
federationapi/api/api.go 100.00% <ø> (ø)
federationapi/internal/federationclient.go 53.33% <0.00%> (-0.75%) ⬇️
federationapi/routing/query.go 19.51% <0.00%> (-14.30%) ⬇️
internal/caching/cache_roomservernids.go 71.42% <ø> (ø)
internal/caching/cache_space_rooms.go 0.00% <0.00%> (-50.00%) ⬇️
roomserver/api/api.go 66.66% <0.00%> (-33.34%) ⬇️
roomserver/api/query.go 25.42% <0.00%> (-7.91%) ⬇️
roomserver/internal/query/query.go 58.22% <ø> (ø)
roomserver/internal/query/query_room_hierarchy.go 0.00% <0.00%> (ø)
... and 10 more

... and 21 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

- CSAPI used wrong function to create pagination cache (which caused nil map errs)
- Copied an array wrong in room hierarchy impl, which caused a nil room list to return
- internal room hierarchy function should treat -1 as no limit, it wasnt
docs/FAQ.md Outdated Show resolved Hide resolved
Comment on lines +348 to +351
type DeviceOrServerName struct {
device *userapi.Device
serverName *spec.ServerName
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this type? NewDeviceNotServerName never seems to be used, we only use the servername variant. In which case the code that uses this should just take a spec.ServerName.
Or am I overlooking a use case somewhere? I'd prefer not to have a type like this.

Copy link
Contributor Author

@swedgwood swedgwood Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this type because room hierarchy querying has slightly different authorisation logic depending on whether you call it from the client API or the server API. NewDeviceNotServerName is used in the client api clientapi/routing/room_hierarchy.go. Annoyingly Go doesn't have any concept of sum types so I improvised a bit

cmd/dendrite/main.go Show resolved Hide resolved
- to use new symbols from matrix-org/gomatrixserverlib#403 that don't mention MSC2946
@swedgwood swedgwood force-pushed the swedgwood/space-summaries branch 6 times, most recently from be7fba6 to 51aafae Compare July 19, 2023 15:51
swedgwood added a commit to matrix-org/gomatrixserverlib that referenced this pull request Jul 20, 2023
Remove mentions of MSC2946, as it it part of the spec. (For
matrix-org/dendrite#3134 ).

Signed-off-by: `Sam Wedgwood <sam@wedgwood.dev>`
@swedgwood swedgwood merged commit 9582827 into main Jul 20, 2023
19 of 24 checks passed
@swedgwood swedgwood deleted the swedgwood/space-summaries branch July 20, 2023 14:06
devonh pushed a commit to matrix-org/gomatrixserverlib that referenced this pull request Jul 21, 2023
Add some constants used in space summaries / room hierarchy (
matrix-org/dendrite#3134 )

### Pull Request Checklist

* [x] Pull request includes a [sign
off](https://github.com/matrix-org/dendrite/blob/master/docs/CONTRIBUTING.md#sign-off)

Signed-off-by: `Sam Wedgwood <sam@wedgwood.dev>`
devonh pushed a commit to matrix-org/gomatrixserverlib that referenced this pull request Jul 21, 2023
Remove mentions of MSC2946, as it it part of the spec. (For
matrix-org/dendrite#3134 ).

Signed-off-by: `Sam Wedgwood <sam@wedgwood.dev>`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants