Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Start using decorations for servlets #8646

Closed

Conversation

ShadowJonathan
Copy link
Contributor

@ShadowJonathan ShadowJonathan commented Oct 24, 2020

Introduces a new decorator: @on (with 5 ease-of-use direct derivatives (@on.get, etc.))

Makes method, path, and client_patterns arguments explicit per-handler

Allows easier placeholder denotation: {argument} instead of (?P<argument>[^/]*)

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
  • Pull request includes a sign off
  • Code style is correct (run the linters)

Signed-off-by: Jonathan de Jong <jonathan@automatia.nl>

@ShadowJonathan
Copy link
Contributor Author

ShadowJonathan commented Oct 24, 2020

I'll keep this PR a draft until i've gotten some feedback from the synapse team on changing over servelet definitions to this kind.

I can easily split this into a PR that adds the decoration and RestServelet changes, and then other ones which starts converting servelets into using them.

Some demonstration

class TestServelet(ClientServlet):
    @on("GET", "/path", v1=True, releases=(0, 1), unstable=False)
    def most_verbose(self, request): ...

    # note that v1 defaults to False in client_patterns
    @on.post("/path/{argument}")
    def with_argument(self, request, argument): ...

    @on.put("/path/{argument_one}/{argument_two}")
    def with_arguments(self, request, argument_one, argument_two): ...

    @on.options("/path/{arg}(?:/.*)?")
    def with_regex(self, request, arg): ...
    
    @on.get({"/path", "/another_path"})
    def more_paths(self, request, arg): ...
    
    @on.get([
        ("/msc/1234/{argument}", {"unstable": True, "v1": False, "releases": []}),
        ("/stable_path/{argument}", dict(unstable=False, v1=True, releases=[1]))
    ])
    def multiple_paths_with_different_prefixes(self, request, argument): ...

The idea here is to make path definition, revision specificity, unstable tag, etc. all nearer and more idiomatic to the actual function.

This echoes practices from modern web frameworks today by making handlers more clear to read and understand from a glance.

@ShadowJonathan
Copy link
Contributor Author

ShadowJonathan commented Oct 24, 2020

(Thinking about it, I just realized I could also dump everything from @servelet straight into RestServelet.register with less fuss, the current method basically monkey-patches the class to do some extra logic. I can probably also put the @on decorator in that module file.)

Edit: (did that, i'll update comments to reflect new situation)

@ShadowJonathan ShadowJonathan changed the title Start using decorations for servelets Start using decorations for servlets Oct 24, 2020
@ShadowJonathan
Copy link
Contributor Author

ShadowJonathan commented Oct 24, 2020

One thought I had while making this: I see that most of these servlets are made to create GET/POST/PUT handlers under one path, but with the addition of these decorators, its possible to consolidate more handlers into less classes, in lesser files, making it possible to have more oversight on those handlers.

I'm also looking at synapse.rest.admin, which uses a different way of laying out the paths, but I think it's possible to make this change easy (by making ClientServlet and AdminServlet, and having each register different prefixes, and accept different kwargs on the @on decorators)
Edit: did this for ClientServlet.
Edit 2: started doing this for AdminServlet on a wip branch.

@ShadowJonathan
Copy link
Contributor Author

Just found out about #5118, i think pre-empting this before changing over to kua, or similar, would work better to change everything smoothely, as the refactor required when doing it in one swoop, as the codebase currently stands, will be a lot of work.

@clokep clokep requested a review from a team October 26, 2020 19:30
@clokep
Copy link
Member

clokep commented Oct 27, 2020

I think there's a couple of issues with this approach and I do not believe that we should pursue it:

  • The current architecture purposefully has separate servlets per endpoint as a way of separating functionality. It is not obvious that changing this to be per-method is an improvement.
  • Building on the above, we use the servlet names to do monitoring and so if you have servlets doing multiple things you start to lose the ability to monitor things. (And yes, this could be overcome, but it is additional changes for questionable value.)
  • Whether using the decorators is easier to use or not is fairly subjective, I personally find that it adds more magic ✨ to an already magical system.

I think that investigating something like #5118 is likely to pay bigger dividends than refactoring our URL registration.

@clokep clokep closed this Oct 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants