Use glob_to_regex from matrix_common

[reivilibre]

This function has moved to matrix-common since Synapse v1.50.0rc1.

Fixes #173.

We should probably make a note on matrix_common that Mjolnir is using it, in case we ever need to know for compatibility reasons.

[Yoric]

You'll probably want to update the README.md to mention the version requirement for the Synapse module.

[reivilibre]

This PR doesn't change the version requirement (except maybe of Python 3.7, but I'm not sure about that) — matrix-common should still be fine and usable on older Synapse installations. Maybe I misunderstand?

[Yoric]

This PR doesn't change the version requirement (except maybe of Python 3.7, but I'm not sure about that) — matrix-common should still be fine and usable on older Synapse installations. Maybe I misunderstand?

When you wrote that "This function has moved to matrix-common since Synapse v1.50.0rc1", I assumed it meant that the function wasn't there previously. Did I misunderstand?

[reivilibre]

Ah, I see. matrix-common has only just been created to deduplicate code from Synapse, Sydent and Sygnal. Earlier versions of Synapse don't use (or have a dependency on) matrix-common.

So if you install matrix-common alongside Synapse v1.49 or earlier, they will just have independent copies of the function.

[clokep]

Removing my review -- I don't think I can do reviews for this project @reivilibre.

[babolivier]

I'm a bit unsure whether making Mjolnir import matrix-common is the right course of action here. matrix-common is supposed to be common utils between Synapse, Sydent and Sygnal, not a package that Synapse modules (as in, plug-in modules) can use to access these utils - that's supposed to be the module API.

Mjolnir shouldn't have been importing glob_to_regex from synapse.utils in the first place (since that's Synapse internals), instead we should have exposed it in https://github.com/matrix-org/synapse/blob/develop/synapse/module_api/__init__.py, and then have Mjolnir import it from there. Fixing it so it now uses matrix-common feels to me like fixing a symptom instead of the actual issue, and imho the correct way to fix this is to expose glob_to_regex in synapse.module_api (and have Mjolnir import it from there). Otherwise it's going to break again if we end up moving this util somewhere else in the future.

[babolivier]

(as per my comment above)

[reivilibre]

' @babolivier

instead we should have exposed it in matrix-org/synapse@develop/synapse/module_api/init.py

It strikes me as very weird to expose a helper function that is basically completely unrelated to Synapse in the Module API.
I'm not super opposed to it, I suppose, but it would be worth having a little bit more consensus in the team about the best approach here (mostly thinking along the lines of 'we have to draw the line somewhere').

Fixing it so it now uses matrix-common feels to me like fixing a symptom instead of the actual issue

The difference here is that we know matrix-common is a library. Synapse is not; Synapse is an application.
We already will be considering stability when shuffling things around in matrix-common.
Respecting semantic versioning will make all the difference here and I can't quite agree that it's fixing a symptom rather than an issue.
The main concern I do have, though, is that the version requirements borne by Synapse and any modules can conflict (this is no different to any other library that Synapse uses, though). This will however be flagged up at installation time.
(I start to appreciate why Node and Cargo allow installation of multiple versions of packages.)

As a note for completeness, with the right tooling we could have modules vendor their copy of matrix-common if that proved useful (https://pypi.org/project/vendoring/ — what pip uses), though I don't generally like it.

matrix-common is supposed to be common utils between Synapse, Sydent and Sygnal, not a package that Synapse modules (as in, plug-in modules) can use to access these utils

I see what you mean; though I would perhaps like to propose that it would be useful more generally as a library for Matrix.org developers — not suggesting we encourage third parties to use it.

[babolivier]

It strikes me as very weird to expose a helper function that is basically completely unrelated to Synapse in the Module API.

We already expose things like parse_json_object_from_request, DirectServeHtmlResource or DirectServeJsonResource this way which are just Twisted helpers and aren't specifically related to Synapse (but happen to live in the Synapse code base).

I agree that these would probably better fit into a library directed at providing helpers to make the lives of module developers easier. However, I seem to remember we made the explicit decision that matrix-common was to just be providing shared internal utils for Synapse, Sydent and Sygnal. Therefore I tend to consider accessing it from another project similar to if it was importing from Synapse's internals (ie, something it shouldn't do).

I think we need to either decide to make matrix-common a general library any Python developer can use (ie for developing Synapse modules), or create a new library with this purpose. The main difference between the current state of things and this is that by declaring a library (whether it's matrix-common or a new one) safe to use outside of the Sy* projects, we clearly commit to some level of stability wrt its API.

though I would perhaps like to propose that it would be useful more generally as a library for Matrix.org developers — not suggesting we encourage third parties to use it.

I'm a bit uneasy with this proposal. In my opinion, we should have the same capabilities as any other developer when it comes to writing a module, rather than e.g. have our own internal lib others shouldn't use. This is one of the reasons we've been aiming at exposing things via synapse.module_api when we need to use it in a module instead of directly fishing them out of Synapse.

[reivilibre]

I'm a bit uneasy with this proposal. In my opinion, we should have the same capabilities as any other developer when it comes to writing a module, rather than e.g. have our own internal lib others shouldn't use. This is one of the reasons we've been aiming at exposing things via synapse.module_api when we need to use it in a module instead of directly fishing them out of Synapse.

The problem here is that I don't think this currently works out in practice.
If a third party came to us and wanted to modify the module API to expose some (until-then) internal function of Synapse, I feel like we'd be pretty reluctant and would suggest that they just copy-pasted the code into their module.
I would note that glob_to_regex is even further removed from Synapse-specificity than parse_json_object_from_request, DirectServeHtmlResource or DirectServeJsonResource as you mentioned, since they at least are related to mounting servlets inside Synapse.

Putting it into matrix-common still means others can use it; they would just have to be mindful of semantic versioning and update their module as such time came that Synapse's dependency gets a major version bump (as would our own modules).

Not saying that's the definite way we want to go for, but mostly raising it because no one way seems perfect — would like to see what the team thinks (again, not super fussed about one instance, but this is the kind of thing that tends to happen again and again and it'd be nice to have a clear-cut decision on what we're up to).

[babolivier]

If a third party came to us and wanted to modify the module API to expose some (until-then) internal function of Synapse, I feel like we'd be pretty reluctant and would suggest that they just copy-pasted the code into their module.

I'm not sure about that. If there's a use case for it, I don't see why we wouldn't approve the PR.

Putting it into matrix-common still means others can use it; they would just have to be mindful of semantic versioning and update their module as such time came that Synapse's dependency gets a major version bump (as would our own modules).

That's my point though: currently it doesn't. afaict the current state of matrix-common is that nothing should use it outside of the Sy* projects and this library only exists so that we don't duplicate code between these projects. A library with common utils we use that people are encouraged to use with a decent level of stability isn't what I perceive matrix-common to be about, and version pinning alone imo doesn't entirely fix this.

Let's consider a concrete example: one day we decide to move glob_to_regex somewhere else. Maybe it's to a different library, maybe it's back to Synapse's internals, maybe we even decided to go another way and get rid of it entirely. A e.g. module developer isn't immediately impacted by this change because they pinned the dependency.

But let's imagine that later we introduce a critical security fix into matrix-common. Or we bump Synapse's dependency on matrix-common to a version that's more recent than the one the module has pinned. Suddenly the module developer needs to update the pin on the dependency immediately, otherwise their module is vulnerable or doesn't work at all. What's the path from there? There won't be any clear one in the state things currently are, because we don't expect people to be using it outside of the Sy* projects.

We currently advertise and maintain matrix-common as "Common utilities for Synapse, Sydent and Sygnal". This, to me, means that if we need to make such a change we won't necessarily advertise it, because we are the ones maintaining all of the projects it's supposed to be used for, so we'll just deal with it and call it a day. Making it usable by a wider audience means that we need to commit to creating migration paths for projects outside of it's initial scope, think about how we ship security fixes (not just for Sy* but for the library as well), how we handle backwards compatibility layers that we don't necessarily use in Sy*, etc.

To me the way we currently maintain and advertise matrix-common doesn't allow this kind of stability, because afaict we maintain and advertise it as an internal library other projects shouldn't use rather than a public-facing one.

If we want to make matrix-common more than "Common utilities for Synapse, Sydent and Sygnal", we need to make an explicit decision about it, and change the way it's perceived both internally and externally. Saying people can just use it outside of Sy* would be the same as saying they can import stuff directly from e.g. Synapse's internals, in that it would be unstable and could break at any point, and fixing the breakage could be very non-trivial if you're not already familiar with the library or projects' code base (which most people aren't). imo this isn't a nice thing to do to people trying to develop things on Matrix/around our projects.

[clokep]

I haven't fully followed the conversation in here, but I think having matrix-common internal, at least for now, is best. I think the way forward is:

• Pin the version and have mjolnir use it in a not-quite-allowed way.
• Import and re-export it from Synapse.
• Copy it into mjolnir.

Of those, I think the last two are more reasonable.

[Yoric]

So, what's the word? Should we move towards landing this?

[babolivier]

Yes we should probably move towards resolving this situation.

3. Copy it into mjolnir.

I think that's the best solution considering the current state of things.

[Yoric]

Closed in favor of #218 .