stripe / stripe-java

Java library for the Stripe API.
https://stripe.com
MIT License
791 stars 357 forks source link

Make Source.ThreeDSecure extend Source.Card #741

Closed Flo354 closed 5 years ago

Flo354 commented 5 years ago

Hi there,

I juste tried to upgrade from Stripe 7 to Stripe 8 and found that HasSourceTypeData interface is removed. This mean that we cannot use getTypeData anymore. While it's a huge improvement, I am struggling with a side effect.

My sources are always either a card or a 3DSecured card. Since they both have the fields three_d_secure, brand, .... I was not performing any check on the type because it was simply useless.

By reviewing both Card and ThreeDSecureclasses, I found that they are the same with the only difference that ThreeDSecure class has three additional fields: authenticated, card, customer.

Would it be possible to extends ThreeDSecure from Card instead of StripeObject?

Thanks,

mickjermsurawong-stripe commented 5 years ago

Hi @Flo354, thank you for the report here!

Just for me to understand you situation better. In v7, I assume you have your own model to deserialize the original untyped map from getTypeData. Now moving to v8, since typed data now is returned (Source.Card, Source.ThreeDSecure), you have options like manual mapping from these nested StripeObject to your own model, or to decommission your own model and use Stripe's ones instead. Now, it seems to me that you want to use Source.Card in your core logic, and want to only deal with Source.Card and not to accommodate 3d-secure because that generalize the type to StripeObject?

I'm less inclined to introduce this complexity in our model resources, especially inheritance between nested class. Moreover, in this case, we have root-class level type indicating which one source type is returned, and the semantic here strongly suggests that these source types are independent. But as you have investigated here, seems like Source.Card and Source.ThreeDSecure are not that independent, one is proper subset of the other. So I agree that we are self-inconsistent here.

But that perfect inheritance assumption might break when Source.Card introduces a few more fields that ThreeDSecure do not need. At that point, we can stop the inheritance which will be breaking to you, or we continue but the relationship is no longer accurate. This example actually motivates us to remove all the inheritance pattern--class getting meaningful fields from others (not StripeObject--for all our resources moving from Stripe 7 to 8.

To work on your particular problem, would it be possible that you map Source.Card or Source.ThreeDSecure to your original internal models? It'd be ideal if our model type-ness helps to explain and promises what's expected, but less of tight coupling to your core application. Your own original abstractions would still be helpful here.

Please let me know if I misunderstand your situation, and if my proposed solution is reasonable.

Flo354 commented 5 years ago

Thanks for your detailed answer.

I fully understand the situation. You have to be consistent in a way that types should be independent, but as of today, there is a little inconsistency between cards and 3DSecure cards. But I understand that there can be problems in the future if some fields are added to cards and not to 3DSecure cards.

For now, I think that I will simply do a simple if - else, but it's irritating because I know that the data will be here anyway.

One idea came to me while I was writing this answer. Could we create some interfaces? For example (example code written quickly without any verification) :

public interface CommonCard {

    String getBrand();

    // ... other getters

    String setBrand();

    // ... other setters
}

public Card extends StripeObject implements CommonCard {
    ...
}

public ThreeDSecure extends StripeObject implements CommonCard {
    ...
}

This way, no one will have any problems if the API inserts some fields to Cards and not to ThreeDSecure in the future.

mickjermsurawong-stripe commented 5 years ago

Thank you Florian for the follow-up! Yup the interface would definitely be a better choice here.

Ideally, our current structure of "one-hot" representation, where i) only one of the fields for possible source type can be active ii) type being discriminator specifying that active source type is designed to avoid any relationship between among these subtypes at all.

Now when Source has this pattern, it internally sees card/3d card as separate concepts. To introduce interface among them seems to me that the client-lib is assuming more than our underlying API response.

May I ask if you implement the type check instead? Sorry for the inconvenience. But if you feel strongly about this, I'm happy to take this further and discuss with the team.

Flo354 commented 5 years ago

Yes, I understand the main reason of separation and "no knowledge". Obviously, there is no link between cards and ach_transfer for instance, but in the end, we can't deny that cards and 3D cards are tightly linked and it cannot be ignored.

For now, I will use the type check because there is no other way, but I definitely think that this question needs more investigation if the team has some time to take a look at.

remi-stripe commented 5 years ago

@Flo354 Thanks for the feedback. We have released PaymentMethod support in the API which replaces Card and Source objects. With PaymentMethod there is no "3DS version" like we have on Sources.

I would recommend investigating upgrading to this on your end so that it removes the confusion. You can read more about this here: https://stripe.com/docs/api/payment_methods

mickjermsurawong-stripe commented 5 years ago

I'd like your input on the scenario that we introduce CommonCard please. Should we consider including Source.CardPresent under that "common" interface?

One can consider this Source.CardPresent as a "valid" card, and therefore should implement this CommonCard. If so, intersections of the three (card/ 3d-secure/cardpresent) are reduced to smaller set like brand, last4, exp_month, and exp_year. At this point, I'm wondering if the suggested interface maybe less useful to your use case, because you originally would like to access all the fields under Source.Card.

Taking this further, I think one can examine these source subtypes closely, and arrive at multiple interfaces--at different level of granularity or from different business viewpoints. While there exist relationship between these subtypes, I'm not sure if it is Stripe client-libraries here that should further encode how it views the resources through offering multiple interfaces.

However, if our model as-is causes significant difficulties because it does not encode a prevalent/obvious view of resources (eg. many other developers find this particular pattern hard to integrate and would like to have a unifying concept only for card/3d-card), we should try to make sure we represent our model better. This can be through the change in the schema of API response itself. Maybe we shouldn't distinguish between these two sub-types in the first place?

cc @ob-stripe if you have more thoughts on responsibility of client-lib to encode "interface" between sub-resources even when they seem reasonable as Florian suggested here between card and 3d-secure card.

mickjermsurawong-stripe commented 5 years ago

Ah thank you @remi-stripe for the thoughts here! I really should refresh my page more often on Git. Seems like we do "represent our model better" through PaymentMethod.

Flo354 commented 5 years ago

Thanks to you two! If I want to use PaymentMethod, it means that I have to "convert" all my sources to PaymentMethod (if I want to use only one code) and do the same with my Android/iOS apps? Or is there a "compatibility" between PaymentMethod and Source?

remi-stripe commented 5 years ago

@Flo354 Today, the PaymentMethod resource only support cards though we're working on supporting all other types of Sources (iDeal, SEPA debit, etc.).

You don't need to "convert" your Card or Sources objects either. They work as PaymentMethod immediately (though again, only cards for now). So if you take a Card Source src_123, you can immediately retrieve it as a PaymentMethod and it will just work!

Flo354 commented 5 years ago

Oh okay, thanks!

By reading the doc I thought that it was a whole new object totally independent. So I will use that and it will fix all my problems.

I am just keeping the issue open until I am taking a look at it if you don't mind.

Thanks,

Le lun. 22 avr. 2019 à 5:48 PM, remi-stripe notifications@github.com a écrit :

@Flo354 https://github.com/Flo354 Today, the PaymentMethod resource only support cards though we're working on supporting all other types of Sources (iDeal, SEPA debit, etc.).

You don't need to "convert" your Card or Sources objects either. They work as PaymentMethod immediately (though again, only cards for now). So if you take a Card Source src_123, you can immediately retrieve it as a PaymentMethod and it will just work!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stripe/stripe-java/issues/741#issuecomment-485455500, or mute the thread https://github.com/notifications/unsubscribe-auth/ABANNX4K4ZGQ2NSXIGAYMADPRXM3NANCNFSM4HF54WGA .

Flo354 commented 5 years ago

Hi there,

So I just had a look to the PaymentIntent object in the documentation and it seems that the object doesn't fit my needs.

There are many fields missing from Source and Cards. For instance :

May be I miss something.

remi-stripe commented 5 years ago

@Flo354 I think you might be mixing PaymentIntent and PaymentMethod in your answer. PaymentMethods do support the tokenization method. The field is card[wallet][type] and documented here.

We don't support usage but this is on purpose as this wasn't a feature we wanted to keep. Card PaymentMethods are re-usable as long as they are attached to a Customer, otherwise they aren't.

Finally, we don't support whether 3DS is not supported, or optional and such. This is something that should not live on the Source object as that logic is built in Radar instead and decided on a per-charge basis which lets you require 3DS for charges above a certain amount for example.

Card and Sources are considered deprecated and PaymentMethod is the new recommended integration path. This is what every integration will move to over time as we start adding more features.

Flo354 commented 5 years ago

Hmmm, ok I understand.

My system is still not yet fully compatible with PaymentIntent (migration will take some time) so I see the usage of PaymentMethod and I will use it in the near future.

Thanks for your time!