Avoid allowing null String for state_key. #4895
Conversation
…tring according to the Matrix specification. There is no functional change, just a change in the SDK API for clarity regarding the Matrix specs.
| @@ -39,7 +39,7 @@ internal class DefaultSendStateTask @Inject constructor( | |||
|
|
|||
| override suspend fun execute(params: SendStateTask.Params) { | |||
| return executeRequest(globalErrorReceiver) { | |||
| if (params.stateKey == null) { | |||
| if (params.stateKey.isEmpty()) { | |||
There was a problem hiding this comment.
I think it is an improvement, I am not sure what would happen if stateKey was an empty String on the build URL (it would end by /).
| @@ -39,7 +39,7 @@ internal class DefaultSendStateTask @Inject constructor( | |||
|
|
|||
| override suspend fun execute(params: SendStateTask.Params) { | |||
| return executeRequest(globalErrorReceiver) { | |||
| if (params.stateKey == null) { | |||
| if (params.stateKey.isEmpty()) { | |||
There was a problem hiding this comment.
Why do we even need to test this?
There was a problem hiding this comment.
probably if
/_matrix/client/r0/rooms/{roomId}/state/{eventType}/ is not equivalent to
/_matrix/client/r0/rooms/{roomId}/state/{eventType}
| */ | ||
| suspend fun sendStateEvent(eventType: String, stateKey: String?, body: JsonDict) | ||
| suspend fun sendStateEvent(eventType: String, stateKey: String, body: JsonDict) |
There was a problem hiding this comment.
are there other endpoints which use empty strings to represent the absence of a value?
thinking out loud, from a public api perspective I would prefer to pass a null than an empty string as it gives us some extra type safety (and then let the internal implementation convert to an empty string)
There was a problem hiding this comment.
We want the SDK API to follow as much as possible the Matrix specification/philosophy. In the spec, the stateKey cannot be null.
Ref: https://matrix.org/docs/spec/client_server/r0.6.1#put-matrix-client-r0-rooms-roomid-state-eventtype-statekey
There was a problem hiding this comment.
extra type safety
I do not understand why?
There was a problem hiding this comment.
a string can contain content or be empty, whereas a null can only be null
the spec is a little bit misleading here when it mentions empty strings because the value of "" is an implementation detail of building a url path, we never send "" to the api.
it actually means we should omit the stateKey segment, which means the stateKey required is actually a lie...
has a state key
/_matrix/client/r0/rooms/{roomId}/state/{eventType}/{stateKey}/
doesn't have a state key
/_matrix/client/r0/rooms/{roomId}/state/{eventType}/
assuming I've understood correctly! not a blocker for the PR, something I found interesting.
There was a problem hiding this comment.
In API paths we do not send "" (or we alsways do it, since its nothing!), but I am wondering if
/_matrix/client/r0/rooms/{roomId}/state/{eventType}/ is equivalent to
/_matrix/client/r0/rooms/{roomId}/state/{eventType} (note the missing trailing /).
Anyway I think the spec make a diff mainly for the Json of the Event content itself:
"state_key": "" is not equivalent to "state_key": null (or missing "state_key").
that's maybe why it is a bit confusing all along.
Should always be an empty String according to the Matrix specification.
There is no functional change, just a change in the SDK API for clarity regarding the Matrix specs.