1025895 - Loop server - Allow URL associated data to be modified via PUT /call-url/{token}

Status: VERIFIED FIXED

(Hello (Loop) :: Server, defect, P3)

Description

[Fernando Jiménez Moreno [:ferjm]]

https://wiki.mozilla.org/Loop/Architecture/MVP#POST_.2Fcall-url.2F.7Btoken.7D

Comment 1

[Alexis Metaireau (:alexis)]

This shouldn't be a POST, but a PUT/PATCH instead, since we're modifying already existing data, and not creating a new one.

Comment 2

[Alexis Metaireau (:alexis)]

ccing adam here as I know he disagrees (I prefer to answer here rather than commenting on the email thread where it could potentially never end).

A POST on a resource should allow to create a new resource, e.g. you normally do POST /calls to create a new call (at /calls/{id}).

To create a new call url (when you don't know the ID) then you do a POST on /call-urls and you're given an id back. If you already know the ID then you should do a PATCH / PUT on the resource url (/call-urls/{id}).

Comment 3

[Alexis Metaireau (:alexis)]

Forgot to needinfo Adam :)

Comment 4

[Adam Roach [:abr]]

(In reply to Alexis Metaireau (:alexis) from comment #2)
> A POST on a resource should allow to create a new resource, e.g. you
> normally do POST /calls to create a new call (at /calls/{id}).
> 
> To create a new call url (when you don't know the ID) then you do a POST on
> /call-urls and you're given an id back. If you already know the ID then you
> should do a PATCH / PUT on the resource url (/call-urls/{id}).

There's some confusion here about the semantics of POST versus those of PUT.

POST is defined here: <http://tools.ietf.org/html/rfc7231#section-4.3.3>. Fundamentally, it is used to create or change resources "according to the resource's own specific semantics."

PUT is defined here: <http://tools.ietf.org/html/rfc7231#section-4.3.4>. By contrast, it is summarized thus: "A successful PUT of a given representation would suggest that a subsequent GET on that same target resource will result in an equivalent representation being sent in a 200 (OK) response."

PATCH is a special case of PUT that allows client to send less than a full body.

Functionally, the distinction is: if you do "PUT /url, body=<X>", and then "GET /url", you should expect to get <X> in response to the PUT. With POST, you would have no such expectation; additionally, POST will frequently impact multiple resources and/or resources other than the URL to which the POST is directed.

Based on that, and based on the intended behavior for this API, I think it's pretty clear that PUT is inappropriate for this functionality.

Comment 5

[Adam Roach [:abr]]

(In reply to Adam Roach [:abr] from comment #4)
> Functionally, the distinction is: if you do "PUT /url, body=<X>", and then
> "GET /url", you should expect to get <X> in response to the PUT.

Sorry, I mean "in response to the GET."

Comment 6

[Alexis Metaireau (:alexis)]

We're following the REST mechanisms. POST is to create new resources and PUT to "put" the new resource at an already existing location.

https://en.wikipedia.org/wiki/REST#Applied_to_web_services states:

- PUT on an element URI: Replace the addressed member of the collection, or if it doesn't exist, create 
it.
- POST on an element URI: Not generally used.

Comment 7

[Alexis Metaireau (:alexis)]

That's one of the latest blocker before we land for MVP. Fernando, do you want me to take ownership here or is everything okay?

Comment 8

[Alexis Metaireau (:alexis)]

Taking this one as we need to finish it for next week and I'm on PTO tomorrow and EOW. Fernando, please comment if I shouldn't.

Comment 9

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Alexis Metaireau (:alexis) from comment #8)
> Taking this one as we need to finish it for next week and I'm on PTO
> tomorrow and EOW. Fernando, please comment if I shouldn't.

Don't worry, take it !
Do you need some help to move faster?

Comment 10

[Alexis Metaireau (:alexis)]

Attachment: link to github PR

Comment 11

[Adam Roach [:abr]]

Comment on attachment 8448718 [details] [review]
link to github PR

Look, guys. I'm happy to engage on the topic, but at the end of the day, I'm the architect for the overall system. I'm the owner for decisions regarding network API.

In this case, I'm adamant that PUT is absolutely the wrong mechanism, and I feel very strongly about it. This patch cannot land as-is.

Comment 12

[Rémy Hubscher (:natim)]

In that case I would prefer to return the same value for GET that we have for PUT to stick with a REST API for loop.

Comment 13

[Alexis Metaireau (:alexis)]

Well, I'm okay with whatever, that's kinda bikeshedding at this point I believe. Previous experience building REST services told me that using PUT for this kind of things makes the more sense, and I believe that's a shared POV by other parties.

If we want to have a discussion on a topic before agreeing, we should be available to discuss if possible, this message had been unanswered for quite some time, so when implementing I took the direction I described.

Now, of course you're the architect ™ (and thanks for all the work you do, that's very useful), but if we have to discuss on something let's do it with this put aside, that's not a title that makes you more right. Telling you are okay to discuss is not enough, let's really discuss and listen to arguments of all parties before making a call here, otherwise that's just testosterone and who shouts more who wins.

Comment 14

[Tarek Ziadé (:tarek)]

(In reply to Adam Roach [:abr] from comment #11)
> Comment on attachment 8448718 [details] [review]
> link to github PR
> 
> Look, guys. I'm happy to engage on the topic, but at the end of the day, I'm
> the architect for the overall system. I'm the owner for decisions regarding
> network API.

Adam, this is not an appropriate tone and approach.

Let's not forget that at the end of the day Alexis and Rémy are the ones building "your" architecture, so if they want to discuss something it probably means that it needs clarification. 

> I'm the owner for decisions regarding network API.
> In this case, I'm adamant that PUT is absolutely the wrong mechanism, and I feel very strongly about it. 

At the end of the day, we are the owner of the server-side, and we are responsible for deploying it and maintaining it. 

For instance if we think something is wrong in 'your' architecture for scalability, we won't deploy it. That's as simple as that.  So far so good, thanks for your work!

Comment 15

[Adam Roach [:abr]]

Hey, you want to make it PUT, make it PUT. As Alexis says, this is bikeshedding at this point. I've changed the API description back to be PUT as well as the title of this bug, and put the patch back up for review.

In broader terms, however, I'll reiterate that I have a lot of bugs to stay on top of, and if you need input from me (cf. "this message had been unanswered for quite some time"), make sure I'm tagged in a needinfo, or I'm likely to miss it.

Comment 16

[Rémy Hubscher (:natim)]

Comment on attachment 8448718 [details] [review]
link to github PR

I did some changes wrt callUrlData expiricy.

Comment 17

[Rémy Hubscher (:natim)]

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

Comment 18

[James Bonacci [:jbonacci]]

Verified in code and in unit tests.