Better URL handling in services #1308

ob-stripe · 2018-09-29T16:23:05Z

Move URL creation in Service as much as possible, so that concrete services don't need to create their own URLs in every method.

Most services will only need to declare their ObjectName, then use the new parent methods for CRUD + list requests, or call this.ClassUrl() / this.InstanceUrl(id) as appropriate.

Some services will need to override ClassUrl if their object name doesn't cleanly map to the URL.

I'm not sure yet how to handle services for nested resources -- maybe another parent class ServiceNested with more helper methods, similar to these ones but with an additional argument for the parent resource's ID.

ptal @remi-stripe

remi-stripe

Excited about those changes (and glad I did not convert all Urls during the previous PR). Left some minor comments.

I am not sure what nested resources would look like either but agree that a NestedService looks like the right approach

remi-stripe · 2018-09-29T16:39:54Z

src/Stripe.net/Services/ApplePayDomains/ApplePayDomainService.cs

        }

        public virtual ApplePayDomain Get(string domainId, RequestOptions requestOptions = null)
        {
-            return this.GetEntity($"{Urls.BaseUrl}/apple_pay/domains/{domainId}", requestOptions);
+            return this.GetEntityNew(domainId, null, requestOptions);


Why do we need null on GET requests?

The GetEntityNew method is capable of adding parameters to retrieve requests, even though we don't do this in the library (mostly because the only case where this is needed is expansion, and this is handled separately via the Expand* properties on the service).

That said, I added an override to be able to call the method without an options class.

But why since expansion is on the other options and no GET (non list) method takes an option?

remi-stripe · 2018-09-29T16:40:16Z

src/Stripe.net/Services/ApplePayDomains/ApplePayDomainService.cs

        }

        public virtual ApplePayDomain Delete(string domainId, RequestOptions requestOptions = null)
        {
-            return this.DeleteEntity($"{Urls.BaseUrl}/apple_pay/domains/{domainId}", requestOptions);
+            return this.DeleteEntityNew(domainId, null, requestOptions);


I'm assuming there's no way to get rid of the null for most DELETE?

No, this is possible too. I added an override to be able to call DeleteEntityNew without an options class.

remi-stripe · 2018-09-29T16:42:09Z

src/Stripe.net/Services/ApplePayDomains/ApplePayDomainService.cs

+        {
+            baseUrl = baseUrl ?? StripeConfiguration.GetApiBase();
+
+            return $"{baseUrl}/apple_pay/domains";


Not a fan of that override. I would rather calculate the relative path instead. Can we do "ObjectPath" or something instead?

Yeah, that makes way more sense for a service actually. I changed ObjectName to BasePath.

remi-stripe · 2018-09-29T16:43:48Z

src/Stripe.net/Services/Issuing/Authorizations/AuthorizationService.cs

        }

        public virtual Task<Authorization> ApproveAsync(string authorizationId, AuthorizationApproveOptions options = null, RequestOptions requestOptions = null, CancellationToken cancellationToken = default(CancellationToken))
        {
-            return this.PostAsync($"{classUrl}/{authorizationId}/approve", requestOptions, cancellationToken, options);
+            return this.PostRequestAsync<Authorization>($"{this.InstanceUrl(authorizationId)}/approve", options, requestOptions, cancellationToken);


I wonder if it'd be cleaner to pass the authorization id + append name? Not sure what that would look like but I feel like it could be cleaner? Sorry it's a bit vague and likely not possible but I was thinking of this.Url(authorizationId, "/approve") or similar?

Hm, not sure it's worth it. this.InstanceUrl(authorizationId, "/approve") would be the same thing as this.InstanceUrl(authorizationId) + "/approve", which is already the same thing as $"{this.InstanceUrl(authorizationId)}/approve".

yeah fair, it's not the same approach in my head (and the former is easier to mess up). I ask because I made a mistake a few times yesterday with the /approve, /decline and all where stripe-mock still just worked because params were identical and I had messed up my URL calculation

remi-stripe · 2018-09-29T20:33:02Z

src/Stripe.net/Services/Products/ProductService.cs

        }

        public virtual Product Delete(string productId, RequestOptions requestOptions = null)
        {
-            return this.DeleteEntity($"{Urls.BaseUrl}/products/{WebUtility.UrlEncode(productId)}", requestOptions);
+            return this.DeleteEntityNew(productId, requestOptions);


Flagging that I had to remove the encoding for now. I think it makes more sense to do it at the next level. What do you think?

InstanceUrl already takes care of encoding the ID! :)

remi-stripe · 2018-09-29T20:37:32Z

@ob-stripe I did most of the services but left the nested ones for now. I think this one needs a thorough review because so much has changed and it's easy to mess up

remi-stripe · 2018-09-29T21:48:02Z

I'm not sure why it fails on appveyor and works locally :(

remi-stripe

Thanks for fixing the Issuing ones, I was sure I did them and forgot to double check

remi-stripe · 2018-09-29T22:17:20Z

src/Stripe.net/Services/Balance/BalanceService.cs

        }

        public virtual Task<Balance> GetAsync(RequestOptions requestOptions = null, CancellationToken cancellationToken = default(CancellationToken))
        {
-            return this.GetRequestAsync<Balance>($"{Urls.BaseUrl}/{this.BasePath}", null, requestOptions, cancellationToken);
+            return this.GetRequestAsync<Balance>(this.ClassUrl(), null, requestOptions, cancellationToken);


Can you explain why this works and mine did not?

Your path was /v1//balance, and stripe-mock doesn't like the double slash.

yeah I know but it worked locally without any issue which is weird

Hm, good point. The different path is probably being handled different in .NET Framework and .NET Core.

remi-stripe · 2018-09-29T22:18:32Z

src/Stripe.net/Services/Files/FileService.cs

        public virtual File Create(FileCreateOptions options, RequestOptions requestOptions = null)
        {
            return Mapper<File>.MapFromJson(
                Requestor.PostFile(
-                    $"{Urls.BaseFilesUrl}{classPath}",
+                    this.ClassUrl(Urls.BaseFilesUrl),


ah neat I totally misread the code when I looked for whether this was supported

remi-stripe

Some minor comments but I think it's ready otherwise

remi-stripe · 2018-09-30T14:05:43Z

src/Stripe.net/Services/Cards/CardService.cs

@@ -6,7 +6,7 @@ namespace Stripe
    using System.Threading.Tasks;
    using Stripe.Infrastructure;

-    public class CardService : Service<Card>,
+    public class CardService : ServiceNested<Card, Customer>,


Likely not a problem but flagging since I thought about it. What if we have a generic nested resource in the future (like card and bank account used to be) and we want to have it shared? Would we simply add a parent class/interface or similar instead?

ServiceNested doesn't actually use the parent resource class for anything, I should probably remove it.

That said, if we wanted to have a single service be able to support different parent URLs, it probably would have to manage the URLs itself instead of delegating it to the Service / ServiceNested parent class.

remi-stripe · 2018-09-30T14:10:37Z

src/Stripe.net/Services/ServiceNested.cs

+            return this.PostRequest<EntityReturned>(this.InstanceUrl(parentId, id), options, requestOptions);
+        }
+
+        protected Task<EntityReturned> UpdateNestedEntityAsync(string parentId, string id, BaseOptions options = null, RequestOptions requestOptions = null, CancellationToken cancellationToken = default(CancellationToken))


Minor but I wonder if we should avoid the default values on parameters like requestOptions and cancellationToken. Why you ask? Because it's easy to mess up your copy/paste (we both did in this PR) and have cancellationToken be the default value instead of what the upper level passed.

👍 Agreed, I also thought of this.

remi-stripe · 2018-09-30T14:13:08Z

src/Stripe.net/Services/TransferReversals/TransferReversalService.cs

        }

        public virtual TransferReversal Get(string transferId, string reversalId, RequestOptions requestOptions = null)
        {
-            return this.GetEntity($"{Urls.BaseUrl}/transfers/{transferId}/reversals/{reversalId}", requestOptions);
+            return this.GetNestedEntity(transferId, reversalId, requestOptions);


I see you used the version without null for options. but you did not use it everywhere. I know I kinda asked for it but per my previous comment I'm starting to think being explicit with what params we pass and why is likely safer/better after all (soooorry)

Haha, okay. I prefer the explicit versions as well.

remi-stripe · 2018-09-30T14:14:28Z

src/Stripe.net/Services/Discounts/DiscountService.cs

@@ -16,24 +16,26 @@ public DiscountService(string apiKey)
        {
        }

+        public override string BasePath => null;


Oh well I guess this is the example of a resource nested under different parents?

Yeah, exactly. It's not as pretty, but thankfully those cases should be pretty rare.

ob-stripe · 2018-09-30T16:42:33Z

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

remi-stripe · 2018-10-01T12:45:12Z

LGTM assuming the tests pass (since we had a few failures on the branch and not locally on this work)

ob-stripe force-pushed the ob-service-url branch 2 times, most recently from 9bf0b5d to 20cae29 Compare September 29, 2018 16:40

remi-stripe reviewed Sep 29, 2018

View reviewed changes

remi-stripe assigned ob-stripe Sep 29, 2018

remi-stripe reviewed Sep 29, 2018

View reviewed changes

ob-stripe added the breaking-api-change label Sep 29, 2018

ob-stripe force-pushed the ob-service-url branch from 1a563f3 to 3c8d34a Compare September 29, 2018 23:09

remi-stripe reviewed Sep 30, 2018

View reviewed changes

ob-stripe changed the title ~~[WIP] Better URL handling in services~~ Better URL handling in services Sep 30, 2018

ob-stripe assigned remi-stripe and unassigned ob-stripe Sep 30, 2018

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

ob-stripe force-pushed the ob-service-url branch from c082108 to 5df993a Compare October 1, 2018 12:06

ob-stripe mentioned this pull request Oct 1, 2018

stripe-dotnet v20.0 #1293

Merged

32 tasks

ob-stripe force-pushed the ob-service-url branch from 5df993a to fa31092 Compare October 1, 2018 12:39

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

remi-stripe added the approved label Oct 1, 2018

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

Better URL handling in services

2df527c

ob-stripe force-pushed the ob-service-url branch from fa31092 to 2df527c Compare October 1, 2018 12:54

ob-stripe merged commit af9e467 into integration-next-major-version Oct 1, 2018

ob-stripe deleted the ob-service-url branch October 1, 2018 13:11

ob-stripe mentioned this pull request Oct 24, 2018

Fix handling of Expand* properties in list requests #1355

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better URL handling in services #1308

Better URL handling in services #1308

ob-stripe commented Sep 29, 2018

remi-stripe left a comment

remi-stripe Sep 29, 2018

ob-stripe Sep 29, 2018

remi-stripe Sep 29, 2018

remi-stripe Sep 29, 2018

ob-stripe Sep 29, 2018

remi-stripe Sep 29, 2018

ob-stripe Sep 29, 2018

remi-stripe Sep 29, 2018 •

edited

Loading

ob-stripe Sep 29, 2018 •

edited

Loading

remi-stripe Sep 29, 2018

remi-stripe Sep 29, 2018

ob-stripe Sep 29, 2018

remi-stripe commented Sep 29, 2018

remi-stripe commented Sep 29, 2018

remi-stripe left a comment

remi-stripe Sep 29, 2018

ob-stripe Sep 29, 2018

remi-stripe Sep 30, 2018

ob-stripe Sep 30, 2018

remi-stripe Sep 29, 2018

remi-stripe left a comment

remi-stripe Sep 30, 2018

ob-stripe Sep 30, 2018

remi-stripe Sep 30, 2018

ob-stripe Sep 30, 2018

remi-stripe Sep 30, 2018

ob-stripe Sep 30, 2018

remi-stripe Sep 30, 2018

ob-stripe Sep 30, 2018

ob-stripe commented Sep 30, 2018

remi-stripe commented Oct 1, 2018

Better URL handling in services #1308

Better URL handling in services #1308

Conversation

ob-stripe commented Sep 29, 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

remi-stripe Sep 29, 2018 • edited Loading

Choose a reason for hiding this comment

ob-stripe Sep 29, 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

remi-stripe commented Sep 29, 2018

remi-stripe commented Sep 29, 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

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 Sep 30, 2018

remi-stripe commented Oct 1, 2018

remi-stripe Sep 29, 2018 •

edited

Loading

ob-stripe Sep 29, 2018 •

edited

Loading