savonrb / wasabi

A simple WSDL parser
MIT License
90 stars 84 forks source link

Parse Input/Output When Operation Style is Document #114

Closed mattpolito closed 1 year ago

mattpolito commented 1 year ago

Problem:

When parsing operations of the Workday API we were getting incorrect input values. The operation here is not declared with a soapAction so it never made it into the logic to parse inputs/outputs.

Note:

Please review changes to specs. As far as we can tell the specs were not accurately describing parsing of inputs/outputs, hence the changes to reflect 'correct' parsing.

However, we fully understand that someone wrote the specs with those assertions in mind. So there is an expectation that they should work that way.

Please let us know if these assumptions are incorrect so we can try and adjust.

mattpolito commented 1 year ago

Any feedback on this one?

pcai commented 1 year ago

Thanks for your patience. I'm going through this today to try and understand the change

pcai commented 1 year ago

OK This seems good to me, sorry for the silly questions as I am no longer involved in an org that uses savon/wasabi, but my main concern is --

Is there any universe where the current behavior (without this PR) would be considered correct? In context these changes appear good, but I am somewhat apprehensive about merging this in, cutting a bugfix release, and then getting a complaint that it breaks someone's working code. If there's a risk of that, then perhaps a major version bump is in order?

Note that I have updated main branch to clarify we are 2.7+ so the use of lonely operator should not be an issue

mattpolito commented 1 year ago

Thanks for taking a look. We were hoping you might have some insight into if it was a sane change. It works for us and obviously we made it so all tests pass. However we can only verify it against the wsdl we're currently using.

pcai commented 1 year ago

OK so in summary we aren't sure if this is safe but this fixes a problem with a well known provider, I assume this affects others we don't know about too. I will merge this and cut a release and we'll deal with any side effects as they come up. I will also do a major version bump alongside this for the change in supported ruby versions and to ensure this gets extra scrutiny downstream.

Thanks for your contribution.

mattpolito commented 1 year ago

This seems like an appropriate move. Thanks for looking over it.

If you need help maintaining the savon repos, let me know. I'd like to help where you need it.

pcai commented 1 year ago

@mattpolito i would like to take you up on that, couldn't find your contact info and twitter dms seem broken 👎🏻 , can you email me?

mattpolito commented 1 year ago

@pcai I reached out via email.