This project is archived and is in readonly mode.
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
-
-
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.
-
-
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.
-
-
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 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 July 16th, 2008 @ 12:55 AM
Added a patch that fixes the whitespace errors added by formats_patch.diff
-
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 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 August 30th, 2008 @ 02:11 AM
- Assigned user changed from “Rick” to “Jeremy Kemper”
Could you rebase against master?
-
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 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 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 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 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 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.
-
-
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 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....
-
-
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 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 »
<h2 style="font-size: 14px">Tickets have moved to Github</h2>
The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>
People watching this ticket
Doug Ramsay
Jeremy Kemper
Michael Koziarski
Pratik
Rick
Tarmo Tänav
Wesley Moxam
Attachments
Tags
Referenced by
-
450
format patches to support serializing data out in the correct format with correct http request headers per http method type
(from [caabe228bc6c6e920043334d717e72093559e118]) Format ...