Revamp interfaces on Stripe model classes #1317

ob-stripe · 2018-10-02T21:07:45Z

r? @remi-stripe
cc @stripe/api-libraries

Adds an IStripeEntity interface implemented by StripeEntity
Adds IHasId and IHasObject interfaces
Removes StripeEntityWithId
Ensures that all model classes derive from StripeEntity, and add IHasId and IHasObject interfaces where needed
Removes ISupportMetadata (ISupportMetadata was only useful to properly encode metadata with the old plugin system, it's useless now)
Renames AdditionalOwners to AdditionalOwner (the model represents a single additional owner)
Removes support for old format of request attribute on event objects (cf. https://stripe.com/docs/upgrades#2017-05-25)

ob-stripe · 2018-10-02T21:11:13Z

Also, to clarify the motivation behind this: having both IHasId and IHasObject interfaces makes it possible to cleanly define e.g. IExternalAccount that implements both interfaces above (since an external account always has both an id and an object attribute).

IHasId can be used for field expansion (I've updated the StringOrObject generic in this PR) and IHasObject can be used in a generic JSON deserializer that instantiates the correct concrete class based on the value of the object attribute.

remi-stripe

Some minor comments in the code.

Should we do something similar for Deleted and Metadata to not have to put it on every resource that needs it?
Would be neat but then StripeEntityWithIdAndObjectAndDeletedAndMetdata is not really cool. Maybe metadata should be the default for StripeEntityWithId and you'd have to explicitly use StripeEntityWithIdAndNoMetadata for the ones that explicitly don't support it? But it's easy to miss if so.

remi-stripe · 2018-10-03T13:30:40Z

src/Stripe.net/Entities/Disputes/Dispute.cs

@@ -5,11 +5,8 @@ namespace Stripe
    using Newtonsoft.Json;
    using Stripe.Infrastructure;

-    public class Dispute : StripeEntityWithId, ISupportMetadata
+    public class Dispute : StripeEntityWithIdAndObject


I don't think Discount has an id, at least it's not in the docs.

👍 Fixed.

remi-stripe · 2018-10-03T13:38:41Z

src/Stripe.net/Entities/Recipients/RecipientActiveAccount.cs

@@ -3,7 +3,7 @@ namespace Stripe
    using System.Collections.Generic;
    using Newtonsoft.Json;

-    public class RecipientActiveAccount : StripeEntityWithId, ISupportMetadata
+    public class RecipientActiveAccount : StripeEntityWithId


Why not StripeEntityWithIdAndObject

👍 Fixed.

remi-stripe · 2018-10-03T13:40:41Z

src/Stripe.net/Entities/_contracts/ISupportMetadata.cs

@@ -1,12 +0,0 @@
-namespace Stripe


Wasn't this used to ensure we added Metadata to the Resource too?

I'm not sure why resource classes used this interface at all, but I don't think this is a good reason: the interface would only force you add the Metadata property if you remembered to add the interface...

remi-stripe · 2018-10-03T13:41:04Z

src/Stripe.net/Infrastructure/StringOrObject.cs

@@ -22,7 +22,7 @@ public static void Map(object value, Action<string> updateId, Action<T> updateOb
            else if (value is string)
            {
                updateId((string)value);
-                updateObject(null);
+                updateObject(default(T));


Can you put a real comment on this line?

ob-stripe · 2018-10-03T14:24:12Z

Would be neat but then StripeEntityWithIdAndObjectAndDeletedAndMetdata is not really cool. Maybe metadata should be the default for StripeEntityWithId and you'd have to explicitly use StripeEntityWithIdAndNoMetadata for the ones that explicitly don't support it? But it's easy to miss if so.

Yeah, tbh I'm already not thrilled about all the StripeEntityWith... superclasses. I'd rather we always derive from StripeEntity and add the interfaces implemented by each class.

I'd also rather avoid adding more interfaces / classes for other common fields (like metadata and deleted) unless we actually need them in the infrastructure part of the code.

remi-stripe · 2018-10-03T14:44:22Z

Yeah, tbh I'm already not thrilled about all the StripeEntityWith... superclasses. I'd rather we always derive from StripeEntity and add the interfaces implemented by each class.

Should we do StripeEntity and StripeEntityWithId and revert the rest? Do we need an interface for object/metadata/deleted? Or should we just rely on us catching it during PRs? All tests access Object already anyway to verify deserialization so it would be hard to miss

ob-stripe · 2018-10-03T20:13:10Z

Should we do StripeEntity and StripeEntityWithId and revert the rest?

To be clear, I was suggesting going one step further and get rid of StripeEntityWithId as well. All model classes would derive from StripeEntity and those that have an ID would implement IHasId and explicitly declare the Id property.

Do we need an interface for object/metadata/deleted?

Right now we have no need for the latter two. Having an IHasObject will help me with a clean implementation of #1313: a generic converter that needs to look at the object property to choose which class to instantiate could use such an interface.

remi-stripe · 2018-10-03T20:43:15Z

I do like the distinction between StripeEntity and StripeEntityWithId. I think it's worth having it because it makes it clear what should have an Id so I guess we're mostly back to the old state right?

ob-stripe · 2018-10-03T20:55:38Z

I'd argue that if the goal is to make it clear what resources should have an ID, then using the IHasId interface directly would be a better fit since telling you what should go into a class is exactly what interfaces are for :)

remi-stripe · 2018-10-03T21:02:16Z

But IHasId requires adding the Id property everywhere and it's just repetitive. But yeah sorry I'm running out of opinion on that one to be honest :(
I don't really feel strongly so I trust you to do the right choice (easy to say I know!)

ob-stripe · 2018-10-04T13:31:00Z

When it comes to model classes, I think explicitness is more valuable than DRY. Having each model class directly define all of its properties makes it easier to compare the model to a reference (e.g. the API ref or the OpenAPI spec). I also think it's cleaner to have both IHasId and IHasObject interfaces (and future IHasFoo interfaces, if we ever need them) be added to model classes in the same way.

ptal @remi-stripe

remi-stripe

not sure we use IHasId properly

remi-stripe · 2018-10-04T14:10:15Z

src/Stripe.net/Entities/Events/EventRequest.cs

@@ -2,8 +2,11 @@ namespace Stripe
 {
    using Newtonsoft.Json;

-    public class EventRequest : StripeEntityWithId
+    public class EventRequest : StripeEntity, IHasId


I don't think IHasId makes sense here. It's not a Stripe resource, it has the field id because it's the name of the field for the request. I don't think it makes sense to use IHasId in that case

👍 Agreed.

remi-stripe · 2018-10-04T14:10:32Z

src/Stripe.net/Entities/EphemeralKeys/EphemeralkeyAssociatedObject.cs

@@ -2,8 +2,11 @@ namespace Stripe
 {
    using Newtonsoft.Json;

-    public class EphemeralKeyAssociatedObject : StripeEntityWithId
+    public class EphemeralKeyAssociatedObject : StripeEntity, IHasId


I don't think IHasId makes sense here. It's not a Stripe resource, it has the field id because it's the name of the field for the associated object. I don't think it makes sense to use IHasId in that case

👍 Agreed.

remi-stripe · 2018-10-04T14:13:17Z

src/Stripe.net/Entities/Common/PaymentSource.cs

    {
+        [JsonProperty("id")]
+        public string Id { get; set; }


OOC does this really have an Id field? It's a fake union right? Is it because of the expansion logic where we set the field Id?

I'm planning on removing all these union classes in #1313, so this is kind of moot.

no you're moot

remi-stripe · 2018-10-04T14:15:24Z

src/Stripe.net/Entities/ExternalAccounts/ExternalAccount.cs

    {
+        [JsonProperty("id")]
+        public string Id { get; set; }
+


Same as PaymentSource

Same, this will go away in #1313.

remi-stripe · 2018-10-04T14:16:19Z

src/Stripe.net/Entities/Orders/ShippingMethod.cs

    {
+        [JsonProperty("id")]
+        public string Id { get; set; }


Same as the other ones

👍 Agreed.

ob-stripe · 2018-10-04T14:47:27Z

ptal @remi-stripe

ob-stripe force-pushed the ob-interfaces-interfaces-everywhere branch from b09bd74 to 04415cc Compare October 3, 2018 11:35

ob-stripe assigned remi-stripe Oct 3, 2018

ob-stripe changed the title ~~[wip] More interfaces on Stripe model classes~~ Revamp interfaces on Stripe model classes Oct 3, 2018

remi-stripe reviewed Oct 3, 2018

View reviewed changes

remi-stripe assigned ob-stripe and unassigned remi-stripe Oct 3, 2018

ob-stripe force-pushed the ob-interfaces-interfaces-everywhere branch from 8f016e4 to 467b140 Compare October 3, 2018 14:21

remi-stripe reviewed Oct 4, 2018

View reviewed changes

ob-stripe force-pushed the ob-interfaces-interfaces-everywhere branch from ccdf2f2 to 3762630 Compare October 4, 2018 14:46

remi-stripe approved these changes Oct 4, 2018

View reviewed changes

remi-stripe added the approved label Oct 4, 2018

ob-stripe added the breaking-api-change label Oct 4, 2018

Revamp interfaces on Stripe model classes

6a5df72

ob-stripe force-pushed the ob-interfaces-interfaces-everywhere branch from 317bb20 to 6a5df72 Compare October 4, 2018 15:09

ob-stripe merged commit e191637 into integration-next-major-version Oct 4, 2018

ob-stripe deleted the ob-interfaces-interfaces-everywhere branch October 4, 2018 15:29

ob-stripe mentioned this pull request Oct 4, 2018

stripe-dotnet v20.0 #1293

Merged

32 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp interfaces on Stripe model classes #1317

Revamp interfaces on Stripe model classes #1317

ob-stripe commented Oct 2, 2018 •

edited

Loading

ob-stripe commented Oct 2, 2018

remi-stripe left a comment

remi-stripe Oct 3, 2018

ob-stripe Oct 3, 2018

remi-stripe Oct 3, 2018

ob-stripe Oct 3, 2018

remi-stripe Oct 3, 2018

ob-stripe Oct 3, 2018

remi-stripe Oct 3, 2018

ob-stripe Oct 3, 2018

ob-stripe commented Oct 3, 2018

remi-stripe commented Oct 3, 2018

ob-stripe commented Oct 3, 2018

remi-stripe commented Oct 3, 2018

ob-stripe commented Oct 3, 2018

remi-stripe commented Oct 3, 2018

ob-stripe commented Oct 4, 2018

remi-stripe left a comment

remi-stripe Oct 4, 2018

ob-stripe Oct 4, 2018

remi-stripe Oct 4, 2018

ob-stripe Oct 4, 2018

remi-stripe Oct 4, 2018

ob-stripe Oct 4, 2018

remi-stripe Oct 4, 2018

remi-stripe Oct 4, 2018

ob-stripe Oct 4, 2018

remi-stripe Oct 4, 2018

ob-stripe Oct 4, 2018

ob-stripe commented Oct 4, 2018

Revamp interfaces on Stripe model classes #1317

Revamp interfaces on Stripe model classes #1317

Conversation

ob-stripe commented Oct 2, 2018 • edited Loading

ob-stripe commented Oct 2, 2018

remi-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ob-stripe commented Oct 3, 2018

remi-stripe commented Oct 3, 2018

ob-stripe commented Oct 3, 2018

remi-stripe commented Oct 3, 2018

ob-stripe commented Oct 3, 2018

remi-stripe commented Oct 3, 2018

ob-stripe commented Oct 4, 2018

remi-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ob-stripe commented Oct 4, 2018

ob-stripe commented Oct 2, 2018 •

edited

Loading