Replace BalanceTransactionSource class by IBalanceTransactionSource interface #1321
Conversation
| // the ID from the raw JObject. | ||
| var id = ((JObject)value).SelectToken("id")?.ToString(); | ||
| updateId(id); | ||
| updateObject(default(T)); |
There was a problem hiding this comment.
I'm glad you found that, I was planning to ask you about it since I felt we "lost" the ability to handle "unknown" types.
Out of curiosity, how does that work? IBalanceTransactionSource is an interface. It implements IHasId but it does not have an Id property. Where does the property go? I see in the test that it seems to go in SourceId but I don't think it's correct. If you have Source expanded, you expect Source to not be null and it would not make sense to write code that falls back to SourceId right?
Also we likely (absolutely?) want to deserialize Object too since it's the only way for someone to know what type is not handled properly?
There was a problem hiding this comment.
Previously, when encountering an unknown type:
SourceIdis set to the IDSourceis set to aBalanceTransactionSourceinstance with the following properties:Typeis set toBalanceTransactionSourceType.Unknown- all other properties are
null
Now, when encountering an unknown type:
SourceIdis set to the IDSourceis set tonull
So basically, instead of checking that Source.Type != BalanceTransactionSourceType.Unknown, now you just check that Source != null.
In both cases (before and after), you can check the unknown object type and the rest of the JSON via the StripeResponse.ResponseJson property on the top-level BalanceTransactiopn instance.
There was a problem hiding this comment.
Ah okay I see what I misunderstood in all that deserialization. Part of me thought Object and Id would just magically work but it can't really have worked that way. And I assume having a small number of properties filled out is even more confusing than having the entire fields be null.
Thank you for always taking the time to explain this thoroughly.
There was a problem hiding this comment.
No worries. I also think it'd be useful to make the unknown object's name explicit in some way, but I think we'd need to add some logging capabilities to the library first. I'll try and look into that.
r? @remi-stripe
cc @stripe/api-libraries
Like #1320, but for
BalanceTransactionSource.I also fixed an issue with
StringOrObject. Since the interface deserializer can now returnnull(in case of an unknown object), we need to handle this case inStringOrObjectotherwise an exception would be raised when trying to access theIdattribute on thenullobject.