Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed xml formatter to work with entity presenter #309

Closed
wants to merge 3 commits into from
+42 −2
Closed

fixed xml formatter to work with entity presenter #309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2cd8d7c
fixed entity to xml functionality
johnnyiller Jan 8, 2013
dccee02
change to endpoint spec to remove ruby 1.9 dependency
johnnyiller Jan 8, 2013
22c1122
updated changelog
johnnyiller Jan 8, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
0.2.4 (Next Release)
====================

* [#228](https://github.com/intridea/grape/issues/228): Fix: xml formatter to work with entity - [@johnnyiller](https://github.com/johnnyiller)
* [#296](https://github.com/intridea/grape/issues/296): Fix: ArgumentError with default error formatter - [@dblock](https://github.com/dblock).
* [#297](https://github.com/intridea/grape/issues/297): Added `default_error_formatter` - [@dblock](https://github.com/dblock).
* [#297](https://github.com/intridea/grape/issues/297): Setting `format` will automatically set `default_error_formatter` - [@dblock](https://github.com/dblock).
Expand Down
14 changes: 13 additions & 1 deletion lib/grape/formatter/xml.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,19 @@ module Xml
class << self

def call(object, env)
object.respond_to?(:to_xml) ? object.to_xml : object.to_s
if object.respond_to?(:to_xml)
ret = object if object.is_a?(String)
ret = object.to_json if object.respond_to?(:to_json)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is incorrect. If the object responds to to_xml, why would you first convert it to JSON?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So i'm not really sure how to go about it but basically if you look at this file https://github.com/intridea/grape/blob/master/lib/grape/entity.rb you will see this at

line 305 # The serializable hash is the Entity's primary output. It is the transformed
# hash for the given data model and is used as the basis for serialization to
# JSON and other formats.
#
# @param runtime_options [Hash] Any options you pass in here will be known to the entity
# representation, this is where you can trigger things from conditional options
# etc.
def serializable_hash(runtime_options = {})

So what i'm trying to do is convert the object coming into def call to be a serialized hash and then generate the xml from that. The reason I converted to json is that when looking at the other formatters I didn't see a clear way to get directly to a serializable hash. Perhaps it would be better to create some other object for dealing with this conversion process but as a first stab at it I put it directly in the formatter.

At the very least I think I added a test in the correct area so if you have a better idea of how to solve this then i'd love to see what you come up with.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the strategy should be similar to how we do JSON: if the object can do to_json, call that. Otherwise use a well known serializer, like MultiXml, to transform the object to XML.

In the end, we need tests that make sure that all these types transform to valid xml: String, Array, a class that supports to_xml, a class that doesn't support to_xml, etc.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right but isn't the entity.rb responsible for making the object respond to to_xml? That's sort of the point as I understand it. My object may not support to_xml but once I expose some properties on it then those methods will be able to be represented via xml. If that's not the case then i'm not really sure what the point of the Entity class is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're correct. I didn't think I contradicted myself though - Entity should define to_xml in this case, it's not the job of the formatter to know it's dealing with something called Entity.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the problem I encountered. Perhaps i'm missing some key piece of evidence. If you go into the Entity class in entity.rb and override to_xml you will see that you get xml that looks like this.
<?xml version="1.0" encoding="UTF-8"?>
<hash></hash>

And basically everything inside to_xml is ignored. So even if to_xml returns a simple string it doesn't print out. That being the case it lead me to what you see.

ret = MultiJson.dump(object)
hash = MultiJson.load(ret)
if Hash === hash
hash.first.last.to_xml(:root => hash.first.first.to_s)
else
object.to_xml
end
else
object.to_s
end
end

end
Expand Down
28 changes: 28 additions & 0 deletions spec/grape/endpoint_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,34 @@ def memoized
get '/example'
end

it 'presents with xml' do
entity = Class.new(Grape::Entity)
entity.root "examples", "example"
entity.expose :name

subject.format :xml

subject.get '/example' do
c = Class.new do
attr_reader :name
def initialize(args)
@name = args[:name] || "no name set"
end
end
present c.new({:name => "johnnyiller"}), :with => entity
end
get '/example'
last_response.status.should == 200
nicexml=<<-BEGIN
<?xml version="1.0" encoding="UTF-8"?>
<example>
<name>johnnyiller</name>
</example>
BEGIN
last_response.body.should == nicexml

end

[ :json, :serializable_hash ].each do |format|

it 'presents with #{format}' do
Expand Down