-
Notifications
You must be signed in to change notification settings - Fork 572
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Interfaces for polymorphic API resources (external accounts and payment sources) #1320
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
namespace Stripe | ||
{ | ||
/// <summary> | ||
/// Resources that implement this interface can be used as external accounts, i.e. they can | ||
/// be attached to a Stripe account to receive payouts. | ||
/// </summary> | ||
public interface IExternalAccount : IStripeEntity, IHasId, IHasObject | ||
{ | ||
Account Account { get; set; } | ||
|
||
string AccountId { get; set; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why we have properties in this interface. I thought maybe you wanted to be smart and use this for the nested service but it does not seem to be the case. Can you clarify? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having the properties declared in the interface lets you access those properties without casting, e.g.: var payout = payoutService.Get("po_123");
var account = payout.Destination.AccountId; vs. var payout = payoutService.Get("po_123");
var destination = payout.Destination;
var account = null;
if (destination is BankAccount)
{
account = ((BankAccount)destination).AccountId;
}
else if (destination is Card) {
account = ((Card)destination).AccountId;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also wanted to do something similar with I could add an additional There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would you access the Account id on an external account? You already know which account it's on since you retrieved the Payout on that account right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think "why" matters. Conceptually, an external account is a payment destination that is attached to a (Stripe) account, so it makes sense that the interface reflects this. It also costs us literally nothing to add this since the two concrete classes that implement this interface already fulfill that contract. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I disagree though. I think it matters to be consistent though and the fact that it does not happen on IPaymentSource feels confusing to me and not useful. If you were using it explicitly for the service I would say maybe but here I don't see the benefits. Like you could also say the same about This is my personal opinion and I am fine to merge as is though as I agree it ultimately works and I don't want to block. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
namespace Stripe | ||
{ | ||
/// <summary> | ||
/// Resources that implement this interface can be used to create charges. | ||
/// </summary> | ||
public interface IPaymentSource : IStripeEntity, IHasId, IHasObject | ||
{ | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
namespace Stripe.Infrastructure | ||
{ | ||
using System; | ||
using System.Collections.Generic; | ||
using Newtonsoft.Json; | ||
using Newtonsoft.Json.Linq; | ||
|
||
/// <summary> | ||
/// This converter is used to deserialize polymorphic resources. The resources must implement | ||
/// a common interface. | ||
/// </summary> | ||
/// <typeparam name="I">interface implemented by all resources that this converter can | ||
/// return</typeparam> | ||
internal abstract class AbstractStripeObjectConverter<I> : JsonConverter | ||
where I : IHasObject | ||
{ | ||
public override bool CanWrite => false; | ||
|
||
/// <summary> | ||
/// This is the only property that needs to be declared by concrete converters. It is a | ||
/// mapping of object names (e.g. <c>"card"</c>) to mapper functions (e.g. | ||
/// <c>Mapper<Card>.FromJson</c>). | ||
/// </summary> | ||
protected abstract Dictionary<string, Func<string, I>> ObjectsToMapperFuncs { get; } | ||
|
||
public override bool CanConvert(Type objectType) | ||
{ | ||
return objectType == typeof(I); | ||
} | ||
|
||
public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer) | ||
{ | ||
var incoming = JObject.FromObject(value); | ||
incoming.WriteTo(writer); | ||
} | ||
|
||
public override object ReadJson(JsonReader reader, Type objectType, object existingValue, JsonSerializer serializer) | ||
{ | ||
var incoming = JObject.Load(reader); | ||
var obj = default(I); | ||
var objectName = incoming.SelectToken("object")?.ToString(); | ||
if (this.ObjectsToMapperFuncs.ContainsKey(objectName)) | ||
{ | ||
var mapperFunc = this.ObjectsToMapperFuncs[objectName]; | ||
obj = mapperFunc(incoming.ToString()); | ||
} | ||
|
||
return obj; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,47 +1,15 @@ | ||
namespace Stripe.Infrastructure | ||
{ | ||
using System; | ||
using Newtonsoft.Json; | ||
using Newtonsoft.Json.Linq; | ||
using System.Collections.Generic; | ||
|
||
internal class ExternalAccountConverter : JsonConverter | ||
internal class ExternalAccountConverter : AbstractStripeObjectConverter<IExternalAccount> | ||
{ | ||
public override bool CanWrite => false; | ||
|
||
public override bool CanConvert(Type objectType) | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
|
||
public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer) | ||
{ | ||
var incoming = JObject.FromObject(value); | ||
|
||
incoming.WriteTo(writer); | ||
} | ||
|
||
public override object ReadJson(JsonReader reader, Type objectType, object existingValue, JsonSerializer serializer) | ||
protected override Dictionary<string, Func<string, IExternalAccount>> ObjectsToMapperFuncs | ||
=> new Dictionary<string, Func<string, IExternalAccount>>() | ||
{ | ||
var incoming = JObject.Load(reader); | ||
|
||
var externalAccount = new ExternalAccount | ||
{ | ||
Id = incoming.SelectToken("id").ToString() | ||
}; | ||
|
||
if (incoming.SelectToken("object")?.ToString() == "bank_account") | ||
{ | ||
externalAccount.Type = ExternalAccountType.BankAccount; | ||
externalAccount.BankAccount = Mapper<BankAccount>.MapFromJson(incoming.ToString()); | ||
} | ||
|
||
if (incoming.SelectToken("object")?.ToString() == "card") | ||
{ | ||
externalAccount.Type = ExternalAccountType.Card; | ||
externalAccount.Card = Mapper<Card>.MapFromJson(incoming.ToString()); | ||
} | ||
|
||
return externalAccount; | ||
} | ||
{ "bank_account", Mapper<BankAccount>.MapFromJson }, | ||
{ "card", Mapper<Card>.MapFromJson }, | ||
}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,59 +1,17 @@ | ||
namespace Stripe.Infrastructure | ||
{ | ||
using System; | ||
using Newtonsoft.Json; | ||
using Newtonsoft.Json.Linq; | ||
using System.Collections.Generic; | ||
|
||
internal class PaymentSourceConverter : JsonConverter | ||
internal class PaymentSourceConverter : AbstractStripeObjectConverter<IPaymentSource> | ||
{ | ||
public override bool CanWrite => false; | ||
|
||
public override bool CanConvert(Type objectType) | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
|
||
public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer) | ||
{ | ||
var incoming = JObject.FromObject(value); | ||
|
||
incoming.WriteTo(writer); | ||
} | ||
|
||
public override object ReadJson(JsonReader reader, Type objectType, object existingValue, JsonSerializer serializer) | ||
protected override Dictionary<string, Func<string, IPaymentSource>> ObjectsToMapperFuncs => | ||
new Dictionary<string, Func<string, IPaymentSource>>() | ||
{ | ||
var incoming = JObject.Load(reader); | ||
|
||
var source = new PaymentSource | ||
{ | ||
Id = incoming.SelectToken("id").ToString() | ||
}; | ||
|
||
if (incoming.SelectToken("object")?.ToString() == "account") | ||
{ | ||
source.Type = PaymentSourceType.Account; | ||
source.Account = Mapper<Account>.MapFromJson(incoming.ToString()); | ||
} | ||
|
||
if (incoming.SelectToken("object")?.ToString() == "bank_account") | ||
{ | ||
source.Type = PaymentSourceType.BankAccount; | ||
source.BankAccount = Mapper<BankAccount>.MapFromJson(incoming.ToString()); | ||
} | ||
|
||
if (incoming.SelectToken("object")?.ToString() == "card") | ||
{ | ||
source.Type = PaymentSourceType.Card; | ||
source.Card = Mapper<Card>.MapFromJson(incoming.ToString()); | ||
} | ||
|
||
if (incoming.SelectToken("object")?.ToString() == "source") | ||
{ | ||
source.Type = PaymentSourceType.Source; | ||
source.SourceObject = Mapper<Source>.MapFromJson(incoming.ToString()); | ||
} | ||
|
||
return source; | ||
} | ||
{ "account", Mapper<Account>.MapFromJson }, | ||
{ "bank_account", Mapper<BankAccount>.MapFromJson }, | ||
{ "card", Mapper<Card>.MapFromJson }, | ||
{ "source", Mapper<Source>.MapFromJson }, | ||
}; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we implementForgot Account is a source now, damn that is confusing lolIPaymentSource
on Account?