[PATCH] ActiveResource find(:all) method returns "NoMethodError: undefined method `collect!' for #<Hash:0xb72fe790>"

What is the output from your controller? If you can, paste the output from curl http://localhost:3000/peoples.xml here.

Sorry ... should have done that the first time. Here's the output (but "people" is already plural so it's actually 'http://localhost:3000/people/xml' :-)

<?xml version="1.0" encoding="UTF-8"?>
<people>
  <person>
    <first-name>Craig</first-name>
    <hashed-password>3192448eb3c040ccbfddc6bcd574ca6c33a1f31b</hashed-password>
    <id>1</id>
    <last-name>McClanahan</last-name>
    <salt>NaCl</salt>
    <username>craigmcc</username>
  </person>
  <person>
    <first-name>New</first-name>
    <hashed-password>xyz</hashed-password>
    <id>2</id>
    <last-name>User</last-name>
    <salt>NaCl</salt>
    <username>newuser</username>
  </person>
</people>

The algorithms for hashed_password and salt are from AWDWR's "depot" example.

Enclosed below is a patch that works around this issue for me (and does not break any unit tests), but I do *not* recommend applying it directly. See the discussion below for a better approach.

Index: base.rb
===================================================================
--- base.rb	(revision 7155)
+++ base.rb	(working copy)
@@ -440,7 +440,41 @@
         end
         
         def instantiate_collection(collection, prefix_options = {})
-          collection.collect! { |record| instantiate_record(record, prefix_options) }
+#          collection.collect! { |record| instantiate_record(record, prefix_options) }
+          # Deal with the following use cases:
+          # * collection is actually an array (as the original code expected).
+          #   Default to the previous behavior
+          # * collection is a Hash with a single key/value pair with a key equal
+          #   to the element_name of this class.  There are two subcases:
+          #   - The value for this pair is an Array.  Perform the original
+          #     processing on this value.
+          #   - The value is a Hash.  That means the collection that was returned
+          #     contained only a single element.  Therefore, make an array that
+          #     contains only the result of instantiating the single row.
+          if collection.is_a? Array
+            collection.collect! { |record| instantiate_record(record, prefix_options) }
+          elsif collection.is_a? Hash
+            if collection.size == 1
+              value = collection.values[0]
+              if value.is_a? Array
+                # Assume the value contains an array of elements to create
+                return value.collect!  { |record| instantiate_record(record, prefix_options) }
+              elsif value.is_a? Hash
+                # Assume the value contains a single row result
+                result = []
+                result << instantiate_record(value, prefix_options)
+              else
+                raise ArgumentError "Expected a nested Array or Hash but got #{value.inspect}"
+              end
+            else
+              # Assume the result was a single row, so encapsulate it in an array
+              result = []
+              result << instantiate_record(collection, prefix_options)
+              return result
+            end
+          elsif
+            raise ArgumentError, "Expected an Array or Hash, but got #{collection.inspect}"
+          end
         end
 
         def instantiate_record(record, prefix_options = {})

IMHO, the real underlying issue is that ActiveResource::Connection#get is being a little too cute for its own good (probably the stuff that #from_xml_data is trying to do), coupled with the fact that this same method is being called for both a single element (Base#find_one) *and* a collection (Base#find_every). With the current implementation of #get for a collection, there are two different kinds of returns from #get for a #find_every call:

• If the returned collection has exactly one row, a Hash with a single element, with the key equal to the name of the model ("person" for my use case) and a value equal to a hash of the relevant attributes.

• If the returned collection contains multiple rows, a Hash with a single element, with a key equal to the name of the model ("person" for my use case) and a value equal to an *array* of hashes of the relevant attributes for each row.

Note that neither case is correctly handled with the unpatched edge code.

The patch above handles this by dealing with the variations in ActiveResource::Base#instantiate_collection, but it would be better to address this by having the lower level ActiveResource::Collection#get method return a Hash representation of the response's XML data that does not try to be "smarter than the average bear" (yes, I'm showing my age :-) about what the caller expects. It is the caller that has an understanding of whether the XML response should be interpreted as a collection or a single element, not the low level parsing code.

Much better patch for Ticket #8798

The attachment "8798-patch.txt" contains a much better patch for this issue, localizing the changes to the only method that seems to need changing. With this patch, all the current unit tests pass, plus the following scenarios (that used to fail) now work:

• Project.find(:all) that returns exactly one row
• Project.find(:all) that returns multiple rows
• Project.find(:first) that returns exactly one row

Thanks craigmcc, I've had the same problem and your patch fixed it.

<ActiveResourceModel>.find(:all)

caused the same problem.

Craig, In order to get this patch considered for inclusion one of us will need to add a test that fails without the patch applied and succeeds with the patch applied.

See: Patch Requirements

Do you have experience writing a test like that? I haven't but am willing to work one out unless you'd consider writing one.

FYI: this bug was not present in edge r7111 -- so something in ActiveResource changed that caused this bug to show up.

Sure ... I can do a test case for this patch. Look for an updated attachment soon.

I've attached a test case that shows the patch working. However, during development of this unit test, I realized that this most likely isn't the right way to fix this problem. The difference is subtle but very important, pointing to a possible bug in Hash#from_xml:

Works as of current Edge

<?xml version="1.0" encoding="UTF-8"?>
<people type="array">
  <person>
    <first-name>Craig</first-name>
    <hashed-password>3192448eb3c040ccbfddc6bcd574ca6c33a1f31b</hashed-password>
    <id>1</id>
    <last-name>McClanahan</last-name>
    <salt>NaCl</salt>
    <username>craigmcc</username>
  </person>
  <person>
    <first-name>New</first-name>
    <hashed-password>xyz</hashed-password>
    <id>2</id>
    <last-name>User</last-name>
    <salt>NaCl</salt>
    <username>newuser</username>
  </person>
</people>

Throws "collect! on Hash" error

<?xml version="1.0" encoding="UTF-8"?>
<people>
  <person>
    <first-name>Craig</first-name>
    <hashed-password>3192448eb3c040ccbfddc6bcd574ca6c33a1f31b</hashed-password>
    <id>1</id>
    <last-name>McClanahan</last-name>
    <salt>NaCl</salt>
    <username>craigmcc</username>
  </person>
  <person>
    <first-name>New</first-name>
    <hashed-password>xyz</hashed-password>
    <id>2</id>
    <last-name>User</last-name>
    <salt>NaCl</salt>
    <username>newuser</username>
  </person>
</people>

When there's a "type" attribute on the root node, Hash#from_xml builds the data into the proper type, otherwise it creates the "singular_name" => [{item1}, {item2}] structure that causes ARes to crash.

Can someone more knowledgeable about Hash.from_xml comment on this?

Test case proving the applied patch works.

Seems like patch still fails when using xml doc (sample from Wesabe API) like this:

<?xml version="1.0" encoding="UTF-8"?> <account>

<id type="integer">1</id> <account-number>1234</account-number> <name>Checking</name>

<txactions>

<txaction>

<date>2007-05-03</date> <amount>-89.72</amount> <merchant>

<name>Berkeley Bowl</name> <rating>Fan</rating>

</merchant>

</txaction>

</txactions>

</account>

Yes, I had also run into yet another case where this dies but didn't then have the time to update this ticket. Thanks for this XML, I've added a new patch with code and test that makes this new batch of XML parse properly.

For quick-fix sake, all you need to add is:

          elsif collection.is_a?(Hash)
            instantiate_record(collection, prefix_options)

to ARES::Base#instantiate_collection, right before the "else" statement.

This is the most recent fix and test of this bug.

Here's one verification: JasonRoelofs' patch fixes the problem on my systems, and all the tests pass.

Please don't release ARes w/2.0 without accepting this patch or otherwise fixing the bug. ARES::Base#find(:all) being completely broken seems like a big deal.

+1 (forgot the magic code in my earlier edit)

+1

I work at Wesabe, and we're using Builder::XmlMarkup to generate our XML for the API. hasmanyjosh dropped me a note about this incompatibility, and we're fixing it ASAP.

-1

You're using XML that does not comform to the ARes requirements. Collection elements need a type="array" attribute for the XML to parse correctly.

Which is why I said what I did, about this maybe not being the right place for the fix.

Josh, the XML I'm using that causes problems is built by ActiveSupport's #to_xml. So the problem is not me, it's something in Rails.

Josh: to_xml in Rails 1.2.3 doesn't include type="array"... So, are we just supposed to forget about using ARes with all the sites still using 1.2.3?

The change to include the array type attribute was introduced in [7074]. To get a correctly formed collection use the :root option like so:

[{ :id => 1, :name => 'Matz' }, { :id => 2, :name => 'David' }].to_xml(:root => 'people')

ARes is a 2.0 feature, so compatibility with pre 2.0 Rails is not guaranteed. Your patch might be a good place to start for creating that compatibility, but the whole thing sounds a bit iffy to me. There's a reason the XML needs the type="array" attribute, and without that it's impossible to guess what some configurations of XML mean.

That's all I was looking for. Thanks for the explanation. I'll just upgrade my server code to a newer version of Edge, should take care of the problem.

I'm reopening this ticket because the problem still exists on 2.0.

An up-to-date server generates the following XML:

<?xml version="1.0" encoding="UTF-8"?>
<taxes>
  <city-tax type="float">0.0</city-tax>
  <county-tax type="float">0.0</county-tax>
  <state-tax type="float">0.0</state-tax>
</taxes>

An up-to-date client reads this and throws our beloved error:

 undefined method `collect!' for {"county_tax"=>0.0, "city_tax"=>0.0, "state_tax"=>0.0}:Hash

Applying my patch attached to this ticket fixes the problem.

Please, there is still an issue here, whether it be in #to_xml or the proper fix is posted already in this ticket, I don't know. But for now, I have to be sure to apply this patch each time I upgrade the Rails code in my app or stuff breaks terribly.

hasmanyjosh's comment still applies. You need type="array" in the taxes element.

Then there's a bug with ActiveRecord::Base#to_xml. Here's the server side of the problem:

class TaxRate < ActiveRecord::Base
  # has city_tax, county_tax, state_tax

  def self.calculate_taxes_for(...)
    # Calculates out the appropriate taxes and puts them in the fields
    # returns new TaxRate with information
  end
end

class TaxesController < ApplicationController

  before_filter :login_required, :except => [:index]

  XML_OPTIONS = {:only => [:city_tax, :state_tax, :county_tax], :root => "taxes" }

  # GET taxes_url
  def index
    respond_to do |wants|
      wants.xml {
        render :xml => TaxRate.calculate_taxes_for(:city => params[:city], :state => params[:state], :zip => params[:zip], :amount => params[:amount]).to_xml(XML_OPTIONS)
      }
    end
  end
end

On Rails Edge r8413, doing a wget http://localhost:3000/taxes.xml gives me:

<?xml version="1.0" encoding="UTF-8"?>
<taxes>
  <city-tax type="float">0.0</city-tax>
  <county-tax type="float">0.0</county-tax>
  <state-tax type="float">0.0</state-tax>
</taxes>

As you can see, there is no type="array" in the output. If this is a new ticket, please let me know and I'll start one, but there definitely is a problem somewhere. If ActiveResource can't work without the "type" attribute, then ActiveRecord / ActiveSupport better be adding that attribute no matter what.

Replying to JasonRoelofs:

As you can see, there is no type="array" in the output. If this is a new ticket, please let me know and I'll start one, but there definitely is a problem somewhere. If ActiveResource can't work without the "type" attribute, then ActiveRecord / ActiveSupport better be adding that attribute no matter what.

There's no type="array" because your taxes object isn't an array. It looks like it might be a hash, but I can't tell without seeing your code that creates it. An array entity contains a uniformly named set of sub-entities, e.g. <taxes type="array"><tax>...</tax><tax>...</tax></taxes>. It looks like you're trying to serialize a hash instead. Give that a shot using the to_xml API correctly and see what happens. Closing the ticket, but reopen if you can provide a failing test case that is applicable.

Replying to hasmanyjosh:

Replying to JasonRoelofs:

As you can see, there is no type="array" in the output. If this is a new ticket, please let me know and I'll start one, but there definitely is a problem somewhere. If ActiveResource can't work without the "type" attribute, then ActiveRecord / ActiveSupport better be adding that attribute no matter what.

There's no type="array" because your taxes object isn't an array. It looks like it might be a hash, but I can't tell without seeing your code that creates it. An array entity contains a uniformly named set of sub-entities, e.g. <taxes type="array"><tax>...</tax><tax>...</tax></taxes>. It looks like you're trying to serialize a hash instead. Give that a shot using the to_xml API correctly and see what happens. Closing the ticket, but reopen if you can provide a failing test case that is applicable.

If you read his code and comments, it's pretty clear that he is returning "render :xml => TaxRate.new(stuff).to_xml(:only => [:city_tax, :state_tax, :county_tax], :root => "taxes" })". Is ARes failing to understand it because he's renamed the "root"? I can't see how ARes could infer the type with the root renamed.

For the record, if I don't rename the root it simply changes to:

<?xml version="1.0" encoding="UTF-8"?>
<tax-rates>
  <city-tax type="float">0.0</city-tax>
  <county-tax type="float">0.0</county-tax>
  <state-tax type="float">0.0</state-tax>
</tax-rates>

And yeah, I know it isn't an array, it's not supposed to be an array. It's supposed to be a simple hash of 3 values.

What exactly am I doing wrong here? Until someone shows me exactly what's going on, I'm convinced that this is a bug in the system!

Trac is not a support forum. I suggest you use the rubyonrails-talk email list, IRC, or some other forum where people can help you out with this.

Just got Rails 2.0.2 and looks like there are still problems with the NoMethodError getting thrown. I threw in some raise blocks and it looks like I am getting a hash back on line 438 of base.rb instead of array, hence the collect! error method from line 465.

I have a simple implementation of a class extending ActiveResource and my usage is something similar to this: s = FooResource.find(:all, :params=> {:key => "somekey2343453454", :keywords => "sushi", :market => "somemarket"}, :from => "/ws/w/foo_service"})

ActiveResource expects an array when receiving a collection. I'd really like to keep the amount of wacky code down if possible, if it's just for some edge cases. It's easy enough to modify ActiveResource through plugins if you want it to consume non-standard-rails rest apis.

Replying to niczak79:

Just got Rails 2.0.2 and looks like there are still problems with the NoMethodError getting thrown.

If you're going to report a bug, please include enough information for us to evaluate it. That would include a full failing test case, not just a piece of code with no context.

For everyone else, please make sure you are actually calling to_xml on an array, not something else. This ticket seems to be about people using incompatible versions of Rails and ActiveResource, or calling to_xml on a non-array and expecting ARes to treat it as an array. I'm closing this ticket until we see a proper failing test case.

Just to give anyone reading this thread the context of the problem I encountered is this:

1.) The service I am attempting to call resides on a Rails 1.2.3 stack and for all intensive purposes it conforms to all RESTful conventions that the Rails community accepts. 2,) I was building a plugin to consume this service on Rails 2.0.2 stack making use of and extending ARes 3.) The XML output of the service is not using ActiveRecord's to_xml method (and even if it did, this implementation would still break because there is no type="array" outputted in that version of Rails). So instead the web service is using XML Builders / RXML templates to strictly control and enhance the XML results. That said, it does not contain an attribute type="array" in the root node (and yes what I am outputting is truly an array and even if it did contain this attribute it still breaks because my XML isn't simply ActiveRecord to_xml).

IMHO I think many people won't just use ActiveRecord to_xml to provide a tried and true predictable RESTful web service, especially if anyone is serializing data in their tables or has legacy schemas to adhere to.

Meanwhile, I will be thinking of a way to enhance this to give back to the community.

There isn't any way to reliably guess what types XML is providing without a clear schema. Adding the type="array" attribute gives us just enough to do it reliably. You should look into writing a small mixin for your ActiveResource models that interact with your custom solution.

Replying to hasmanyjosh:

-1 You're using XML that does not comform to the ARes requirements. Collection elements need a type="array" attribute for the XML to parse correctly.

OUCH! That was like a knife in the heart!

Wouldn't this suggest the contrary? (from readme)

"Active Resource is built on a standard XML format for requesting and submitting resources over HTTP. It mirrors the RESTful routing built into ActionController but will also work with any other REST service that properly implements the protocol."

First of all, I'd be surprised if you can find any non-rails web service that returns xml with a type="array" attribute. Limiting ares to web services that implement type="array" is effectively cutting off every non-rails web service in the world. And the fact is, most of the web services out there are not impalement in rails, so I think this is a disappointing copout.

Ares is such a connivence to use, why do we want to derail (pun) the progression of ruby/rails with this requirement?

Am I totally off base here or what?

Replying to eggie5:

First of all, I'd be surprised if you can find any non-rails web service that returns xml with a type="array" attribute. Limiting ares to web services that implement type="array" is effectively cutting off every non-rails web service in the world. And the fact is, most of the web services out there are not impalement in rails, so I think this is a disappointing copout.

That is a valid point, and I'd love to see ARes be compatible with relevant web standards. Can you suggest an XML data packaging format that you think would work better? By the way, the rails-core email list may be a better place to have this kind of conversation.

Replying to technoweenie:

ActiveResource expects an array when receiving a collection. I'd really like to keep the amount of wacky code down if possible, if it's just for some edge cases. It's easy enough to modify ActiveResource through plugins if you want it to consume non-standard-rails rest apis.

Every non-rails based web service in the world is an edge case?

All you need is a plugin to change 1 method apparently, what's the big deal? I'd love to get rid of type="array" but it's necessary when you have no idea what the schema is.

Exhibit a:

  <txactions>
    <txaction>
      <date>2007-05-03</date> 
      <amount>-89.72</amount> 
      <merchant>
        <name>Berkeley Bowl</name> <rating>Fan</rating>
      </merchant>
    </txaction>
  </txactions>

In the test case, that's accessed with "got.txactions.txaction" Add one more txaction, and now txactions is an array. These are the kinds of edge cases I'm talking about.

My biggest gripe with this whole conversation is this:

Rails cannot read what Rails generates.

I am not doing anything at all special with XML generation. I'm using to_xml on an ActiveRecord::Base object specifying what fields to include. So any XML is generated by Rails itself. Then, the exact same version of Rails receives this XML and barfs.

Were I hand-generating XML, I would understand, but this is Rails tripping over it's own feet! Yes we can put together a plugin that would monkey-patch this one method, but it feels extraneous and almost a waste of time when this patch can just go into the code itself, doesn't break anything, and makes the ARes parser more robust.

What I am doing is *not* an edge-case, and that is why I keep pushing for this patch. If you don't want to make the change, please give a precise and logical reason, because all I've seen so far is "No, because it's you who is wrong".

You're hand generating it with an old version of rails. If you'll notice, the tests in ActiveResource go by rails-generated XML, except for your patch. It has to use hand-generated because it doesn't match what Rails generates anymore. Sorry for the change. I'm not a fan of it, but it's necessary to avoid issues like the one I highlighted above.

Just want to +1 eggie5 here.

Blocking all other APIs that don't happen to include type="array" is extremely naive and shortsighted. 99% of the time developers don't have control over the data they're consuming. If that's the _only_ defect in the returned XML shutting out these other APIs is a losing proposition.

The simplest solution for this problem is The problem occured when you get xml as <person>

<id>1</id>

</person>

for single record if anybody use Person.find(:all) then experienced this error because user need single record not collection so change this Person.find(:one)

for collection <persons type="array"> <person>

<id>1</id>

</person> </persons>

use Person.find(:all) or Person.find(:first)