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

allow per-message soap headers #470

Closed
cout opened this issue Jun 21, 2013 · 6 comments
Closed

allow per-message soap headers #470

cout opened this issue Jun 21, 2013 · 6 comments
Assignees
Milestone

Comments

@cout
Copy link

cout commented Jun 21, 2013

I thought I had already entered this bug report/feature request, but I don't see it now, so I'm entering it again.

We need to be able to add additional soap headers per-message for WS-Addressing (each message needs its own message id, which afaict goes in the soap header). Currently we are using the following:

# The Savon client by default does not allow adding new soap headers
# except via the global configuration.  This monkey patch allows adding
# soap headers via local (per-message) configuration.
module Savon
  class LocalOptions < Options
    def soap_header(header)
      @options[:soap_header] = header
    end
  end

  class Header
    def header
      @header ||= build_header
    end

    def build_header
      header = {}
      header.update(@globals.include?(:soap_header) ? @globals[:soap_header] : {})
      header.update(@locals.include?(:soap_header) ? @locals[:soap_header] : {})
      return header
    end
  end
end

This allows the soap_header option to be used globally or locally.

@kenberland
Copy link

+1

@rubiii
Copy link
Contributor

rubiii commented Jun 29, 2013

ok, so how would you expect savon to work when someone uses a global and local soap_header option?
as far as i can see, we would need to either deep merge both hashes or prefer the local over the global hash.

i'm not really sure if it makes sense to keep the global option at all. it might be a little more tedious for people who always send the same header, but everything can be automated and if you properly abstract external libraries, it should be easy to update your abstraction.

what do you think?

ps. of course we would need to implement some solution, deprecate the global option and remove it in a future 2.x release to give people time to upgrade.

@cout
Copy link
Author

cout commented Jun 30, 2013

I would expect a shallow merge (which is what I implemented).

I did not consider a deep merge. A deep merge would make it more difficult to override an entire subtree, but would allow overriding individual leaf nodes.

I don't know which is preferable, because I don't know what typical use cases would be. Any of the options (shallow merge, deep merge, local completely overriding global, and completely eliminating global) would work for what I want to do.

I suspect users would be most likely to use either local or global and not both. A deep merge is probably unnecessarily complex.

Eliminating global might break existing code for a very small benefit.

@rubiii
Copy link
Contributor

rubiii commented Jun 30, 2013

thanks. the shallow merge sounds fine to me. removing the global option really doesn't make things easier.
i just searched through issues and pull requests but couldn't find the original issue you mentioned.
would you be able to open a pull request for this?

@ghost ghost assigned rubiii Jun 30, 2013
@cout
Copy link
Author

cout commented Jun 30, 2013

I can. I didn't do the change as part of a branch, just did the monkey patch. I can have a real patch and pull request some time tomorrow.

@rubiii
Copy link
Contributor

rubiii commented Jun 30, 2013

great! thank you.

@rubiii rubiii mentioned this issue Jul 20, 2013
27 tasks
rubiii added a commit that referenced this issue Jul 26, 2013
if both the global :soap_header and the local :soap_header options
are provided, savon will merge the global with the local header if
both are hashes. otherwise, it will prefer the the local over the
global option.
@rubiii rubiii closed this as completed Jul 26, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants