Rails Ruby on Rails → 3.0.2

Comments and changes to this ticket

• You flagged this item as spam.
  

  Clemens Kofler July 10th, 2008 @ 08:07 PM

  • Tag set to “2.1, activerecord, bug, to_xml”

  I think that's by design. As far as I see it, Rails converts a few types into "special" types and handles everything else as a string:

  # taken from ActiveSupport::CoreExtensions::Hash::Conversions
  XML_TYPE_NAMES = {
            "Symbol"     => "symbol",
            "Fixnum"     => "integer",
            "Bignum"     => "integer",
            "BigDecimal" => "decimal",
            "Float"      => "float",
            "Date"       => "date",
            "DateTime"   => "datetime",
            "Time"       => "datetime",
            "TrueClass"  => "boolean",
            "FalseClass" => "boolean"
          } unless defined?(XML_TYPE_NAMES)
  

  Everything that isn't in this hash will automatically be turned into a string. I guess you could just call serialize the method yourself (which is not very DRY). But I honestly don't know - I think I wouldn't expose associations like this but rather use the :include option.

  Just my 2 cents, of course.

• You flagged this item as spam.
  

  Clemens Kofler July 10th, 2008 @ 08:09 PM

  Oops, didn't mean to change the tags - that just happened ... (but I guess it's right that way anyways)

• You flagged this item as spam.
  

  ronin-24025 (at lighthouseapp) July 10th, 2008 @ 08:35 PM

  Hi Clemens, thanks for the comment. When it comes to type conversion, Rails certainly uses this to elegant effect in the JSON class.

  However, the problem is that this is not a design issue (as applied to type conversion or content types). It's a data issue.

  In other words, JSON and XML are merely data formats. The data itself should not change. In this case it does.

  Rails to_json gets it right (Ruby array -> Javascript array). Rails to_xml gets it wrong (Ruby array -> concatenated string). to_xml gets resource collection arrays right. I see no reason why it can't get this right.

  But regardless, it's a problem because :methods => other_ids is in the Rails docs and is supposed to be able to handle this. And to prove the point, the newly revamped Rails 2.1 to_json (which added :include and :methods support as far as I know) can handle this.

  So it's actually quite a discrepancy. To resolve it, we either need to force to_json to concatenate any :methods => other_ids arrays to strings as to_xml does (which would serve no utilitarian purpose). Or update to_xml to the level that to_json is at in this regard.

  Re: using :include vs :methods, I do use :include in other resources, but not for many-to-many resources where it makes more sense to use id references, as these kinds of relationships can often be of a more categorical/static/lookup nature.

  In any event, the nature of REST is atomicity (i.e. smaller, simpler, lighter, factorized resources that can be cached and combined more easily with others in different ways as opposed to large data clumps). :methods is far closer to this ideal than :include.

  It's important we get this fixed.

• 

  josh October 28th, 2008 @ 04:31 PM

  • State changed from “new” to “stale”

  Staling out, please reopen if this is still a problem.

• You flagged this item as spam.
  

  ronin-24025 (at lighthouseapp) October 28th, 2008 @ 05:27 PM

  Yep, still a problem.

  Including the result of an other_ids method for JSON gives an array eg. [1,2,3,4,5]

  Including the result of the same other_ids method for XML gives a string eg. "12345"

  I'm not yet familiar enough with the workings of ActiveRecord to provide any form of patch.

• You flagged this item as spam.
  

  Garrett Bjerkhoel April 21st, 2010 @ 03:37 AM

  • Tag changed from “2.1, activerecord, bug, to_xml” to “2.1, 3.0, activerecord, bug, to_xml”

  Has this been fixed or is this not going to be addressed? I come up with this problem quite a bit and it'd be awesome if this was solved in Rails 3.

• 

  Neeraj Singh April 21st, 2010 @ 05:08 PM

  • Tag changed from “2.1, 3.0, activerecord, bug, to_xml” to “2.3.6, 3.0, activerecord, bug, to_xml”

  I recently migrated an app from using json to xml and got bit by this one. I will work on it and ,hopefully, will have a patch soon.

• 

  Neeraj Singh April 21st, 2010 @ 08:40 PM

  As Clements mentioned earlier any data type that is not in the mentioned list will be treated as string. Two major data types that are causing problems are hash and array.

  A recap of the issue.

  class User
  def lab

  ["a","b"]
  

  end def lab2

  {:foo => 'bar'}
  

  end end

  User.first.to_xml(:methods => [:lab, :lab2])
  abc
  foobar

  User.first.to_json(:methods => [:lab, :lab2])
  {lab {}lab2
  

  This issue exists in both 2.3.5 branch and rails3 edge.

  I understand that in rails community mostly people use json but "to_xml" should be as close to "to_json" as much possible.

• 

  Neeraj Singh April 21st, 2010 @ 08:42 PM

  lighthouse app team:

  Please put preview next to "Update Ticket". Even better would be a "live preview".

• 

  Neeraj Singh April 22nd, 2010 @ 04:02 PM

  • Assigned user set to “José Valim”

  Attaching a proof of concept patch for 2-3-stable branch.

  Please take a look and let me know. If I am heading in the right direction then I will add tests and will also provide patch for rails3 branch.

  Assigning this ticket to Jose Valim since he recently did some code cleanup on xml_serializer in rails3 branch. http://github.com/rails/rails/commit/9476daa829f7dd0681121e042dd135...

  • 2-3-branch.diff 2.9 KB
• 

  Neeraj Singh April 22nd, 2010 @ 04:07 PM

  With that above patch the output would be like this for hash and array instead of their to_s value.

  
{'john' : 'smith','foo' : 'bar'}
['a','b','c']


• 

  José Valim April 22nd, 2010 @ 04:11 PM

  • State changed from “stale” to “new”

  Today hash.to_xml already works and I would expect a result similar to it. About the array, the result described in this ticket description looks good.

• 

  Neeraj Singh April 22nd, 2010 @ 04:42 PM

  "hash.to_xml already works"

  Can you elaborate on that?

  #on 2-3-edge
  h = {:foo => 'bar'}
  => {:foo=>"bar"}
  h.to_xml
  => "<hash>\n  <foo>bar</foo>\n</hash>\n"
  

  hash.to_xml prints to_s value. However the values that are printed for hash when model.to_json is called is entirely different.

  Take a look at this case.

  class User
   def lab
     %w( a b c)
   end
   def lab2
    {:foo => 'bar'
   end
  end
  
  # current 2-3-edge
  User.first.to_xml(:methods => [:lab, :lab2])
  <user>
   <id type="integer">1</id>
   <name>john</name>
   <lab>abc</lab>
   <lab2>foobar</lab2>
  </user>
  
  # current 2-3-edge
  User.first.to_json(:methods => [:lab, :lab2])
  "{"user":{"lab2":{"foo":"bar"},"name":"john","id":1,"lab":["a","b","c"]}}"
  

  I think the value of to_xml for hash should match with the to_json value as much as possible.

  Thanks for quick update.

  Am I missing something here?

• 

  José Valim April 22nd, 2010 @ 04:46 PM

  I cannot tell Rails 2.3 behavior, but on Rails 3.0:

  { :a => { :b => :c} }.to_xml #=>
    "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<hash>\n  <a>\n    <b type=\"symbol\">c</b>\n  </a>\n</hash>\n"
  

  This is the behavior I expect when serializing a hash. It should not match JSON. It should match XML. The same for an array.

• 

  Neeraj Singh April 22nd, 2010 @ 04:58 PM

  Got it. I see what you mean. Thanks.

  In that case I will first work on a patch for rails3 and will later work on 2-3 branch.

• 

  Neeraj Singh April 22nd, 2010 @ 09:37 PM

  • Tag changed from “2.3.6, 3.0, activerecord, bug, to_xml” to “2.3.6, 3.0, activerecord, bug, patch, to_xml”

  Attaching a patch that takes care of rendering array and hash in proper format. The current patch is only for rails3. If all goes well then I will backport it to 2-3-branch.

  Tests are attached too.

  • to_xml_patch_against_rails3.diff 4.9 KB
• 

  José Valim April 23rd, 2010 @ 07:47 AM

  Hey, that's nice! We just need to wrap a few loose ends:

  1) Why you removed the yaml test?

  2) You should not handle the hash in a conditional inside "add_attributes". Try to use just XML_FORMATTING;

  3) The array formatting should output something like this:

    [1,2,3].to_xml #=> <socials type="array"><social>1</social><social>2</social><social>3</social></socials>
  

  4) And we should add XML_PARSERS, to ensure that arrays and hash can be parsed back.

  Good work!

• 

  Neeraj Singh April 23rd, 2010 @ 09:35 AM

  Thanks for the quick feedback. Will send you a new patch soon.

• 

  Neeraj Singh April 23rd, 2010 @ 03:43 PM

  Attached is a new patch.

  1) yaml test has been restored.

  2) For the "array" and "hash" data type the content is prebuilt and adding the content to the builder using tag! method escapes all the content. The only way to ensure that content is not escaped is to use "<<" and I could not find a better place to put the logic. If you find a better place where the logic should be shifted then do let me know.

  3) Array behaves as you suggested.

  4) Completely missed your 4th point when I read your message in the morning. Have not worked on implementing same features in XML_PARSERS. Will wait for your feedback on the work done so far. If everything looks good then I will work on parsing the content back.

  • to_xml_patch_against_rails3.diff 7 KB
• 

  José Valim April 25th, 2010 @ 09:08 AM

  Hey, just want to let you know that I'm discussing your patch with Jeremy Kemper and I hope to have feedback soon.

• 

  José Valim April 25th, 2010 @ 01:07 PM

  • Milestone cleared.

  Hey mate, your patch is walking in the proper direction, but I guess you misunderstood me about the 2) comment.

  Rails already knows how to convert array and hash types to XML and the build_tag method in your patch is kinda duplicating this behavior. Instead, it should be something like this:

      formatted_name = reformat_name(attribute.name)
      if attribute.raw_value.respond_to?(:to_xml)
        builder << attribute.raw_value.to_xml(options.merge(:root => formatted_name))
      else
        builder.tag!(
          formatted_name,
          attribute.value.to_s,
          attribute.decorations(!options[:skip_types])
        )
      end
  

  And now you are just going to use Array#to_xml and Hash#to_xml to do the hard work for you. You do not even need to write the formatters! A few gotchas I can think on top of my head:

  1) "attribute.raw_value" is not accessible, so you need to change that (just add it to the attr_accessor list);

  2) Array#to_xml only handles arrays where all the elements responds to to_xml. So for instance, [1,2,3].to_xml won't work. So you first need to change Array#to_xml implementation to handle [1,2,3].to_xml. Basically, you have to check if the element in the array responds to to_xml, if not, if there is a XML_FORMATTER and if not, just convert it to string (very similar as you did in your patch).

  After our discussion, I'm realizing that this change is really welcome! Feel free to break into different patches. You can for example create one initial patch that ensures [1,2,3].to_xml work as expected and with the proper tests in ActiveSupport test suite. And then you can change the ActiveModel implementation (the ActiveModel tests in the current patch looks great!).

  Finally, since we are talking about hashes and arrays, Rails already knows how to parse them so I guess there is no need to write XML_PARSERS (I was wrong earlier :)).

  If you have any doubts, please let me know!

• 

  Neeraj Singh April 25th, 2010 @ 02:10 PM

  Thanks for the detailed feedback.

  I should have checked if array respond to to_xml or not. Well now I know. :-)

  As you suggested, I will take baby steps and will create incremental patches. Will ping you in the mailing list if I have any concerns.

  Thanks again for the detailed feedback.

• 

  Neeraj Singh April 29th, 2010 @ 10:42 PM

  No code was required to fix this issue since all the work is now being done by to_xml itself. Look at this ticket for more information.

  https://rails.lighthouseapp.com/projects/8994/tickets/4490

  Attached is a patch which covers some test cases. Patch is against rails master.

  • test_cases_model_to_xml_against_rails_master.diff 3.1 KB
• 

  Repository April 30th, 2010 @ 01:28 PM

  • State changed from “new” to “resolved”

  (from [883f27aa9a6fae5194400bb8f00e343dcb610270]) test cases for record.to_xml [#458 state:resolved]

  Signed-off-by: José Valim jose.valim@gmail.com
  http://github.com/rails/rails/commit/883f27aa9a6fae5194400bb8f00e34...

• 

  Ryan Bigg October 9th, 2010 @ 09:55 PM

  • Tag cleared.

  Automatic cleanup of spam.

• 

  Jeremy Kemper October 15th, 2010 @ 11:01 PM

  • Milestone set to “3.0.2”

  [bulk edit]

• 

  Jeff Kreeftmeijer November 7th, 2010 @ 04:55 PM

  Automatic cleanup of spam.

• You flagged this item as spam.
  

  bingbing March 26th, 2011 @ 02:12 AM

  louis vuitton watches