AttributeError on accessing Charge.refunds

[twavv]

Describe the bug

The typing for Charge indicates that there should be a refunds property:

    refunds: Optional[ListObject["Refund"]]
    """
    A list of refunds that have been applied to the charge.
    """

Attempting to access it raises an AttributeError.

To Reproduce

Fetch a charge and attempt to access it's refunds attribute.

Expected behavior

It should work.

Code snippets

OS

macOS

Language version

Python 3.120

Library version

12.1.0

API version

Additional context

No response

[twavv]

Worth noting that this works if expand=["refunds"] is given, and is probably(?) a result of the 2022-11-15 API change that stops auto-expanding charge.refunds, but this still should not raise an AttributeError.

[remi-stripe]

The refunds property exists and is documented here but it's what we call includable which means it's not returned by default unless you explicitly request it with our Expand feature. I recommend reading this doc or watching this Youtube video.

This is an integration support question though so I recommend reaching out to our support team if you have more questions: https://support.stripe.com/contact

I do think we could improve the docstring here though so I'll flag internally!

[twavv]

Hey @remi-stripe please read my response above (I think I wrote it while you were composing yours). Either way, to the best of my knowledge, accessing charge.refunds without expanding it shouldn't raise an AttributeError, it should return None.

[remi-stripe]

I disagree raising an error is incorrect in this case though because the property was never returned. We could support None but how do you distinguish whether the property was never included/expanded (so not returned) and whether it was but it was null in the API? It can't happen for refunds on Charge because it'd be an "empty list" but it can happen for other properties that would be a scalar. That's partly why we designed it this way in multiple of our SDKs.

[twavv]

It certainly violates the principle of least surprise (which is that if the type signature says a property is defined, it should be defined). I only caught this bug because of manual testing, and it's certainly the class of bugs I would expect to be caught at type-checking time.

I understand the need for consistency, and if that wins out, I definitely ✨get it✨. But it's certainly confusing. In other places where I've used expand, those properties usually return ID's by default (in the Python SDK using the ExpandableField[T] type which is defined as T | str).

This is the first place I've seen it just not be included at all by default and raise an AttributeError.

Even with what you've said, the typing of Charge.refunds is still (presumably?) wrong. It should be ListObject["Refund"] instead of Optional[ListObject["Refund"]] – unless I misunderstand, it will never actually be None.

[twavv]

For what it's worth, this doesn't seem to be an issue in the Go SDK:

type Charge struct {
	...
	// A list of refunds that have been applied to the charge.
	Refunds *RefundList `json:"refunds"`
	...
}

Go has significantly different conventions around optional fields, so probably not a perfect example, but definitely a pastability 🍝.

[remi-stripe]

In stripe-python it is what we mean with Optional which is how you'd know. If it was the other flavour of expandable where you get the object id customer: cus_123 or the full object customer: { id: 'cus_123', object: 'customer', ...} it is typed differently.

Go does have a different convention. But for example for the second case it would have Customer *Customer json: 'customer'making you think you always get the full object but you don't and have to check ifCustomer.Object` is set or not which is definitely worse and something we hope to fix in the future.

I do hear you on the lack of discoverability and it's something we will continue to work on and aim to improve!

[twavv]

I think this issue is definitely more than ✨discoverability✨. The convention here is not at all expected – accessing an attribute that's defined to exist should not cause an AttributeError. The current version requires either 1) knowing that charge.refunds was explicitly expanded in the code that accesses it or 2) doing a "refunds" in charge check first.

I've chosen the first approach with my own code, and we just pass around separate Charge and list[Refund] objects, but that also requires anyone who touches this code to remember that accessing charge.refunds is not "safe" and you will get no help from the type checker.

The second approach seems required in the more general case, but also is at odds with #1454 – how would you check without the in check (which only works if the object is a Mapping)?

I definitely understand that y'all have your own constraints and considerations, but the current approach feels far from ideal (it's likely to cause unanticipated runtime errors and bugs). I can imagine some alternatives like charge.refunds_expanded to determine if refunds were expanded and then have charge.refunds return ListObject[Refund] | None (no AttributeError) – that at the very least is a lot less likely to introduce runtime errors that make it to production.

I also don't really buy your argument about Optional – Optional[T] is just T | None, but the value actually can never be none. It heavily implies that you can do a if charge.refunds: check, but that will raise a runtime error if refunds wasn't expanded.

Thanks for hearing me out. I hope you'll give it some thought. :^)

[remi-stripe]

I appreciate the thoughtful feedback and I definitely agree with your sentiment. Your Github issue has sparked some discussions internally on how we could approach this better at least so hopefully we can improve the types in the future to address this further than just a docstring improvement.

[twavv]

Yay!

Just as another piece of data, this actually did cause a bug for us today in production (I just had to fix it as I'm on call, yay! ✨)

Essentially, we had this code:

res = stripe.PromotionCode.list(
  code=promo_code,
  active=True,
  expand=["data.coupon.applies_to"],
  stripe_account=stripe_connect_account_id,
)
for code in res.data:
    applies_to = code.coupon.applies_to
    # do something with applies_to

This feels even more unexpected. If I'm requesting to include the field via expand=, it certainly shouldn't raise an AttributeError here, no? Now we have to do

applies_to = code.coupon.applies_to if "applies_to` in code.coupon else None

[remi-stripe]

This one is quite interesting. My take here is that this is an API bug. If you haven't set applies_to on the Coupon it should in theory be returning applies_to: null when you expand it instead of ignoring the property entirely. The API Reference for the property even says it is nullable.

It's likely too late to change as developers likely rely on that but I do think it's a problem with the API and not the types for stripe-python here. But I know that explaining the why doesn't really solve your problem in this case.