AttributeError: service_agreement

[pslowinski-wn]

Describe the bug

Account class has tos_acceptance: Optional[TosAcceptance].
TosAcceptance class has service_agreement: Optional[str]

The following code:

if (
            connected_account.tos_acceptance is not None
            and connected_account.tos_acceptance.service_agreement == "recipient"
        ):

Should not rais exceptions, but it does:
AttributeError: service_agreement

To Reproduce

1. Fetch the account with non-recipient service agreement
2. Run the above if statementthe

Expected behavior

Exception should not be raised

Code snippets

No response

OS

macOS

Language version

3.11.4

Library version

8.1.0

API version

2019-08-14

Additional context

No response

[remi-stripe]

@pslowinski-wn I don't fully grasp the issue you're reporting here. The service_agreement property is only returned if its value is recipient, otherwise it's not returned. So you have to check if the value is defined/present before checking the content I think

[pslowinski-wn]

@remi-stripe Optional[X] is equivalent to Union[X, None].. I should never get AttributeError: service_agreement when calling service_agreement on not None tos_acceptance. The only possible values are str or None

I hope it makes sense.

[remi-stripe]

@pslowinski-wn Your code checks whether tos_acceptance is None and you have to do the same for service_agreement. For example this works for me:

if (
  accountServiceAgreementRecipient.tos_acceptance is not None and
  accountServiceAgreementRecipient.tos_acceptance.service_agreement is not None and
  accountServiceAgreementRecipient.tos_acceptance.service_agreement == "recipient"
):
  print(accountServiceAgreementRecipient.tos_acceptance.service_agreement)

I do think you have to handle the None case before you can check the equality to a string like you were doing.

[remi-stripe]

@pslowinski-wn I confirmed this is also covered in our wiki here under the Your type checker may require additional proof of safety section.

[pslowinski-wn]

@pslowinski-wn Your code checks whether tos_acceptance is None and you have to do the same for service_agreement. For example this works for me:

if (
  accountServiceAgreementRecipient.tos_acceptance is not None and
  accountServiceAgreementRecipient.tos_acceptance.service_agreement is not None and
  accountServiceAgreementRecipient.tos_acceptance.service_agreement == "recipient"
):
  print(accountServiceAgreementRecipient.tos_acceptance.service_agreement)

I do think you have to handle the None case before you can check the equality to a string like you were doing.

When I run this code, it only works for accounts with recipient service agreement. For other accounts, I am getting the following exception:

  File ".../.venv/lib/python3.11/site-packages/stripe/_stripe_object.py", line 172, in __getattr__
    raise AttributeError(*err.args)
AttributeError: service_agreement

[remi-stripe]

Damn you're right I'm sorry. I thought I thoroughly tested the examples but I double checked it and I tested on the wrong account. Right now the only solution I found is to use hasattr() first. I'm checking with a few other people if there's a better alternative or not and will update you after!

[pslowinski-wn]

I am doing the below:

if (
            connected_account.tos_acceptance is not None
            and getattr(connected_account.tos_acceptance, "service_agreement", None)
            == "recipient"
        )

However, it is rather a hack ;-)

[remi-stripe]

@pslowinski-wn Okay so I think our types are incorrect for the case where a property is sometimes completely absent. Optional means that is can be None but not that it can be absent. Instead we should use NotRequired for this. I got a bit confused because in other languages "optional" means "can be absent" and otherwise it's called "nullable" when it can return null (what Python calls None).

For now, I think using hasattr() is your best approach like this

if (
  accountServiceAgreementFull.tos_acceptance is not None and
  hasattr(accountServiceAgreementFull.tos_acceptance, 'service_agreement') and
  accountServiceAgreementFull.tos_acceptance.service_agreement is not None
):
  print(accountServiceAgreementFull.tos_acceptance.service_agreement)
else: 
  print ("This account has no service_agreement property, so I assume it's 'full'")

And we're going to investigate changing the types to use NotRequired but that will take a bit of time to scope and design. I'll keep your issue open in the meantime though!

[pslowinski-wn]

I am definitely not a Python expert. However, NotRequired doesn't look like a very client-friendly type. The client still would have to make a similar check to validate whether the field exists. It feels like providing a default None value would be more client-friendly. 

[remi-stripe]

I don't disagree but we avoid doing that in our SDKs and we mirror what the API does instead. It's a common pattern to have some properties really be optional. The most canonical example would be the PaymentMethod API where you have type: 'card' | 'klarna' | 'sepa_debit' | ... and then based on the value of type you also have a property with that name with more details so card: {....} or klarna: {....} but only one returned, everything else is not returned.

But I agree it can make it awkward in some languages and that's what we're going to investigate further.

And to be clear, I've never liked that service_agreement is not returned for full. I've been trying to get it fixed internally since it launched but we haven't prioritized that fix just yet. But in this specific case I think service_agreement should neither be null or optional and should always return the exact value.

[richardm-stripe]

If we wanted to change the types to match what happens at runtime, we would need NotRequired -- except that's not actually possible with the state of Python types today, because NotRequired only exists for TypedDict types and StripeObject is an actual class.

If we wanted to change the runtime to match what's described by the types, your suggestion

It feels like providing a default None value would be more client-friendly
is what would accomplish this. But this would be a very breaking change (too breaking for us to consider, I think), as it would cause code

if hasattr(account.tos_acceptance, "service_agreement"):
    # Assume it's there and set to an actual value
else:
    # It's not there

to break and need to change to an account.tos_acceptance.service_agreement is not None check.

---

We could consider making "absent as none" behavior opt-in, and recommend this to anybody who uses types -- which I am experimenting with in #1229. Then it would be

stripe.absent_as_none = False
if (account.tos_acceptance.service_agreement is not None):
  # this would no longer raise

and this would make the runtime behavior match what's described in the types. But this seems hard to discover and doesn't feel like a true fix, and the types would still be inaccurate for anybody unable to discover this option.

[richardm-stripe]

Another option would be to leave the possibly-completely-absent fields at runtime, omit them from the type definitions, and expose properties or getters with a different name and the desired semantics, e.g. instead of

>>> account.tos_acceptance.service_agreement
KeyError: 'service_agreement'

you would do

>>> account.tos_acceptance.get_service_agreement()
None

or maybe

>>> account.tos_acceptance.service_agreement_value
None

but so far I can't really think of any naming scheme that would make this feel good.

[pslowinski-wn]

We could consider making "absent as none" behavior opt-in, and recommend this to anybody who uses types -- which I am experimenting with in #1229. Then it would be

I like that idea. I would also hope absent_as_none=True will be a default with future releases.
Note: I come from the JVM world, so I have some biases :D

[richardm-stripe]

I opened a "typing" discussion on the Python discourse about this issue to see if anybody there has any ideas.

https://discuss.python.org/t/typing-notrequired-like-for-potentially-absent-class-attributes/46963

If not, I think probably my preferred approach for addressing this is the add additional getters I described, which we would pursue if enough users were running into this and having trouble.