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

WIP: Implement archived rooms (include_leave filter on /sync) #1671

Closed
wants to merge 26 commits into from

Conversation

MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Dec 23, 2020

Implement archived rooms (include_leave filter on /sync)

http://localhost:8008/_matrix/client/r0/sync?filter={ "room": { "include_leave": true, "timeline": { "limit": 5 } } }

Fix #1323

Dev notes

Internal doc about getting started developing on dendrite: https://docs.google.com/document/d/14USwq1faRk7I2m_LacBUipFGF0-hdrglmiFpiNeGM5o/edit

Internal doc syncing on this PR: https://docs.google.com/document/d/1F3EKyzZL5xOP2GQuqmC6H3BhYESXCLjUEPNEW_dMO94/edit



less +G /usr/local/var/log/postgres.log

$ ./build.sh && ./bin/dendrite-monolith-server --tls-cert server.crt --tls-key server.key --config dendrite.yaml

$ curl -XPOST -d '{"auth":{"type":"m.login.dummy"},"username":"root","password":"foobarbaz123"}' http://localhost:8008/_matrix/client/r0/register
http://localhost:8008/_matrix/client/r0/sync
Authorization: Bearer <access_token>

http://localhost:8008/_matrix/client/r0/sync?filter={ "room": { "include_leave": true, "timeline": { "limit": 5 } } }

Sytest

POSTGRES=1 ./run-tests.pl tests/31sync/09archived.pl -I Dendrite::Monolith -d ../dendrite/bin -W ../dendrite/sytest-whitelist --stop-on-fail
docker run --rm --name sytest-dendrite -v "/Users/eric/Documents/github/sytest:/sytest" \
  -v "/Users/eric/Documents/github/dendrite:/src" \
  -v "/Users/eric/logs:/logs" \
  -v "/Users/eric/go:/gopath" -e "POSTGRES=1" -e "DENDRITE_TRACE_HTTP=1" \
  matrixdotorg/sytest-dendrite:latest tests/31sync/09archived.pl



docker run --rm --name sytest -v "/Users/eric/Documents/github/sytest:/sytest" \
  -v "/Users/eric/Documents/github/synapse:/src" \
  -v "/Users/eric/logs:/logs" \
  -v "/Users/eric/go:/gopath" -e "POSTGRES=1" \
  matrixdotorg/sytest-synapse:py37 tests/31sync/09archived.pl

util.GetLogger(req.Context()).WithFields(log.Fields{
	"filter":        f,
	"IncludeLeave":  *f.Room.IncludeLeave,
	"timelineLimit": timelineLimit,
	"filterQuery":   filterQuery,
	"err":           err,
}).Info("Input filter")


log.WithFields(logrus.Fields{
	"filter":     *filter,
	"filterAsdf": fmt.Sprintf("%+v", *filter),
	"limit1":     filter.Room.Timeline.Limit,
}).Info("waeweafewfafewa Adding rooms to sync response")
dumping the events
type myadsfevent struct {
		event_id, sender, eventType, origin_server_ts, content string
	}

	events := make([]myadsfevent, len(recentStreamEvents))
	for i, v := range recentStreamEvents {
		events[i] = myadsfevent{
			event_id:         v.HeaderedEvent.Event.EventID(),
			sender:           v.HeaderedEvent.Event.Sender(),
			eventType:        v.HeaderedEvent.Event.Type(),
			origin_server_ts: strconv.FormatUint(uint64(v.HeaderedEvent.Event.OriginServerTS()), 10),
			content:          string(v.HeaderedEvent.Event.Content()),
		}
	}

	logrus.Info("sheehrhsegrehrehr")

	spew.Dump(events)
	logrus.WithFields(logrus.Fields{
		"joinEventIndex":     joinEventIndex,
		"leaveEventIndex":    leaveEventIndex,
		"recentStreamEvents": fmt.Sprintf("%+v", events),
	}).Info("cutting down the events")

Todo

  • Handle CompleteSync
  • Handle IncrementalSync
  • Make sure it works with inline filter and filter ID
  • Do we need to worry about This will fail on join -> leave -> sensitive msg -> join -> leave?

Pull Request Checklist

  • I have added any new tests that need to pass to sytest-whitelist as specified in docs/sytest.md
  • Pull request includes a sign off

Signed-off-by: Your Name <your@email.example.org>

@MadLittleMods MadLittleMods added the are-we-synapse-yet This issue or PR involves Sytests in AWSY label Dec 23, 2020
@MadLittleMods MadLittleMods force-pushed the 1323-archived-rooms-sync-left-rooms branch from 88c0656 to 61740f9 Compare December 23, 2020 19:47
@MadLittleMods MadLittleMods force-pushed the 1323-archived-rooms-sync-left-rooms branch 2 times, most recently from ab5da23 to 9a7f6e4 Compare December 25, 2020 04:01
sytest-whitelist Outdated Show resolved Hide resolved
@MadLittleMods MadLittleMods force-pushed the 1323-archived-rooms-sync-left-rooms branch from 225b98c to 634ca4e Compare December 31, 2020 23:17
@MadLittleMods
Copy link
Contributor Author

MadLittleMods commented Jan 4, 2021

@kegsay Can you do a once over review on this to sanity check, make sure I am not doing something the hard way or should go at a different angle?

I just have the CompleteSync part sorted out and need to look into IncrementalSync. The Sytest, Archived rooms only contain history from before the user left, gets a bit further now.

@MadLittleMods MadLittleMods force-pushed the 1323-archived-rooms-sync-left-rooms branch from d6796e1 to 29c697f Compare January 6, 2021 09:33
// TODO: How do we apply filter.Room.State to this as well?
// Is there a generic function where we can pass a list of events and filter and get the result?
// This is based off of Synapse where we derive the state from the resultant timeline events
// https://github.com/matrix-org/synapse/blob/14950a45d6ff3a5ea737322af1096a49b079f2eb/synapse/handlers/sync.py#L791-L795
Copy link
Contributor Author

@MadLittleMods MadLittleMods Jan 15, 2021

Choose a reason for hiding this comment

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

@neilalexander Do you know of a utility function to accomplish this?

Maybe I'm going about this wrong? Should I be doing the filterStreamEventsAccordingToHistoryVisibility filtering in p.DB.RecentEvents(...) call and that will setup the state to come out correct with p.DB.CurrentState(...) as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filtering when we select the events is probably the way to go. Related to #1671 (comment)

@MadLittleMods MadLittleMods force-pushed the 1323-archived-rooms-sync-left-rooms branch from 78f77c0 to fe7acc6 Compare January 15, 2021 08:28
@MadLittleMods
Copy link
Contributor Author

MadLittleMods commented Jan 18, 2021

As an update here, @neilalexander is going to take a look at adding timeline types filtering to d.OutputEvents.SelectRecentEvents(...) to and then we can come back to this one. Adding this is tricky because we also have to support SQLite which has problems with the dynamic array queries.

@MadLittleMods
Copy link
Contributor Author

Type filtering now available thanks to @neilalexander via #1721 🚀

Will merge that in here and see how far I get!

Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

From what I can see, this code will continue to serve live events for rooms the user has left. We need to be limiting the max stream position (types.Range) to the leave event of the user at the very very least. That still isn't ideal since it'll leak events between a join->leave->join->leave in a single request (which if it's a complete sync will be the entire room.

@@ -85,6 +84,28 @@ func (p *PDUStreamProvider) CompleteSync(
}
}

if req.Filter.Room.IncludeLeave {
var leaveRoomIDs []string
Copy link
Member

Choose a reason for hiding this comment

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

Given we're := on :90 this line is spurious.

return recentStreamEvents[sliceStart:sliceEnd], limited
}

func removeDuplicates(stateEvents, recentEvents []*gomatrixserverlib.HeaderedEvent) []*gomatrixserverlib.HeaderedEvent {
Copy link
Member

Choose a reason for hiding this comment

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

It's unclear which slice will have dupes removed from. Please document this.

@kegsay
Copy link
Member

kegsay commented Jun 7, 2021

What's the status on this PR @MadLittleMods ?

@kegsay kegsay added the stale This issue or PR is at risk of being closed without further feedback label Jun 7, 2021
@MadLittleMods
Copy link
Contributor Author

@kegsay It's definitely stale but it's still in my browser tabs.

From #1323 (comment), "Might require better/full history visibility implementation" if we want to handle the join -> leave -> join.

Otherwise, may just need to give this some time again to get it back in shape.

@kegsay
Copy link
Member

kegsay commented Jul 9, 2021

Please re-open this PR against latest master. Until then, closing it so it isn't constantly outstanding on our open PR list.

@kegsay kegsay closed this Jul 9, 2021
@kegsay kegsay deleted the 1323-archived-rooms-sync-left-rooms branch April 6, 2022 13:31
brianathere added a commit to HereNotThere/dendrite that referenced this pull request Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
are-we-synapse-yet This issue or PR involves Sytests in AWSY stale This issue or PR is at risk of being closed without further feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement archived rooms
3 participants