[WIP] [experimental] Interfaces for polymorphic API resources #1313

ob-stripe · 2018-10-01T22:05:28Z

An experiment to see if we could replace classes like ExternalAccount and PaymentSource (which don't really exist in the API) by interfaces.

Make internals visible to Newtonsoft.Json

Cross target netstandard2.0

This ensures that params/class structure matches the API. Also changed DateFilter to match the API and remove custom encoding and renamed it to DateRangeOptions

Move all services to BasicService

Remove +x flag from source files

Also made a few changes to some resources to match the API naming and added support for AddressJapan on the Account resource.

Move tests to their own folder

remi-stripe

So this looks really neat in concept. I really like it. I am just a little unsure about removing the Type property on the ExternalAccount class. I know why you did it but is using GetType really common enough that people would prefer that over an explicit type check?

EDIT: I realize that my sentence is confusing. GetType is an explicit type check. I meant is it more common than doing if externalAccount.Type == "bank_account" ... or whatever?

ob-stripe · 2018-10-02T09:26:02Z

I think calling GetType is the standard way of checking the concrete type of an "interface instance".

I also just discovered the dynamic keyword which can be used instead of var to tell the compiler that the actual type is not known until runtime, and prevents the need for casting entirely. I've updated the tests to use it.

remi-stripe

Added a few more comments, mostly for my own education

remi-stripe · 2018-10-02T12:31:11Z

src/Stripe.net/Infrastructure/JsonConverters/ExternalAccountConverter.cs

-            {
-                Id = incoming.SelectToken("id").ToString()
-            };
+            var externalAccount = default(IExternalAccount);


didn't you say you did not need to use var?

To clarify: dynamic can (but it's not a requirement) be used to tell the compiler that the exact type won't be known until runtime. It's not a silver bullet, because it disables compile-type checks. E.g. if I write this:

dynamic foo = account.ExternalAccounts.Data[0]; Console.WriteLine(foo.AccountHolderName)

The compiler has no way of knowing whether AccountHolderName is a valid call. If it's not (because foo is a Card instead of a BankAccount), a runtime exception will be raised.

A safer but more verbose alternative:

var foo = account.ExternalAccounts.Data[0]; // foo is an `IExternalAccount` if (foo is BankAccount) { Console.WriteLine(((BankAccount)foo).AccountHolderName); // need to manually cast to BankAccount } else if (foo is Card) { Console.WriteLine(((Card)foo).Name); // need to manually cast to Card } else { // this should never be reached, unless a new concrete type is introduced for IExternalAccount }

In the specific place of the code, we can use var because we don't need to access any of specific properties of the concrete types. We could replace var with IExternalAccount an it would be a no-op (it's the type that is inferred by the compiler).

Ack thanks for the thorough explanation. So the logic becomes similar to what we do in stripe-java. More verbose but safer and cleaner.

Sorry for the lurk, but +1 the choice to avoid dynamic if possible. It's one of these tools that can be useful in a huge variety of situations, but probably should be avoided in almost all of them because of loss of typedness is a very significant drawback (no more compiler checks, Intellisense, etc.).

remi-stripe · 2018-10-02T12:32:34Z

src/Stripe.net/Infrastructure/Public/Mapper.cs


    public static class Mapper<T>
    {
+        private static JsonConverter[] converters = {
+            new ExternalAccountConverter(),


so this is the new place where we would list all converters?

Yep. Not sure if this is the best way to achieve this, but it seems to work and should be relatively efficient (it's static, so only initialized once).

remi-stripe · 2018-10-02T12:33:06Z

src/Stripe.net/Infrastructure/StringOrObject.cs

                updateId(item.Id);
                updateObject(item);
            }
            else if (value is string)
            {
                updateId((string)value);
-                updateObject(null);
+                updateObject(default(T));


can you clarify what this line does/changes?

In practice, nothing. default(T) returns the default value for the type T. For reference types, the default value is null.

However, since T is now constrained to be an interface rather than a class, the compiler complains that it cannot convert from null to T. This is (I think) because structs are value types but can implement interfaces.

remi-stripe · 2018-10-02T12:33:20Z

src/Stripe.net/Infrastructure/StringOrObject.cs

    {
        public static void Map(object value, Action<string> updateId, Action<T> updateObject)
        {
            if (value is JObject)
            {
-                T item = ((JToken)value).ToObject<T>();
+                // T item = ((JToken)value).ToObject<T>();
+                T item = Mapper<T>.MapFromJson(value.ToString());


Not fully sure what all of this does

It uses our custom deserialization instead of the default one, to ensure that our registered converters are used.

remi-stripe · 2018-10-02T12:34:55Z

src/StripeTests/Entities/ExternalAccounts/ExternalAccountTest.cs

-            Assert.Equal("bank_account", externalAccount.BankAccount.Object);
+            Assert.IsType<BankAccount>(externalAccount);
+            var bankAccount = (BankAccount)externalAccount;
+            Assert.Equal("bank_account", bankAccount.Object);


Same you mentioned you did not need to use var anymore

I forgot to update this test. But as said above, using dynamic is optional, it's basically syntactic sugar at the cost of type safety.

Thinking about this a bit more, I think we could make Object part of the IStripeEntity interface (or maybe on a new IStripeEntityWithObject interface) so that we can check its value without using dynamic.

remi-stripe · 2018-10-02T12:35:17Z

src/StripeTests/StripeTests.csproj

@@ -20,6 +20,7 @@
  </ItemGroup>

  <ItemGroup Condition=" '$(TargetFramework)' == 'net452' ">
+    <Reference Include="Microsoft.CSharp" />


What is this exactly?

This is needed to use dynamic with .NET Framework (at least for version 4.5.2). Note that this is only added in the tests project. Users of the library who use .NET Framework and want to use dynamic would need to do the same thing, but the library doesn't make this a requirement.

Add support for InvoiceItem separate from InvoiceLineItem

Automatically convert Unix timestamps to DateTimes

ob-stripe · 2018-10-04T16:13:40Z

Closing this one because rebasing is a pain.

iamcarbon and others added 30 commits September 25, 2018 07:49

Cross target netstandard2.0

2981b78

Merge pull request #1300 from stripe/ob-perms-json

c7e4c09

Make internals visible to Newtonsoft.Json

Bump version to 19.9.1

b63a2b5

Merge pull request #1297 from carbon/master

62b3656

Cross target netstandard2.0

Bump version to 19.9.2

7ba25c5

Fix the Source/PaymentSource abstraction

0e61b70

Rewrite parameter encoding logic

a51e8d9

Rename event to match type name and add a related one.

edbfba5

Handle DateTimes directly in the encoder

f634371

Remove StripeDeleted entirely.

6ee8447

Rename two properties that start with a lower case letter

6aa9871

Rename all resources and classes to remove the Stripe prefix

7d6b76d

Fix services

09e63eb

Service interfaces

d5b034d

Ensure that all value types in options classes are nullable

2a4bcd3

Upgrade to API version 2018-09-24

887f93f

Move all nested options to their own class

7b38925

This ensures that params/class structure matches the API. Also changed DateFilter to match the API and remove custom encoding and renamed it to DateRangeOptions

Rename LiveMode to Livemode

0b8c6f1

Sanitize files (remove UTF-8 BOMs, add missing final newlines)

36a715e

Move all services to BasicService

d450f4c

Merge pull request #1305 from stripe/remi-move-all-services

542842c

Move all services to BasicService

Remove +x flag from source files

5b70ab5

Merge pull request #1306 from stripe/ob-remove-exec-flag

8015b56

Remove +x flag from source files

Sort service methods alphabetically

5e12016

Mock for verifying requests

d03fb59

Better URL handling in services

dfff930

Group resources with sub-resources under one folder

1869975

Also made a few changes to some resources to match the API naming and added support for AddressJapan on the Account resource.

Add missing fields, remove old ones, rename others

977cef3

Move all Entities tests to a sub-folder and add missing Dispute test.

a05aa13

Merge pull request #1312 from stripe/remi-fix-tests-folders

3b71dec

Move tests to their own folder

ob-stripe changed the title ~~[WIP] [experimental] Replace~~ [WIP] [experimental] Replace intermediary classes by interfaces Oct 1, 2018

remi-stripe reviewed Oct 1, 2018

View reviewed changes

ob-stripe force-pushed the ob-polymorphic-resource-interfaces branch from 48462ff to e172fd7 Compare October 2, 2018 09:23

ob-stripe force-pushed the ob-polymorphic-resource-interfaces branch from e172fd7 to 1e48083 Compare October 2, 2018 09:27

ob-stripe changed the title ~~[WIP] [experimental] Replace intermediary classes by interfaces~~ [WIP] [experimental] Interfaces for polymorphic API resources Oct 2, 2018

remi-stripe reviewed Oct 2, 2018

View reviewed changes

remi-stripe and others added 5 commits October 2, 2018 09:38

Split InvoiceLineItem in a separate resource InvoiceItem

99c7d0b

Merge pull request #1315 from stripe/remi-fix-invoiceitem

e7b3dfd

Add support for InvoiceItem separate from InvoiceLineItem

Automatically convert Unix timestamps to DateTimes

54dee88

Merge pull request #1316 from stripe/ob-auto-convert-datetime

971915b

Automatically convert Unix timestamps to DateTimes

Replace ExternalAccount object by IExternalAccount interface

9eaf9b8

ob-stripe force-pushed the ob-polymorphic-resource-interfaces branch from 1e48083 to 9eaf9b8 Compare October 2, 2018 14:46

More generic converter

6f20f46

ob-stripe mentioned this pull request Oct 3, 2018

Revamp interfaces on Stripe model classes #1317

Merged

ob-stripe force-pushed the integration-next-major-version branch from bd39334 to 79e5289 Compare October 4, 2018 15:32

ob-stripe closed this Oct 4, 2018

ob-stripe deleted the ob-polymorphic-resource-interfaces branch October 4, 2018 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] [experimental] Interfaces for polymorphic API resources #1313

[WIP] [experimental] Interfaces for polymorphic API resources #1313

ob-stripe commented Oct 1, 2018

remi-stripe left a comment •

edited

Loading

ob-stripe commented Oct 2, 2018

remi-stripe left a comment

remi-stripe Oct 2, 2018

ob-stripe Oct 2, 2018 •

edited

Loading

remi-stripe Oct 2, 2018

brandur-stripe Oct 2, 2018

remi-stripe Oct 2, 2018

ob-stripe Oct 2, 2018

remi-stripe Oct 2, 2018

ob-stripe Oct 2, 2018

remi-stripe Oct 2, 2018

ob-stripe Oct 2, 2018

remi-stripe Oct 2, 2018

ob-stripe Oct 2, 2018

ob-stripe Oct 2, 2018

remi-stripe Oct 2, 2018

ob-stripe Oct 2, 2018

ob-stripe commented Oct 4, 2018

[WIP] [experimental] Interfaces for polymorphic API resources #1313

[WIP] [experimental] Interfaces for polymorphic API resources #1313

Conversation

ob-stripe commented Oct 1, 2018

remi-stripe left a comment • edited Loading

Choose a reason for hiding this comment

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

ob-stripe Oct 2, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ob-stripe commented Oct 4, 2018

remi-stripe left a comment •

edited

Loading

ob-stripe Oct 2, 2018 •

edited

Loading