Skip to content

Commit

Permalink
Move several configuration values from Hash to ActiveSupport::XmlMini…
Browse files Browse the repository at this point in the history
…, which both Hash and Array depends on.

Also, refactored ActiveModel serializers to just use ActiveSupport::XmlMini.to_tag. As consequence, if a serialized attribute is an array or a hash, it's not encoded as yaml, but as a hash or array.
  • Loading branch information
josevalim committed Apr 29, 2010
1 parent 1bea5c7 commit 2e9af36
Show file tree
Hide file tree
Showing 13 changed files with 255 additions and 336 deletions.
2 changes: 2 additions & 0 deletions actionpack/CHANGELOG
@@ -1,5 +1,7 @@
*Rails 3.0.0 [beta 4/release candidate] (unreleased)*

* Both :xml and :json renderers now forwards the given options to the model, allowing you to invoke them as render :xml => @projects, :include => :tasks [José Valim, Yehuda Katz]

* Renamed the field error CSS class from fieldWithErrors to field_with_errors for consistency. [Jeremy Kemper]

* Add support for shorthand routes like /projects/status(.:format) #4423 [Diego Carrion]
Expand Down
162 changes: 51 additions & 111 deletions activemodel/lib/active_model/serializers/xml.rb
@@ -1,5 +1,6 @@
require 'active_support/core_ext/array/wrap'
require 'active_support/core_ext/class/attribute_accessors'
require 'active_support/core_ext/array/conversions'
require 'active_support/core_ext/hash/conversions'
require 'active_support/core_ext/hash/slice'

Expand All @@ -15,65 +16,29 @@ class Attribute #:nodoc:

def initialize(name, serializable, raw_value=nil)
@name, @serializable = name, serializable
@raw_value = raw_value || @serializable.send(name)

@value = value || @serializable.send(name)

This comment has been minimized.

Copy link
@andreas

andreas Sep 1, 2010

Contributor

Shouldn't this line be

@value = raw_value || @serializable.send(name)

or change the name of the argument from "raw_value" to "value"?

This comment has been minimized.

Copy link
@josevalim

josevalim Sep 1, 2010

Author Contributor

Yes, patch with tests please!

This comment has been minimized.

Copy link
@farnoy

farnoy Sep 1, 2010

Should it look like:
http://gist.github.com/561115 ?
It's my first patch, and I'm not sure it's correct.

This comment has been minimized.

Copy link
@josevalim

josevalim Sep 1, 2010

Author Contributor

Yes, that is it! Do you think you can add a test as well? How did you come up with this error?

This comment has been minimized.

Copy link
@farnoy

farnoy Sep 1, 2010

I've just read andreas's note and wanted to try create a patch, I had no error with this. I'll try to create some test tommorow.

This comment has been minimized.

Copy link
@andreas

andreas Sep 1, 2010

Contributor

This is the minimal example I can find to reproduce the error:

require 'active_resource'
class Foo < ActiveResource::Base
  self.site = ""
end

Foo.new(:test => nil).to_xml

This will throw an ArgumentError. The problem is that :test is a private method on Object, which will be invoked since "value" is always nil.

This comment has been minimized.

Copy link
@josevalim

josevalim Sep 2, 2010

Author Contributor

Awesome! Farnoy, you can add the example above as a test case to ActiveResource and then add a fix to ensure it is going to work!

This comment has been minimized.

Copy link
@andreas

andreas Sep 2, 2010

Contributor

Here's an attempt at a patch with test: http://gist.github.com/562146

This comment has been minimized.

Copy link
@farnoy

farnoy Sep 2, 2010

For me this function is not needed, this issue has been undetected for months now, Serializer structure has overloaded constructor method, and this one that we are trying to fix is not used:
http://gist.github.com/562383
From documentation.

EDIT:

andreas test won't work, as the second constructor is triggered by .to_xml

@type = compute_type
@value = compute_value
end

# There is a significant speed improvement if the value
# does not need to be escaped, as <tt>tag!</tt> escapes all values
# to ensure that valid XML is generated. For known binary
# values, it is at least an order of magnitude faster to
# Base64 encode binary values and directly put them in the
# output XML than to pass the original value or the Base64
# encoded value to the <tt>tag!</tt> method. It definitely makes
# no sense to Base64 encode the value and then give it to
# <tt>tag!</tt>, since that just adds additional overhead.
def needs_encoding?
![ :binary, :date, :datetime, :boolean, :float, :integer ].include?(type)
end

def decorations(include_types = true)
def decorations
decorations = {}

if type == :binary
decorations[:encoding] = 'base64'
end

if include_types && type != :string
decorations[:type] = type
end

if value.nil?
decorations[:nil] = true
end

decorations[:encoding] = 'base64' if type == :binary
decorations[:type] = type unless type == :string
decorations[:nil] = true if value.nil?
decorations
end

protected
def compute_type
type = Hash::XML_TYPE_NAMES[@raw_value.class.name]
type ||= :string if @raw_value.respond_to?(:to_str)
type ||= :yaml
type
end
protected

def compute_value
if formatter = Hash::XML_FORMATTING[type.to_s]
@raw_value ? formatter.call(@raw_value) : nil
else
@raw_value
end
end
def compute_type
type = ActiveSupport::XmlMini::TYPE_NAMES[value.class.name]
type ||= :string if value.respond_to?(:to_str)
type ||= :yaml
type
end
end

class MethodAttribute < Attribute #:nodoc:
protected
def compute_type
Hash::XML_TYPE_NAMES[@raw_value.class.name] || :string
end
end

attr_reader :options
Expand All @@ -92,7 +57,7 @@ def initialize(serializable, options = nil)
# then because <tt>:except</tt> is set to a default value, the second
# level model can have both <tt>:except</tt> and <tt>:only</tt> set. So if
# <tt>:only</tt> is set, always delete <tt>:except</tt>.
def serializable_attributes_hash
def attributes_hash
attributes = @serializable.attributes
if options[:only].any?
attributes.slice(*options[:only])
Expand All @@ -104,91 +69,66 @@ def serializable_attributes_hash
end

def serializable_attributes
serializable_attributes_hash.map { |name, value| self.class::Attribute.new(name, @serializable, value) }
attributes_hash.map do |name, value|
self.class::Attribute.new(name, @serializable, value)
end
end

def serializable_method_attributes
def serializable_methods
Array.wrap(options[:methods]).inject([]) do |methods, name|
methods << self.class::MethodAttribute.new(name.to_s, @serializable) if @serializable.respond_to?(name.to_s)
methods
end
end

def serialize
args = [root]

if options[:namespace]
args << {:xmlns => options[:namespace]}
end
require 'builder' unless defined? ::Builder

if options[:type]
args << {:type => options[:type]}
end

builder.tag!(*args) do
add_attributes
procs = options.delete(:procs)
options[:procs] = procs
add_procs
yield builder if block_given?
end
end
options[:indent] ||= 2
options[:builder] ||= ::Builder::XmlMarkup.new(:indent => options[:indent])

private
def builder
@builder ||= begin
require 'builder' unless defined? ::Builder
options[:indent] ||= 2
builder = options[:builder] ||= ::Builder::XmlMarkup.new(:indent => options[:indent])
@builder = options[:builder]
@builder.instruct! unless options[:skip_instruct]

unless options[:skip_instruct]
builder.instruct!
options[:skip_instruct] = true
end
root = (options[:root] || @serializable.class.model_name.singular).to_s
root = ActiveSupport::XmlMini.rename_key(root, options)

builder
end
end

def root
root = (options[:root] || @serializable.class.model_name.singular).to_s
reformat_name(root)
end
args = [root]
args << {:xmlns => options[:namespace]} if options[:namespace]
args << {:type => options[:type]} if options[:type] && !options[:skip_types]

def dasherize?
!options.has_key?(:dasherize) || options[:dasherize]
@builder.tag!(*args) do
add_attributes_and_methods
add_extra_behavior
add_procs
yield @builder if block_given?
end
end

def camelize?
options.has_key?(:camelize) && options[:camelize]
end
private

def reformat_name(name)
name = name.camelize if camelize?
dasherize? ? name.dasherize : name
end
def add_extra_behavior
end

def add_attributes
(serializable_attributes + serializable_method_attributes).each do |attribute|
builder.tag!(
reformat_name(attribute.name),
attribute.value.to_s,
attribute.decorations(!options[:skip_types])
)
end
def add_attributes_and_methods
(serializable_attributes + serializable_methods).each do |attribute|
key = ActiveSupport::XmlMini.rename_key(attribute.name, options)
ActiveSupport::XmlMini.to_tag(key, attribute.value,
options.merge(attribute.decorations))
end
end

def add_procs
if procs = options.delete(:procs)
[ *procs ].each do |proc|
if proc.arity > 1
proc.call(options, @serializable)
else
proc.call(options)
end
def add_procs
if procs = options.delete(:procs)
Array.wrap(procs).each do |proc|
if proc.arity == 1
proc.call(options)
else
proc.call(options, @serializable)
end
end
end
end
end

def to_xml(options = {}, &block)
Expand Down
2 changes: 2 additions & 0 deletions activerecord/CHANGELOG
@@ -1,5 +1,7 @@
*Rails 3.0.0 [beta 4/release candidate] (unreleased)*

* Serialized attributes are not converted to YAML if they are any of the formats that can be serialized to XML (like Hash, Array and Strings). [José Valim]

* Destroy uses optimistic locking. If lock_version on the record you're destroying doesn't match lock_version in the database, a StaleObjectError is raised. #1966 [Curtis Hawthorne]

* PostgreSQL: drop support for old postgres driver. Use pg 0.9.0 or later. [Jeremy Kemper]
Expand Down
85 changes: 35 additions & 50 deletions activerecord/lib/active_record/serializers/xml_serializer.rb
Expand Up @@ -182,16 +182,31 @@ def initialize(*args)
options[:except] |= Array.wrap(@serializable.class.inheritance_column)
end

def add_extra_behavior
add_includes
end

def add_includes
procs = options.delete(:procs)
@serializable.send(:serializable_add_includes, options) do |association, records, opts|
add_associations(association, records, opts)
end
options[:procs] = procs
end

# TODO This can likely be cleaned up to simple use ActiveSupport::XmlMini.to_tag as well.
def add_associations(association, records, opts)
association_name = association.to_s.singularize
merged_options = options.merge(opts).merge!(:root => association_name)

if records.is_a?(Enumerable)
tag = reformat_name(association.to_s)
type = options[:skip_types] ? {} : {:type => "array"}
tag = ActiveSupport::XmlMini.rename_key(association.to_s, options)
type = options[:skip_types] ? { } : {:type => "array"}

if records.empty?
builder.tag!(tag, type)
@builder.tag!(tag, type)
else
builder.tag!(tag, type) do
association_name = association.to_s.singularize
@builder.tag!(tag, type) do
records.each do |record|
if options[:skip_types]
record_type = {}
Expand All @@ -200,60 +215,30 @@ def add_associations(association, records, opts)
record_type = {:type => record_class}
end

record.to_xml opts.merge(:root => association_name).merge(record_type)
record.to_xml merged_options.merge(record_type)
end
end
end
else
if record = @serializable.send(association)
record.to_xml(opts.merge(:root => association))
end
end
end

def serialize
args = [root]
if options[:namespace]
args << {:xmlns=>options[:namespace]}
end

if options[:type]
args << {:type=>options[:type]}
end

builder.tag!(*args) do
add_attributes
procs = options.delete(:procs)
@serializable.send(:serializable_add_includes, options) { |association, records, opts|
add_associations(association, records, opts)
}
options[:procs] = procs
add_procs
yield builder if block_given?
elsif record = @serializable.send(association)
record.to_xml(merged_options)
end
end

class Attribute < ActiveModel::Serializers::Xml::Serializer::Attribute #:nodoc:
protected
def compute_type
type = @serializable.class.serialized_attributes.has_key?(name) ? :yaml : @serializable.class.columns_hash[name].type

case type
when :text
:string
when :time
:datetime
else
type
end
end
end
def compute_type
type = @serializable.class.serialized_attributes.has_key?(name) ?
super : @serializable.class.columns_hash[name].type

This comment has been minimized.

Copy link
@knoopx

knoopx Sep 30, 2010

Contributor

This will miserably fail if the attribute was not part of the model but was dynamically generated by the resulting SQL. There's a bug on this:
https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/4840-to_xml-doesnt-work-in-such-case-eventselecttitle-as-tto_xml#ticket-4840-5


class MethodAttribute < Attribute #:nodoc:
protected
def compute_type
Hash::XML_TYPE_NAMES[@serializable.send(name).class.name] || :string
case type
when :text
:string
when :time
:datetime
else
type
end
end
protected :compute_type
end
end
end
5 changes: 1 addition & 4 deletions activerecord/test/cases/base_test.rb
Expand Up @@ -2085,6 +2085,7 @@ def test_to_xml
assert_equal "topic", xml.root.name
assert_equal "The First Topic" , xml.elements["//title"].text
assert_equal "David" , xml.elements["//author-name"].text
assert_match "Have a nice day", xml.elements["//content"].text

assert_equal "1", xml.elements["//id"].text
assert_equal "integer" , xml.elements["//id"].attributes['type']
Expand All @@ -2095,10 +2096,6 @@ def test_to_xml
assert_equal written_on_in_current_timezone, xml.elements["//written-on"].text
assert_equal "datetime" , xml.elements["//written-on"].attributes['type']

assert_match(/^--- Have a nice day\n/ , xml.elements["//content"].text)
assert_equal 'Have a nice day' , YAML.load(xml.elements["//content"].text)
assert_equal "yaml" , xml.elements["//content"].attributes['type']

assert_equal "david@loudthinking.com", xml.elements["//author-email-address"].text

assert_equal nil, xml.elements["//parent-id"].text
Expand Down
4 changes: 2 additions & 2 deletions activerecord/test/cases/xml_serialization_test.rb
Expand Up @@ -79,8 +79,8 @@ def test_should_serialize_boolean
assert_match %r{<awesome type=\"boolean\">false</awesome>}, @xml
end

def test_should_serialize_yaml
assert_match %r{<preferences type=\"yaml\">---\s?\n:gem: ruby\n</preferences>}, @xml
def test_should_serialize_hash
assert_match %r{<preferences>\s*<gem>ruby</gem>\s*</preferences>}m, @xml
end
end

Expand Down
2 changes: 2 additions & 0 deletions activesupport/CHANGELOG
@@ -1,5 +1,7 @@
*Rails 3.0.0 [beta 4/release candidate] (unreleased)*

* Array#to_xml is more powerful and able to handle the same types as Hash#to_xml #4490 [Neeraj Singh]

* Harmonize the caching API and refactor the backends. #4452 [Brian Durand]
All caches:
* Add default options to initializer that will be sent to all read, write, fetch, exist?, increment, and decrement
Expand Down
1 change: 0 additions & 1 deletion activesupport/lib/active_support/core_ext/array.rb
Expand Up @@ -5,4 +5,3 @@
require 'active_support/core_ext/array/extract_options'
require 'active_support/core_ext/array/grouping'
require 'active_support/core_ext/array/random_access'
require 'active_support/core_ext/hash/conversions_xml_value'

0 comments on commit 2e9af36

Please sign in to comment.