Added XML support #448

jokklan · 2013-11-07T15:28:10Z

Hallo

I know that you have said before that you are not focusing on XML support at the time being, but I''ll need it for one of my own projects, so i have build it anyway. Even though it's not done, was it really simple to implement, as a hash easily can be transformed into XML. The only obstacle i came across was that XML needs a root, and json does not, but that doesn't seem to bad. Rabl eg can also render json without root with the same template as it will render xml.

I don't know if you are at all interested, but i can't see a reason not to?

Best regards
/Johan

byroot · 2013-11-19T18:41:14Z

👍

ncr · 2013-11-28T15:10:19Z

I would love it merged.

spastorino · 2013-11-28T19:02:25Z

lib/action_controller/serialization.rb

+        if serializer
+          super(serializer, options)
+        else
+          super(resource, options)


There's no need to change super to this

Methods created using define_method require explicit params in super

Ahh ya, we move to define_method 👍

spastorino · 2013-11-28T19:06:24Z

There are no tests for this, we need tests.

jokklan · 2013-11-29T11:06:17Z

I have developed more on this already and written tests for it all. I will push it, hopefully this weekend. The one big (and only real) concern is still however that XML needs a root, and that elements in arrays need a wrapper tag as well, so eg:

{ posts: [
  { title: "XML",
  body: "ActiveModel::Serializer should support XML" }
]},
{ comments: [
  { user_id: 1,
  body: "Agreed!" },
  { user_id: 5,
  body "I don't..."}
]}

Would need to be something like this XML

<result>
   <posts>
     <post>
       <title>XML</title>
       <body>ActiveModel::Serializer should support XML</body>
     </post>
  </posts>
  <comments>
    <comment>
      <user_id>1</user_id>
      <body>Agreed!</body>
    </comment>
    <comment>
      <user_id>5</user_id>
      <body>I don't...</body>
    </comment>
  </comments>
</result>

ncr · 2013-11-29T11:13:33Z

I can help if you push.

jokklan · 2013-11-29T14:52:31Z

@ncr I have pushed what i have created so far. It's not done, and few of the specs still fails... but at least there is something to work from, and to discuss :). I will continue to work on this.

dyanisse · 2014-02-03T21:40:12Z

@jokklan I am interested in this too. Can I help?

rewritten · 2014-02-15T09:01:48Z

I would love to help getting this integrated.

jsborjesson · 2014-02-15T14:40:46Z

I'm using your fork now. You might have saved me a lot of trouble - cheers!

jsborjesson · 2014-02-15T15:33:54Z

I'm sad to say that things like whatever_url(object) stops working in this fork. Any idea why?

jokklan · 2014-02-15T15:45:04Z

@alcesleo I haven't have time to complete this yet, but i will come back to it soon (hopefully this weekend). The whole implementation needs some refactorings and clean up.

@rewritten Any help would be appreciated 👍 I can ping you when i have completed the last clean up, and then maybe you could look it through?

jsborjesson · 2014-02-15T15:48:27Z

@jokklan That sounds fantastic, I would totally buy you a beer (or three).

dyanisse · 2014-02-16T23:23:14Z

@jokklan thanks for merging, let me know if there is anything else to refactor.

jokklan · 2014-02-16T23:32:39Z

@dyanisse I'll rebase with master and then let's see :)

…alizers::Xml

rewritten · 2014-02-17T09:04:52Z

@jokklan please ping me!

jokklan · 2014-02-19T17:08:18Z

@alcesleo @rewritten or any one else that want to help: their need to be made a decision on XML roots. I think the class name would be a good default and how the XML parser works now, but their also have to be a option to set your own root for each serializer object, and/or in a global configuration (partly implemented). Adding tests and documentation for this would be really helpful, then can I (or anyone willing) implement it soon.

If it's to unclear then please ask :)! Otherwise know that I plan getting back to this real soon. I know I keep postponing this PR I'm just a little busy, but I promise I'll finish this!

rewritten · 2014-02-19T20:12:29Z

@jokklan The underscored name of the serialized class could be a good default option, but also it should be configurable, yes. Do ou want us to provide (failing) tests for that?

jokklan · 2014-02-19T21:08:42Z

Im writing some refactorings and improvements now. I'll try to add some tests, but i'll have to figure out a clever way to test it without just copy all the json tests :)

jokklan · 2014-02-19T22:26:24Z

This should now work :). The old root option still works as before, and applies to both XML and JSON. I have also added two new options (json_root and xml_root) that takes precedence over root. Nothing is changed in how JSON is rendered. XML renders by creating a hash with root keys for every object, and then parse this to XML (as_xml in serializable).

Their still needs to be made some tests, right now is their only integration tests. Also a good documentation section needs to be added.

I will continue to work on this, but should anyone want to make some unit test or write some documentation, then please do 👍, would help a lot!

Would also be nice to get some feedback from some of the moderators. Any thoughts @spastorino ?

jsborjesson · 2014-02-20T13:52:37Z

@jokklan Hi! Sorry I haven't been able to look it over as much as I wanted to, I am trying it out now but I still can't get the url-helpers working. Not quite sure what has changed that made them stop functioning.

jokklan · 2014-02-20T14:35:13Z

@alcesleo that sounds very strange, i can't figure out how this should affect url helpers.

jsborjesson · 2014-02-20T14:37:05Z

@jokklan Me neither, but it works in the original version.

jsborjesson · 2014-02-20T14:40:02Z

Some obscure inheritance or super-call that has changed maybe?

jokklan · 2014-02-20T14:42:48Z

Properly something to do with that this branch comes from master, so it's something that's introduced in master since last stable release.

jsborjesson · 2014-02-20T14:54:49Z

faceplam it actually says something about it in the first paragraph of masters readme.

jsborjesson · 2014-02-21T12:45:57Z

This works great for me now! Thanks @jokklan! Let me know if you want to cash in on those beers.

GeekOnCoffee · 2014-05-29T12:40:06Z

Is there more work needed to get this merged? Hoping to use AMS but unfortunately XML generation is a requirement for us.

xymbol · 2014-05-30T22:59:47Z

@GeekOnCoffee Hi, there is an ongoing discussion around the future of AMS. Adding XML is not a priority and, even though pluggable formatters are somewhere in the plan, an XML variant doesn't seem to be within what we'd like to implement first. Please check the discussion here.

steveklabnik · 2014-07-28T16:27:43Z

Yes. Please see here: https://groups.google.com/forum/#!topic/rails-api-core/8zu1xjIOTAM

I have some concerns about putting this in 0.9, but I wouldn't mind for 0.10 (and the eventual 1.0) to have XML support, and it should be even easier, with the new architecture.

Let me think about this some more before merging.

steveklabnik · 2014-08-22T19:43:52Z

Yes. I wouldn't mind this feature in 0.10, as one of our Adapter classes. I don't plan on backporting it to older versions, however. Thanks!

reidmorrison · 2016-06-03T17:38:05Z

Now that v0.10 is available, is it worth re-opening the discussion around XML support?

Is it as simple as just adding a new adapter that derives from the JSON adapter and/or the JSON API adapter, and then just calls to_xml instead of to_json on the final Hash to render the output as XML?

The challenge this would address is being able to render the "equivalent" JSON or XML output on demand. Over time clients can move away from the XML interface to the better JSON layout.

bf4 · 2016-06-03T18:49:37Z

@reidmorrison I would help you write an add-on that adds an XML adapter, but, at present, it's not a good fit for AMS. Additionally, active model xml serializers have been extracted from Rails 5, so it would be problematic to make that a dependency.

Please open a new issue if you'd like to discuss more, esp with an example of what your use-case is.

jokklan mentioned this pull request Nov 7, 2013

XML support #447

Closed

spastorino reviewed Nov 28, 2013
View reviewed changes

jokklan and others added 6 commits February 17, 2014 01:32

Added XML support

a4033aa

Removed debugger, and refactored a bit

c17e480

Added root key

b06b14c

Created tests for XML integration

0f79bae

to_xml was undefined in ActionController, including ActiveModel::Seri…

96cc363

…alizers::Xml

fixed the remaining specs

63ded94

jokklan added 3 commits February 19, 2014 23:06

Improved xml and json root configuration for more consistency

8e1df22

Updated array serializer and serializable for full XML support

d19923b

Fixed test

8b603a0

espen mentioned this pull request May 30, 2014

Rails support ohler55/ox#94

Closed

steveklabnik removed the waiting label Aug 22, 2014

steveklabnik closed this Aug 22, 2014

sophiabits mentioned this pull request Jan 30, 2022

Expose more data over the API NZOI/nztrain#143

Open

Added XML support #448

Added XML support #448

Conversation

jokklan commented Nov 7, 2013

byroot commented Nov 19, 2013

ncr commented Nov 28, 2013

spastorino Nov 28, 2013

Choose a reason for hiding this comment

ncr Nov 29, 2013

Choose a reason for hiding this comment

jokklan Nov 29, 2013

Choose a reason for hiding this comment

spastorino Nov 29, 2013

Choose a reason for hiding this comment

spastorino commented Nov 28, 2013

jokklan commented Nov 29, 2013

ncr commented Nov 29, 2013

jokklan commented Nov 29, 2013

dyanisse commented Feb 3, 2014

rewritten commented Feb 15, 2014

jsborjesson commented Feb 15, 2014

jsborjesson commented Feb 15, 2014

jokklan commented Feb 15, 2014

jsborjesson commented Feb 15, 2014

dyanisse commented Feb 16, 2014

jokklan commented Feb 16, 2014

rewritten commented Feb 17, 2014

jokklan commented Feb 19, 2014

rewritten commented Feb 19, 2014

jokklan commented Feb 19, 2014

jokklan commented Feb 19, 2014

jsborjesson commented Feb 20, 2014

jokklan commented Feb 20, 2014

jsborjesson commented Feb 20, 2014

jsborjesson commented Feb 20, 2014

jokklan commented Feb 20, 2014

jsborjesson commented Feb 20, 2014

jsborjesson commented Feb 21, 2014

GeekOnCoffee commented May 29, 2014

xymbol commented May 30, 2014

steveklabnik commented Jul 28, 2014

steveklabnik commented Aug 22, 2014

reidmorrison commented Jun 3, 2016

bf4 commented Jun 3, 2016