This project is archived and is in readonly mode.

#450 ✓ duplicate
rbpandey

format patches to support serializing data out in the correct format with correct http request headers per http method type

Reported by rbpandey | June 19th, 2008 @ 05:30 PM

Currently all data is serialized out in XML for PUT and POST requests even when you choose JSON as your exchange format. The simple fix was to remove all references when serializing data with the to_xml method and replace them to use a method which passes through the selected format's encode method (ex. ActiveResource::Formats::JsonFormat.encode). Please see the ActiveResource::Base.encode method in the patch.

Secondly, the proper format based http request headers were not being set. Constructive requests (POST/PUT) should be using the 'Content-Type' header to describe the format of their payloads, while DELETE and GET requests should be using the 'Accept' header to indicate what format of data they will accept from the reqeust in question. So you would have something like:

Content-Type:application/xml (for POST/PUT)

Accept:application/xml (for GET/DELETE)

Thirdly, this also helped me uncover that the CustomMethods "post" instance method was only serializing data to_xml when the body provided to that method was explicitly nil. This would never happen because the default "body" argument to that method was an empty String, hence data was never serialized to_xml.

Lastly, the http_mock.rb did not take into account request payloads, which are quite important to testing. I modified the mock to support a request_body parameter as well as the ability to override the format based request header for testing the various formats. It is backwards compatible with the current tests.

All the tests run.

Comments and changes to this ticket

  • rhg_mbrooks

    rhg_mbrooks June 19th, 2008 @ 05:37 PM

    +1. A very much needed change. Good work Rus.

  • Doug Ramsay

    Doug Ramsay June 19th, 2008 @ 05:43 PM

    +1.

    The changes to the request headers follow RESTful principles. And calling .nil? on the default empty string body argument was apparently an oversight.

  • jeff

    jeff June 19th, 2008 @ 06:28 PM

    +1 excellent - i could really use this in my current project!

  • rbpandey

    rbpandey June 19th, 2008 @ 07:49 PM

    The latest patch file includes an update to support parsing errors from both xml and json per the validations.rb class.

  • Ryan Daigle
  • Rick

    Rick July 4th, 2008 @ 07:53 AM

    • State changed from “new” to “open”
    • Tag set to activeresource, bug, patch
    • Milestone set to 2.1.1
    • Assigned user set to “Rick”

    Shouldn't #to_json in #encode take an options hash too? Also, is #encode really the right name for it? #serialize maybe?

  • rbpandey

    rbpandey July 7th, 2008 @ 04:32 PM

    Rick, good catch, I have attached another patch file which includes the to_json(options) call.

    As for the naming, I'll leave that for the community to decide. I prefer "serialize" myself as that is how I had named my original implementation, but after seeing DHH's implementation I didn't want to start renaming things.

  • Tarmo Tänav

    Tarmo Tänav July 16th, 2008 @ 12:55 AM

    Added a patch that fixes the whitespace errors added by formats_patch.diff

  • rbpandey

    rbpandey July 17th, 2008 @ 04:27 PM

    TT, I don't think you patched the latest patch. Thanks anyway though, I'll make the correction and submit a new patch. Thx, -RBP

  • rbpandey

    rbpandey August 12th, 2008 @ 07:39 PM

    i think TT patched the patch for whitespace issues. so we don't really need another patch.

  • Jeremy Kemper

    Jeremy Kemper August 30th, 2008 @ 02:11 AM

    • Assigned user changed from “Rick” to “Jeremy Kemper”

    Could you rebase against master?

  • Repository

    Repository August 30th, 2008 @ 02:47 AM

    • State changed from “open” to “resolved”

    (from [caabe228bc6c6e920043334d717e72093559e118]) Format related patches to support serializing data out in the correct format with correct http request headers per http method type [#450 state:resolved]

    Signed-off-by: Tarmo Tänav tarmo@itech.ee Signed-off-by: Jeremy Kemper jeremy@bitsweat.net http://github.com/rails/rails/co...

  • Wesley Moxam

    Wesley Moxam September 17th, 2008 @ 10:42 PM

    • Tag changed from activeresource, bug, patch to activeresource, bug, patch

    While this is probably a good idea overall, the patch stank.

    • breaks the API in a minor point release
    • documentation wasn't updated, so those browsing the docs are all "WTF, why doesn't this work?"

    Yet it got 3 (+1's) within an hour of being posted. That's hardly enough time to apply the patch, test it and review the code.

    The +1 system is broken. And API changes have to be taken a little more seriously. I thought there was a process in place for it (deprecate, then change in the next release).

  • Tarmo Tänav

    Tarmo Tänav September 17th, 2008 @ 11:47 PM

    • State changed from “resolved” to “open”
    • Milestone changed from 2.1.1 to 2.1.2

    Thank you for noticing this. Do you intend to prepare a patch adressing it?

  • Doug Ramsay

    Doug Ramsay September 17th, 2008 @ 11:47 PM

    Wesley,

    How about some details? How does the patch break the API? Perhaps you could provide a test that shows how it breaks the API.

    The original code had flaws in serializing and setting the headers, so I think a correction needed to be made in one form or another. Do you have a suggestion about how the solution could be better implemented? If so, I would be very interested in checking it out.

    Finally, the three people who posted +1's within an hour of it being posted all had used the fixes in the patch for several weeks prior to it being posted here.

  • Tarmo Tänav

    Tarmo Tänav September 18th, 2008 @ 12:39 AM

    Hi Doug,

    The API is broken by the removal of to_xml (being replaced with encode), it's in fact the first change in the patch, funny how of the many reviewers here noone noticed it :)

    As for the +1's, to me it doesn't seem fair to have people working in the same team pushing eachothers patches without stating their affiliation, especially due to the fact that being coworkers likely means they all tested it in similiar environments.

  • Doug Ramsay

    Doug Ramsay September 18th, 2008 @ 02:14 AM

    Fair enough, perhaps we should have indicated our affiliation. We obviously had no issues with the change while working in a similar environment.

    However, if patches are accepted purely on the basis of +1's, then Wesley is right - the system is seriously broken.

    The fact that a change that broke the API was missed by tests tells me I need to write more tests for ActiveResource. I'll work on that.

  • Tarmo Tänav

    Tarmo Tänav September 18th, 2008 @ 04:00 AM

    This patch actually seems to have triggered both #1011 and #1053, it would be great if someone who worked on this patch could help solve/review those tickets.

  • Michael Koziarski

    Michael Koziarski September 18th, 2008 @ 09:54 AM

    • Milestone cleared.

    I'm going to revert this change from 2-1-stable, the api breaking and change of behaviour with Accept headers isn't worth it.

    But these issues also need to be addressed in the 2.2 lifetime.

  • Michael Koziarski

    Michael Koziarski September 18th, 2008 @ 09:59 AM

    The breakage introduced here was accidental and could and should have been caught during the review process. But shit happens, let's fix it and move forward :) We're all on the same team here.

    Clearly this shows there's some test coverage sorely missing in active resource though....

  • Wesley Moxam
  • rbpandey

    rbpandey September 23rd, 2008 @ 08:55 PM

    I just submitted a fix for 1011. Please have a look. It looks like 1053 was already fixed in the master branch with the patch provided in the issue report.

  • Pratik

    Pratik October 17th, 2008 @ 05:22 PM

    • State changed from “open” to “duplicate”
    • Tag changed from activeresource, bug, patch to activeresource, bug, patch

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Tickets have moved to Github

The new ticket tracker is available at https://github.com/rails/rails/issues

Shared Ticket Bins

Referenced by

Pages