Code Monkey home page Code Monkey logo

Comments (37)

fxn avatar fxn commented on May 13, 2024 3

While PATCH should definitely be in the radar, let me say that Rails does not force you to do partial updates.

For example, if an invoice has a "paid" flag, and you have a check box to toggle it via Ajax, it is up to you which request is that. In particular, a REST design would have the "paid" flag of an existing invoice exposed as a resource, with its own URI and actions. The fact that the boolean may belong to the invoices table is irrelevant from a REST viewpoint.

So, in my view the current features are fine, I would not change the default method for edit actions.

How to add support for PATCH to the framework is something to be discussed.

from rails.

krekoten avatar krekoten commented on May 13, 2024

In most cases you are actually replacing data and not updating it. Your code renders full resource data, user modifies it and sends it back. That is what PUT designed for. PATCH meant to be used when partial data sent back to server.

Also I do not agree that update action is not idempotent.

from rails.

dlee avatar dlee commented on May 13, 2024

I would argue that the vast majority of users implement update as "updating" the resource and not "replacing" it. Even the word "update" says, "update".

Furthermore, convention and the myriads of Rails examples (official and unofficial) show a controller's update calling an ActiveRecord object's update, which is definitely meant to be PATCH-type action and not a PUT-type action.

from rails.

dlee avatar dlee commented on May 13, 2024

@fxn I would agree to a certain degree that Rails does not force you to do partial updates. ActiveRecord's semantics for update is partial updates, and that's the method called the majority of time in an "update" action.

Regardless of whether it's forced, it is acceptable to do partial updates in Rails, and that means we should not be using PUT, because it is **un__acceptable to do partial updates in PUT.

from rails.

dmathieu avatar dmathieu commented on May 13, 2024

Based on the way most forms currently exists, I believe 97% of currently PUT requests should be PATCH.

Please don't forget though that this is a very big change, for any application. It shouldn't be applied on a minor version.
I don't see this kind of change in any version earlier to 4.0.

from rails.

fxn avatar fxn commented on May 13, 2024

@dlee but what do you understand by "replacing"?

The "body" of a resource is something you define. In particular the ID or whatever identifier you use in the URL does not need to change. The same way the name of a static file whose contents you are "replacing" need no change.

You are not forced by HTTP to delete and create database rows. Database rows are implementation. Resources are conceptual. The standard update_attributes idiom is OK.

from rails.

krekoten avatar krekoten commented on May 13, 2024

@fxn that is what I was trying to say, so +1 :)

from rails.

dlee avatar dlee commented on May 13, 2024

@dmathieu My proposal is to put PATCH alongside PUT in the routes and to make PATCH the default method in forms. This should not break backwards compatibility for the majority of applications.

Later releases of 3.x can then deprecate the PUT. 4.0 can remove it completely.

@fxn I agree that concept and implementation are two different things. I argue that in Rails, the concept of "update" means partial updates. I also agree with dmathieu that 97% of how "update" is used is in the partial update way.

from rails.

fxn avatar fxn commented on May 13, 2024

@dlee there's no disagreement in that Rails does not support PATCH.

from rails.

dlee avatar dlee commented on May 13, 2024

@fxn rereading your comment, I think you're misunderstanding what I meant by "replacing" the content on a PUT. I'm not saying that PUT should issue a new URI or a resource ID. I'm saying that the contents of the resource are completely replaced (not updated or patched) by a PUT.

from rails.

fxn avatar fxn commented on May 13, 2024

@dlee do you agree that if you get the full public resource representation (all public fields, no ID, no timestamps), you are updating the resource with update_attributes as per PUT semantics?

from rails.

fxn avatar fxn commented on May 13, 2024

@dlee another thought: if it was true that PUT can be seen as a particular case of PATCH, then I agree with your proposal in principle, including the change in the default.

from rails.

dlee avatar dlee commented on May 13, 2024

@fxn If you are replacing every single field of a resource, and you do that every time without exception, then I agree that that's PUT semantics.

However, if you only update some of the fields, then that's PATCH semantics.

from rails.

dlee avatar dlee commented on May 13, 2024

Hopefully the nail in the coffin from the HTTP spec (emphasis mine):

_The PUT method is already defined to overwrite a resource
with a complete new body, and _cannot be reused to do partial changes.**

from rails.

fxn avatar fxn commented on May 13, 2024

@dlee Yes I know.

My only concern is that the RFC does not seem to allow partial bodies, or at least that's not totally clear to me. It says you need to send a description of how to modify the resource... Do you have real world example of valid PATCH requests?

from rails.

dlee avatar dlee commented on May 13, 2024

@fxn in my understanding, nothing needs to change from the way we currently use POST params. The body of a PATCH request is system-specific and unspecified in the HTTP spec.

from rails.

fxn avatar fxn commented on May 13, 2024

@dlee if that's the case PUT seems to be unnecessary for most web programming, you could perfectly update always using PATCH, partial or full, doesn't matter most of the time.

from rails.

dmathieu avatar dmathieu commented on May 13, 2024

We could then keep the default update action, which would by default support PUT and PATCH.
And if the controller responds to an other action (like replace for example), it'd be used for PUT actions and only update would be used for PATCH.

This would keep backward compatibility and allow advanced customization of the thing.

from rails.

dlee avatar dlee commented on May 13, 2024

@fxn yes, you should be able to use PATCH whenever you want to update a resource. The only thing PUT can do that PATCH cannot is to place a new resource at an explicit location:

PUT /posts/my_new_post

But this ability of PUT is not being used by Rails.

@dmathieu yes, that's my proposal. I think it should be possible in 3.x. In 4.0 PUT should by default not route to update

from rails.

fxn avatar fxn commented on May 13, 2024

@dlee You probably mean that's not what the resources macro produces. Rails does support that use case of PUT via the put macro for example.

from rails.

apotonick avatar apotonick commented on May 13, 2024

The problem is not PUT or PATCH but #update_attributes which lotta people use in their #update action. This model method is definitely not PUT-conform in a RESTful meaning, as it doesn't replace but extends. Here's a clarification: http://nicksda.apotomo.de/2010/12/rails-misapprehensions-understanding-restful-put-and-post/

from rails.

fxn avatar fxn commented on May 13, 2024

@apotonick This debate about PUT is old.

Before PATCH (and PATCH is very recent) there was no theoretical solution to partial updates. I was subscribed for a while to the REST mailing list and saw big names discussing about it. Consensus was, you can't. If you need to, be pragmatic and use something, for example PUT. Not pure, but there's no pure solution (other than definining ad-hoc resources and do proper PUTs to them).

So people using update_attributes for PUT are/were doing it right. It is the best you can do.

On the other hand, it was up to you to define REST-conformant interfaces. There's nothing wrong with the update_attributes method. If you are using update_atrributes for partial updates then it is the programmer who is not following strictly REST, not the AR method. You are responsible for your design, and there's nothing in Rails that forces you to do partial updates. The routing DSL is very rich.

Now that there's PATCH, we (all) need to start catching up. Rails has to address PATCH necessarily at some point.

from rails.

apotonick avatar apotonick commented on May 13, 2024

Of course, it's not the AR #update_attributes method which is wrong but the programmer who uses it in PUT and calls his interface "RESTful". I didn't know that PATCH is available in Rails now, and as "this debate about PUT is old" I will remain silent for now ;-)

from rails.

fxn avatar fxn commented on May 13, 2024

@apotonick not yet in Rails, this issue is proposing a roadmap for it :).

from rails.

dcrec1 avatar dcrec1 commented on May 13, 2024

The point is that Rails by default is patching the resource (update_attributes) when receiving a PUT request and doing nothing when receiving a PATCH one and that the patch will not affect anybody.

I think @dlee should be working on the patch right now, shouldn't he?

from rails.

fxn avatar fxn commented on May 13, 2024

@dcrec1 Rails does not do that by default. It depends on how the programmer uses the method. If you publish a REST API and your application is designed so that update actions only receive full updates, then everything is fine.

I'd wait green light/guidelines from someone in core before starting a patch.

from rails.

myronmarston avatar myronmarston commented on May 13, 2024

I agree that it would be good for rails to sort out partial updates vs. full replacement with PATCH and PUT.

A few people on this thread have mentioned that update_attributes can be used in combination with PUT if the client gets the full public representation of the resource, and includes that, with the desired changes, in the PUT request. This is true, but unless you control both the server and client code, there's no way to enforce this. The problem is that update_attributes only affects mentioned attributes, not all attributes included in the resource representation. So if a client does a put with a JSON hash like { "name": "John Doe"} and the resource also contains an age attribute, update_attributes will leave age set to the current value...but the semantics of PUT dictate that it should be a complete replacement, and therefore age should be set to nil (or, it is't invalid to let age be nil, the entire change should be rejected with an appropriate status code and explanatory message).

You can of course work around this, and write a custom replace method on your model that sets unmentioned attributes to nil, but it would be nice to have better out-of-the-box support in rails itself. I think we need a replace_attributes method as part of the ActiveModel API so that PUT requests can use this. PATCH requests could then continue to use update_attributes.

I think we could add at least come of this functionality without breaking backwards compatibility:

  • Add a replace_attributes method to the AM API.
  • Add support for the PATCH method to the router.

The hard part is finding a way to make the resource(s) declaration in the router support routing PATCH to Controller#update and PUT to Controller#replace without breaking backwards compatibility. Maybe it could be a configuration option that defaults to true in newly generated rails apps (kinda like the rails 3 default configs that got generated with new rails 2.3 apps)

from rails.

dcrec1 avatar dcrec1 commented on May 13, 2024

@fxn when you create a scaffold in Rails, this code is generated:

# PUT /cars/1
# PUT /cars/1.xml
def update
  @car = Car.find(params[:id])

  respond_to do |format|
    if @car.update_attributes(params[:car])
      format.html { redirect_to(@car, :notice => 'Car was successfully updated.') }
      format.xml  { head :ok }
    else
      format.html { render :action => "edit" }
      format.xml  { render :xml => @car.errors, :status => :unprocessable_entity }
    end
  end

This is the reason I said that by default Rails patches a resources when receiving a PUT request.

from rails.

dlee avatar dlee commented on May 13, 2024

How do we get the green light from core?

from rails.

fxn avatar fxn commented on May 13, 2024

@dlee I believe this definitely needs to be addressed.

We are totally focused on publishing a beta right now, have not talked a lot about it because of that but there are some +1s as first impression. I'd like to work on this for 3.2 (I was not in core when I wrote the comment!). Some discussion is needed about the design, I hope we can provide some concrete feedback soon.

Thanks! Will write back here.

from rails.

dlee avatar dlee commented on May 13, 2024

@fxn "discussion is needed" within core or here in this issue? I'd like to be involved in the discussion if possible. I'm itching to cook up this patch.

from rails.

fxn avatar fxn commented on May 13, 2024

@dlee green light to your proposal, PATCH all the way :). No need for a replace action.

from rails.

dlee avatar dlee commented on May 13, 2024

@fxn: sweet! For clarification, we still want the PUT backwards compatibility, right? Or by "PATCH all the way", did you mean no more PUT?

from rails.

fxn avatar fxn commented on May 13, 2024

@dlee Yeah, as you proposed: both PUT and PATCH route to #update for backwards compatibility. And default _method for editing existing records is patch. Also for completeness the routes and test APIs should have a patch method as well, etc.

from rails.

dlee avatar dlee commented on May 13, 2024

Basically, the question boils down to whether or not we want #resources to keep the PUT => controller#update route alongside the new PATCH => controller#update route.

from rails.

fxn avatar fxn commented on May 13, 2024

@dlee Not sure if I follow. Since REST routing is so fundamental, goal is full backwards compatibility. If you upgrade a 3.1 application, it just works. No matter whether you use #resources and helpers that encapsulate all of this, or ad-hoc put routes and manuals :method => :put in your views.

from rails.

josevalim avatar josevalim commented on May 13, 2024

Closing this as now we have a pull request with real code. :)

from rails.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.