Tidy up integer parsing (#17339)

The parse_integer function was previously made to reject negative values by
default in #16920, but the
documentation stated otherwise. This fixes the documentation and also:

- Removes explicit negative=False parameters from call sites.
- Brings the negative default of parse_integer_from_args in alignment with
  parse_integer.

Author: dkasak

changelog.d/17339.misc

@@ -0,0 +1 @@
+Tidy up `parse_integer` docs and call sites to reflect the fact that they require non-negative integers by default, and bring `parse_integer_from_args` default in alignment. Contributed by Denis Kasak (@dkasak).

synapse/http/servlet.py

@@ -119,14 +119,15 @@ def parse_integer(
         default: value to use if the parameter is absent, defaults to None.
         required: whether to raise a 400 SynapseError if the parameter is absent,
             defaults to False.
-        negative: whether to allow negative integers, defaults to True.
+        negative: whether to allow negative integers, defaults to False (disallowing
+            negatives).
     Returns:
         An int value or the default.
 
     Raises:
         SynapseError: if the parameter is absent and required, if the
             parameter is present and not an integer, or if the
-            parameter is illegitimate negative.
+            parameter is illegitimately negative.
     """
     args: Mapping[bytes, Sequence[bytes]] = request.args  # type: ignore
     return parse_integer_from_args(args, name, default, required, negative)
@@ -164,7 +165,7 @@ def parse_integer_from_args(
     name: str,
     default: Optional[int] = None,
     required: bool = False,
-    negative: bool = True,
+    negative: bool = False,
 ) -> Optional[int]:
     """Parse an integer parameter from the request string
 
@@ -174,15 +175,16 @@ def parse_integer_from_args(
         default: value to use if the parameter is absent, defaults to None.
         required: whether to raise a 400 SynapseError if the parameter is absent,
             defaults to False.
-        negative: whether to allow negative integers, defaults to True.
+        negative: whether to allow negative integers, defaults to False (disallowing
+            negatives).
 
     Returns:
         An int value or the default.
 
     Raises:
         SynapseError: if the parameter is absent and required, if the
             parameter is present and not an integer, or if the
-            parameter is illegitimate negative.
+            parameter is illegitimately negative.
     """
     name_bytes = name.encode("ascii")
 

synapse/rest/admin/federation.py

@@ -61,8 +61,8 @@ def __init__(self, hs: "HomeServer"):
     async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
         await assert_requester_is_admin(self._auth, request)
 
-        start = parse_integer(request, "from", default=0, negative=False)
-        limit = parse_integer(request, "limit", default=100, negative=False)
+        start = parse_integer(request, "from", default=0)
+        limit = parse_integer(request, "limit", default=100)
 
         destination = parse_string(request, "destination")
 
@@ -181,8 +181,8 @@ async def on_GET(
         if not await self._store.is_destination_known(destination):
             raise NotFoundError("Unknown destination")
 
-        start = parse_integer(request, "from", default=0, negative=False)
-        limit = parse_integer(request, "limit", default=100, negative=False)
+        start = parse_integer(request, "from", default=0)
+        limit = parse_integer(request, "limit", default=100)
 
         direction = parse_enum(request, "dir", Direction, default=Direction.FORWARDS)
 

synapse/rest/admin/media.py

@@ -311,8 +311,8 @@ async def on_POST(
     ) -> Tuple[int, JsonDict]:
         await assert_requester_is_admin(self.auth, request)
 
-        before_ts = parse_integer(request, "before_ts", required=True, negative=False)
-        size_gt = parse_integer(request, "size_gt", default=0, negative=False)
+        before_ts = parse_integer(request, "before_ts", required=True)
+        size_gt = parse_integer(request, "size_gt", default=0)
         keep_profiles = parse_boolean(request, "keep_profiles", default=True)
 
         if before_ts < 30000000000:  # Dec 1970 in milliseconds, Aug 2920 in seconds
@@ -377,8 +377,8 @@ async def on_GET(
         if user is None:
             raise NotFoundError("Unknown user")
 
-        start = parse_integer(request, "from", default=0, negative=False)
-        limit = parse_integer(request, "limit", default=100, negative=False)
+        start = parse_integer(request, "from", default=0)
+        limit = parse_integer(request, "limit", default=100)
 
         # If neither `order_by` nor `dir` is set, set the default order
         # to newest media is on top for backward compatibility.
@@ -421,8 +421,8 @@ async def on_DELETE(
         if user is None:
             raise NotFoundError("Unknown user")
 
-        start = parse_integer(request, "from", default=0, negative=False)
-        limit = parse_integer(request, "limit", default=100, negative=False)
+        start = parse_integer(request, "from", default=0)
+        limit = parse_integer(request, "limit", default=100)
 
         # If neither `order_by` nor `dir` is set, set the default order
         # to newest media is on top for backward compatibility.

synapse/rest/admin/statistics.py

@@ -63,10 +63,10 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
             ),
         )
 
-        start = parse_integer(request, "from", default=0, negative=False)
-        limit = parse_integer(request, "limit", default=100, negative=False)
-        from_ts = parse_integer(request, "from_ts", default=0, negative=False)
-        until_ts = parse_integer(request, "until_ts", negative=False)
+        start = parse_integer(request, "from", default=0)
+        limit = parse_integer(request, "limit", default=100)
+        from_ts = parse_integer(request, "from_ts", default=0)
+        until_ts = parse_integer(request, "until_ts")
 
         if until_ts is not None:
             if until_ts <= from_ts:

synapse/rest/admin/users.py

@@ -90,8 +90,8 @@ def __init__(self, hs: "HomeServer"):
     async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
         await assert_requester_is_admin(self.auth, request)
 
-        start = parse_integer(request, "from", default=0, negative=False)
-        limit = parse_integer(request, "limit", default=100, negative=False)
+        start = parse_integer(request, "from", default=0)
+        limit = parse_integer(request, "limit", default=100)
 
         user_id = parse_string(request, "user_id")
         name = parse_string(request, "name", encoding="utf-8")

synapse/rest/client/room.py

@@ -510,7 +510,7 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
             if server:
                 raise e
 
-        limit: Optional[int] = parse_integer(request, "limit", 0, negative=False)
+        limit: Optional[int] = parse_integer(request, "limit", 0)
         since_token = parse_string(request, "since")
 
         if limit == 0:
@@ -1430,16 +1430,7 @@ async def on_GET(
         requester = await self._auth.get_user_by_req(request, allow_guest=True)
 
         max_depth = parse_integer(request, "max_depth")
-        if max_depth is not None and max_depth < 0:
-            raise SynapseError(
-                400, "'max_depth' must be a non-negative integer", Codes.BAD_JSON
-            )
-
         limit = parse_integer(request, "limit")
-        if limit is not None and limit <= 0:
-            raise SynapseError(
-                400, "'limit' must be a positive integer", Codes.BAD_JSON
-            )
 
         return 200, await self._room_summary_handler.get_room_hierarchy(
             requester,

synapse/streams/config.py

@@ -75,9 +75,6 @@ async def from_request(
             raise SynapseError(400, "'to' parameter is invalid")
 
         limit = parse_integer(request, "limit", default=default_limit)
-        if limit < 0:
-            raise SynapseError(400, "Limit must be 0 or above")
-
         limit = min(limit, MAX_LIMIT)
 
         try: