#2521 hash#to_xml :dasherize defaults to true - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#2521 new

hash#to_xml :dasherize defaults to true

Reported by John Small | April 19th, 2009 @ 02:54 PM | in 3.0.6

This is Rails 2.3.2 and going back to 2.2.2 The documentation in ActiveResource describes to_xml having the option :dasherize, which defaults to false. But in active_support/core_ext/hash/conversions.rb we have

def rename_key(key, options = {})

      camelize = options.has_key?(:camelize) && options[:camelize]
      dasherize = !options.has_key?(:dasherize) || options[:dasherize]
      key = key.camelize if camelize
      dasherize ? key.dasherize : key
    end

Where dasherize is defaulting to true on line 147

The consequence of this is that ActiveResource#to_xml and #encode both assume it's defaulting to false, when it isn't and so they will replace underscores with hyphens. It's not picked up in any tests on ActiveResource because the test cases don't use attributes with underscores and don't test to see that the field names aren't changed when you save the record. Hence if you read in an ActiveResource record from an external server not running Rails and the field names have underscores, you can't save it because the field names get changed to have hyphens in the POST data.

Probably the default should leave keys as they are and you have to add the option :dasherize=>true if you want it different.

Solution : remove the ! from !options.has_key?(:dasherize) and do things the same way camelize is done to be consistent.

I couldn't find the test suite for ActiveSupport where is it?

John Small

Comments and changes to this ticket

You flagged this item as spam.
John Small April 19th, 2009 @ 05:15 PM
Ok, I've found the tests for hash#to_xml added a test to check the default for :dasherize is false, fixed the code and now lots of things all over the place are broken.

It seems that a lot of code has been written with the expectation that hash#to_xml defaults to :dasherize=>true, even though that's obviously daft, you don't let the default change something unexpectedly, you should let the default leave things as they are. It violates the rule of "least surprise". The result is that { :name => "David", :street_name => "Paulina" }.to_xml contains <street-name> and not <street_name>.

Also the test for dasherize assumes the default for camelize is false (as it should be) but it shouldn't be implied in the test for dasherize. There's no test for camelize == false and no test for a default value for camelize and no test for what should happen if both camelize and dasherize are set to true.

I can see that hash#to_xml is setup to behave the same way that array#to_xml behaves, that is by default :dasherize=>true, so that when serializing an active record to XML underscores are changed to dashes. Is that a policy decision? That seems odd to me.

Acting on the premise of "least surprise" sending a record with underscores in the field names to xml and getting <created-at> in the xml when you expect <created_at> would certainly surprise me.

If it is a policy decision to change field names by default then it needs to be made clear in the documentation and we need to have a way to override the default behavior.

I'll argue with that choice but there's so much code written on the assumption that "created_at" becomes <created-at> when XMLized that it will have to be lived with unless there's a way to set it as a class default in RAILS initialization so that by default :dasherize=>false unless you set it to true on in Rails initialization.

Currently I've got no way to override the default behavior when I save an ActiveResource. This will cause problems in future when Rails starts to be used more for integration with non-Rails systems. At the moment Rails is an island so it doesn't matter much. But this will become known as an annoying "Rails quirk" when more people start doing system integration using ActiveResource.

Comments please
You flagged this item as spam.
Taryn East April 20th, 2009 @ 11:51 AM
...no Rails is an island

We're currently running into this problem right now trying to integrate with another API and it's a right pain to force it to use dashes instead of underscores. I'd be happy with just some config option where I can tell ActiveResource to always use underscores, but right now there seems to be no way to do this.
You flagged this item as spam.
John Small April 20th, 2009 @ 01:04 PM
Mmm, well at least I know that it's not just me begin ignorant about some undocumented feature that everyone else knows about.

I've raised this issue in the Rails-Core google group. I've worked out what the fixes are and am ready to push my changes, but there's an awful lot of code out there, and in the Rails test suite that assumes "created_at" will become "created-at" when converted to XML. It seems daft to me, but it's been in the code for a long time so there might even be a rationale behind it.

I suspect that this is something that's slipped through because there aren't thousands of people using ActiveResource & REST for systems integration. Though I need to find out if it's a policy choice of some sort, the absence of a test for default values makes me think that it's just something no one has thought about.

You might get a fix in the next few days. If you need the fix right now and don't mind lots of tests in the Rails test suite breaking then go to active_support/core_ext/hash/conversions.rb find def rename_key(key, options = {}) go to line 147 (as it was last night when I cloned the source) then change

dasherize = !options.has_key?(:dasherize) || options[:dasherize]

to

dasherize = options.has_key?(:dasherize) && options[:dasherize]

That will do the magic required to get your XML api working. The rest of my patch just adds the missing tests, and alters the tests for the things that break. I also need to add a way to set the default behaviour for those that prefer the existing default.
You flagged this item as spam.
John Small April 24th, 2009 @ 06:00 PM
I've attached a patch which allows you to set the default behaviour for :dasherize in Hash#to_xml. This patch covers

1) Extra tests for the default values of :dasherize and :camelize 2) Make sure that the tests for each values of :dasherize and :camelize make no assumptions about the default value of the other one. E.g. the tests for :dasherize true and false, set :camelize=>false in the option, and the tests for :camelize true and false set :dasherize=>false in the options.

3) added cattr_accessor for :dasherize_xml and :camelize_xml. In this patch Hash.dasherize_xml = true and Hash.camelize_xml = false to work with existing code. But this allows people to set Hash.dasherize_xml = false so ActiveResource doesn't dasherize underscores when sending XML back to non-Rails services.

For existing users who expect underscores to be converted to dashes in Hash#.to_xml they don't have to do anything. For users that need the default to be correct, they can set Hash.dasherize_xml = false in an initializer.

Because the default should be to leave things alone I recommend that in future the default should be changed to Hash.dasherize_xml = false.

Because Ruby cannot use dashes in attribute names incoming XML with dashes in the tag names must be converted to underscores. But the converse is not true, outbound XML from attributes with underscores in the names does not need to have the underscores converted into dashes. If XML comes in with underscores it should go back out with underscores.

If you have both dashes and underscores in your XML tags then you're in trouble and this patch won't help. Try XSLT.
You flagged this item as spam.
John Small April 24th, 2009 @ 06:03 PM
- Tag changed from “2.3.2, activeresource, activesupport, core_ext, dasherize, hash#to_xml” to “2.3.2, activeresource, activesupport, core_ext, dasherize, hash#to_xml, patch”
You flagged this item as spam.
John Small April 25th, 2009 @ 07:21 AM
Whoops I missed off the changed documentation to ActiveResource#to_xml. Here's the updated patch with the changed documentation.

The old documentation incorrectly said that the default value of :dasherize was false, in fact it was and still is true. Which is the source of our problems when integrating ActiveResource with non-Rails applications. The new documentation says that the default is true, and tells people how to change the default if they need to so they can integrate with non-Rails apps. I've also added documentation for the :camelize option, which was missing.
- allow_setting_default_dasherize_on_hash_to_xml.diff 9.7 KB
Michael Koziarski May 7th, 2009 @ 02:07 AM
- Assigned user set to “Michael Koziarski”
- Milestone cleared.
Awesome stuff, looks good apart from a few minor things.

I don't know that I'm sold on using the class vars on Hash, perhaps instead something like we do with JSON.
```
ActiveSupport.use_standard_json_time_format = true
```
Also you don't need the begins in your test methods
```
  def test_something 
    #...
  ensure
    # ...
  end
```
Finally can you rebase it down to a single patch, and provide it for 2.3?
Michael Koziarski May 7th, 2009 @ 02:07 AM
Sorry if it wasn't clear but I meant 2.3 as well as master, not instead of it :)
You flagged this item as spam.
John Small May 7th, 2009 @ 01:16 PM
Ok do you want it based of off 2.3.0 or 2.3.2, the latest stable release?

The layout of module and class containers has changed a lot from 2.3.3 to current edge hasn't it.

Let me know which basis you want and I'll do it asap.
CancelProfileIsBroken May 7th, 2009 @ 01:45 PM
You should pull the 2-3-stable branch from github, and base 2.3 patches on the edge of that branch.
Michael Koziarski May 7th, 2009 @ 05:46 PM
Like mike said, base the 2.3 patches off 2-3-stable and the master patches off master. You'll probably have to upload two patches but hopefully git-cherry-pick will help you.
You flagged this item as spam.
John Small May 8th, 2009 @ 09:45 AM
Ok, here are the patches, three in one file. I've done two with the attributes dasherize_xml and camelize_xml added to ActiveSupport as requested. Then I've done one for edge only with dasherize_xml and camelize_xml on the Hash class. Putting attributes on ActiveSupport, when the attributes are only required for the Hash class doesn't look right to me.

In 2-3-stable it kind of makes sense because of the way the modules are arranged and the fact that the code in 2-3-stable has Hash as a module contained in the ActiveSupport module.

In the development since 2-3-stable the modules have been re-arranged and the conversion code for Hash is now attached to the class and as far as I can see the Hash class doesn't live inside the ActiveSupport module. In the code I've had to introduce the ActiveSupport module into conversions.rb, but it sits outside the Hash class. It looks very wrong to me, but maybe it's a Rails convention to do things like that. If so I'd question the convention.

The patch 2-3-stable_hash_to_xml_patch.diff is the patch for 2-3-stable (obviously) with mattr_accessors on ActiveSupport

The patch edge_AS_hash_to_xml_patch.diff is the version for edge with mattr_accessors on ActiveSupport

The patch edge_hash_to_xml_path.diff is the version for edge with cattr_accessors on Hash

I've removed the extraneous 'begin' from the tests for default values in all variants of the patches.

I'll also start looking at what needs to be done to bring ActiveRecord into line with ActiveResource in .to_xml. It seems the code base between the two has diverged.

Thanks for adding this patch. We'll now be able to read-write via REST to non-Rails apps without having to go through XSLT.

Cheers

John Small
- hash_to_xml_patches.tar.gz 3.6 KB
Michael Koziarski May 10th, 2009 @ 02:14 AM
- Milestone cleared.
This went into 2.3 as of 7bf9bf3

Moving the milestone to 3.0.

Do other contributors have any thoughts on the two options here for configuration options for core_ext stuff?
```
  Hash.dasherize_xml
```
OR
```
  ActiveSupport.dasherize_xml
```
The second is consistent with other 2-3isms, but for 3.0 and forward do we want to move it?
You flagged this item as spam.
John Small May 10th, 2009 @ 08:10 PM
I can see the benefit of keeping all the options/attributes for mixins to Ruby core modules that are introduced by ActiveSupport attached to the ActiveSupport module. It makes it very clear that you're altering the behaviour of something that's not part of standard Ruby, but part of something added by ActiveSupport. But I still feel ill about adding an attribute that's intended solely for an extension to Hash to something outside of Hash and which isn't up the inheritance chain. It breaks the idea of encapsulation of object behaviour. As you can see from the code, it looks really weird having to open ActiveSupport inside conversions.rb just to attach some new attributes to the module, and it looks just as weird having to refer to an outside attribute while inside Hash#rename_keys to determine the output.

There must be other instances like this dotted about in the code and as you mentioned in your post, it's the way things have been done in the past, but it might not be a bright idea for the future.

Maybe something like;-
```
ActiveSupport::CoreExt::HashMixins.dasherize_xml
```
That keeps the attribute localised to the mixins added to Hash, and at the same time makes sure that if you set the thing you know you're setting an attribute introduced as part of the ActiveSupport extensions to Ruby core. It would require re-adjusting the modules though, and they've just been through an extensive house cleaning exercise.

I'm all in favour of extremely descriptive variable names even if it requires more typing. I can well see that a few months or years hence someone who hasn't read this conversation will be looking at
```
Hash.dasherize_xml = false
```
in someone's code and thinking "dasherize_xml!? wtf is that? That's nowhere to be found in the Ruby documentation for Hash!" and they'd then have to spend ages googling about to work out what it is and why it's there.
You flagged this item as spam.
John Small May 11th, 2009 @ 07:59 AM
Ho hum. I've just downloaded the new 2-3-stable and found that someone has changed something in XmlMini so that parsing XML preserves case in element names, which it should, but loading ActiveResource hasn't been changed to compensate and now I'm getting attributes starting with upper case which isn't the Ruby convention.

I feel a new ticket coming on but for now I'm going to bodge things via XSLT on my server.
Michael Koziarski May 14th, 2009 @ 07:09 AM
- Assigned user changed from “Michael Koziarski” to “Jeremy Kemper”
Assigning to jeremy who has been active in the 3.0 core_ext rejigging.
You flagged this item as spam.
Chris Hapgood November 2nd, 2009 @ 06:11 PM
Am I missing something, or is the "symmetricity" of Hash.to_xml and Hash.from_xml still broken even with the latest patch on this ticket?

We've introduced the configuration options "dasherize_xml" and "camelize_xml" but we don't apply them in the #unrename_keys method, only in #rename_keys. Consequently, when reading from an ARes service that has an "itemId" attribute, there is no way to have that attribute converted to a Rails-conventional #item_id accessor. It used to be converted automatically and unconditionally before changeset ebe8dd61. Now I'm stuck with method_missing to approximate the same behavior.
Jeremy Kemper August 30th, 2010 @ 04:10 AM
- Milestone cleared.
- Importance changed from “” to “Low”
[bulk edit]
Jeremy Kemper October 15th, 2010 @ 11:01 PM
- Milestone set to “3.0.2”
[bulk edit]
Santiago Pastorino November 15th, 2010 @ 09:15 PM
- Milestone cleared.
[bulk edit]
Santiago Pastorino February 12th, 2011 @ 09:58 PM
- Milestone set to “3.0.5”
[bulk edit]
Santiago Pastorino February 27th, 2011 @ 03:15 AM
- Milestone changed from “3.0.5” to “3.0.6”
[bulk edit]