suds-community / suds

Suds is a lightweight SOAP python client for consuming Web Services. A community fork of the jurko fork.
https://suds.readthedocs.io/
GNU Lesser General Public License v3.0
173 stars 56 forks source link

Breaking behavior change was introduced in Jurko unreleased code #14

Closed guifran001 closed 5 years ago

guifran001 commented 5 years ago

The following commit https://github.com/suds-community/suds/commit/b6d1d0979e1847a6b8f267be1888892e9327c2a6 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 commented 5 years ago

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 commented 5 years ago

@guifran001 I've added a failing test for the undesired behavior in https://github.com/suds-community/suds/commit/d34c3a58e6ff54a48de07d6ee1fa557d1ded69f7.

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 commented 5 years ago

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

guifran001 commented 5 years ago

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 commented 5 years ago

Submitted a fix in pr #15

phillbaker commented 5 years ago

Closed by https://github.com/suds-community/suds/pull/15