facilityregistry / fred-api

Facility Registry API Documentation Website
11 stars 4 forks source link

Partial updates #35

Open ctford opened 11 years ago

ctford commented 11 years ago

Currently, our only update mechanism is PUT. As per the HTTP spec, PUT requires the entire resource to be supplied. We could allow clients to POST to the facility URL, and only update the supplied top-level keys.

This would be of benefit to clients who only persist and care about certain fields. If, for example, I had a client that only existed to put facilities on a map, it might not care about anything other than name and geo-coordinates.

However, unless we allow partial updates, that client would need to process everything else a facility might have, or else risk inadvertently deleting data or having the update rejected.

mortenoh commented 11 years ago

I disagree. If we want this, lets use PATCH for it. But this was voted down on the list before I think.

Morten

On Wed, Jan 23, 2013 at 4:20 PM, Chris Ford notifications@github.comwrote:

Currently, our only update mechanism is PUT. As per the HTTP spec, PUT requires the entire resource to be supplied. We could allow clients to POST to the facility URL, and only update the supplied top-level keys.

This would be of benefit to clients who only persist and care about certain fields. If, for example, I had a client that only existed to put facilities on a map, it might not care about anything other than name and geo-coordinates.

However, unless we allow partial updates, that client would need to process everything else a facility might have, or else risk inadvertently deleting data or having the update rejected.

— Reply to this email directly or view it on GitHubhttps://github.com/facilityregistry/fred-api/issues/35.

ctford commented 11 years ago

The semantics of PATCH fit, but it's not as widely supported as POST.

mortenoh commented 11 years ago

Then I suggest we don't support partial updates.

On Wed, Jan 23, 2013 at 4:31 PM, Chris Ford notifications@github.comwrote:

The semantics of PATCH fit, but it's not as widely supported as POST.

— Reply to this email directly or view it on GitHubhttps://github.com/facilityregistry/fred-api/issues/35#issuecomment-12600439.

ctford commented 11 years ago

If we don't want to use PATCH, then the question still stands is whether partial updates via POST is valuable enough to be worth the trouble and complexity of supporting.

If I understand you correctly, @mortenoh, you're saying that if we don't want to take advantage of PATCH, then you're suggesting it's not worthwhile?

mortenoh commented 11 years ago

Well, what I'm saying is that if we want to support partial updates, lets use PATCH. But do we really need it? are you also suggestion doing bulk partial updates, or would it only be supported for one?

ctford commented 11 years ago

I wasn't suggesting bulk partial updates.

Another way to prevent lost updates is conditional PUT (which @mortenoh and I have spoke about offline). Essentially, conditional PUTs behave the same way as conditional GETs. The server can reject the PUT if you supply an out-of-date ETag, which allows clients to ensure they aren't unintentionally clobbering updates they didn't see. That way, clients can perform a psuedo-partial-update by GET-ing the resource, changing fields, then PUT-ing it back, supplying an ETag so that there's no chance of another update happening in between.

This is possible within the current FRED specification, so long as the server provides ETags and supports it. If the server doesn't support these things then the behaviour gracefully degrades.

edjez commented 11 years ago

Suggest to postpone implementing/spec'ing other verbs and update behaviors for after 1.1

mberg commented 11 years ago

Postpone for discussion in 1.1

mverzilli commented 11 years ago

We discussed this issue during today's technical call with @bobjolliffe, Jorge and @steesdale. We mostly agreed in 2 main points, but since none of us had attended previous discussions on these topic we'd like to know everybody's opinion:

1) We all agreed that it would be nice if the current update facility method (http://facilityregistry.org/#update-facility) simply accepted a partial facility representation instead of requiring a "Full new resource representation". For example, if a client just wants to update a facility's location, it may well PUT { coordinates: { lat: xxx, long: yyy } } to the facility's resource address which would cause the FR to update only the coordinates leaving the rest of the properties untouched.

2) A related use case would be when the client sends fields that the FR doesn't know or care about. We agreed that it could simply ignore them and return the full facility resource after update. A possible enhancement to this approach, as suggested by Jorge, would be for the FR to return an additional entity informing the client about fields it doesn't know of/care about.

ctford commented 11 years ago

I don't think it's a good idea to use PUT for partial update, as PUT specifically replaces the resource. For example, if two PUTs happen on a resource, it should be safe to ignore the first one according to the HTTP spec, but if they're both partial updates that's not the case.

POST would be fine, and using PUT to replace the whole thing and POST for just part of it could be a good way to support both intentions.

edjez commented 11 years ago

+1 on POST

On Jul 30, 2013, at 11:23 AM, Chris Ford notifications@github.com wrote:

I don't think it's a good idea to use PUT for partial update, as PUT specifically replaces the resource. For example, if two PUTs happen on a resource, it should be safe to ignore the first one according to the HTTP spec, but if they're both partial updates that's not the case.

POST would be fine, and using PUT to replace the whole thing and POST for just part of it could be a good way to support both intentions.

— Reply to this email directly or view it on GitHub.

bobjolliffe commented 11 years ago

POST would be fine. It is working currently fine on ResourceMapper with PUT, but POST makes semantically better sense. In fact of course Morten is right and PATCH makes the most sense.

I wonder Chris, are there particular implementation libraries/frameworks which you know which would have trouble with PATCH?

On 4 August 2013 19:44, edjez notifications@github.com wrote:

+1 on POST

On Jul 30, 2013, at 11:23 AM, Chris Ford notifications@github.com wrote:

I don't think it's a good idea to use PUT for partial update, as PUT specifically replaces the resource. For example, if two PUTs happen on a resource, it should be safe to ignore the first one according to the HTTP spec, but if they're both partial updates that's not the case.

POST would be fine, and using PUT to replace the whole thing and POST for just part of it could be a good way to support both intentions.

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHubhttps://github.com/facilityregistry/fred-api/issues/35#issuecomment-22076535 .

bobjolliffe commented 11 years ago

The discussion about http method is more stylistic than substantial. PATCH is most correct. POST is not wrong. PUT is wrong. If it means moving forward I would accept POOCH. I think it is much more important at this stage to discuss the substantive issues around update. For example being clear about how a consumer should interpret the absence of a property, which properties should be updateable etc. Any proposals here?

mverzilli commented 11 years ago

To sum up:

1) We'll recommend PATCH for partial updates, but also support POST. 2) Properties that are absent on the request will remain untouched, but the response will still contain the resource in full as it is after the update.

ctford commented 11 years ago

I'd rather go with @mortenoh and @bobjolliffe and just support PATCH than support both.

Practically speaking, the main problems for us in terms of not respecting "exotic" HTTP verbs are browsers, but that'll only hit us if we have a rich app that e.g. is used by IE8 or IE9 in IE8 compatibility mode.

Supporting both gets us extra complexity, loses the clarity of PATCH's semantics and doesn't protect us against misbehaving infrastructure or middleware anyway.