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

Xml/attachments options bugfix, MTOM support for attachments, better Nori options passthrough #944

Closed
wants to merge 4 commits into from

Conversation

ekzobrain
Copy link
Contributor

What kind of change is this?

Bugfixes + new feature

Did you add tests for your changes?

Currently no

Summary of changes

Add MTOM support for SOAP attachments (previously no support)
Fix request logging when message containt non-ascii characters (log writing failure occured)
Fix using attachments together with 'xml' option (when xml option was provided, attachments option was ignored)

Fix request logging when message containt non-ascii characters (log writing failure occured)
Fix using attachments together with 'xml' option (when xml option was provided, attachments option was ignored)
@ekzobrain ekzobrain changed the title Bug fixes and MTOM support for attachments Xml/attachments options bugfix, MTOM support for attachments, better nori options passthrough Feb 22, 2021
@ekzobrain ekzobrain changed the title Xml/attachments options bugfix, MTOM support for attachments, better nori options passthrough Xml/attachments options bugfix, MTOM support for attachments, better Nori options passthrough Feb 22, 2021
@ekzobrain
Copy link
Contributor Author

@pcai what about merging this? we use it in production for two years already

@Skulli
Copy link

Skulli commented Jan 9, 2023

Thanks for your work. Could you describe how to use it? For example get the attachments of a given request?
In https://github.com/savonrb/savon-multipart/blob/master/spec/savon/soap/response_spec.rb you can access attachments via response.attachments.

@ekzobrain
Copy link
Contributor Author

ekzobrain commented Jan 10, 2023

@Skulli is far as I remember, there were no changes in public API regarding request/response attachments, except fixing a bug, when both "attachments" and "xml" options were used together in locals.

The main feature was support for MTOM attachments encoding: https://www.ibm.com/docs/en/integration-bus/10.0?topic=services-what-is-soap-mtom
It automatically recognizes this encoding (and decodes as appropriate) for response attachments, and one should explicitely define wheather he wants to send request attachments in this encoding (new "mtom" option, defaults to false, backward compatible).
We implemented it for an API, that only supported sending attachments in this encoding.

Request example:

client = Savon.client({
  multipart: true,
  ...
})
client.call(:some_operation, {
  mtom: true, # new option, default=false
  attachments: [...],
})

@pcai
Copy link
Member

pcai commented Jan 12, 2023

@netcitylife please address the merge conflicts and lack of tests and I can merge.

# Conflicts:
#	lib/savon/options.rb
@bihterulu
Copy link

This is working, what can we do to solve these issues which blocks the merge process?

@@ -101,9 +101,10 @@ def build_request(builder)
request.body = builder.to_s

if builder.multipart
request.gzip
type = @locals[:mtom] ? 'application/xop+xml"; start-info="text/xml' : SOAP_REQUEST_TYPE[@globals[:soap_version]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: this MTOM mime type could also be extracted to a constant, so that both branches are harmonious, looking uniform.

@olleolleolle
Copy link
Contributor

This is working, what can we do to solve these issues which blocks the merge process?

Missing in order to merge: tests for the new behaviour.

@pcai pcai mentioned this pull request Jul 15, 2024
@pcai
Copy link
Member

pcai commented Jul 15, 2024

rebased, tested, and merged as #1012

@pcai pcai closed this Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants