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

Support for soap attachments in response. #1148

Merged
merged 14 commits into from
Jul 13, 2021
Merged

Conversation

davefej
Copy link
Contributor

@davefej davefej commented Jun 2, 2021

Summary

With this modification the lib can parse the multipart/related http response, and get the (binary) attachments from it.

Configuration

The funcionality is behind a boolean option called parseReponseAttachments (default: false, not to affect previous functionality)
If mtomResponse is set to true we expect a multipart response however it can handle normal soap response (without attachment) as well.

Modification list

  • encoding is set to null before sending http request (it cause response to be Buffer instead of string)
  • checking response header for multipart content-type and get boundary
  • parse multipart content with formidable MultipartParser (first part is the soap response xml)
  • attachments are reachable in soapClient.lastResponseAttachments
  • request-response-samlples test is extended with attachment.js and responseHttpHeaders.json

Dependencies

Example usage

const client = soap.createClientAsync(wsdl, { parseReponseAttachments: true} );
await client.methodAsync(json)
const attachments = client.lastResponseAttachments

Docs

W3C not about soap attachments: https://www.w3.org/TR/SOAP-attachments/

Other

The modification is similar to #1119 but configurable and working.

@coveralls
Copy link

coveralls commented Jun 2, 2021

Coverage Status

Coverage increased (+0.02%) to 95.085% when pulling d3e8954 on davefej:master into 22c234f on vpulim:master.

@DPM1
Copy link
Contributor

DPM1 commented Jun 29, 2021

@davefej great work, thank you! I just tried it and it worked out of the box.
I think it would be a good idea to also include mtomAttachements in the results instead of having to use lastResponseAttachments. What do you think?
MR draft here:
davefej#1

@davefej
Copy link
Contributor Author

davefej commented Jul 1, 2021

@DPM1 Thanks, Absolutely agreed on include attachments into the result woult be great and the preferred solution.
However that would be a major modification.
Maybe I could implement that in the future in another pull request what do you think of this solution?

UPDATE :)
Sorry, I haven't seen you already linked an MR here and you implemented that.
I merged it now thanks for the implementation

feat: add mtomAttachments to result
@davefej
Copy link
Contributor Author

davefej commented Jul 1, 2021

Hi There!
Travis ci is not started automatically for 49e2e63

Is it becaues of travis-ci.org is deprecated and moved to travis-ci.com?

@DPM1
Copy link
Contributor

DPM1 commented Jul 5, 2021

Would be great to get this into the next release. If anyone would be nice enough to do a review, I would be happy to then do any work required to get this released.
For a start it probably needs at least 1 unit test for the mtomAttachments return, I did not add one in my draft MR. I will try to add one this week.
Additional review comments are always welcome.
Thanks

@jsdevel
Copy link
Collaborator

jsdevel commented Jul 6, 2021

please resolve the conflicts

@davefej
Copy link
Contributor Author

davefej commented Jul 7, 2021

Hi There!
@DPM1 Tests are already done for MTOM. Those are in the request-response-samlpes.
(should_handle_MTOM_response and should_handle_not_MTOM_response)

@jsdevel Merge done with new axios mods on master

@DPM1
Copy link
Contributor

DPM1 commented Jul 7, 2021

@davefej great! I just quickly tested the rebase on module I have running with MTOM attachments and it worked perfectly.
I noticed the "request" lib was re-added on rebase so created MR to remove it again.
Thanks!

remove accidentally re-added request package
package.json Outdated
"tslint": "^5.18.0",
"typedoc": "^0.20.36",
"typescript": "^3.3.3333"
"name": "soap",
Copy link
Collaborator

Choose a reason for hiding this comment

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

please revert the white space changes in this file.

Copy link
Contributor Author

@davefej davefej Jul 9, 2021

Choose a reason for hiding this comment

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

Hi!
You compared with a previous commit, it is already fixed in the latest commit in master.
https://github.com/vpulim/node-soap/pull/1148/files

if(responseXML) requestContext.responseToSend = responseXML;
if (responseXML) {
if (wsdlOptions.parseReponseAttachments) {//all LF to CRLF
responseXML = responseXML.replace(/\r\n/g, "\n");
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

you're converting rn, to n, then you're converting n back to rn. please revert this.

Copy link
Contributor Author

@davefej davefej Jul 9, 2021

Choose a reason for hiding this comment

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

Hi!

It is needed, might be implemented in different way, but needed.

The reason for that is only CRLF allowed in the multipart/related response. And in linux systems (for example travis-ci) git repo pulled with LF only so the responseXML had LF lineendings. However if response xml has LF line endings the standart http multipart response parser cannot parse it so tests fails.

What that two lines of code does is to replace all "\n" to "\r\n" .
If line 156 would have been left out responseXML = responseXML.replace(/\r\n/g, "\n"); and only line 157 would be executed than it might occur that some line endings will have "\r\r\n" and that is not correct of course.

So someone with better regexp knowledge might add a solution to replace all LF to CRLF without replacing the LF parts of CRLF's but it is needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

so you're saying there may be a mix of rn initially, so your strategy is to convert those to n then convert all n to rn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.
These lines are because of I noticed that tests fails on linux system, and the reason was that there were \n -s in the file.
So I replaced all to \n to \r\n in a dirty way I mean taking care of not to replace \r\n to \r\r\n...

@dursk
Copy link

dursk commented Jul 29, 2021

@jsdevel Is there a target to get this into a release? I desperately need this!

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.

5 participants