Context in requests - beginning of a breaking change discussion

[andrewhoff]

From @orian on June 8, 2018 7:59

It should be possible to add context.Context to requests. It's particularly useful for a request we want to control a timeout, so one can use context.WithTimeout().

Not sure about how to make it as the current implementation assumes that options is just query.Values.
The go' standard way is to pass context as first argument.

Copied from original issue: getconversio#93

[andrewhoff]

From @dlebech on June 11, 2018 20:27

@orian This is a good suggestion. I'm curious what you mean by "The go' standard way is to pass context as first argument.". As far as I know, the standard way is to call Request.WithContext(...) when you want to add context. There are no standard POST, GET etc. functions that take context as a parameter. Perhaps I'm missing something or misunderstanding what you mean?

Here's a suggestion in the meantime: If you want to specify e.g. a timeout globally for all your Shopify requests, then you could look into replacing the http.Client being used. It's exposed as the Client field on the object returned from goshopify.NewClient(). Alternatively, you could also experiment with calling the NewRequest and Do functions directly if you need more control.

[andrewhoff]

From @orian on June 12, 2018 10:54

Thanks. I'm aware about client method.

By "standard" I mean that at least few APIs (e.g. gRPC) and context.Context guideline itself suggest to pass context as a first argument.
func (Api) DoSmth(context.Context, ...) (ReturnValue, error) {}

[andrewhoff]

From @dlebech on June 14, 2018 19:32

I understand your point about the "official" recommendation being to always add the context as first argument to functions. Similarly to #92, I'll let this one brew for a few days and see what people think.

Perhaps you have some suggestions on how to best manage a breaking change like this?

[andrewhoff]

From @orian on June 15, 2018 9:41

I think using: http://labix.org/gopkg.in would make the most sense.
We can then leave master at v1 and create v2 with breaking changes if we don't want to break go get for current API users. We should recommend to import through a gopkg.in in future and probably provide an import instruction for those using golang's dep tool (now officially recommended dependence management).

Or we can break the API and provide instructions for current users to switch to gopkg.in/getconversio/go-shopify.v1 and move master forward.

Moving master forward is good idea for future so dep and other more advanced tools always fetch the most recent version.

[andrewhoff]

From @dlebech on June 15, 2018 19:45

I'll be honest and say that I haven't followed the progress on this front, so I'm not familiar with the go dep tool besides seeing it mentioned in various places. I did use gopkg.in before, and I like the idea that it brings to the table, but I have also seen mentions about it being a bottle-neck and potentially not maintained anymore, which creates a bit of a tricky situation :-)

Reading the documentation for go dep, it seems that it will prefer semantic version branches first and then branches like master second. So as I understand it, we could actually avoid breaking changes for now by just creating a new v2.x branch and v2.0.0 tag (this is inspired by the gRPC branch naming). That would work both for existing go get (it would just continue using master) and go dep users. It would also allow people to use gopkg.in if they wish (see my comment below as well).

What do you think? Is this similar to what you had in mind as well?

Ideally, I would like to follow official guidelines about dependency management, so focusing on supporting go dep going forward is most appealing to me :-)

PS. I changed the title of this issue to better reflect this is now kind of a discussion topic. 🍻

[epelc]

@gordcurrie are you open to breaking changes on this?

I am working on adding ctx context.Context as the first param to all operations/resources and adding to the client method calls. This is the nice way to do it in go(the req.WithContext was only done to not break the api of standard lib which has a very strict breaking change policy).

Would you be open to potentially doing a v4? I am thinking this + fixing #85 by switching everything from interface{} to the actual types which are used. I have been using the lib for a while but these have been the two largest pain points so I just made an internal fork to work towards some reworks like this kind of a v4.

[gordcurrie]

Hello @epelc thank you for the interest in resolving this issue. In general we try and not make breaking changes when it can be avoided, but at the same time we aren't afraid to do a major version bump if the benefits outweigh the cost and it is a change that that makes things better for the consumers of the library. That being said, what are the cost and benefits of your proposed changes? From the above thread, I'm understanding benefits would be the ability to set timeouts and cancel the requests as well as being more consistent with a normal Go pattern of passing the context. The cost would be having to update all calls using the library to be updated to pass the context along? Am I missing anything on either front? Thanks again for your work on this.

[epelc]

Hi @gordcurrie that is the gist of it for the context update. It should be a simple update for most people especially since you can defer it by using context.TODO().

I'm working on some other things which could maybe be bundle with this to make it a more worthwhile update.

[shuqingzai]

@oliver006 Hi
Is there a plan for a 4.0 release date? When we make a request from HTTP Client, we urgently need to set context to connect opentelemetry and request timeout control in series.

[oliver006]

No plans right now to implement this myself but a PR with the change is welcome. If you intend to submit a PR it might make sense to outline and agree on the change first before starting the actual implementation.

[ar3s3ru]

@oliver006 just hit the same issue as @shuqingzai (sort of).

Happy to work on the changes on a PR.

In terms of changes, I'd expect that context.Context is introduced in all the methods that do start a request with Client -- that is, any *Service. The context is then passed to Client.CreateAndDo:

go-shopify/goshopify.go

Lines 617 to 623 in 6898174

	func (c *Client) CreateAndDo(method, relPath string, data, options, resource interface{}) error {
	_, err := c.createAndDoGetHeaders(method, relPath, data, options, resource)
	if err != nil {
	return err
	}
	return nil
	}

which will pass it in the call stack until this point, where the context.Context is set with req.WithContext(ctx):

go-shopify/goshopify.go

Lines 230 to 233 in 6898174

	req, err := http.NewRequest(method, u.String(), bytes.NewBuffer(js))
	if err != nil {
	return nil, err
	}

Pretty straightforward I would say.

Any concerns?

[ar3s3ru]

For your consideration, I've made the changes in #257.

[oliver006]

@ar3s3ru - this is great, thanks for the PR, let's get this done!

I think the path forward for a release of a new major version should be

• cut one last 3.x release and tag master as `version_3.x" or something (we can use that fot future 3.x releases if needed)
• merge your PR feat: add context.Context propagation #257 and my backwards-incompatible changes Backwards incompatible changes for 4.0 #258
• once we collected all other backwards-incompatible changes we can release the first 4.x release from that master

What do you think?

[ar3s3ru]

@oliver006 thanks for reviewing the PR 🙏🏻

The plan you outlined sounds great.

Especially your last point, if you still want to consider other breaking changes before cutting the first v4.x release (e.g. removing the usage of interface{} from {Resource}Services, maybe use generics to remove code duplication).