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

Breaking behavior change was introduced in Jurko unreleased code #14

Closed
guifran001 opened this issue Jan 23, 2019 · 6 comments
Closed

Comments

@guifran001
Copy link
Contributor

guifran001 commented Jan 23, 2019

The following commit
b6d1d09
introduced a behavior change that breaks code that was using suds-jurko 0.6.

Since, this code was never released, I wonder if the commit could be reverted.
Originally, it was to fix this problem:
https://bitbucket.org/jurko/suds/issues/81

The original behavior was (suds-jurko 0.6):
When creating a sudsojbect.Object this way: client.factory.create("myObjectr"), the instanciated object has all its optional attributes were set to None and not added into the payload when calling a service with this object as argument.

The new behavior is (suds-community):
When creating a sudsojbect.Object this way: client.factory.create("myObject"), the instantiated object has all its optional attributes set. When calling a service with with this object as argument will result with having those optional attributes in the payload as .

I don't think the new behavior is correct, because if one updates a SOAP service to add an optional attribute to an object, it is absurd that all the clients using suds will start to add it by default and therefore change the expected behavior (the absence of an optional attribute is different than the presence of an empty attribute (for the server we are using at least)).

I've not been able to think of a solution for the original issue though, but it should not be fixed like that.

Can the commit be reverted ?

@phillbaker
Copy link
Member

phillbaker commented Jan 24, 2019

Hi @guifran001 thanks for opening an issue, very interesting!

I've also noticed the unfortunate side effect that instantiating an object with optional attributes instantiates that object recursively.

To quote from the commit message:

Before, passing an empty suds object as an optional parameter value was treated
the same as not passing that parameter's value or passing it None - the value
would not get marshalled into the constructed SOAP request at all.

Now, user can still not have the value marshalled by passing nothing or None,
but passing an empty object will get marshalled as an actual SOAP request XML
element.

This seems correct as passing an empty object and not passing an object are two
distinct use-cases and there are web-services out there (e.g.
https://ads.google.com/apis/ads/publisher/v201502/LineItemService?wsdl) that
do differentiate between the two.

I don't think simply reverting the change is the right course, as the fix does address a valid usecase: passing an empty object. But perhaps we can separate the instantiation from the serialization? For example, instead of:

# suds-community now
client.factory.create("SomeObject") 
(SomeObject){
   Foo = 123456
   Required = 
      (Foo){
         Bar = None
      }
   Optional = 
      (Foo){
         Bar = None
      }
   _MessageId = "1a88625d-57d4-4c4e-b3e4-7c5caa7bfe5f"
 }

we instead do (ie forcing the explicit creation of empty objects):

# suds-community proposed change
o = client.factory.create("SomeObject") 
o
(SomeObject){
   Foo = 123456
   Required = 
      (Foo){
         Bar = "baz"
      }
   Optional = None
   _MessageId = "1a88625d-57d4-4c4e-b3e4-7c5caa7bfe5f"
 }
o.Optional = client.factory.create("Foo")
o.Optional
(Foo){
  Bar = None
}

I'm a little puzzled that the Appender in the commit is used in the creation of objects as well. Any chance you can dig into the factory.create method to see how it ends up calling the Appender?

@phillbaker
Copy link
Member

phillbaker commented Jan 27, 2019

@guifran001 I've added a failing test for the undesired behavior in d34c3a5.

Unfortunately adding the two lines back in don't make the test pass. Can you take a look at the test and make sure that it's describing the behavior in this issue?

@guifran001
Copy link
Contributor Author

guifran001 commented Jan 28, 2019

I'm going to take a look. Your test seem to test the right behavior

@guifran001
Copy link
Contributor Author

guifran001 commented Jan 28, 2019

I've found a fix. The test I've created that represent our current problem has a WSDL too complicated though, so I will add a service to your test because my first attempt to fix it was successful with your test, but not mine.
I should submit a pr tomorrow

@guifran001
Copy link
Contributor Author

guifran001 commented Jan 30, 2019

Submitted a fix in pr #15

@phillbaker
Copy link
Member

phillbaker commented Jan 31, 2019

Closed by #15

phillbaker added a commit that referenced this issue Jan 29, 2022
In some unreleased versions of suds-jurko, all elements that were populated with empty lists. This was fixed in suds-commnunity as a regression, see #14, #15, and #16.

This changes allows consumers of suds to have the suds-jurko behavior by doing the following:
```
from suds.client import Client, Builder

class MyBuilder(Builder):
    def skip_value(self, type):
        return False # always set value

client = Client('...')
client.factory.builder = MyBuilder(client.factory.builder.resolver)
```
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

No branches or pull requests

2 participants