All users were logged out of Bugzilla on October 13th, 2018

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

VERIFIED FIXED

Status

P3
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: ferjm, Assigned: alexis+bugs)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa?] ft:Loop)

Attachments

(1 attachment)

(Reporter)

Updated

4 years ago
Priority: -- → P3
(Assignee)

Comment 1

4 years ago
This shouldn't be a POST, but a PUT/PATCH instead, since we're modifying already existing data, and not creating a new one.
Whiteboard: [qa?]
(Assignee)

Comment 2

4 years ago
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}).
Whiteboard: [qa?] → [qa?] ft:Loop
Assignee: nobody → frsela
(Assignee)

Comment 3

4 years ago
Forgot to needinfo Adam :)
Flags: needinfo?(adam)
Summary: Loop server - Allow URL associated data to be modified via POST /call-url/{token} → Loop server - Allow URL associated data to be modified via PUT /call-url/{token}

Comment 4

4 years ago
(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.
Flags: needinfo?(adam)
Summary: Loop server - Allow URL associated data to be modified via PUT /call-url/{token} → Loop server - Allow URL associated data to be modified via POST /call-url/{token}

Comment 5

4 years ago
(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."
(Assignee)

Comment 6

4 years ago
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.
(Assignee)

Comment 7

4 years ago
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?
Status: NEW → ASSIGNED
Flags: needinfo?(frsela)
(Assignee)

Comment 8

4 years ago
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.
Assignee: frsela → alexis+bugs
(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?
Flags: needinfo?(frsela)
Status: ASSIGNED → NEW
(Assignee)

Comment 10

4 years ago
Created attachment 8448718 [details] [review]
link to github PR
Attachment #8448718 - Flags: review?(rhubscher)

Comment 11

4 years ago
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.
Attachment #8448718 - Flags: review?(rhubscher) → review-
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.
(Assignee)

Comment 13

4 years ago
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.
(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

4 years ago
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.
Summary: Loop server - Allow URL associated data to be modified via POST /call-url/{token} → Loop server - Allow URL associated data to be modified via PUT /call-url/{token}

Updated

4 years ago
Attachment #8448718 - Flags: review- → review?(rhubscher)
Comment on attachment 8448718 [details] [review]
link to github PR

I did some changes wrt callUrlData expiricy.
Attachment #8448718 - Flags: review?(rhubscher) → review+
https://github.com/mozilla-services/loop-server/commit/203a483bf3fc307e19c736c590f6c18b451e0ccd
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Verified in code and in unit tests.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.