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

Fix elementFormDefault="qualified" regression #917

Merged
merged 2 commits into from
Jul 5, 2020

Conversation

lukaso
Copy link

@lukaso lukaso commented Dec 3, 2019

Types were not properly qualified

What kind of change is this?

Bugfix

Did you add tests for your changes?

Yes. I've now added a breaking test which ensures the message is properly encoded.

Summary of changes

This fixes a bug raised in #916. This means more recent versions of Savon do not work properly in certain specific circumstances when elements are embedded in types.

Other information

The test is based on a real-world wsdl file in which messages are being incorrectly "qualified".

Lukas Oberhuber added 2 commits December 3, 2019 09:17
Types were not properly qualified
This test is based on a simplified real world wsdl
@lukaso
Copy link
Author

lukaso commented May 25, 2020

Given the slow pace of incorporating changes, I've created a gem that patches this issue. All you have to do is include it in your Gemfile:

gem 'savon_fixes'

You can find the gem here: https://github.com/lukaso/savon_fixes. It currently covers off these issues and PRs: #916, #899, #820, #895, #862, #905 and #549 (they are all duplicates).

I haven't yet tested it in anger, but all tests pass, so please feedback if there are any issues. Adaptations of PRs here are also welcome.

@sergioisidoro
Copy link

@lukaso does those fixes also patch this one?
#830

@lukaso
Copy link
Author

lukaso commented Jun 3, 2020

@lukaso does those fixes also patch this one?
#830

Yes, I believe it does. Thanks for tracking that down.

@pcai
Copy link
Member

pcai commented Jul 5, 2020

@lukaso thanks! I apologize for the long turnaround. It looks like you did this by the book and added a failing test. I read through this change and can confirm it makes it pass now. Just acknowledging this while I dig into the details to understand the intended behavior and the bug.

@pcai
Copy link
Member

pcai commented Jul 5, 2020

OK I reread #916 and it was mentioned there, but the most succinct way to summarize the actual/expected behavior is the diff of the test output before and after the fix in this PR:

image

Given a WSDL with elementFormDefault="qualified", the attribute will now be correctly qualified with the namespace.

This LGTM, I'll check if I can confirm the jruby failure is bogus and merge this in.

Copy link
Member

@pcai pcai left a comment

Choose a reason for hiding this comment

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

@pcai pcai merged commit 5171a56 into savonrb:master Jul 5, 2020
pcai pushed a commit that referenced this pull request Jul 5, 2020
Types were not properly qualified
This test is based on a simplified real world wsdl
pcai pushed a commit that referenced this pull request Jul 5, 2020
Types were not properly qualified
This test is based on a simplified real world wsdl
pcai pushed a commit that referenced this pull request Jul 5, 2020
Types were not properly qualified
This test is based on a simplified real world wsdl
@lukaso
Copy link
Author

lukaso commented Jul 7, 2020

Thank you!

@lukaso lukaso deleted the fix_element_form_default branch July 7, 2020 08:36
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.

3 participants