Bring auto-accept invite logic into Synapse

[devonh]

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.

[anoadragon453]

This looks good on the whole! A few minor changes but nothing fundamental.

[anoadragon453]

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?

[devonh]

Relies on #17166

[anoadragon453]

Some very minor requested changes now, plus a small q:

[anoadragon453]

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.

[anoadragon453]

Should this not go in #17166 instead? How come it was moved back to this PR?

[devonh]

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 :)

[erikjohnston]

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?

[devonh]

I'm pretty sure this parcitular lint is related to this issue: matrix-org/synapse#11165
Which seems like quite the can of worms.

[devonh]

The particular error is:
synapse/handlers/sso.py:820: error: Cannot determine type of "store" [has-type]

[devonh]

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.

[reivilibre]

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?

[devonh]

The type is generic & is specified at the following locations:

synapse/synapse/app/homeserver.py

Line 84 in 68dca80

DATASTORE_CLASS = DataStore # type: ignore

synapse/synapse/app/generic_worker.py

Line 166 in 68dca80

DATASTORE_CLASS = GenericWorkerStore # type: ignore

synapse/synapse/app/admin_cmd.py

Line 113 in 68dca80

DATASTORE_CLASS = AdminCmdStore # type: ignore

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.

[MatMaul]

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?

[devonh]

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.
The overall rationale is to bring something that provides generally useful functionality into synapse proper.

This allows us to:

• make deployment easier (as you mentioned)
• make maintenance & testing easier

It's a very fine line as to what should be in a module vs. in synapse itself.
We made the decision that this functionality has become generally useful enough that it should now be moved directly within synapse.

[anoadragon453]

This LGTM, thanks!