Tidy up integer parsing

[dkasak]

During https://github.com/element-hq/synapse/pull/16920, we seem to have inadvertently disallowed negative integers in parse_integer by default. This was contrary to the parse_integer documentation.

Surprisingly, nothing really broke, which indicates that things mostly do expect positive integers. Running an ast-grep query confirms it:

❯ ast-grep run --pattern 'parse_integer($A, $PARAM, $$$)' --json=pretty | jq -r '.[].metaVariables.single.PARAM.text' | sort -u
"before_ts"
"from"
"from_token"
"from_ts"
"height"
"limit"
"size_gt"
"timeout"
"timeout_ms"
"ts"
"upto_token"
"width"

As can be seen, these boil down to timestamps, limits, timeouts, sizes, etc, none of which really make sense to be negative. So it seems we've inadvertently stumbled into

This PR:

• Brings the docs into alignment with the reality of the situation.
• Removes some dead validation code from the C2S /hierarchy handler

• Brings the default of parse_integer_from_args into alignment, by defaulting to disallowing negatives. This is a behaviour change, but that function is only directly called with the default in federation handlers for the following endpoints/params:

  • limit for https://spec.matrix.org/v1.10/server-server-api/#get_matrixfederationv1publicrooms
  • limits for https://spec.matrix.org/v1.10/server-server-api/#get_matrixfederationv1backfillroomid
  • ts for https://spec.matrix.org/v1.10/server-server-api/#get_matrixfederationv1timestamp_to_eventroomid

  All of these are meant to be non-negative.

One remaining questionable thing is that due to the previous change in default, some code which used to return M.BAD_JSON now returns M.INVALID_PARAM instead, due to the validation being done within parse_integer instead of directly in the caller. One such example is the aforementioned C2S /hierarchy endpoint. Is this a problem? The spec doesn't mention which error code should be returned in this case and there were obviously no checks for this. Is there a general principle on whether M.BAD_JSON or M.INVALID_PARAM should be returned on a disallowed (but well formed) value within a JSON body?

Pull Request Checklist

• [x] Pull request is based on the develop branch
• [x] 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.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• [x] Code style is correct (run the linters)

[erikjohnston]

One remaining questionable thing is that due to the previous change in default, some code which used to return M.BAD_JSON now returns M.INVALID_PARAM instead, due to the validation being done within parse_integer instead of directly in the caller. One such example is the aforementioned C2S /hierarchy endpoint. Is this a problem? The spec doesn't mention which error code should be returned in this case and there were obviously no checks for this. Is there a general principle on whether M.BAD_JSON or M.INVALID_PARAM should be returned on a disallowed (but well formed) value within a JSON body?

None of these are JSON are they? They all look like query parameters?

[dkasak]

Uhh, you're right. The previous error code was wrong then, which caused my thinko. Even better, then this fixes the error codes too.