Closed tpow closed 2 years ago
Hi @tpow thanks for opening an issue and for providing a test case, that's fantastic.
My guess is that multi_occurrence() needs to look recursively into the nesting before failing. In our case the top level is optional, but the next is not.
To clarify here, Payments
, Charges
, Refunds
, Voids
are marked as optional with the minOccurs="0"
:
<s:element minOccurs="0" maxOccurs="1" name="Payments" type="s1:ArrayOfPaymentInMsg" />
<s:element minOccurs="0" maxOccurs="1" name="Charges" type="s1:ArrayOfChargeInMsg" />
<s:element minOccurs="0" maxOccurs="1" name="Refunds" type="s1:ArrayOfRefundInMsg" />
<s:element minOccurs="0" maxOccurs="1" name="Voids" type="s1:ArrayOfVoidInMsg" />
where is the next level marked as required? Does the WSDL itself leave these objects as optional, but the server interpretation require them?
req.Charges.ChargeInMsg = [sample_charge,] # if req.Charges is None, cannot do above, but # the following doesn't seem to work correctly # req.Charges = [sample_charge,]
Would something like req.Charges = client.factory.create("ns1:ArrayOfChargeInMsg")
work?
Since this is less convenient and a pain to convert codebases if you're trying to convert to suds-community, one workaround is here: https://github.com/BingAds/BingAds-Python-SDK/commit/8d1a51595f9f4776d2b719be7f3fc1378cee4f4f#diff-84a7c57706647cffbba7321972ce82a9aa06e7d49b8c32b4d7a34417ffba9e45R13, provide a custom Builder class to client.factory.builder
that has the desired behavior.
Since this has come up a few times, I could see a few options going forward:
class Builder:
# ...
def skip_value(self, type):
""" get whether or not to skip setting the value """
return type.optional() and not type.multi_occurrence()
class MyBuilder: def skip_value(self, type): return False # always set value
What do you think?
@phillbaker Thank you for the helpful feedback and suggestions. It is possible that this API is incorrectly constructed, but as we say at work "the real world is messy" and we need to deal with it.
When I said that multi_occurrence()
may need to look recursively, I was thinking about this (sorry I didn't note this explicitly):
Although as you noticed, these are "optional" with minOccurs = "0"
<s:element minOccurs="0" maxOccurs="1" name="Payments" type="s1:ArrayOfPaymentInMsg" />
<s:element minOccurs="0" maxOccurs="1" name="Charges" type="s1:ArrayOfChargeInMsg" />
<s:element minOccurs="0" maxOccurs="1" name="Refunds" type="s1:ArrayOfRefundInMsg" />
<s:element minOccurs="0" maxOccurs="1" name="Voids" type="s1:ArrayOfVoidInMsg" />
However, the elements they reference are not (and also are complex types). For example, the ArrayOfChargeInMsg
has maxOccurs="unbounded"
:
<s:complexType name="ArrayOfChargeInMsg">
<s:sequence>
<s:element minOccurs="0" maxOccurs="unbounded" name="ChargeInMsg" nillable="true"
type="s1:ChargeInMsg" />
</s:sequence>
</s:complexType>
Because of this, I was thinking Payments
, Charges
, Refunds
and Voids
would all be created and set to empty lists like with suds-jurko.
I'm not sure if this postledger
API would handle some of them being None. Based on the schema, I would hope so. I suspect the real reason it fails is because it needs at least one of them to be populated, but I'm not certain. You might be correct that I could do:
req.Charges = client.factory.create("ns1:ArrayOfChargeInMsg")
but then I would still need to create the ChargeInMsg and do the setup. It would be a little inconvenient to have to do both. That was my understanding of why multi_occurrence()
was created in the first place -- it was trying to make it so arrays existed even though they are "optional". I think this is a nested complex type variant of the same problem. (In other words, your first bullet point: I believe there a way to determine whether the attribute is optional, or rather is a list. The others may be helpful for those messy real world situations, but I think it would be easier if suds provided the empty lists in this case.)
What are your thoughts? Tim
Update: we tested the API with additional code:
if req.Charges is None:
req.Charges = client.factory.create("ns1:ArrayOfChargeInMsg")
This generated a request like this:
(PostLedgerRequest){
TokenId = "<redacted>"
Payments = None
Charges =
(ArrayOfChargeInMsg){
ChargeInMsg[] =
(ChargeInMsg){
StudentId = 1234
Amount = 987.65
Description = "Room and Board"
... plus other parameters redacted for simplicity and clarity ...
},
}
Refunds = None
Voids = None
}
This worked fine, so the API is handling the "optional" values as the schema indicates. I still think it would be convenient (and assist with the transition from suds-jurko) to construct the empty lists so we don't need to build the ArrayOfChargeInMsg
, but this works.
Thanks!
I pushed a change which tries to implement the suggestion on https://github.com/suds-community/suds/tree/bugfix/complex-sequence, however, it causes other tests to fail.
For now, I've implemented the alternative approach involving subclassing, see 366f7f1.
Thanks for working on this @phillbaker. I believe the subclassing approach may be all that is needed and is easy enough. Is it worth adding it to the readme/docs?
One point of clarification: I believe the description in 366f7f1 is inaccurate. This behavior was in the latest released version of suds-jurko which is why I reported it. I believe suds-community implemented the new behavior before any official release, but suds-jurko definitely had the old behavior in the released versions.
Is it worth adding it to the readme/docs?
https://github.com/suds-community/suds/commit/366f7f1616595b9e4163a3f90fc6e84ac0ae23f5 includes an update to the readme, https://github.com/suds-community/suds#initializing-optional-arrays-with-lists
This behavior was in the latest released version of suds-jurko
Hm, the last released version of suds-jurko is 0.6 (https://pypi.org/project/suds-jurko/#history), corresponding to this tag: https://github.com/suds-community/suds/releases/tag/release-0.6, released in 2014. Issue #14 identifies this commit as introducing the change: https://github.com/suds-community/suds/commit/b6d1d0979e1847a6b8f267be1888892e9327c2a6, from 2015. So it doesn't seem like it was in suds-jurko, but it was released in suds-community in v0.7.0 (the first suds-community release) and then the change in #15 was released in suds-community 0.8.0. So suds-community definitely changed.
Thanks for opening the issue and providing helpful feedback! I'm going to close this issue for now, if folks can make bugfix/complex-sequence work, would be happy to review a PR that implements that behavior as well.
We recently evaluated switching to suds-community from suds-jurko and found a compatibility problem. Elements that were previous populated with empty lists are now set to None. Here's the comparison:
suds-community gives this:
Versus suds-jurko:
I've dug in to the problem and narrowed it down to the change in behavior due to #14, #15, and #16. Reverting the changed line (as in
setattr(data, type.name, value) # if not type.optional() or type.multi_occurrence() else None)
) in builder.py fixes the problem for us, but think I understand why the change was made in the first place so suspect that removing all of it is not the right solution.My guess is that
multi_occurrence()
needs to look recursively into the nesting before failing. In our case the top level is optional, but the next is not.It is also possible that we aren't using suds correctly or misunderstand the structure somehow, but I can say that the code works with sud-jurko and not with suds-community.
Although the API we are calling is not public and is controlled by a vendor, I have built the following test that demonstrates the problem. If dropped into the test suite, it should run and fail. Note that this is a pruned down and somewhat modified version of the vendor's schema and almost certainly could be simplified more. Sorry that it is a bit noisy.
It looks like incorporating parts of this into the test suite, perhaps as an expansion to the changes made for the #16 pull request would be beneficial.
We would appreciate any clarification if we are misusing suds somehow and/or confirmation that this is a bug.
Thanks! Tim