Skip to content
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

send 404 as http-status when filter-id is unknown to the server #2380

Merged
merged 12 commits into from Oct 10, 2019

Conversation

@krombel
Copy link
Contributor

krombel commented Jul 20, 2017

This fixed the weirdness of 400 vs 404 as http status code in the case
the filter id is not known by the server.
E.g. matrix-js-sdk expects 404 here to catch this situation.

See the discussion in #matrix-dev:matrix.org

This fixed the weirdness of 400 vs 404 as http status code in the case
the filter id is not known by the server.
As e.g. matrix-js-sdk expects 404 to catch this situation this leads
to unwanted behaviour.
@matrixbot

This comment has been minimized.

Copy link
Member

matrixbot commented Jul 20, 2017

Can one of the admins verify this patch?

2 similar comments
@matrixbot

This comment has been minimized.

Copy link
Member

matrixbot commented Jul 20, 2017

Can one of the admins verify this patch?

@matrixbot

This comment has been minimized.

Copy link
Member

matrixbot commented Jul 20, 2017

Can one of the admins verify this patch?

krombel and others added 3 commits Jul 20, 2017
respond 404 - No such filter with errorcode "M_NOT_FOUND" instead of
404 - No such row with errorcode "M_UNKNOWN" when filter-id is unknown
to the server
@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented May 8, 2018

@matrixbot test this please

krombel and others added 2 commits Jul 13, 2018
@richvdh richvdh added this to In progress in Homeserver Task Board via automation Jun 27, 2019
@richvdh richvdh moved this from In progress to Community PRs in Homeserver Task Board Jun 27, 2019
@richvdh richvdh requested a review from matrix-org/synapse-core Jul 26, 2019
Copy link
Member

richvdh left a comment

the changes to the GET api look ok, but I'm unconvinced by the change to /sync

synapse/rest/client/v2_alpha/sync.py Outdated Show resolved Hide resolved
synapse/rest/client/v2_alpha/filter.py Outdated Show resolved Hide resolved
synapse/rest/client/v2_alpha/filter.py Outdated Show resolved Hide resolved
@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Jul 29, 2019

I can't believe it's taken us over 2 years to land this patch. @anoadragon453 are you able to take it on?

changelog.d/2380.bugfix Outdated Show resolved Hide resolved
@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Oct 3, 2019

I'm fed up with the fact that this trivial PR has been bitrotting for so long, and am going to fix it up and land it.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 3, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (develop@da815c1). Click here to learn what that means.
The diff coverage is 25%.

@@            Coverage Diff             @@
##             develop    #2380   +/-   ##
==========================================
  Coverage           ?   63.31%           
==========================================
  Files              ?      331           
  Lines              ?    36433           
  Branches           ?     6018           
==========================================
  Hits               ?    23069           
  Misses             ?    11722           
  Partials           ?     1642
@richvdh richvdh self-assigned this Oct 4, 2019
@richvdh richvdh merged commit 2efd050 into matrix-org:develop Oct 10, 2019
18 checks passed
18 checks passed
buildkite/synapse Build #4894 passed (21 minutes, 34 seconds)
Details
buildkite/synapse/check-sample-config Passed (1 minute, 48 seconds)
Details
buildkite/synapse/check-style Passed (1 minute, 35 seconds)
Details
buildkite/synapse/isort Passed (15 seconds)
Details
buildkite/synapse/mypy Passed (21 seconds)
Details
buildkite/synapse/newspaper-newsfile Passed (16 seconds)
Details
buildkite/synapse/packaging Passed (24 seconds)
Details
buildkite/synapse/pipeline Passed (3 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-postgres-9-dot-5 Passed (18 minutes, 10 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite Passed (5 minutes, 28 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite-slash-old-deps Passed (14 minutes, 33 seconds)
Details
buildkite/synapse/python-3-dot-6-slash-sqlite Passed (6 minutes, 34 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-11 Passed (17 minutes, 50 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-9-dot-5 Passed (18 minutes, 43 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-sqlite Passed (6 minutes, 39 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-monolith Passed (13 minutes, 41 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-workers Passed (13 minutes, 26 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-sqlite-slash-monolith Passed (14 minutes, 39 seconds)
Details
Homeserver Task Board automation moved this from Community PRs to Done Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.