Bring auto-accept invite logic into Synapse #17147
Conversation
|
I think it would make sense to archive https://github.com/matrix-org/synapse-auto-accept-invite upon merging this PR, with a note in the README to inform folks that it has been moved in-tree? |
This reverts commit a50d304.
|
Relies on #17166 |
There was a problem hiding this comment.
Indeed, that is one of the downsides. While this code doesn't actually need any private APIs, it is inevitably handy.
I can actually add another downside. While we wouldn't end up adding to Synapse's config if we made this a module... it would beg the question of how we'd actually document the config of this module. We wouldn't be able to put it in https://element-hq.github.io/synapse/latest/usage/configuration/config_documentation.html, unless we added a new section for each in-tree module... and then you've ended up making the user-visible config larger anyhow.
That leads me to think that the only benefit of keeping the modules separate would be if we ever wanted to move them out-of-tree again in future. But I think the times we'll actually do that are minimal. And if we really need to do so, then untangling it from deep within Synapse isn't impossible, just slightly more fiddly.
The initial reason for me suggesting that we keep this code separate is that internal code using the module API felt weird. But after reflection I don't think it's really an issue. It doesn't block us from modifying the API since the code is internal and can change. I also don't believe we have any assumptions in the code that all consumers of the API are external.
So all in all, I'm OK with leaving this code how it is and where it is.
| @@ -817,7 +817,7 @@ def is_allowed_mime_type(content_type: str) -> bool: | |||
| server_name = profile["avatar_url"].split("/")[-2] | |||
| media_id = profile["avatar_url"].split("/")[-1] | |||
| if self._is_mine_server_name(server_name): | |||
| media = await self._media_repo.store.get_local_media(media_id) | |||
| media = await self._media_repo.store.get_local_media(media_id) # type: ignore[has-type] | |||
There was a problem hiding this comment.
Should this not go in #17166 instead? How come it was moved back to this PR?
There was a problem hiding this comment.
haha because when I put it in that PR, the linter complained about an unnecessary ignore....
So rather than fight a circle of nonsense, I feel like it's easier just to keep it here :)
There was a problem hiding this comment.
I'm a bit cautious about introducing more ignore for types without trying to spend some time in understanding what has happened. By the looks of it the type inference has failed somehow, so am wondering if we've managed to introduce a loop in the typing maybe?
There was a problem hiding this comment.
I'm pretty sure this parcitular lint is related to this issue: matrix-org/synapse#11165
Which seems like quite the can of worms.
There was a problem hiding this comment.
The particular error is:
synapse/handlers/sso.py:820: error: Cannot determine type of "store" [has-type]
There was a problem hiding this comment.
For further reference, this is the only instance of using self._media_repo.store outside of the media_repo source.
That probably has something to do with the linter error only showing up here.
Since this code is completely unrelated to the changes in this PR, I suggest we allow the addition of the ignore here until the larger issue of #11165 is brought to a resolution. Adding an ignore keeps things in effectively the same state as before since mypy should have been treating this as an error, but it was silently ignoring it. At least with the ignore added, now the issue is explicit and visible.
There was a problem hiding this comment.
synapse/handlers/sso.py:820: error: Cannot determine type of "store" [has-type] this error sounds like one where we could hopefully add a type annotation instead?
There was a problem hiding this comment.
The type is generic & is specified at the following locations:
synapse/synapse/app/homeserver.py
Line 84 in 68dca80
synapse/synapse/app/generic_worker.py
Line 166 in 68dca80
synapse/synapse/app/admin_cmd.py
Line 113 in 68dca80
All of which also have an explicit type ignore on them.
I can't seem to come up with any annotation that would make mypy happy - maybe someone else knows a way, but this seems like a much deeper issue around DataStore typing.
|
What is the rationale for moving a feature that is easily concealed in a module, and seems to work fine that way, into synapse? Is it mainly because of the deployment burden, with docker for example? |
That is definitely part of the reason. This allows us to:
It's a very fine line as to what should be in a module vs. in synapse itself. |
This PR ports the logic from the synapse_auto_accept_invite module into synapse.
I went with the naive approach of injecting the "module" next to where third party modules are currently loaded. If there is a better/preferred way to handle this, I'm all ears. It wasn't obvious to me if there was a better location to add this logic that would cleanly apply to all incoming invite events.
Relies on #17166 to fix linter errors.