Synapse accepts a PUT /createRoom request

[DMRobertson]

But the spec says it's only to be POSTed.

https://spec.matrix.org/v1.5/client-server-api/#creation

In particular, /createRoom is not idempotent, so it shouldn't be a PUT.

[DMRobertson]

matrix-org/matrix-spec#222 confuses the matter somewhat

[DMRobertson]

Pinged clients here'; no requests to PUT /_matrix/client/[^/]*/createRoom HTTP in the last day. Let's bin it.

[DMRobertson]

Hmm. On reflection it looks like the implementation is handling a transaction id:

synapse/synapse/rest/client/room.py

Lines 153 to 158 in 86c5a71

	def on_PUT(
	self, request: SynapseRequest, txn_id: str
	) -> Awaitable[Tuple[int, JsonDict]]:
	set_tag("txn_id", txn_id)
	return self.txns.fetch_or_execute_request(request, self.on_POST, request)

which means it's doing exactly what matrix-org/matrix-spec#222 asks for. Maybe we just add this to the spec?

[tulir]

How is it handling a transaction ID without a transaction ID parameter in the path? Also, is it actually idempotent or does it just eat a transaction ID and do nothing with it?

I'd say it's safe to remove either way and re-add properly once there's a MSC.

[DMRobertson]

How is it handling a transaction ID without a transaction ID parameter in the path?

See

synapse/synapse/rest/client/room.py

Lines 149 to 151 in 86c5a71

	def register(self, http_server: HttpServer) -> None:
	PATTERNS = "/createRoom"
	register_txn_path(self, PATTERNS, http_server)

and the implementation of register_txn_path.

Also, is it actually idempotent or does it just eat a transaction ID and do nothing with it?

It looks idempotent enough to me. The transaction id is included here in the key we use to deduplicate requests:

synapse/synapse/rest/client/transactions.py

Lines 70 to 85 in fa0eab9

	def fetch_or_execute_request(
	self,
	request: Request,
	fn: Callable[P, Awaitable[Tuple[int, JsonDict]]],
	*args: P.args,
	**kwargs: P.kwargs,
	) -> Awaitable[Tuple[int, JsonDict]]:
	"""A helper function for fetch_or_execute which extracts
	a transaction key from the given request.
	See:
	fetch_or_execute
	"""
	return self.fetch_or_execute(
	self._get_transaction_key(request), fn, *args, **kwargs
	)

synapse/synapse/rest/client/transactions.py

Lines 53 to 68 in fa0eab9

	def _get_transaction_key(self, request: Request) -> str:
	"""A helper function which returns a transaction key that can be used
	with TransactionCache for idempotent requests.
	Idempotency is based on the returned key being the same for separate
	requests to the same endpoint. The key is formed from the HTTP request
	path and the access_token for the requesting user.
	Args:
	request: The incoming request. Must contain an access_token.
	Returns:
	A transaction key
	"""
	assert request.path is not None
	token = self.auth.get_access_token_from_request(request)
	return request.path.decode("utf8") + "/" + token

A comment suggests that we guarantee deduplication within a 30 minute range.

synapse/synapse/rest/client/transactions.py

Lines 46 to 50 in fa0eab9

	self.transactions: Dict[
	str, Tuple[ObservableDeferred[Tuple[int, JsonDict]], int]
	] = {}
	# Try to clean entries every 30 mins. This means entries will exist
	# for at *LEAST* 30 mins, and at *MOST* 60 mins.

[tulir]

Huh, so synapse also has undocumented PUT versions of all the membership and join endpoints 🤔

I guess that just needs a MSC for PUT /createRoom then, although it'd be nice to switch to /create_room too