Support for passing arbitrary parameters #1057

ob-stripe · 2017-11-16T18:13:41Z

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

This PR adds support for passing arbitrary parameters when making API requests. If the Options class inherit from StripeParamsBase, users can call AddExtraParam to add arbitrary key/value pairs to the request.

remi-stripe

added a few comments

remi-stripe · 2017-11-16T18:16:03Z

src/Stripe.net/Infrastructure/ParameterBuilder.cs

+                    foreach (KeyValuePair<string, string> pair in (Dictionary<string, string>) field.GetValue(obj))
+                    {
+                        var key = WebUtility.UrlEncode(pair.Key);
+                        RequestStringBuilder.ApplyParameterToRequestString(ref requestString, key, pair.Value);


Does this mean that you could pass extra_params[sub_params][subsub_value] and it would work?

The brackets would be URL-encoded. I think the API doesn't mind and would still interpret the value as a nested object, but not entirely sure tbh. Maybe URL-encoding the key is not the right thing to do here, and instead we'd make it the caller's responsibility to pass a properly URL-encoded key?

hmmm that seems frustrating for users to have to do this (url encoding themselves). Maybe not supporting sub-hashes for now is fine then. Also we could do a plugin that says "oh this is a dictionary, let's properly encode those"

I'd rather keep the feature simple for now. If someone actually needs to use this method with nested objects we can think about it then.

remi-stripe · 2017-11-16T18:16:35Z

src/Stripe.net/Services/StripeParamsBase.cs

+
+namespace Stripe
+{
+    public class StripeParamsBase


I like the new name but should it be StripeBaseOptions to match the rest of the class naming?

Yeah, that's probably a better name.

remi-stripe · 2017-11-16T18:17:20Z

src/Stripe.net/Infrastructure/ParameterBuilder.cs

@@ -33,6 +33,16 @@ public static string ApplyAllParameters(this StripeService service, object obj,
                            RequestStringBuilder.ProcessPlugins(ref requestString, attribute, property, value, obj);
                    }
                }
+
+                var field = obj.GetType().GetRuntimeField("ExtraParams");
+                if (field != null)


This was added temporarily right? Should we assert if you option doesn't have that value so that you know you forgot to inherit from StripeParamsBase?

Unless that breaks the reflection logic beyond saving, I think a better solution would be to change the ApplyAllParameters signature to receive a StripeParamsBase (or whatever name we choose for the parent class) instead of an object.

But to answer your question, yes, all params classes should inherit from StripeParamsBase and so there's no reason why ExtraParams would not be available.

So what I meant is that if we remove the if, then we can catch cases where someone creates a new Options class but forgets to inherit from the Base class.

@ob-stripe Now that all class derive from StripeBaseOptions should we remove the check? Or assert if field == null as it should not happen?

Yeah, an assert seems reasonable 👍

remi-stripe · 2017-11-16T22:53:14Z

@ob-stripe flagging that I just pushed this:

Move all top-level Options classes to StripeBaseOptions

Ensure that RequestOptions does not inherit from it as it's used in a different way in the library (extra headers)
Same for INestedOptions as it's specific to sub-resources and should not have extra parameters.
Ensure StripeBaseOptions does not reference itself!

ob-stripe · 2017-11-21T15:44:25Z

src/Stripe.net/Infrastructure/ParameterBuilder.cs

@@ -10,7 +10,7 @@ namespace Stripe.Infrastructure
 {
    internal static class ParameterBuilder
    {
-        public static string ApplyAllParameters(this StripeService service, object obj, string url, bool isListMethod = false)
+        public static string ApplyAllParameters(this StripeService service, StripeBaseOptions obj, string url, bool isListMethod = false)


So I actually went with what I mentioned before: the method now expects a StripeBaseOptions instance instead of just an object. That way if we forget to derive from StripeBaseOptions the error will be immediately visible at compile time rather than at runtime. (I actually caught a few classes that you'd forgotten this way ;)

Plus, there's no need to muck with reflection to access ExtraParams now, so the code is a bit cleaner.

ob-stripe · 2017-11-21T15:45:23Z

ptal @remi-stripe

remi-stripe · 2017-11-21T16:04:41Z

src/Stripe.net/Services/StripeBaseOptions.cs

+            ExtraParams.Add(key, value);
+        }
+
+        public Dictionary<string, string> ExtraParams = new Dictionary<string, string>();


@ob-stripe did we want to figure out how to make that one not public before merging?

remi-stripe

LGTM

remi-stripe reviewed Nov 16, 2017

View reviewed changes

remi-stripe force-pushed the ob-add-extra branch from 567fce1 to 808c09a Compare November 16, 2017 22:54

ob-stripe force-pushed the ob-add-extra branch from 44c24f5 to 44169ad Compare November 21, 2017 15:42

ob-stripe commented Nov 21, 2017

View reviewed changes

Support for passing arbitrary parameters

ba16843

ob-stripe force-pushed the ob-add-extra branch from 44169ad to ba16843 Compare November 21, 2017 15:51

remi-stripe reviewed Nov 21, 2017

View reviewed changes

remi-stripe approved these changes Nov 21, 2017

View reviewed changes

ob-stripe merged commit 20ce1d1 into master Nov 21, 2017

ob-stripe deleted the ob-add-extra branch November 21, 2017 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for passing arbitrary parameters #1057

Support for passing arbitrary parameters #1057

ob-stripe commented Nov 16, 2017

remi-stripe left a comment

remi-stripe Nov 16, 2017

ob-stripe Nov 16, 2017

remi-stripe Nov 16, 2017

ob-stripe Nov 21, 2017

remi-stripe Nov 16, 2017

ob-stripe Nov 16, 2017

remi-stripe Nov 16, 2017

ob-stripe Nov 16, 2017

remi-stripe Nov 16, 2017

remi-stripe Nov 18, 2017

ob-stripe Nov 21, 2017

remi-stripe commented Nov 16, 2017 •

edited

Loading

ob-stripe Nov 21, 2017

ob-stripe commented Nov 21, 2017

remi-stripe Nov 21, 2017

remi-stripe left a comment

Support for passing arbitrary parameters #1057

Support for passing arbitrary parameters #1057

Conversation

ob-stripe commented Nov 16, 2017

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

remi-stripe commented Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

ob-stripe commented Nov 21, 2017

Choose a reason for hiding this comment

remi-stripe left a comment

Choose a reason for hiding this comment

remi-stripe commented Nov 16, 2017 •

edited

Loading