Skip to content

To allow user to customize the root element of xml output - #272

Merged
bajtos merged 1 commit into
masterfrom
feature/customize-xml-root
Jan 28, 2016
Merged

To allow user to customize the root element of xml output#272
bajtos merged 1 commit into
masterfrom
feature/customize-xml-root

Conversation

@davidcheung

Copy link
Copy Markdown
Contributor

To take wrapperElement as an option in remoteMethod setup

{
  returns: { 
    root: true,
      xml: {wrapperElement: 'a'}
    },
    http: { path: '/' }
}

Objective is to allow user to produce output as follow

<a>1</a>

Fixes #262

connected to /issues/262

@davidcheung davidcheung self-assigned this Jan 6, 2016
Comment thread lib/http-context.js Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@bajtos when input is string, js2xmlparser gets syntax error, then try catch block from sendBodyXml() throws 500 , should we modify the data so it doesn't internal server error? or should we not let user pass in just one string. (We do modify the data when passed as array)

@davidcheung

Copy link
Copy Markdown
Contributor Author

also added declaration=false flag

{
  "returns": { 
    "root": true,
    "xml": { "declaration": false }
  },
  "http": { "path": "/" }
}

to not include the declaration of

<?xml version="1.0" encoding="UTF-8"?>

To address this https://github.com/strongloop-internal/scrum-cs/issues/396

Everytime I return something like:

cb(null, '{a:blah}');

I receive xml like: <response><a>blah</a></response>
But what I want is xml like: <a>blah</a>

@bajtos, @ritch not sure if this feature is needed, and not sure where to add documentation for such, any advice?

@davidcheung davidcheung assigned ritch and unassigned davidcheung Jan 8, 2016
@davidcheung davidcheung added #review and removed #wip labels Jan 8, 2016
@ritch

ritch commented Jan 11, 2016

Copy link
Copy Markdown
Member

To address this strongloop-internal/scrum-cs#396

I don't think this exactly addresses the issue, but it might be a welcome feature.

Comment thread lib/http-context.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

use typeof input not typeof(input)

@davidcheung
davidcheung force-pushed the feature/customize-xml-root branch from b8ecc8f to ff0e7a9 Compare January 12, 2016 18:05
@davidcheung

Copy link
Copy Markdown
Contributor Author

@ritch, I have addressed the 2 suggestions you made, please review

@davidcheung davidcheung assigned bajtos and unassigned ritch Jan 19, 2016
@davidcheung

Copy link
Copy Markdown
Contributor Author

@bajtos review please :)

@davidcheung

Copy link
Copy Markdown
Contributor Author

@slnode test please

Comment thread lib/http-context.js

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AFAICT, this is adding another feature, which is to allow the developer to specify whether XML declaration is included in the response. I can live with the fact that it's introduced together with other changes, but please add a unit-test to verify.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Unit-test added.

@bajtos

bajtos commented Jan 22, 2016

Copy link
Copy Markdown
Member

@davidcheung reviewed, see my comments above.

@bajtos bajtos assigned davidcheung and unassigned bajtos Jan 22, 2016
@davidcheung

Copy link
Copy Markdown
Contributor Author

hi @bajtos, have added unit test for the changes. Please review again :)

@davidcheung davidcheung removed their assignment Jan 25, 2016
Comment thread test/rest.test.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please modify the test to use a custom root element, e.g. <text> instead of <response>.

@bajtos

bajtos commented Jan 26, 2016

Copy link
Copy Markdown
Member

@davidcheung great, this is almost ready. Please address my comments above and squash the commits into a single one. Make sure the commit message mentions all new features (declaration: false, wrapperElement: 'name').

@bajtos bajtos assigned davidcheung and unassigned bajtos Jan 26, 2016
@davidcheung
davidcheung force-pushed the feature/customize-xml-root branch 4 times, most recently from 3adba80 to b4689ac Compare January 26, 2016 20:00
@davidcheung

Copy link
Copy Markdown
Contributor Author

@bajtos i have addressed the comments above, I believe the PR is good to go. Please review again

@davidcheung davidcheung assigned bajtos and unassigned davidcheung Jan 26, 2016
Comment thread test/rest.test.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

...xml declaration..., not decoration.

@bajtos

bajtos commented Jan 28, 2016

Copy link
Copy Markdown
Member

@slnode test please

- Add option to customize the XML rootElement tag
  (default: "response")

- Add flag to disable declaration from response
  (default: declaration is present)

- Bugfix changing behavior: `string` results from remote methods are
now quoted to avoid json2xmlparser throwing syntax error and returning
500 instead of a regular response body.

User can set the returned XML root element as follows:

    "returns": {
      "xml": { "wrapperElement": "Parent" }
    }

User can disable XML declaration as follows:

    "returns": {
      "xml": { "declaration": false }
    }
@bajtos
bajtos force-pushed the feature/customize-xml-root branch from b4689ac to ba5b064 Compare January 28, 2016 09:30
@bajtos

bajtos commented Jan 28, 2016

Copy link
Copy Markdown
Member

@davidcheung I have fixed the test name myself and improved the commit message a bit. The patch LGTM, I'll land it as soon as I get CI build results.

bajtos added a commit that referenced this pull request Jan 28, 2016
To allow user to customize the root element of xml output
@bajtos
bajtos merged commit 00b9851 into master Jan 28, 2016
@bajtos bajtos removed the #review label Jan 28, 2016
@bajtos
bajtos deleted the feature/customize-xml-root branch January 28, 2016 12:10
bajtos pushed a commit that referenced this pull request Jan 28, 2016
- Add option to customize the XML rootElement tag
  (default: "response")

- Add flag to disable declaration from response
  (default: declaration is present)

- Bugfix changing behavior: `string` results from remote methods are
now quoted to avoid json2xmlparser throwing syntax error and returning
500 instead of a regular response body.

User can set the returned XML root element as follows:

    "returns": {
      "xml": { "wrapperElement": "Parent" }
    }

User can disable XML declaration as follows:

    "returns": {
      "xml": { "declaration": false }
    }

This is a back-port of the following GitHub pull request:
#272
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