New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch verb #505

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
@dlee
Contributor

dlee commented May 11, 2011

Update: as ncreuschling points out, there was a typo in the pull request message that said "HTML verb" instead of "HTTP verb".

Updated: update forms default to PUT instead of PATCH for current apps. Defaults to PATCH for new apps.

PATCH is the correct HTTP verb to map to the #update action. The semantics for
PATCH allows for partial updates, whereas PUT requires a complete replacement.

Changes:

  • adds the #patch verb to routes to detect PATCH requests
  • adds #patch? to Request
  • adds the PATCH -> update mapping in the #resource(s) routes.
  • changes default form helpers to prefer :patch instead of :put for updates only for new apps
  • changes documentation and comments to indicate the preference for PATCH

This change tries to maintain complete backwards compatibility by keeping the
original PUT -> update mapping. Users using the #resource(s) routes should not
notice a change in behavior since both PUT and PATCH requests get mapped to
update.

dlee added some commits May 6, 2011

Use PATCH instead of PUT; Fixes issue #348
PATCH is the correct HTML verb to map to the #update action. The semantics for
PATCH allows for partial updates, whereas PUT requires a complete replacement.

Changes:
* adds the #patch verb to routes to detect PATCH requests
* adds #patch? to Request
* adds the PATCH -> update mapping in the #resource(s) routes.
* changes default form helpers to prefer :patch instead of :put for updates
* changes documentation and comments to indicate the preference for PATCH

This change tries to maintain complete backwards compatibility by keeping the
original PUT -> update mapping. Users using the #resource(s) routes should not
notice a change in behavior since both PUT and PATCH requests get mapped to
update.
Make method for update forms configurable
Make :put the default method for backwards compatibility. The generator for new
Rails projects configures :patch a the default method in config/application.rb.
@dasch

This comment has been minimized.

Show comment
Hide comment
@dasch

dasch May 13, 2011

Contributor

GitHub needs a +1 button.

Contributor

dasch commented May 13, 2011

GitHub needs a +1 button.

@benatkin

This comment has been minimized.

Show comment
Hide comment
@benatkin

benatkin May 14, 2011

I googled for HTTP verbs and clicked the first result and PATCH isn't listed.

http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html

Where is it?

Also I hardly think that updating everything except the primary key and the created_at timestamp is a PATCH.

benatkin commented May 14, 2011

I googled for HTTP verbs and clicked the first result and PATCH isn't listed.

http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html

Where is it?

Also I hardly think that updating everything except the primary key and the created_at timestamp is a PATCH.

@dlee

This comment has been minimized.

Show comment
Hide comment
@dlee

dlee May 14, 2011

Contributor

@benatkin http://www.w3.org/Protocols/rfc2068/rfc2068

For past discussion, see issues #348 and #425.

Contributor

dlee commented May 14, 2011

@benatkin http://www.w3.org/Protocols/rfc2068/rfc2068

For past discussion, see issues #348 and #425.

Show outdated Hide outdated actionpack/lib/action_dispatch/routing/mapper.rb
#
# Example:
#
# patch 'bacon', :to => 'food#bacon'

This comment has been minimized.

@dasch

dasch Jun 12, 2011

Contributor

The example needs to be indented 2 spaces so it'll show up correctly in the docs.

@dasch

dasch Jun 12, 2011

Contributor

The example needs to be indented 2 spaces so it'll show up correctly in the docs.

@dlee

This comment has been minimized.

Show comment
Hide comment
@dlee

dlee Jun 25, 2011

Contributor

@josevalim, @fxn: Hey guys, this pull request has been sitting here for over a month unnoticed. It seems like master is now hosting code for Rails 3.2, so I'm thinking it can be merged in?

Contributor

dlee commented Jun 25, 2011

@josevalim, @fxn: Hey guys, this pull request has been sitting here for over a month unnoticed. It seems like master is now hosting code for Rails 3.2, so I'm thinking it can be merged in?

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Jun 25, 2011

Contributor

It seems fine for me with the view configuration option. I am a bit concerned about generating more routes (routes generation is already slow), but I don't consider it a blocker at first. /cc @NZKoz

Contributor

josevalim commented Jun 25, 2011

It seems fine for me with the view configuration option. I am a bit concerned about generating more routes (routes generation is already slow), but I don't consider it a blocker at first. /cc @NZKoz

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Jun 25, 2011

Member

Yes I had it in mind, but waiting for 3.1 to start working on 3.2 proper.

Member

fxn commented Jun 25, 2011

Yes I had it in mind, but waiting for 3.1 to start working on 3.2 proper.

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Jul 27, 2011

Member

Just a ping to say I am still waiting for 3.1 and have not forgotten this patch. That's because albeit 3-1-stable is branched, I prefer a master that does not move too much because cherry-picking back to 3-1-stable is very common these days. Focus is 3.1 now.

The patch does not merge cleanly anymore, by the way, in case you want to maintain it in sync.

Member

fxn commented Jul 27, 2011

Just a ping to say I am still waiting for 3.1 and have not forgotten this patch. That's because albeit 3-1-stable is branched, I prefer a master that does not move too much because cherry-picking back to 3-1-stable is very common these days. Focus is 3.1 now.

The patch does not merge cleanly anymore, by the way, in case you want to maintain it in sync.

@dlee

This comment has been minimized.

Show comment
Hide comment
@dlee

dlee Aug 5, 2011

Contributor

@fxn, cool, glad to know this hasn't been forgotten. Let me know when you're ready to merge so I can update the pull request.

Contributor

dlee commented Aug 5, 2011

@fxn, cool, glad to know this hasn't been forgotten. Let me know when you're ready to merge so I can update the pull request.

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Oct 9, 2011

Member

Nice patch!

I think this will break apps (and engines) that use explicit put routes to override a default resources route. After upgrading Rails, users will see that the form seems to bypass their put override.

Member

jeremy commented Oct 9, 2011

Nice patch!

I think this will break apps (and engines) that use explicit put routes to override a default resources route. After upgrading Rails, users will see that the form seems to bypass their put override.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Nov 13, 2011

Member

+1 from me.

I wish GitHub had a better way to subscribe to issues instead of just commenting. I feel like "+1" is just noise, but I want to see what happens with this pull...

Member

steveklabnik commented Nov 13, 2011

+1 from me.

I wish GitHub had a better way to subscribe to issues instead of just commenting. I feel like "+1" is just noise, but I want to see what happens with this pull...

@bcardarella

This comment has been minimized.

Show comment
Hide comment
@bcardarella

bcardarella Nov 13, 2011

Contributor

@steveklabnik can't you just click the 'Enable notifications for this Pull Request' link at the bottom?

Contributor

bcardarella commented Nov 13, 2011

@steveklabnik can't you just click the 'Enable notifications for this Pull Request' link at the bottom?

@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran

guilleiguaran Nov 13, 2011

Member

@steveklabnik test clicking in the link at the end of this page:

Notifications for new comments on this Pull Request are off. Enable notifications for this Pull Request

@jeremy This one is candidate for Rails 3.2 or 4.0?

Member

guilleiguaran commented Nov 13, 2011

@steveklabnik test clicking in the link at the end of this page:

Notifications for new comments on this Pull Request are off. Enable notifications for this Pull Request

@jeremy This one is candidate for Rails 3.2 or 4.0?

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Nov 13, 2011

Member

3.2 candidate for sure. This needs thorough attention for a clean upgrade, however.

The guides need closer attention. Perusing the diff, I see a lot of search/replace changes from PUT to PATCH. I think having PUT just disappear will be too confusing. Gotta explain this change each step of the way!

Anyone care to take up the torch on this?

Member

jeremy commented Nov 13, 2011

3.2 candidate for sure. This needs thorough attention for a clean upgrade, however.

The guides need closer attention. Perusing the diff, I see a lot of search/replace changes from PUT to PATCH. I think having PUT just disappear will be too confusing. Gotta explain this change each step of the way!

Anyone care to take up the torch on this?

@stevegraham

This comment has been minimized.

Show comment
Hide comment
@stevegraham

stevegraham Nov 13, 2011

Contributor

i must be the only person in the world that disagrees with "PUT requires a complete replacement", as per RFC2616 "HTTP/1.1 does not define how a PUT method affects the state of an origin server"

that said i like the name PATCH better than PUT for the purposes of what it is used for here.

Contributor

stevegraham commented Nov 13, 2011

i must be the only person in the world that disagrees with "PUT requires a complete replacement", as per RFC2616 "HTTP/1.1 does not define how a PUT method affects the state of an origin server"

that said i like the name PATCH better than PUT for the purposes of what it is used for here.

@dlee

This comment has been minimized.

Show comment
Hide comment
@dlee

dlee Nov 13, 2011

Contributor

I can refine this patch if the core team decides it will definitely go into Rails 3.2... don't want to write another huge patch just to be pushed off to the next version :).

@jeremy can you give an example scenario where the put overrides would break? We might be able to provide a workaround, or at least put some notes in the documentation so that users aren't caught off-guard. Please suggest a workaround or an excerpt that can be added to the documentation.

Contributor

dlee commented Nov 13, 2011

I can refine this patch if the core team decides it will definitely go into Rails 3.2... don't want to write another huge patch just to be pushed off to the next version :).

@jeremy can you give an example scenario where the put overrides would break? We might be able to provide a workaround, or at least put some notes in the documentation so that users aren't caught off-guard. Please suggest a workaround or an excerpt that can be added to the documentation.

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Nov 13, 2011

Member

Yes, we talked about this patch after 3.1 and it is in the "roadmap" for 3.2 if it is good to go by then.

@stevegraham RFCs are not axiomatic systems, but I think there's no controversy in that PUT means "put this resource at that URL" ("The PUT method requests that the enclosed entity be stored under the supplied Request-URI.").

You're sending the resource itself, and in the target URL you either create a new resource ("If the Request-URI does not point to an existing resource, and that URI is capable of being defined as a new resource by the requesting user agent, the origin server can create the resource with that URI."), or else replace the existing one ("If the Request-URI refers to an already existing resource, the enclosed entity SHOULD be considered as a modified version of the one residing on the origin server.").

It is the app's choice to conform to that spec. Nowadays to do partial updates in a RESTFul way you need to define ad-hoc resources. For example, to toggle the "paid" flag of an invoice you may PUT to /invoices/:id/paid, but if you play by the rules you need PATCH for partial updates to /invoices/:id.

Member

fxn commented Nov 13, 2011

Yes, we talked about this patch after 3.1 and it is in the "roadmap" for 3.2 if it is good to go by then.

@stevegraham RFCs are not axiomatic systems, but I think there's no controversy in that PUT means "put this resource at that URL" ("The PUT method requests that the enclosed entity be stored under the supplied Request-URI.").

You're sending the resource itself, and in the target URL you either create a new resource ("If the Request-URI does not point to an existing resource, and that URI is capable of being defined as a new resource by the requesting user agent, the origin server can create the resource with that URI."), or else replace the existing one ("If the Request-URI refers to an already existing resource, the enclosed entity SHOULD be considered as a modified version of the one residing on the origin server.").

It is the app's choice to conform to that spec. Nowadays to do partial updates in a RESTFul way you need to define ad-hoc resources. For example, to toggle the "paid" flag of an invoice you may PUT to /invoices/:id/paid, but if you play by the rules you need PATCH for partial updates to /invoices/:id.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Nov 13, 2011

Contributor

Aside the docs concern, this pull request looks good to me. I would just add the following changes (but I can do it myself in later commits after this is merged, this is more of a mental note):

  1. We could move config.action_view.default_method_for_update to config.default_method_for_update so other frameworks can read it as well (see 2 below);

  2. There is no need to generate routes for both PUT and PATCH. We could read the config in 1) and generate just one of the routes;

  3. config.default_method_for_update should be uncommented in new applications as we already changed all docs

Contributor

josevalim commented Nov 13, 2011

Aside the docs concern, this pull request looks good to me. I would just add the following changes (but I can do it myself in later commits after this is merged, this is more of a mental note):

  1. We could move config.action_view.default_method_for_update to config.default_method_for_update so other frameworks can read it as well (see 2 below);

  2. There is no need to generate routes for both PUT and PATCH. We could read the config in 1) and generate just one of the routes;

  3. config.default_method_for_update should be uncommented in new applications as we already changed all docs

@@ -312,6 +312,11 @@ Rails will also automatically set the +class+ and +id+ of the form appropriately
WARNING: When you're using STI (single-table inheritance) with your models, you can't rely on record identification on a subclass if only their parent class is declared a resource. You will have to specify the model name, +:url+, and +:method+ explicitly.
NOTE: Rails can use the +PATCH+ method instead of +PUT+ for update forms if you set the following option in your +config/application.rb+:
<ruby>
config.action_view.default_method_for_update = :update

This comment has been minimized.

@josevalim

josevalim Nov 13, 2011

Contributor

Typo: should be = :patch.

@josevalim

josevalim Nov 13, 2011

Contributor

Typo: should be = :patch.

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Nov 16, 2011

Member

@stevegraham I have found a crystal clear and authoritative answer to whether PUT allows partial updates: The very RFC 5789 (http://tools.ietf.org/html/rfc5789) states that "A new method is necessary to improve interoperability and prevent errors. The PUT method is already defined to overwrite a resource with a complete new body, and cannot be reused to do partial changes."

Member

fxn commented Nov 16, 2011

@stevegraham I have found a crystal clear and authoritative answer to whether PUT allows partial updates: The very RFC 5789 (http://tools.ietf.org/html/rfc5789) states that "A new method is necessary to improve interoperability and prevent errors. The PUT method is already defined to overwrite a resource with a complete new body, and cannot be reused to do partial changes."

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Nov 16, 2011

Member

I like the idea of moving this to be config.action_view.default_method_for_update as it lets people who care about these things choose to use patch without hitting anyone with 2x route proliferation.

However I'm pretty strongly opposed to switching the default to PATCH just because of a neurotic interpretation of the RFC. There are people with blog posts, printed books and screen casts where they do

form_for(@user, :html=>{:method=>:put}) 

Or

link_to("something", @user, :remote=>true, :method=>:put)

Switching the default will make all of those tutorials and chunks of code fail with routing errors, and "the RFC says X" doesn't seem like anywhere near a good enough reason to do that.

Maybe for a 4.0 change but absent some genuine bug or difficulty we can't do this in a random minor release.

Member

NZKoz commented Nov 16, 2011

I like the idea of moving this to be config.action_view.default_method_for_update as it lets people who care about these things choose to use patch without hitting anyone with 2x route proliferation.

However I'm pretty strongly opposed to switching the default to PATCH just because of a neurotic interpretation of the RFC. There are people with blog posts, printed books and screen casts where they do

form_for(@user, :html=>{:method=>:put}) 

Or

link_to("something", @user, :remote=>true, :method=>:put)

Switching the default will make all of those tutorials and chunks of code fail with routing errors, and "the RFC says X" doesn't seem like anywhere near a good enough reason to do that.

Maybe for a 4.0 change but absent some genuine bug or difficulty we can't do this in a random minor release.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Nov 16, 2011

Member

Maybe for a 4.0 change but absent some genuine bug or difficulty we can't do this in a random minor release.

Yeah, sure, I mean, the asset pipeline from 3.0 -> 3.1 was a waaaaaaay smaller change. ;)

of a neurotic interpretation of the RFC

This is not neurotic. It's simply following the way that things are supposed to work. Many more people want partial updates than an upsert, that's what PATCH is.

Member

steveklabnik commented Nov 16, 2011

Maybe for a 4.0 change but absent some genuine bug or difficulty we can't do this in a random minor release.

Yeah, sure, I mean, the asset pipeline from 3.0 -> 3.1 was a waaaaaaay smaller change. ;)

of a neurotic interpretation of the RFC

This is not neurotic. It's simply following the way that things are supposed to work. Many more people want partial updates than an upsert, that's what PATCH is.

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Nov 16, 2011

Member

@dlee see my previous comment for a concrete example

Member

jeremy commented Nov 16, 2011

@dlee see my previous comment for a concrete example

@dlee

This comment has been minimized.

Show comment
Hide comment
@dlee

dlee Nov 16, 2011

Contributor

@jeremey I was actually responding to your first comment in this thread. You say that explicit put overrides would break apps, but I don't see how they would. Can you clarify what you mean by put overrides?

Contributor

dlee commented Nov 16, 2011

@jeremey I was actually responding to your first comment in this thread. You say that explicit put overrides would break apps, but I don't see how they would. Can you clarify what you mean by put overrides?

@dlee

This comment has been minimized.

Show comment
Hide comment
@dlee

dlee Nov 16, 2011

Contributor
Contributor

dlee commented Nov 16, 2011

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Nov 16, 2011

Member

@dlee Declare a resource in config/routes. Declare a put route that overrides the resource's update. Now upgrade the app and enable PATCH. Oops, now your forms submit to patch and your put route override is bypassed.

Not the end of the world, and docs should be enough to cover that.

Member

jeremy commented Nov 16, 2011

@dlee Declare a resource in config/routes. Declare a put route that overrides the resource's update. Now upgrade the app and enable PATCH. Oops, now your forms submit to patch and your put route override is bypassed.

Not the end of the world, and docs should be enough to cover that.

@dlee

This comment has been minimized.

Show comment
Hide comment
@dlee

dlee Nov 16, 2011

Contributor

@jeremy One of the points of the change was:

  • changes default form helpers to prefer :patch instead of :put for updates only for new apps

Note that form helpers use :patch only for new apps. Would this still not cover that case?

Contributor

dlee commented Nov 16, 2011

@jeremy One of the points of the change was:

  • changes default form helpers to prefer :patch instead of :put for updates only for new apps

Note that form helpers use :patch only for new apps. Would this still not cover that case?

@getconor

This comment has been minimized.

Show comment
Hide comment
@getconor

getconor Nov 16, 2011

Contributor

I agree with @NZKoz that this should not be included before 4.0 at the earliest. Unless I'm mistaken, RFC-5789 is still just a proposed standard, and it can still be changed or retracted. See: http://www.rfc-editor.org/info/rfc5789

@steveklabnik This isn't the way things are supposed to work yet, and even without all of the recent changes, we should only implement the standard when we know it won't change or be retracted- to do otherwise would be irresponsible. PATCH has been discussed by the IETF for almost ten years, why the rush to implement it in Rails now?

Contributor

getconor commented Nov 16, 2011

I agree with @NZKoz that this should not be included before 4.0 at the earliest. Unless I'm mistaken, RFC-5789 is still just a proposed standard, and it can still be changed or retracted. See: http://www.rfc-editor.org/info/rfc5789

@steveklabnik This isn't the way things are supposed to work yet, and even without all of the recent changes, we should only implement the standard when we know it won't change or be retracted- to do otherwise would be irresponsible. PATCH has been discussed by the IETF for almost ten years, why the rush to implement it in Rails now?

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Nov 16, 2011

Contributor

Good point. Agreed @NZKoz.

Contributor

josevalim commented Nov 16, 2011

Good point. Agreed @NZKoz.

@benatkin

This comment has been minimized.

Show comment
Hide comment
@benatkin

benatkin Nov 16, 2011

@mhutchin Interesting. I looked back at the link I posted at the top of the discussion and the RFC @dlee cited, which contains PATCH, appears to be obsoleted by one that I cited, which only has a reference to PATCH at the end. Here are the two documents:

The reference to PATCH at the end says:

The PATCH, LINK, UNLINK methods were defined but not commonly implemented in previous versions of this specification. See RFC 2068 [33].

benatkin commented Nov 16, 2011

@mhutchin Interesting. I looked back at the link I posted at the top of the discussion and the RFC @dlee cited, which contains PATCH, appears to be obsoleted by one that I cited, which only has a reference to PATCH at the end. Here are the two documents:

The reference to PATCH at the end says:

The PATCH, LINK, UNLINK methods were defined but not commonly implemented in previous versions of this specification. See RFC 2068 [33].

@getconor

This comment has been minimized.

Show comment
Hide comment
@getconor

getconor Nov 16, 2011

Contributor

@benatkin Thanks, I knew they had been discussing it for a long time, I underestimated how long. :-)

I would only point out for those interested that RFC-2616 for HTTP/1.1 is now a DRAFT STANDARD, but at least it is further along in the process than the RFC that now covers PATCH, RFC-5789, which is a PROPOSED STANDARD, the "entry-level maturity for the standards track". For the meaning of the statuses see: http://www.rfc-editor.org/rfc/rfc2026.txt

Contributor

getconor commented Nov 16, 2011

@benatkin Thanks, I knew they had been discussing it for a long time, I underestimated how long. :-)

I would only point out for those interested that RFC-2616 for HTTP/1.1 is now a DRAFT STANDARD, but at least it is further along in the process than the RFC that now covers PATCH, RFC-5789, which is a PROPOSED STANDARD, the "entry-level maturity for the standards track". For the meaning of the statuses see: http://www.rfc-editor.org/rfc/rfc2026.txt

@benatkin

This comment has been minimized.

Show comment
Hide comment
@benatkin

benatkin Nov 16, 2011

Probably not the best way to check this, but on this list RFC-2616 is on Draft Standards and RFC-5789 is on Proposed Standards.

http://www.rfc-editor.org/rfcxx00.html

benatkin commented Nov 16, 2011

Probably not the best way to check this, but on this list RFC-2616 is on Draft Standards and RFC-5789 is on Proposed Standards.

http://www.rfc-editor.org/rfcxx00.html

@benatkin

This comment has been minimized.

Show comment
Hide comment
@benatkin

benatkin Nov 16, 2011

@mhutchin I didn't see your most recent comment until I posted mine. You said exactly the same thing! It took me a bit of thinking to see that there's a big difference between draft and proposed besides draft being higher up the list, but I can see it now.

benatkin commented Nov 16, 2011

@mhutchin I didn't see your most recent comment until I posted mine. You said exactly the same thing! It took me a bit of thinking to see that there's a big difference between draft and proposed besides draft being higher up the list, but I can see it now.

@dlee

This comment has been minimized.

Show comment
Hide comment
@dlee

dlee Nov 16, 2011

Contributor

@NZKoz I don't think the "route proliferation" is so bad. In fact, even if Rails ends up supporting PATCH in the distant future, I'd prefer PUT be left as a standard route, perhaps linked to a different action (replace/upsert/etc.) This might not jive with a must-map-to-CRUD mindset, but it definitely jives with HTTP.

The interpretation of the RFC is not neurotic; I think we all share a clear and straightforward interpretation. I'll assume you meant "neurotic adherence to the RFC".

If so, let me reiterate that supporting PATCH is not to merely adhere to an RFC. The reason is because Rails currently has broken support for HTTP as commonly understood--leading to potentially problematic interoperability with RESTful clients, caching mechanisms, and proxies.

I'd say implementing, documenting, and promoting a broken implementation of an HTTP VERB is a genuine bug, warranting a fix in a minor release, but I'll leave the decision up to the Rails core team.

Finally, Rails shouldn't inhibit change just to retain backwards compatibility with blogs, books, and screencasts. In fact, I don't think Rails ever did care about that.

Contributor

dlee commented Nov 16, 2011

@NZKoz I don't think the "route proliferation" is so bad. In fact, even if Rails ends up supporting PATCH in the distant future, I'd prefer PUT be left as a standard route, perhaps linked to a different action (replace/upsert/etc.) This might not jive with a must-map-to-CRUD mindset, but it definitely jives with HTTP.

The interpretation of the RFC is not neurotic; I think we all share a clear and straightforward interpretation. I'll assume you meant "neurotic adherence to the RFC".

If so, let me reiterate that supporting PATCH is not to merely adhere to an RFC. The reason is because Rails currently has broken support for HTTP as commonly understood--leading to potentially problematic interoperability with RESTful clients, caching mechanisms, and proxies.

I'd say implementing, documenting, and promoting a broken implementation of an HTTP VERB is a genuine bug, warranting a fix in a minor release, but I'll leave the decision up to the Rails core team.

Finally, Rails shouldn't inhibit change just to retain backwards compatibility with blogs, books, and screencasts. In fact, I don't think Rails ever did care about that.

@dlee

This comment has been minimized.

Show comment
Hide comment
@dlee

dlee Nov 16, 2011

Contributor

@mhutchin, @benatkin, the Draft Standard is actually going away: http://www.rfc-editor.org/rfc/rfc6410.txt. RFCs currently in Draft Standard status will either be promoted to Internet Standard or demoted to Proposed Standard.

BTW, Rails supports modern cookies and content-disposition headers which are also Proposed Standards, so there's no basis to reject PATCH just because it's a Proposed Standard.

In fact, Rails should be the framework that helps PATCH attain the Internet Standard status since Rails clearly needs PATCH in order to support non-replacing updates.

Whether that happens in 4.0 or 3.2, I defer to the Rails core team.

Contributor

dlee commented Nov 16, 2011

@mhutchin, @benatkin, the Draft Standard is actually going away: http://www.rfc-editor.org/rfc/rfc6410.txt. RFCs currently in Draft Standard status will either be promoted to Internet Standard or demoted to Proposed Standard.

BTW, Rails supports modern cookies and content-disposition headers which are also Proposed Standards, so there's no basis to reject PATCH just because it's a Proposed Standard.

In fact, Rails should be the framework that helps PATCH attain the Internet Standard status since Rails clearly needs PATCH in order to support non-replacing updates.

Whether that happens in 4.0 or 3.2, I defer to the Rails core team.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Nov 16, 2011

Contributor

@NZKoz I don't think the "route proliferation" is so bad. In fact, even if Rails ends up supporting PATCH in the distant future, I'd prefer PUT be left as a standard route, perhaps linked to a different action (replace/upsert/etc.) This might not jive with a must-map-to-CRUD mindset, but it definitely jives with HTTP.

If @tenderlove says an extra route is almost free, I am +1 for including all routes, but I doubt this is the case. Remember some apps have more hundreds of resources routes. In the past, generating the format route proved to be quite expensive (for example).

If so, let me reiterate that supporting PATCH is not to merely adhere to an RFC. The reason is because Rails currently has broken support for HTTP as commonly understood--leading to potentially problematic interoperability with RESTful clients, caching mechanisms, and proxies.

Do you have anything to support this? PATCH is a new verb in the specs, so I doubt proxies and caching mechanisms went crazy to support it and change PUT behavior.

Finally, Rails shouldn't inhibit change just to retain backwards compatibility with blogs, books, and screencasts. In fact, I don't think Rails ever did care about that.

Yes, we should and we do care about that (we always consider it when doing big changes. If one of us forgets, other is there to remember). It is funny how everyone complains when Rails breaks backwards compatibility when moving from 3.0 to 3.1 but no one cares to ensure it. It is not just documentation that would be broken, but generators, engines and everything else. As I said in other ticket, each plugin checking for request.put? would now be broken in an app using PATCH as default.

In other words, +1 for this pull but leaving PUT as default to allow the community to catch up. Also, +1 if we can avoid routes proliferation.

Contributor

josevalim commented Nov 16, 2011

@NZKoz I don't think the "route proliferation" is so bad. In fact, even if Rails ends up supporting PATCH in the distant future, I'd prefer PUT be left as a standard route, perhaps linked to a different action (replace/upsert/etc.) This might not jive with a must-map-to-CRUD mindset, but it definitely jives with HTTP.

If @tenderlove says an extra route is almost free, I am +1 for including all routes, but I doubt this is the case. Remember some apps have more hundreds of resources routes. In the past, generating the format route proved to be quite expensive (for example).

If so, let me reiterate that supporting PATCH is not to merely adhere to an RFC. The reason is because Rails currently has broken support for HTTP as commonly understood--leading to potentially problematic interoperability with RESTful clients, caching mechanisms, and proxies.

Do you have anything to support this? PATCH is a new verb in the specs, so I doubt proxies and caching mechanisms went crazy to support it and change PUT behavior.

Finally, Rails shouldn't inhibit change just to retain backwards compatibility with blogs, books, and screencasts. In fact, I don't think Rails ever did care about that.

Yes, we should and we do care about that (we always consider it when doing big changes. If one of us forgets, other is there to remember). It is funny how everyone complains when Rails breaks backwards compatibility when moving from 3.0 to 3.1 but no one cares to ensure it. It is not just documentation that would be broken, but generators, engines and everything else. As I said in other ticket, each plugin checking for request.put? would now be broken in an app using PATCH as default.

In other words, +1 for this pull but leaving PUT as default to allow the community to catch up. Also, +1 if we can avoid routes proliferation.

@dlee

This comment has been minimized.

Show comment
Hide comment
@dlee

dlee Nov 16, 2011

Contributor

If so, let me reiterate that supporting PATCH is not to merely adhere to an RFC. The reason is because Rails currently has broken support for HTTP as commonly understood--leading to potentially problematic interoperability with RESTful clients, caching mechanisms, and proxies.

Do you have anything to support this? PATCH is a new verb in the specs, so I doubt proxies and caching mechanisms went crazy to support it and change PUT behavior.

I was referring to the broken behavior of PUT in Rails, not the absence of PATCH.

For example, caches can reasonably assume that if I PUT {"a" => "b"} to a resource, then that's what it should return on a GET. However, if the original resource was {"c" => "d"}, Rails would by convention make the resource {"a" => "b", "c" => "d"}, not the expected {"a" => "b"}.

BTW, how did you reply to a comment? Or did you just insert ">" before each line?

Contributor

dlee commented Nov 16, 2011

If so, let me reiterate that supporting PATCH is not to merely adhere to an RFC. The reason is because Rails currently has broken support for HTTP as commonly understood--leading to potentially problematic interoperability with RESTful clients, caching mechanisms, and proxies.

Do you have anything to support this? PATCH is a new verb in the specs, so I doubt proxies and caching mechanisms went crazy to support it and change PUT behavior.

I was referring to the broken behavior of PUT in Rails, not the absence of PATCH.

For example, caches can reasonably assume that if I PUT {"a" => "b"} to a resource, then that's what it should return on a GET. However, if the original resource was {"c" => "d"}, Rails would by convention make the resource {"a" => "b", "c" => "d"}, not the expected {"a" => "b"}.

BTW, how did you reply to a comment? Or did you just insert ">" before each line?

@mikekelly

This comment has been minimized.

Show comment
Hide comment
@mikekelly

mikekelly Dec 19, 2011

please consider my response to that, and wait for the discussion to play out.

obligatory meme:

recognize, bitches.

mikekelly commented Dec 19, 2011

please consider my response to that, and wait for the discussion to play out.

obligatory meme:

recognize, bitches.

@mikekelly

This comment has been minimized.

Show comment
Hide comment
@mikekelly

mikekelly Dec 19, 2011

Roy is responsible for the changes in httpbis so it's hardly an earth shattering revelation that he believes 2616 should be interpreted that way.

mikekelly commented Dec 19, 2011

Roy is responsible for the changes in httpbis so it's hardly an earth shattering revelation that he believes 2616 should be interpreted that way.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Dec 19, 2011

Member

He's also an author of 2616, so I'd expect him to be a pretty valid source when requesting clarification of it, too.

Knowing which are the rules without doubt, we have the right mindset to work on the PR.

Right. The real question is this: how far do we want Rails to go? Obviously, Rails cannot make every web service RESTful; that relies on human design. I would like to see Rails make it easier to comply with HTTP and HATEOAS, though, by providing helpful things in that regard. That's obviously longer-term and out of the scope of this pull request. However, this is a good start. I think the correct option as far as this pull request goes is this:

  1. Add two new actions: patch and upsert. The update action should match to POST, the patch to PATCH, and upsert to PUT.
  2. Start off with having this as a configuration option, ActionController.update_verb, which is set to :post in new rails apps, but has a default of :put so that old apps still work.
  3. In Rails+1, change the default to :post but leave the option so that people can set it if they still don't want to update.

'upsert' isn't a great name. I've run across some people who don't know what this means, but it does capture what PUT is supposed to do in one, short word.

Member

steveklabnik commented Dec 19, 2011

He's also an author of 2616, so I'd expect him to be a pretty valid source when requesting clarification of it, too.

Knowing which are the rules without doubt, we have the right mindset to work on the PR.

Right. The real question is this: how far do we want Rails to go? Obviously, Rails cannot make every web service RESTful; that relies on human design. I would like to see Rails make it easier to comply with HTTP and HATEOAS, though, by providing helpful things in that regard. That's obviously longer-term and out of the scope of this pull request. However, this is a good start. I think the correct option as far as this pull request goes is this:

  1. Add two new actions: patch and upsert. The update action should match to POST, the patch to PATCH, and upsert to PUT.
  2. Start off with having this as a configuration option, ActionController.update_verb, which is set to :post in new rails apps, but has a default of :put so that old apps still work.
  3. In Rails+1, change the default to :post but leave the option so that people can set it if they still don't want to update.

'upsert' isn't a great name. I've run across some people who don't know what this means, but it does capture what PUT is supposed to do in one, short word.

@reschke

This comment has been minimized.

Show comment
Hide comment
@reschke

reschke Dec 19, 2011

@steveklabnik keep in mind that just changing PUT to PATCH doesn't make things better magically, you also need an Internet Media Type describing the patch format. And no, "application/json" doesn't.

reschke commented Dec 19, 2011

@steveklabnik keep in mind that just changing PUT to PATCH doesn't make things better magically, you also need an Internet Media Type describing the patch format. And no, "application/json" doesn't.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Dec 19, 2011

Member

Also, @mikekelly asked me to make something perfectly clear, so here it is:

I think that standards should be followed, because they're standards. They're the working agreements that we all go by, and if we discover that we're not compliant, software should changed based on that and nothing else. This doesn't mean they're infallible, and when they get revised, things should be changed again. Because compliance with a spec is the most important thing.

We now return to your regularly scheduled pull request.

Member

steveklabnik commented Dec 19, 2011

Also, @mikekelly asked me to make something perfectly clear, so here it is:

I think that standards should be followed, because they're standards. They're the working agreements that we all go by, and if we discover that we're not compliant, software should changed based on that and nothing else. This doesn't mean they're infallible, and when they get revised, things should be changed again. Because compliance with a spec is the most important thing.

We now return to your regularly scheduled pull request.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Dec 19, 2011

Member

@reschke right. Baby steps. That's why I think that POST actually works better for what rails calls update.

Member

steveklabnik commented Dec 19, 2011

@reschke right. Baby steps. That's why I think that POST actually works better for what rails calls update.

@mikekelly

This comment has been minimized.

Show comment
Hide comment
@mikekelly

mikekelly Dec 19, 2011

it was tongue in cheek, and my point was that until httpbis makes it completely clear - there is no requirement to churn rails, because there are valid interpretations of 2616 which are perfectly fine with the current behaviour.

So, essentially, jumping the httpbis gun w/ this PR is driven by speculation and/or pedantry and not a need to conform to a spec which unquestionably requires the change (there isn't one, HTTPbis is a WIP) or fix a functional problem with rails (there isn't one of these, either). I think everyone should be clear about that.

mikekelly commented Dec 19, 2011

it was tongue in cheek, and my point was that until httpbis makes it completely clear - there is no requirement to churn rails, because there are valid interpretations of 2616 which are perfectly fine with the current behaviour.

So, essentially, jumping the httpbis gun w/ this PR is driven by speculation and/or pedantry and not a need to conform to a spec which unquestionably requires the change (there isn't one, HTTPbis is a WIP) or fix a functional problem with rails (there isn't one of these, either). I think everyone should be clear about that.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 19, 2011

Contributor

So, essentially, jumping the httpbis gun w/ this PR is driven by speculation and/or pedantry rather than conforming to a spec which unquestionably requires the change (there isn't one, HTTPbis is a WIP) or fixing a functional problem with rails (there isn't one of these, either). I think everyone should be clear about that.

That's not it at all. We have the one of the authors of the HTTP 1.1 spec, and the inventor of REST (which rails at least tries to adhere to...) saying that PUT means only full representations, and that rails is not in line with the HTTP or REST to allow it any other way.

Contributor

myronmarston commented Dec 19, 2011

So, essentially, jumping the httpbis gun w/ this PR is driven by speculation and/or pedantry rather than conforming to a spec which unquestionably requires the change (there isn't one, HTTPbis is a WIP) or fixing a functional problem with rails (there isn't one of these, either). I think everyone should be clear about that.

That's not it at all. We have the one of the authors of the HTTP 1.1 spec, and the inventor of REST (which rails at least tries to adhere to...) saying that PUT means only full representations, and that rails is not in line with the HTTP or REST to allow it any other way.

@mikekelly

This comment has been minimized.

Show comment
Hide comment
@mikekelly

mikekelly Dec 19, 2011

tl;dr cliffnotes for @tenderlove: change from this PR is significant, doesn't need to happen - may do in future if/when next revision of HTTP invalidates the approach. Roy Fielding is the ineffable fountain of HTTP wisdom.

mikekelly commented Dec 19, 2011

tl;dr cliffnotes for @tenderlove: change from this PR is significant, doesn't need to happen - may do in future if/when next revision of HTTP invalidates the approach. Roy Fielding is the ineffable fountain of HTTP wisdom.

@mikekelly

This comment has been minimized.

Show comment
Hide comment
@mikekelly

mikekelly Dec 19, 2011

That's not it at all. We have the one of the authors of the HTTP 1.1 spec, and the inventor of REST (which rails at least tries to adhere to...) saying that PUT means only full representations, and that rails is not in line with the HTTP or REST to allow it any other way.

Roy is an insanely smart guy and definitely an authoritative source on HTTP. His insight and opinion is important. But his opinion does not magically make 2616 more clear on this issue than it is. It's not clear. This is why Roy himself has revised it.

mikekelly commented Dec 19, 2011

That's not it at all. We have the one of the authors of the HTTP 1.1 spec, and the inventor of REST (which rails at least tries to adhere to...) saying that PUT means only full representations, and that rails is not in line with the HTTP or REST to allow it any other way.

Roy is an insanely smart guy and definitely an authoritative source on HTTP. His insight and opinion is important. But his opinion does not magically make 2616 more clear on this issue than it is. It's not clear. This is why Roy himself has revised it.

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Dec 19, 2011

Member

@mikekelly we have not outlined yet the path, as I said before I don't expect to do any dramatic changes any time soon. Let's explore.

At this point in the thread, the conclusion is that we are going to add some support for PATCH and promote good practices regarding PUT/PATCH in Rails applications. Good practices as described by Roy Fielding.

But progressively, and with backwards compatibility as a must. At least for the time being.

Member

fxn commented Dec 19, 2011

@mikekelly we have not outlined yet the path, as I said before I don't expect to do any dramatic changes any time soon. Let's explore.

At this point in the thread, the conclusion is that we are going to add some support for PATCH and promote good practices regarding PUT/PATCH in Rails applications. Good practices as described by Roy Fielding.

But progressively, and with backwards compatibility as a must. At least for the time being.

@mikekelly

This comment has been minimized.

Show comment
Hide comment
@mikekelly

mikekelly Dec 19, 2011

At this point in the thread, the conclusion is that we are going to add some support for PATCH and promote good practices regarding PUT/PATCH in Rails applications. Good practices as described by Roy Fielding.

But progressively, and with backwards compatibility as a must. At least for the time being.

Ok fair enough, in that case it looks like @NZKoz hit the nail on the head over a month ago.. #505 (comment)

mikekelly commented Dec 19, 2011

At this point in the thread, the conclusion is that we are going to add some support for PATCH and promote good practices regarding PUT/PATCH in Rails applications. Good practices as described by Roy Fielding.

But progressively, and with backwards compatibility as a must. At least for the time being.

Ok fair enough, in that case it looks like @NZKoz hit the nail on the head over a month ago.. #505 (comment)

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Dec 19, 2011

Member

The point of following the discussion was to know whether we should have to add anything at all. At that point we didn't know.

If Roy said "yeah, just do partial updates with PUT" no flag should be added, no code should be in core.

If Roy says "PUT means PUT. There are no partial updates in PUT." that justifies working on the PR. Working on the PR means what I said above, it does not mean applying the very patch.

Member

fxn commented Dec 19, 2011

The point of following the discussion was to know whether we should have to add anything at all. At that point we didn't know.

If Roy said "yeah, just do partial updates with PUT" no flag should be added, no code should be in core.

If Roy says "PUT means PUT. There are no partial updates in PUT." that justifies working on the PR. Working on the PR means what I said above, it does not mean applying the very patch.

@gducharme

This comment has been minimized.

Show comment
Hide comment
@gducharme

gducharme Dec 20, 2011

Allowing access to PATCH as a configuration option will help drive awareness and adoption. Further down the line, we might well see a larger number of devs and applications clamoring for a change of default configuration, in much the same way we saw jQuery make its way in core.

Count this as +1 to a configuration option.

gducharme commented Dec 20, 2011

Allowing access to PATCH as a configuration option will help drive awareness and adoption. Further down the line, we might well see a larger number of devs and applications clamoring for a change of default configuration, in much the same way we saw jQuery make its way in core.

Count this as +1 to a configuration option.

@dlee

This comment has been minimized.

Show comment
Hide comment
@dlee

dlee Dec 20, 2011

Contributor

Rails 4 has been announced for the summer. How much PATCH support should we get in? There have been various proposals of how to support PATCH in Rails 4:

  • make PATCH and PUT both map to update and:
    • make edit forms use PATCH by default and PUT by configuration opt-in
    • make edit forms use PUT by default and PATCH by configuration opt-in
  • split update into different actions that map better to PUT and PATCH
Contributor

dlee commented Dec 20, 2011

Rails 4 has been announced for the summer. How much PATCH support should we get in? There have been various proposals of how to support PATCH in Rails 4:

  • make PATCH and PUT both map to update and:
    • make edit forms use PATCH by default and PUT by configuration opt-in
    • make edit forms use PUT by default and PATCH by configuration opt-in
  • split update into different actions that map better to PUT and PATCH
@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Dec 20, 2011

Member

@dhh mentioned on Twitter that he supports some kind of support.

Really, to be honest, I think this pull request should probably be closed, and a discussion thread should be made on rails-core about it. That's sorta out of the scope of this particular PR.

Member

steveklabnik commented Dec 20, 2011

@dhh mentioned on Twitter that he supports some kind of support.

Really, to be honest, I think this pull request should probably be closed, and a discussion thread should be made on rails-core about it. That's sorta out of the scope of this particular PR.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Dec 20, 2011

Contributor

We should have a config option called config.method_for_update. Both routes and forms should respect this option. There is no need to generate both routes. It makes no sense to split update in two actions.

Contributor

josevalim commented Dec 20, 2011

We should have a config option called config.method_for_update. Both routes and forms should respect this option. There is no need to generate both routes. It makes no sense to split update in two actions.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Dec 20, 2011

Contributor

In other words, this pull request is almost good imo. It just needs to provide a config.method_for_update, make both AV and the router read from this config and change the router to stop generating both put and patch for update.

Contributor

josevalim commented Dec 20, 2011

In other words, this pull request is almost good imo. It just needs to provide a config.method_for_update, make both AV and the router read from this config and change the router to stop generating both put and patch for update.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Dec 20, 2011

Member

@josevalim It really depends on how far you actually want to go. A 'rails 4' gives a big opportunity to step back and evaluate the whole 'Rails REST' thing in general.

Then again, it seems that rails core wants rails 4 to be more of a normal release, so...

Regardless, I agree about this patch. It's good regardless of that stuff.

Member

steveklabnik commented Dec 20, 2011

@josevalim It really depends on how far you actually want to go. A 'rails 4' gives a big opportunity to step back and evaluate the whole 'Rails REST' thing in general.

Then again, it seems that rails core wants rails 4 to be more of a normal release, so...

Regardless, I agree about this patch. It's good regardless of that stuff.

@dlee

This comment has been minimized.

Show comment
Hide comment
@dlee

dlee Dec 20, 2011

Contributor

If Rails core gives the OK, I can fix the patch according to @josevalim's recommendations.

Contributor

dlee commented Dec 20, 2011

If Rails core gives the OK, I can fix the patch according to @josevalim's recommendations.

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Dec 20, 2011

Member

PATCH and PUT have different semantics. PUT means create/replace and it is idempotent. PATCH means update (full or partial) and it is not idempotent.

If your application is a s3 clone, you want to upload assets with PUT. I think we need to be able to have both working, rather than a exclusive option.

Member

fxn commented Dec 20, 2011

PATCH and PUT have different semantics. PUT means create/replace and it is idempotent. PATCH means update (full or partial) and it is not idempotent.

If your application is a s3 clone, you want to upload assets with PUT. I think we need to be able to have both working, rather than a exclusive option.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Dec 20, 2011

Contributor

@fxn I agree both options should work, it is just a matter of what we choose as default. For instance, the router should generate just put or patch routes by default for resources, however if for some specific case I want to use both PUT and PATCH or another, I should be able to do it. The same for the view, responders, etc... they should work for both put and patch.

Contributor

josevalim commented Dec 20, 2011

@fxn I agree both options should work, it is just a matter of what we choose as default. For instance, the router should generate just put or patch routes by default for resources, however if for some specific case I want to use both PUT and PATCH or another, I should be able to do it. The same for the view, responders, etc... they should work for both put and patch.

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn
Member

fxn commented Dec 20, 2011

@josevalim 👍

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Dec 20, 2011

Contributor

So as both me and @fxn agree, we would love to receive a new pull request that adds those small changes to this already existing pull request. Thanks @dlee for holding on all this time.

Contributor

josevalim commented Dec 20, 2011

So as both me and @fxn agree, we would love to receive a new pull request that adds those small changes to this already existing pull request. Thanks @dlee for holding on all this time.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 22, 2011

Contributor

I hinted at this above, but to reiterate: besides changing rails so that it supports PATCH, I'd also like to see improved support for proper PUT semantics. Currently, rails idioms and conventions do not make this easy. Specifically:

  • The common AR method used for updates is update_attributes. It supports updates of the full representation, but it also supports partial updates, which, as we've discussed above, is a violation of PUT semantics (at least in the mind of Fielding and other HTTP experts).
  • update_attributes also supports non-idempotent updates when used in combination w/ accepts_nested_attributes_for.
  • I think the simplest way to provide an update method that does provide proper PUT semantics is a new method, replace_attributes. It would set any attribute that is not included in the hash to nil, and thus treat the given attribute hash as the full resource.

Would the rails core team by interested in the addition of this API? I'm willing to take a stab at it, and can open another ticket to discuss further details, but figured it was worth inquiring here first.

Contributor

myronmarston commented Dec 22, 2011

I hinted at this above, but to reiterate: besides changing rails so that it supports PATCH, I'd also like to see improved support for proper PUT semantics. Currently, rails idioms and conventions do not make this easy. Specifically:

  • The common AR method used for updates is update_attributes. It supports updates of the full representation, but it also supports partial updates, which, as we've discussed above, is a violation of PUT semantics (at least in the mind of Fielding and other HTTP experts).
  • update_attributes also supports non-idempotent updates when used in combination w/ accepts_nested_attributes_for.
  • I think the simplest way to provide an update method that does provide proper PUT semantics is a new method, replace_attributes. It would set any attribute that is not included in the hash to nil, and thus treat the given attribute hash as the full resource.

Would the rails core team by interested in the addition of this API? I'm willing to take a stab at it, and can open another ticket to discuss further details, but figured it was worth inquiring here first.

@dlee

This comment has been minimized.

Show comment
Hide comment
@dlee

dlee Dec 22, 2011

Contributor

I like that idea; it'd make adhering to HTTP much easier.

I'd prefer a method name that more strongly indicates the full-resource replacement. What do you think about full_replace, replace_all_attributes, replace_resource, replace_with, or simply replace? An even stronger word might be supplant, but that just doesn't have a nice ring to it.

Contributor

dlee commented Dec 22, 2011

I like that idea; it'd make adhering to HTTP much easier.

I'd prefer a method name that more strongly indicates the full-resource replacement. What do you think about full_replace, replace_all_attributes, replace_resource, replace_with, or simply replace? An even stronger word might be supplant, but that just doesn't have a nice ring to it.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Dec 22, 2011

Contributor

If there would be a method, I would simply call it replace_attributes. BUT in my opinion and experience it doesn't make sense at all to have a method like this in Rails because replace_attributes would be tied to the model while its behavior should actually be given by the resource (which is then defined by the application). Remember that different actors in the system (for example user and admin) may see the same model as two different resources where PUT would behave differently for each. Even without those actors, a single model may be exposed as two different resources or vice-versa.

In other words, replace_attributes (or anything like that) would be a poor fit and ultimately lead to poorly designed APIs as people would believe that a call to replace_attributes would be everything you would need to have proper PUT semantics.

Contributor

josevalim commented Dec 22, 2011

If there would be a method, I would simply call it replace_attributes. BUT in my opinion and experience it doesn't make sense at all to have a method like this in Rails because replace_attributes would be tied to the model while its behavior should actually be given by the resource (which is then defined by the application). Remember that different actors in the system (for example user and admin) may see the same model as two different resources where PUT would behave differently for each. Even without those actors, a single model may be exposed as two different resources or vice-versa.

In other words, replace_attributes (or anything like that) would be a poor fit and ultimately lead to poorly designed APIs as people would believe that a call to replace_attributes would be everything you would need to have proper PUT semantics.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 22, 2011

Contributor

You bring up good points, @josevalim. I agree that people tend to think "model == HTTP resource" (or not even really think in terms of HTTP resources) and replace_attributes may seem to implicitly condone that line of thinking.

That said, rails doesn't currently have anything that helps make it easy to implement proper PUT semantics. If you want to implement proper PUTs you are essentially totally on your own. Do you have a better suggestion? (Also, should we take this to another thread? I don't want to derail the discussion here, but it's certainly related).

As I see it, replace_attributes isn't the perfect answer to HTTP spec or REST compliance any more than the rails scaffolds are. But it is a tool that can help make it easier to implement proper semantics (FWIW--I implemented this in one of my models on a recent project, and it worked great).

Contributor

myronmarston commented Dec 22, 2011

You bring up good points, @josevalim. I agree that people tend to think "model == HTTP resource" (or not even really think in terms of HTTP resources) and replace_attributes may seem to implicitly condone that line of thinking.

That said, rails doesn't currently have anything that helps make it easy to implement proper PUT semantics. If you want to implement proper PUTs you are essentially totally on your own. Do you have a better suggestion? (Also, should we take this to another thread? I don't want to derail the discussion here, but it's certainly related).

As I see it, replace_attributes isn't the perfect answer to HTTP spec or REST compliance any more than the rails scaffolds are. But it is a tool that can help make it easier to implement proper semantics (FWIW--I implemented this in one of my models on a recent project, and it worked great).

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Dec 22, 2011

Contributor

How Rails doesn't provide anything? Rails is a framework, anyone can provide his own PUT setup according to his application semantics, as you did. And probably in less than 30LOC! Please start a new discussion as I have no interest in the direction this is going.

Contributor

josevalim commented Dec 22, 2011

How Rails doesn't provide anything? Rails is a framework, anyone can provide his own PUT setup according to his application semantics, as you did. And probably in less than 30LOC! Please start a new discussion as I have no interest in the direction this is going.

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Dec 22, 2011

Member

Yeah, me neither.

Resources are conceptual, ARs model database tables. There's a gap there that I think would show as a complicated setup for such a simple thing.

I prefer to discuss that in another issue or PR.

Member

fxn commented Dec 22, 2011

Yeah, me neither.

Resources are conceptual, ARs model database tables. There's a gap there that I think would show as a complicated setup for such a simple thing.

I prefer to discuss that in another issue or PR.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 22, 2011

Contributor

@josevalim and @fxn: no need for another ticket. Two core rails members have weighed in against the idea. Thanks for the input. That's one less open source contribution I need to worry about; I have enough on my plate as it is.

Contributor

myronmarston commented Dec 22, 2011

@josevalim and @fxn: no need for another ticket. Two core rails members have weighed in against the idea. Thanks for the input. That's one less open source contribution I need to worry about; I have enough on my plate as it is.

@arunagw

This comment has been minimized.

Show comment
Hide comment
@arunagw

arunagw Feb 22, 2012

Member

#5130 merged.

Member

arunagw commented Feb 22, 2012

#5130 merged.

@miguelsan

This comment has been minimized.

Show comment
Hide comment
@miguelsan

miguelsan Mar 28, 2012

In the risk on being pedant by arguing on a closed discussion against the last three comments (@josevalim, @fxn and @myronmarston), where I even haven't any personal use case yet, I will still make my point here, since I don't know where it goes better.

Rails does not provide a clean way to map resources to models "to and fro", which makes josevalim's point somewhat valid. I mean, devs usually call a model into a variable (normally in the controlle's show method), but they might as well make a mix of some related models to represent the resource that they want to expose. In this second case, they then have to rip the params off in the update action, so that they get the different models to validate and save and so on. That's what I see as a weak part of the framework. I feel that josevalim's argument could backing up a kind of "presenters-and-back" as a best way to translate/map models into resources, and that the implementation of such a paradigm into Rails should be seriously weighted. Were they dismissed (which I suppose, for that is a whole subject in itself), I would then come to my conclusion: I dislike disregarding a new method called replace_resource , which sets all empty attributes to nil (or blank, depending on validation rules), supposed that it gets applied on a single model. You can afterwards put all needed warnings in the documentation about the semantic meaning of it. I mean, Rails 4 is going to promote a new verb just to adhere to standards, without any appreciable gain in the short term, just for the sake of it? Do you really mean that you are going to leave devs on their own, having to understand why to use this or that, without further help? "Boy, if you want to make proper use of this new feature that we advocate, you'll have to write your own methods for it, it's just 30LOC, boy. After all, Rails is just a framework, don't expect too much from us."

See my point? I know I am a bit sarcastic at giving reasons for my petition, but I do it just in order to make it crystal clear and avoid another six months of discussion on the need of such method. I hope I got it.

PS. Sorry for coming in so late, I just saw the thread linked in the Rails Blog. I got through the whole of it, oh my!

miguelsan commented Mar 28, 2012

In the risk on being pedant by arguing on a closed discussion against the last three comments (@josevalim, @fxn and @myronmarston), where I even haven't any personal use case yet, I will still make my point here, since I don't know where it goes better.

Rails does not provide a clean way to map resources to models "to and fro", which makes josevalim's point somewhat valid. I mean, devs usually call a model into a variable (normally in the controlle's show method), but they might as well make a mix of some related models to represent the resource that they want to expose. In this second case, they then have to rip the params off in the update action, so that they get the different models to validate and save and so on. That's what I see as a weak part of the framework. I feel that josevalim's argument could backing up a kind of "presenters-and-back" as a best way to translate/map models into resources, and that the implementation of such a paradigm into Rails should be seriously weighted. Were they dismissed (which I suppose, for that is a whole subject in itself), I would then come to my conclusion: I dislike disregarding a new method called replace_resource , which sets all empty attributes to nil (or blank, depending on validation rules), supposed that it gets applied on a single model. You can afterwards put all needed warnings in the documentation about the semantic meaning of it. I mean, Rails 4 is going to promote a new verb just to adhere to standards, without any appreciable gain in the short term, just for the sake of it? Do you really mean that you are going to leave devs on their own, having to understand why to use this or that, without further help? "Boy, if you want to make proper use of this new feature that we advocate, you'll have to write your own methods for it, it's just 30LOC, boy. After all, Rails is just a framework, don't expect too much from us."

See my point? I know I am a bit sarcastic at giving reasons for my petition, but I do it just in order to make it crystal clear and avoid another six months of discussion on the need of such method. I hope I got it.

PS. Sorry for coming in so late, I just saw the thread linked in the Rails Blog. I got through the whole of it, oh my!

@miguelsan

This comment has been minimized.

Show comment
Hide comment
@miguelsan

miguelsan Mar 29, 2012

I elaborated a bit more on the subject of ResourceAccessor and how and why the replace_resource method is needed..

miguelsan commented Mar 29, 2012

I elaborated a bit more on the subject of ResourceAccessor and how and why the replace_resource method is needed..

@bf4

This comment has been minimized.

Show comment
Hide comment
@bf4

bf4 Feb 11, 2014

Contributor

fwiw, I backported the HTTP PATCH verb work for Rails 3.2 https://gist.github.com/bf4/8940203

Contributor

bf4 commented Feb 11, 2014

fwiw, I backported the HTTP PATCH verb work for Rails 3.2 https://gist.github.com/bf4/8940203

@nextofsearch

This comment has been minimized.

Show comment
Hide comment
@nextofsearch

nextofsearch Dec 1, 2016

Hi @bf4,

Your patch doesn't work with my app. Rails 3.2.22.5

It crashes when running the web server with the following error:

.rvm/gems/ruby-1.9.3-p551@rails3.2/gems/actionpack-3.2.22.5/lib/action_dispatch/routing/mapper.rb:178:in `default_controller_and_action': missing :action (ArgumentError)

When running 'rake routes'

rake aborted!
ArgumentError: missing :action
/Users/danolee/.rvm/gems/ruby-1.9.3-p551@rails3.2/gems/actionpack-3.2.22.5/lib/action_dispatch/routing/mapper.rb:178:in `default_controller_and_action'

Any help will be greatly appreciated.

nextofsearch commented Dec 1, 2016

Hi @bf4,

Your patch doesn't work with my app. Rails 3.2.22.5

It crashes when running the web server with the following error:

.rvm/gems/ruby-1.9.3-p551@rails3.2/gems/actionpack-3.2.22.5/lib/action_dispatch/routing/mapper.rb:178:in `default_controller_and_action': missing :action (ArgumentError)

When running 'rake routes'

rake aborted!
ArgumentError: missing :action
/Users/danolee/.rvm/gems/ruby-1.9.3-p551@rails3.2/gems/actionpack-3.2.22.5/lib/action_dispatch/routing/mapper.rb:178:in `default_controller_and_action'

Any help will be greatly appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment