Push parameter encoding into `BackendConfiguration.Call` #581

brandur · 2018-06-09T04:07:33Z

Now that we have a little more consistency throughout the library, here
we modify BackendConfiguration.Call (temporarily renamed to Call2
while we refactor) so that it can take parameter structs directly, then
encode them. This allows us to remove this common boilerplate from every
API call throughout the entire library:

if params != nil {
    commonParams = &params.Params
    body = &form.Values{}
    form.AppendTo(body, params)
}

Temporarily though, only the charge/ package has been converted over
to show what it looks like before we convert everything.

There is a little bit of type trickiness that allows this to happen, and
which requires a somewhat advanced understanding of some Go language
semantics. We define a ParamsContainer interface as this:

type ParamsContainer interface {
    GetParams() *Params
}

Then on the Params struct itself, we define an implementation:

func (p *Params) GetParams() *Params {
    return p
}

In Go, whenever a struct is embedded in another struct, the host
inherits the function implementations of the struct which was embedded.
Because every parameter struct in the library embeds Params, they all
get a GetParams implementation and thus implement ParamsContainer
automatically:

type ChargeParams struct {
    Params `form:"*"`
    ...
}

This allows us to take a ParamsContainer from Call.

Most of this applies similarly for ListParams, and similarly iter.go
also gets a Query2 and GetIter2 to keep everything compiling while
we refactor everything. I did a little more refactoring in iter.go as
well just because there was quite a few very bad names in there. If this
patch lands, I'll probably refactor even more of it in a follow up.

r? @remi-stripe I got the idea to do this from a chat we had the other day. I
think it'll tighten up the implementation even a little more and cut quite a
few LOCs.

cc @stripe/api-libraries

brandur-stripe · 2018-06-09T04:09:02Z

charge/client.go

 	charge := &stripe.Charge{}
-	err := c.B.Call("GET", stripe.FormatURLPath("/charges/%s", id), c.Key, body, commonParams, charge)
-
+	err := c.B.Call2("GET", path, c.Key, params, charge)
 	return charge, err


I wish this was all a little more readable in the diff, but this is essentially a demonstration of the golden sample: almost every new/get/update/other (non-list) call becomes four lines of code.

brandur-stripe · 2018-06-09T04:09:44Z

iter.go

@@ -9,6 +9,9 @@ import (
 // Query is the function used to get a page listing.
 type Query func(*form.Values) ([]interface{}, ListMeta, error)

+// Query2 is the function used to get a page listing.
+type Query2 func(*Params, *form.Values) ([]interface{}, ListMeta, error)


As with Call2 and GetIter2, Query2 becomes Query as soon as we finish refactoring everything. It'll never hit master.

brandur-stripe · 2018-06-09T04:10:10Z

iter.go

-	values []interface{}
+	cur        interface{}
+	err        error
+	formValues *form.Values


Naming like qs was terrible, so renamed a few of these to be more obvious.

brandur-stripe · 2018-06-09T04:11:15Z

stripe.go

@@ -72,6 +73,8 @@ func (a *AppInfo) formatUserAgent() string {
 // This interface exists to enable mocking for during testing if needed.
 type Backend interface {
 	Call(method, path, key string, body *form.Values, params *Params, v interface{}) error
+	Call2(method, path, key string, params ParamsContainer, v interface{}) error
+	CallRaw(method, path, key string, body *form.Values, params *Params, v interface{}) error


Call2 will become Call. CallRaw needs to stick around for now so that we can still implement the iterator closures in our list functions (we may be able to simplify those in the future too, but I haven't looked into it yet).

brandur-stripe · 2018-06-09T04:26:52Z

charge/client.go

 		list := &stripe.ChargeList{}
-		err := c.B.Call("GET", "/charges", c.Key, b, p, list)
+		err := c.B.CallRaw("GET", "/charges", c.Key, b, p, list)

 		ret := make([]interface{}, len(list.Data))
 		for i, v := range list.Data {


List functions are still shorter than before (lots of the frontmatter is removed), but they keep their closures for now, so they're not down to ~4 lines like some of the other API calls (not yet at least :).

remi-stripe

This change is amazing, and it relies on such a cool feature of Go. You've explained that one to me a few times but it had not sinked in until this change the the UnmarshallJSON one.

I like how much cleaner the calls are now and it will make it a lot easier to enforce that params is never nil lower in the stack which will require that all methods take a params (which some do not yet).

remi-stripe · 2018-06-09T16:35:39Z

charge/client.go

@@ -188,8 +145,7 @@ func CloseDispute(id string) (*stripe.Dispute, error) {

 func (c Client) CloseDispute(id string) (*stripe.Dispute, error) {
 	dispute := &stripe.Dispute{}
-	err := c.B.Call("POST", stripe.FormatURLPath("/charges/%s/dispute/close", id), c.Key, nil, nil, dispute)
-
+	err := c.B.Call2("POST", stripe.FormatURLPath("/charges/%s/dispute/close", id), c.Key, nil, dispute)


This is an example where we should fix CloseDispute() to take a real param instead of us passing a fake one internally. This change will force us to fix all of those.

Good call. Hm, we mighttt want to consider doing that change separately though given that I think we could push this one through without it being a breaking change.

remi-stripe · 2018-06-09T16:39:33Z

stripe.go

+	var body *form.Values
+	var commonParams *Params
+
+	if params != nil {


we likely could/should enforce that params never be nil in the first place right?

Yeah, we could go that direction too.

I was thinking though for calls like *Get and even CloseDispute above, it might be kind of nice to be able to pass a nil because while you sometimes care about a parameter and want to pass a structure, you often don't. What do you think?

That works for me!

Ah yes I see how passing nil is easier/better in that case. I meant was that the method itself should not pass nil but ultimately it boils down to the same thing so agreed after all

Excellent. I'll continue work on this after #578 is in!

brandur-stripe · 2018-06-11T17:54:18Z

This change is amazing, and it relies on such a cool feature of Go. You've explained that one to me a few times but it had not sinked in until this change the the UnmarshallJSON one.

I like how much cleaner the calls are now and it will make it a lot easier to enforce that params is never nil lower in the stack which will require that all methods take a params (which some do not yet).

Thanks! Glad you like it :) It was my proudest moment as a semi-literate Go developer when I realized that I kind of understood all the ramifications around struct embedding, haha.

brandur-stripe · 2018-06-13T05:23:21Z

account.go

@@ -444,5 +444,6 @@ type PayoutSchedule struct {

 // AccountRejectParams is the structure for the Reject function.
 type AccountRejectParams struct {
+	Params `form:"*"`


Added this so that it's compliant with the new ParamsContainer. It should've been there anyway.

brandur-stripe · 2018-06-13T05:23:50Z

account/client.go

-	// Type is now required on creation and not allowed on update
-	// It can't be passed if you pass `from_recipient` though
-	if params.FromRecipient != nil {
-		body.Add("type", stripe.StringValue(params.Type))


Given that type already had a form tag, I don't think this was doing anything useful (just duplicating work), so I just removed it.

confirmed that it did not do what I hoped it did at the time so removing was the right call.

brandur-stripe · 2018-06-13T05:25:42Z

bankaccount/client.go

-		err = c.B.Call("POST", stripe.FormatURLPath("/accounts/%s/external_accounts", stripe.StringValue(params.Account)), c.Key, body, &params.Params, ba)
-	}
-
+	err := c.B.CallRaw("POST", path, c.Key, body, &params.Params, ba)


Unfortunately there were a couple of functions in bank account and card where I had to keep a "raw" call like this one — the reason is that we're using the custom parameter encoding functions like AppendToAsSourceOrExternalAccount above.

There might be some way to get these consistent by exploring some other refactoring, but it'd require some more investigation.

ack that sounds fine for now though it's likely worth trying to revisit in the future to make it easier to reason with. It's easy to miss that there are some edge-cases when you build a new module or revamp this one.

It might be worth a comment in the code though wdyt?

ack that sounds fine for now though it's likely worth trying to revisit in the future to make it easier to reason with. It's easy to miss that there are some edge-cases when you build a new module or revamp this one.

Yeah, totally agree. I think with a little more effort we could get this reconciled into the standard framework.

It might be worth a comment in the code though wdyt?

Yep, good call. I'll add that here and to the implementation in card as well (which uses a similar pattern).

brandur-stripe · 2018-06-13T05:26:38Z

bankaccount/client.go

+			stripe.StringValue(listParams.Account))
+	} else {
+		outerErr = errors.New("Invalid bank account params: either Customer or Account need to be set")
+	}


Wherever possible, I hoisted path building outside of the iterator closure. This will be a little faster, but it'll also make all of these outliers consistent with path and outerErr variables.

sounds good. The more I look at this code logic, the more I wish this was also in a method to calculate the right path once.

brandur-stripe · 2018-06-13T05:30:27Z

@remi-stripe Sorry for the big patch, but I ended up pushing this one the rest of the way through. Would you mind taking a review pass?

If you like, you can look at each step in a separate commit:

7c389d7 converted the rest of the API calls over to the new technique.
7130a47 renamed Call2 back to Call, GetIter2 to GetIter, etc.

Thanks!

remi-stripe

The change looks great and the code looks so much cleaner.

I left some comments around style(s) and approach in general as we don't seem to be consistent everywhere and it might be worth trying, but really the decision is on you.

My only "worry" is how do we make sure that any method with a custom AppendToXXXX knows to call the CallRaw approach?

remi-stripe · 2018-06-13T14:17:47Z

account/client.go

-	// Type is now required on creation and not allowed on update
-	// It can't be passed if you pass `from_recipient` though
-	if params.FromRecipient != nil {
-		body.Add("type", stripe.StringValue(params.Type))


confirmed that it did not do what I hoped it did at the time so removing was the right call.

remi-stripe · 2018-06-13T14:26:18Z

account/client.go

 	account := &stripe.Account{}
-	err := c.B.Call("GET", stripe.FormatURLPath("/accounts/%s", id), c.Key, body, commonParams, account)
-
+	err := c.B.Call("GET", path, c.Key, params, account)


random but is there a reason Call can't get to c.Key on its own?

Yeah. Every client package (e.g., charge/, sub/, etc.) defines its own Client which is where a Key is configured:

type Client struct { B stripe.Backend Key string }

Client has a reference to a Backend, but not vice versa, so that backend can't reach back into the client to get at the Key field. That's why it's injected with every call.

I don't think I would have designed it this way, and there might be something we can do to refactor to remove even more boilerplate, but I don't want to mess around with this too much for now.

remi-stripe · 2018-06-13T14:37:59Z

bankaccount/client.go

+	var path string
+	if params.Customer != nil {
+		path = stripe.FormatURLPath("/customers/%s/sources", stripe.StringValue(params.Customer))
+	} else {


food for thought: we might want to not default to account and explicitly test if params.Account is set too and if not error out? We do in the function below

Yeah. I'd basically copied this over from the old implementation, but agreed that it's better and more consistent so I'll change it.

remi-stripe · 2018-06-13T14:38:56Z

bankaccount/client.go

-		err = c.B.Call("POST", stripe.FormatURLPath("/accounts/%s/external_accounts", stripe.StringValue(params.Account)), c.Key, body, &params.Params, ba)
-	}
-
+	err := c.B.CallRaw("POST", path, c.Key, body, &params.Params, ba)


ack that sounds fine for now though it's likely worth trying to revisit in the future to make it easier to reason with. It's easy to miss that there are some edge-cases when you build a new module or revamp this one.

remi-stripe · 2018-06-13T14:46:36Z

bankaccount/client.go

-		err = c.B.Call("POST", stripe.FormatURLPath("/accounts/%s/external_accounts", stripe.StringValue(params.Account)), c.Key, body, &params.Params, ba)
-	}
-
+	err := c.B.CallRaw("POST", path, c.Key, body, &params.Params, ba)


It might be worth a comment in the code though wdyt?

remi-stripe · 2018-06-13T15:03:28Z

bankaccount/client.go

-			err = errors.New("Invalid bank account params: either Customer or Account need to be set")
+
+		if outerErr != nil {
+			return nil, list.ListMeta, outerErr


Why is the variable called listMeta specifically?

Looks like you answered your question below, but yeah it could probably stand to have a slightly better name.

remi-stripe · 2018-06-13T17:23:51Z

card/client.go

 	}

+	card := &stripe.Card{}
+	err := c.B.Call("POST", path, c.Key, params, card)


just confirming we don't need the custom AppendTo + CallRaw here?

Yeah — it's probably best to refer to the implementation before my patch to fully convince yourself, but basically I just took whatever used to be there and carried it over. It was only in the New functions of bank account and card where the custom append functions were being used.

remi-stripe · 2018-06-13T18:00:59Z

iter.go

+	err        error
+	formValues *form.Values
+	listParams ListParams
+	meta       ListMeta


okay I now remember/understand what meta is though it still feels like a weird name, especially as we support metadata in the API. Not a big deal though.

remi-stripe · 2018-06-13T18:04:51Z

order/client.go

-		body = &form.Values{}
-		commonParams = &params.Params
-		form.AppendTo(body, params)
+	if params != nil && params.Source == nil && params.Customer == nil {


I wonder if we should remove this and just let the API fail instead?

Yeah, I kept it because it's already there, but I'm totally peaceful with letting this be an API failure. Removed.

remi-stripe · 2018-06-13T18:08:45Z

paymentsource/client.go

-		commonParams = &params.Params
-		body = &form.Values{}
-		form.AppendTo(body, params)
+	if params.Customer == nil {


Flagging that you started doing this more the "if nil => error and then otherwise assume it works" but not in all methods. You're also not consistently checking if params == nil

Yeah. To be fair though, I just copied over what was already there in all cases, but yes, we should be consistent about this.

I've added checks on params being nil to paymentsource/ as well now.

brandur-stripe · 2018-06-14T00:16:18Z

My only "worry" is how do we make sure that any method with a custom AppendToXXXX knows to call the CallRaw approach?

So I think it'll be relatively obvious in most cases because the new Call won't take a form.Values (only a ParamContainer). So you could instantiate one, do your custom append, but it'd be pretty obvious that you weren't doing anything with it. Also, these are currently only needed in a few corner cases, so I'm very much hoping that we never again introduce a new API call that follows this sort of model.

But anyway, thanks as usual for the fast review! I've addressed all feedback. Let me know if you happen to see anything else, but otherwise I'll bring this in tomorrow.

remi-stripe

LGTM thanks for the quick fixes and all the explanations

Now that we have a little more consistency throughout the library, here we modify `BackendConfiguration.Call` (temporarily renamed to `Call2` while we refactor) so that it can take parameter structs directly, then encode them. This allows us to remove this common boilerplate from every API call throughout the entire library: ``` go if params != nil { commonParams = &params.Params body = &form.Values{} form.AppendTo(body, params) } ``` Temporarily though, only the `charge/` package has been converted over to show what it looks like before we convert everything. There is a little bit of type trickiness that allows this to happen, and which requires a somewhat advanced understanding of some Go language semantics. We define a `ParamsContainer` interface as this: ``` go type ParamsContainer interface { GetParams() *Params } ``` Then on the `Params` struct itself, we define an implementation: ``` go func (p *Params) GetParams() *Params { return p } ``` In Go, whenever a struct is embedded in another struct, the host inherits the function implementation of the struct which was embedded. Because every parameter struct in the library embeds `Params`, they all get a `GetParams` implementation and thus implement `ParamsContainer` automatically: ``` go type ChargeParams struct { Params `form:"*"` ... } ``` This allows us to take a `ParamsContainer` from `Call`. Most of this applies similarly for `ListParams`, and similarly `iter.go` also gets a `Query2` and `GetIter2` to keep everything compiling while we refactor everything. I did a little more refactoring in `iter.go` as well just because there was quite a few very bad names in there. If this patch lands, I'll probably refactor even more of it in a follow up.

brandur-stripe · 2018-06-14T18:08:47Z

No worries Remi and thank you. Pulling this in.

brandur-stripe · 2018-06-14T18:10:40Z

Released as 34.2.0.

…itions (#581) * added `price order item` and `subscription phase` sections to mapper * removed `activeSection` tracked var for each stripe object and moved to a getter * clean up based on TODO comments * updated formatted fields test class to match new parsed phase object * updates based on PR comments * last updates based on pr comments * moved metadatafields string to const per PR comment * added `price order item` and `subscription phase` sections to mapper * removed `activeSection` tracked var for each stripe object and moved to a getter * clean up based on TODO comments * updates based on PR comments * duplication fix

stripe-ci assigned remi-stripe Jun 9, 2018

brandur-stripe reviewed Jun 9, 2018

View reviewed changes

brandur-stripe force-pushed the brandur-less-api-boilerplate branch from be0f61b to 228662e Compare June 9, 2018 04:12

brandur-stripe reviewed Jun 9, 2018

View reviewed changes

remi-stripe reviewed Jun 9, 2018

View reviewed changes

brandur-stripe force-pushed the brandur-less-api-boilerplate branch 6 times, most recently from 261df2b to 7c389d7 Compare June 13, 2018 05:08

brandur-stripe reviewed Jun 13, 2018

View reviewed changes

remi-stripe reviewed Jun 13, 2018

View reviewed changes

remi-stripe assigned brandur-stripe and unassigned remi-stripe Jun 13, 2018

brandur-stripe force-pushed the brandur-less-api-boilerplate branch from 6e62f71 to f085847 Compare June 14, 2018 00:14

remi-stripe approved these changes Jun 14, 2018

View reviewed changes

stripe-ci added the approved label Jun 14, 2018

brandur added 3 commits June 14, 2018 10:57

Reimplement all other APIs on Call2 and GetIter2

4ced113

Rename Call2 -> Call, GetIter2 -> GetIter, Query2 -> Query

e8969e3

brandur added 2 commits June 14, 2018 10:57

Add a little more documentation for containers

aceb46e

More checks on whether expected params are nil + some comments

4f4cd04

brandur-stripe force-pushed the brandur-less-api-boilerplate branch from f085847 to 4f4cd04 Compare June 14, 2018 17:58

stripe-ci removed the approved label Jun 14, 2018

brandur-stripe merged commit 8400a12 into master Jun 14, 2018

brandur-stripe deleted the brandur-less-api-boilerplate branch June 14, 2018 18:08

Push parameter encoding into BackendConfiguration.Call #581

Push parameter encoding into BackendConfiguration.Call #581

Conversation

brandur commented Jun 9, 2018 • edited by brandur-stripe Loading

brandur-stripe Jun 9, 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

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

brandur-stripe Jun 11, 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

brandur-stripe commented Jun 11, 2018

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

brandur-stripe commented Jun 13, 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

brandur-stripe Jun 14, 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

brandur-stripe commented Jun 14, 2018 • edited Loading

remi-stripe left a comment

Choose a reason for hiding this comment

brandur-stripe commented Jun 14, 2018

brandur-stripe commented Jun 14, 2018

Push parameter encoding into `BackendConfiguration.Call` #581

Push parameter encoding into `BackendConfiguration.Call` #581

brandur commented Jun 9, 2018 •

edited by brandur-stripe

Loading

brandur-stripe Jun 9, 2018 •

edited

Loading

brandur-stripe Jun 11, 2018 •

edited

Loading

brandur-stripe Jun 14, 2018 •

edited

Loading

brandur-stripe commented Jun 14, 2018 •

edited

Loading