Fix leaking per-room profiles for local users when rebuilding directory #10761
Conversation
- Distinguish between tests of the incremental updates (handler) versus the initial background process (storage) - Add test cases to futher probe that initial background process - Add test case to check that reactivated users are added back to the directory
I don't fully understand why this was needed, but without this I was missing entries from the users table that upcoming changes are about to expect. (Was getting failures to select from the users table because there were missing rows.)
can't see how to correctly configure the homeserver, so let's hack it
Handled in separate PR #10762
DMRobertson
changed the title
| UserDirectoryBackgroundUpdateStore, | ||
| ApplicationServiceWorkerStore, | ||
| RegistrationWorkerStore, |
There was a problem hiding this comment.
Where did this come from/Can you explain? (sounds sarcastic — it's not. I just wonder if this is what we're meant to do. I guess the store is one of those big mixins we were talking about [finally clicked in my head]. I don't know enough about Python multi-inheritance rules — do we do this anywhere else? Will it cause problems?)
There was a problem hiding this comment.
I guess the store is one of those big mixins we were talking about [finally clicked in my head].
DataStore is a class with a lot of parents; dunno if we'd consider that a mixin. (I think of a mixin as something that's designed to be used with multiple inheritance, but isn't really a standalone Thing/Entity by itself.)
But yeah, the DataStore inherits from 9001 other Stores but... they're not all independent. Sometimes they call each other's methods.
Being honest, my motivation is
- make pycharm's "I don't know what this method is because it's on another store" go away
- so that pycharm's control-click to go to definition works
- maybe make mypy happier if this ever gets mypy-ed?
- make the inter-store dependencies explicit
I just wonder if this is what we're meant to do.
erm, pass. I think the status quo is "yeah it sucks, and @erikjohnston has a plan how we could do things better one day"
I don't know enough about Python multi-inheritance rules — do we do this anywhere else? Will it cause problems?)
I had to be careful to order the parent classes, or else Python will fail to create a class because it can't build a consistent MRO.
Will it cause problems?
Was hoping to rely on unit tests to figure that out. Would appreciate input from the rest of the team here.
Having looked over it, I think this these classes get pulled in by the UserDirectoryBackgroundUpdateStore, not the UserDirectoryStore.
| as_user = "@as_user_potato:test" | ||
| self.get_success(self.store.register_user(user_id=as_user, password_hash=None)) | ||
|
|
||
| # TODO can we configure the app service up front somehow? This is a hack. |
There was a problem hiding this comment.
would have thought it should be doable. There are @override_config annotations for the standard config, but not sure what the equivalent is for appservices...
Looks like you have to create files and configure those, see (& copy?) ApplicationServiceStoreTestCase.setUp...
Co-authored-by: reivilibre <38398653+reivilibre@users.noreply.github.com>
|
A lot of test failures. I wonder if this is the result of me messing about with those inheritance hierarchies? I'll investigate |
DMRobertson
force-pushed
the
dmr/user-directory-nickname-leaks-2
branch
from
a54460e
to
cfe7458
Compare
It was down to that change. I found it easiest to reset, manually apply the typing and lint fixes, then force push (sorry). |
|
the SyTests using workers seem deeply sad, probably worth fixing before I pass a review |
|
I think I've seen that set of failures before, see e.g. https://buildkite.com/matrix-dot-org/sytest/builds/1589 (from a completely different branch with no synapse changes). That's a bit suspicious! |
|
Going to close this and chop up into lots of smaller PRs to make it easier to follow. |
Another step to tackle #5677.
I think this series of changes should ensure that rebuilding the user directory doesn't leak per-room profiles for local users. To convince myself of this I added some extra test cases.
I also took the opportunity to try and apply some extra cleanups.