Repository: matrix-org/synapse

Issue:
Unexpected error code for invalid param on "/_matrix/client/r0/publicRooms" endpoint
<!-- 

**IF YOU HAVE SUPPORT QUESTIONS ABOUT RUNNING OR CONFIGURING YOUR OWN HOME SERVER**: 
You will likely get better support more quickly if you ask in ** #matrix:matrix.org ** ;)


This is a bug report template. By following the instructions below and
filling out the sections with your information, you will help the us to get all
the necessary data to fix your issue.

You can also preview your report before submitting it. You may remove sections
that aren't relevant to your particular case.
-->

### Description

<!-- Describe here the problem that you are experiencing -->

API returns the error code `M_UNKNOWN` instead of `M_INVALID_PARAM` for an invalid parameter in the request.

### Steps to reproduce

Call _listing public rooms_ endpoint with the invalid type of parameter: string instead of integer for the `limit`.

```sh
curl "https://matrix.org/_matrix/client/r0/publicRooms?limit=NOT_INTEGER"
```
#### Expected Result

```json
{
    "errcode": "M_INVALID_PARAM",
    "error": "Query parameter 'limit' must be an integer"
}
```

#### Actual Result

```json
{
    "errcode": "M_UNKNOWN",
    "error": "Query parameter b'limit' must be an integer"
}
```


<!-- 
Describe how what happens differs from what you expected.

If you can identify any relevant log snippets from _homeserver.log_, please include
those (please be careful to remove any personal or private data). Please surround them with
``` (three backticks, on a line on their own), so that they are formatted legibly.
-->

### Version information

<!-- IMPORTANT: please answer the following questions, to help us narrow down the problem -->

<!-- Was this issue identified on matrix.org or another homeserver? -->
- **Homeserver**:  matrix.og


Discussion:

Patch:
diff --git a/changelog.d/5250.misc b/changelog.d/5250.misc
deleted file mode 100644
index 575a299a8..000000000
--- a/changelog.d/5250.misc
+++ /dev/null
@@ -1,1 +0,0 @@
-Simplification to Keyring.wait_for_previous_lookups.
diff --git a/changelog.d/5257.bugfix b/changelog.d/5257.bugfix
new file mode 100644
index 000000000..8334af9b9
--- /dev/null
+++ b/changelog.d/5257.bugfix
@@ -0,0 +1,1 @@
+Fix error code when there is an invalid parameter on /_matrix/client/r0/publicRooms
diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py
index d6ad7f177..eaf41b983 100644
--- a/synapse/crypto/keyring.py
+++ b/synapse/crypto/keyring.py
@@ -180,7 +180,9 @@ class Keyring(object):
 
             # We want to wait for any previous lookups to complete before
             # proceeding.
-            yield self.wait_for_previous_lookups(server_to_deferred)
+            yield self.wait_for_previous_lookups(
+                [rq.server_name for rq in verify_requests], server_to_deferred
+            )
 
             # Actually start fetching keys.
             self._get_server_verify_keys(verify_requests)
@@ -213,11 +215,12 @@ class Keyring(object):
             logger.exception("Error starting key lookups")
 
     @defer.inlineCallbacks
-    def wait_for_previous_lookups(self, server_to_deferred):
+    def wait_for_previous_lookups(self, server_names, server_to_deferred):
         """Waits for any previous key lookups for the given servers to finish.
 
         Args:
-            server_to_deferred (dict[str, Deferred]): server_name to deferred which gets
+            server_names (list): list of server_names we want to lookup
+            server_to_deferred (dict): server_name to deferred which gets
                 resolved once we've finished looking up keys for that server.
                 The Deferreds should be regular twisted ones which call their
                 callbacks with no logcontext.
@@ -230,7 +233,7 @@ class Keyring(object):
         while True:
             wait_on = [
                 (server_name, self.key_downloads[server_name])
-                for server_name in server_to_deferred.keys()
+                for server_name in server_names
                 if server_name in self.key_downloads
             ]
             if not wait_on:
diff --git a/synapse/http/servlet.py b/synapse/http/servlet.py
index 528125e73..197c65285 100644
--- a/synapse/http/servlet.py
+++ b/synapse/http/servlet.py
@@ -55,7 +55,7 @@ def parse_integer_from_args(args, name, default=None, required=False):
             return int(args[name][0])
         except Exception:
             message = "Query parameter %r must be an integer" % (name,)
-            raise SynapseError(400, message)
+            raise SynapseError(400, message, errcode=Codes.INVALID_PARAM)
     else:
         if required:
             message = "Missing integer query parameter %r" % (name,)


Test patch:
diff --git a/tests/crypto/test_keyring.py b/tests/crypto/test_keyring.py
index 4fba462d4..de61bad15 100644
--- a/tests/crypto/test_keyring.py
+++ b/tests/crypto/test_keyring.py
@@ -85,7 +85,7 @@ class KeyringTestCase(unittest.HomeserverTestCase):
         # we run the lookup in a logcontext so that the patched inlineCallbacks can check
         # it is doing the right thing with logcontexts.
         wait_1_deferred = run_in_context(
-            kr.wait_for_previous_lookups, {"server1": lookup_1_deferred}
+            kr.wait_for_previous_lookups, ["server1"], {"server1": lookup_1_deferred}
         )
 
         # there were no previous lookups, so the deferred should be ready
@@ -94,7 +94,7 @@ class KeyringTestCase(unittest.HomeserverTestCase):
         # set off another wait. It should block because the first lookup
         # hasn't yet completed.
         wait_2_deferred = run_in_context(
-            kr.wait_for_previous_lookups, {"server1": lookup_2_deferred}
+            kr.wait_for_previous_lookups, ["server1"], {"server1": lookup_2_deferred}
         )
 
         self.assertFalse(wait_2_deferred.called)