New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add groundwork for new versions of federation APIs #4390

Merged
merged 2 commits into from Jan 21, 2019

Conversation

Projects
None yet
3 participants
@erikjohnston
Copy link
Member

erikjohnston commented Jan 15, 2019

This will be needed for things like MSC1794 (assuming that some form of it gets accepted)

erikjohnston added some commits Jan 15, 2019

@erikjohnston erikjohnston requested a review from matrix-org/synapse-core Jan 15, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 15, 2019

Codecov Report

Merging #4390 into develop will decrease coverage by <.01%.
The diff coverage is 55%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4390      +/-   ##
===========================================
- Coverage    73.67%   73.66%   -0.01%     
===========================================
  Files          300      300              
  Lines        29815    29817       +2     
  Branches      4897     4897              
===========================================
- Hits         21965    21964       -1     
- Misses        6408     6412       +4     
+ Partials      1442     1441       -1
Impacted Files Coverage Δ
synapse/api/urls.py 85.18% <100%> (+0.56%) ⬆️
synapse/federation/transport/server.py 66.73% <100%> (+0.06%) ⬆️
synapse/federation/transport/client.py 69.19% <50.9%> (ø) ⬆️
synapse/storage/background_updates.py 94.4% <0%> (-1.4%) ⬇️
synapse/handlers/user_directory.py 71.08% <0%> (-0.31%) ⬇️
synapse/handlers/search.py 80.24% <0%> (ø) ⬆️
synapse/handlers/federation.py 61.72% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e41545...1a8f413. Read the comment docs.

@richvdh
Copy link
Member

richvdh left a comment

looks fine. Per my comments I'm not entirely sure there is any value in having the v1 and v2 in constants rather than just treating it as part of the endpoint name, but if you'd rather leave it that's fine.

@@ -286,7 +288,7 @@ def new_func(request, *args, **kwargs):
return new_func

def register(self, server):
pattern = re.compile("^" + PREFIX + self.PATH + "$")
pattern = re.compile("^" + self.PREFIX + self.PATH + "$")

This comment has been minimized.

@richvdh

richvdh Jan 17, 2019

Member

I'm unconvinced that:

    PATH = "/invite/(?P<context>[^/]*)/(?P<event_id>[^/]*)"
    PREFIX = FEDERATION_V2_PREFIX

is clearer than

    PATH = "/v2/invite/(?P<context>[^/]*)/(?P<event_id>[^/]*)"

(in other words: although it's a ballache, would it be better to go through adding /v1 to the starts of all the PATHs?)

path (str): String template for the path
args: ([str]): Args to insert into path. Each arg will be url encoded
Returns:
str
"""
return prefix + path % tuple(urllib.parse.quote(arg, "") for arg in args)
return (
FEDERATION_V1_PREFIX

This comment has been minimized.

@richvdh

richvdh Jan 17, 2019

Member

related to my other comment: we may as well hardcode /v1 here.

@erikjohnston

This comment has been minimized.

Copy link
Member Author

erikjohnston commented Jan 21, 2019

I was vaguely copying it from how we do the CS API. Broadly, there the reason to not encode the version in the path is that some endpoints are shared across multiple versions/releases. I don't know if the same will happen to federation API, but it feels somewhat likely.

Though I guess the way that the CS API does this is to actually just have a PATHS parameter and use helper functions...

@erikjohnston

This comment has been minimized.

Copy link
Member Author

erikjohnston commented Jan 21, 2019

In the interest of moving things along, I'm going to merge this as is. I don't think the current way is particularly bad, though I do take your point about whether its worth it. Let's revisit this when we come to figuring out if we want to cut a full v2/ or r0/ style API

@erikjohnston erikjohnston merged commit 5f54765 into develop Jan 21, 2019

5 checks passed

ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Jan 21, 2019

Let's revisit this when we come to figuring out if we want to cut a full v2/ or r0/ style API

I feel like I have already figured this out: we do not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment