Unconfirmed: Got response from sending SMS with no location header exception raised every time we send an SMS

[DMRobertson]

Mypying spotted the following problem. But I can't see this behaviour in Sentry. That is odd. I don't understand the discrepancy and I'd like to.

In the following snippet:

• getAllRawHeaders yields (bytes, List[bytes]). AFAICS it's done this since late 2015.

• So headers: Dict[bytes, List[bytes]].

• Therefore "X-Request-Id in headers" is False .

• Therefore request_id is None and we never log a request ID.

• Additionally, "Location" not in headers is True.

• Therefore we should always raise on line 142.

sydent/sydent/sms/openmarket.py

Lines 109 to 157 in 1f27a2d

	headers = dict(resp.headers.getAllRawHeaders())
	request_id = None
	if "X-Request-Id" in headers:
	request_id = headers["X-Request-Id"][0]
	# Catch errors from the API. The documentation says a success code should be 202
	# Accepted, but let's be broad here just in case and accept all 2xx response
	# codes.
	#
	# Relevant OpenMarket API documentation:
	# https://www.openmarket.com/docs/Content/apis/v4http/send-json.htm
	if resp.code < 200 or resp.code >= 300:
	if response_body is None or "error" not in response_body:
	raise Exception(
	"OpenMarket API responded with status %d (request ID: %s)"
	% (
	resp.code,
	request_id,
	),
	)
	error = response_body["error"]
	raise Exception(
	"OpenMarket API responded with status %d (request ID: %s): %s"
	% (
	resp.code,
	request_id,
	error,
	),
	)
	if "Location" not in headers:
	raise Exception("Got response from sending SMS with no location header")
	# Nominally we should parse the URL, but we can just split on '/' since
	# we only care about the last part.
	parts = headers["Location"][0].split("/")
	if len(parts) < 2:
	raise Exception(
	"Got response from sending SMS with malformed location header"
	)
	logger.info(
	"Successfully sent SMS (ticket ID: %s, request ID %s), OpenMarket API"
	" responded with code %d",
	parts[-1],
	request_id,
	resp.code,
	)

[DMRobertson]

I don't think this causes any user visible problems, since we still send out the request. (I just think we'll fail to do any meaningful logging about the result.)

[DMRobertson]

iweb.IResponse.headers and client.Response.headers are unannotated. I'm only seeing this now because I've added a stub which marks headers: Headers. And thankfully Headers is annotated.

[DMRobertson]

I think we forgot to merge https://github.com/matrix-org/sydent/tree/release-v2.4 into main? See https://github.com/matrix-org/sydent/compare/release-v2.4 . I think main is missing the changes in #413 and #415.

[babolivier]

I think we forgot to merge https://github.com/matrix-org/sydent/tree/release-v2.4 into main?

Ah yes that's probably the issue, let me do this

[babolivier]

(done)