-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Conversation
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Aside of my question in the code, there're two things to do:
|
My basic questions remain the same about this PR: why does the xml formatter know anything about JSON? |
So it doesn't have to know anything about json. I did more investigating and found out that the "object" that is passed into call is actually a hash. I think this is the root of the issue. The object being passed to call should not be a hash, it should be the instance of the Entity class that you created. So I think I would have to fix this behavior before the formatter will really make any sense. |
I haven't dug into this, but if your test is a good repro I can take a look this w/e. |
I committed a fix for this that has a few different parts:
The representation that you want might not be actually what you want in some cases. As you pointed out, the formatter receives a hash when presenting an entity. In order not to hack this I decided to make the default formatter return what ActiveSupport <?xml version="1.0" encoding="UTF-8"?>
<hash>
<example>
<name>johnnyiller</name>
</example>
</hash> Consider other default scenarios where you want to return an actual hash or an array, this creates a consistent response in all these cases. There's a special case of entities where you return 1 single entity and you might want to suppress that root node. I didn't want to hardcode this in the default formatter, if that's something you want, write a formatter that checks the type of the object and if it's 'Hash' and there's a single value and that value is an if object.is_a?(Hash) && object.respond_to?(:to_xml)
return object.count == 1 ?
object.values.first.to_xml({ :root => object.keys.first }) :
object.to_xml
end Seems too special to be included in the default formatter. Let me know what you think. |
I actually programmed the same solution last night that you committed but I thought that having a <hash></hash> around the object was strange so I didn't commit my code. The problem I see is just one of expectation. When I write that i'm presenting an object I would expect that I was calling to_xml on the object not a hashed version of the object. This is a cleaner implementation but it still seems like the wrong solution. What i'd like to get to the bottom of is why the formatter is receiving a hash in the first place. It should just be receiving the entity object. |
It's possible that it's not intended. LMK if you find something useful. |
With respect to the root element being by default (and with no documented override), I can definitely tell you that it surprised the hell out of me. After all, I'd already set my root element when I named the singular and plurals of my Entity. Then I thought that if I returned present(MyThing, :with => MyEntity, :root => 'beer'), I should sure as heck get a root of ; that's completely ignored since the root option isn't delivered to to_xml, and wouldn't help since it's delivered to the wrong hash. I've ended up mixing in an extra .to_xml into every presented object just to get the right behavior. I prefer your solution since it's packaged in the right place (the formatter), but I really think it ought to be the default, not require a custom formatter. |
I'm not sure I have chosen the best solution possible for getting the entity presenter to work with xml but all the tests pass and along with the new test I made for testing the entity presenter with format :xml.