1099063 - [Rooms] PATCH /rooms/{token} doesn't respect optional parameters (overwrites/deletes them in the room data)

Status: RESOLVED FIXED

(Hello (Loop) :: Server, defect)

Description

[Mark Banner (:standard8)]

The documentation for PATCH /rooms/{token} says it all items are optional:

https://docs.services.mozilla.com/loop/apis.html#post-rooms-token

Optional request body parameters:

    roomName, The name of the room.
    roomOwner, The room owner name.
    maxSize, The maximum number of people the room can handle.
    expiresIn, the number of hours for which the room will exist.

However, if I supply a patch to adjust roomName, without also specifying maxSize, then the clientMaxSize that's returned in subsequent GET /rooms/{token} is set to "null".

Comment 1

[Mark Banner (:standard8)]

Actually this bug is slightly worse than I thought - not supplying the roomOwner causes the roomOwner field to no longer be set.

The only thing that appears to be ok is the expiresIn, although I've not tested trying to change that value.

Comment 2

[Alexis Metaireau (:alexis)]

Hi Mark, can you please log the request body you're sending? Are you sending "null" values with the keys? I believe this could be overwriting the data on the server.

Comment 3

[Alexis Metaireau (:alexis)]

I tried to reproduce the error without success. Can you have a look at tell me if there is something I'm testing badly compared to what you're providing? https://github.com/mozilla-services/loop-server/commit/dc79e33f9aaafcb55c43351b9da7882c6e08603b

Comment 4

[Mark Banner (:standard8)]

The body I'm using is:

{"roomName":"jkljlk"}

The next get for the room comes back as:

{"roomUrl":"http://localhost:3000/content/XNdiFmw2ZlU","roomName":"jkljlk","clientMaxSize":null,"creationTime":1415967507,"expiresAt":1418764491,"ctime":1416172541,"participants":[]}

i.e. clientMaxSize is null, and roomOwner is missing.

Using the part of your patch from your commit (map -> forEach), doesn't seem to fix it.

Comment 5

[Rémy Hubscher (:natim)]

Attachment: Link to GitHub PR.

I do reproduce with this test. Now fixing in the PR.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8525952 [details] [review]
Link to GitHub PR.

I've just tested against the branch and it seems to be working fine.

Comment 7

[Alexis Metaireau (:alexis)]

Comment on attachment 8525952 [details] [review]
Link to GitHub PR.

That works for me. Waiting for travis to build correctly and will merge then, thanks!

Comment 8

[Alexis Metaireau (:alexis)]

https://github.com/mozilla-services/loop-server/commit/c29e752b95ed8a1218503670a2cf7eefc5d90107