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.

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
Closed

fixed xml formatter to work with entity presenter #309

wants to merge 3 commits into from

Conversation

johnnyiller
Copy link

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.

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.

@dblock
Copy link
Member

dblock commented Jan 8, 2013

Aside of my question in the code, there're two things to do:

  • Fix the build for Ruby 1.8.7, you are using a 1.9.x syntax somewhere, see Travis CI.
  • Add a line to CHANGELOG.markdown.

@dblock
Copy link
Member

dblock commented Jan 11, 2013

My basic questions remain the same about this PR: why does the xml formatter know anything about JSON?

@johnnyiller
Copy link
Author

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.

@dblock
Copy link
Member

dblock commented Jan 12, 2013

I haven't dug into this, but if your test is a good repro I can take a look this w/e.

@dblock dblock closed this in 11576c8 Jan 12, 2013
@dblock
Copy link
Member

dblock commented Jan 12, 2013

I committed a fix for this that has a few different parts:

  • The formatter has not changed, except that it will raise an exception when it cannot convert a type to XML. For example, if you try to return a string, the formatter will fail instead of returning the raw string (which is not valid XML). For any type to work you have to implement to_xml, which is fully implemented for most of the types by ActiveSupport.
  • Added to_xml to Entity. This falls back to serializable hash, then calls to_xml on it. Now this provides a default representation for entities in XML format.

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 to_xml would return in this case:

<?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 Entity, call to_xml on the entity itself with :root_key being the hash key.

          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.

@johnnyiller
Copy link
Author

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.

@dblock
Copy link
Member

dblock commented Jan 12, 2013

It's possible that it's not intended. LMK if you find something useful.

@cobbr2
Copy link

cobbr2 commented Jan 25, 2013

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants