strongloop / strong-remoting

Communicate between objects in servers, mobile apps, and other servers.
www.strongloop.com
Other
105 stars 93 forks source link

Customize the name of the root XML element #262

Closed bajtos closed 8 years ago

bajtos commented 8 years ago

At the moment, when producing a response in the XML format, strong-remoting always wrap the response body in <response> element (source code).

We should allow developers to customise the name of this root element, for example to produce a response body like this:

<foo>bar</foo>

A possible solution is to implement a new remoting setting at the shared-method level:

var fn = function(cb) {
  cb(null, 'bar');
};

setupRemoting(fn, {
  "accepts": [],
  "returns": { "type": "string", "root": true },
  "xml": { "rootElement": "myCustomName" } }
});
pulkitsinghal commented 8 years ago

That sounds good and what is your opinion if I suggest: "Let's not try to wrap it at all, since the user has already set root:true which means they've got a handle on it and don't need the framework to wrap anything? It should let it flow through as-is."

bajtos commented 8 years ago

@pulkitsinghal That sounds good and what is your opinion if I suggest: "Let's not try to wrap it at all, since the user has already set root:true which means they've got a handle on it and don't need the framework to wrap anything? It should let it flow through as-is."

I'm afraid that's not possible. Consider the following method:

function(cb) {
  cb(null, { a:1, b:2 });
}
// cb arg1 is { type: Object, root: true }

In JSON, the object is serialized as follows:

{ 
  "a": 1,
  "b": 2,
}

However, there is no way how to do the same in XML, the following is not a well-formed document:

<a>1</a>
<b>2</b>

That's why we need a wrapper element.

pulkitsinghal commented 8 years ago

@bajtos - Ok I was making the case that as a developer who has set: root: true, I would be responsible for returning my data as:

function(cb) {
  cb(null, { rootElem: {a:1, b:2 }} );
}
pulkitsinghal commented 8 years ago

Whatever gets a solution sooner, I won't argue :)

bajtos commented 8 years ago

I was making the case that as a developer who has set: root: true, I would be responsible for returning my data as:

function(cb) {
  cb(null, { rootElem: {a:1, b:2 }} );
}

Makes sense. It is what I was initially thinking about too. However, to preserve backwards compatibility, we would have to add a new option for that, e.g.

returns: { type: 'object', root: true, xml: { wrapperElement: false } }

Implementation wise, I think it will be easier for us to simply change the name of the wrapper element depending on the configuration, rather than detecting whether the returned root object has a single property.

pulkitsinghal commented 8 years ago

@bajtos - Agreed!

If you want me to make code changes and submit PR ... would you mind pointing me to where to start making these changes? I'm often hesitant to submit PRs because in my opinion, the PRs could get stuck when asked to add an unit test ... as I don't really know how or where to do it within loopback.

bajtos commented 8 years ago

@pulkitsinghal I asked @davidcheung to implement this for you. But if you don't mind to contribute the patch yourself, then that would be even better!

The hard-coded 'response' element name is here: https://github.com/strongloop/strong-remoting/blob/df59c64249cabf54cee361f21f51a95be98ec7d9/lib/http-context.js#L347

I found only one unit-test for XML: https://github.com/strongloop/strong-remoting/blob/c43b8ed8518534cf6cd49e0abbf13b3fb28dd948/test/rest.test.js#L258-L282