Allow passing context in requests

[jackhopner]

Currently the msgraph-sdk-go doesn't allow passing the context when making requests which removes the ability to cancel API requests when the context is canceled.

Example usage could look like:

user, err := client.Me().GetWithContext(ctx)

user, err := client.Me().GetWithRequestConfigurationAndResponseHandlerWithContext(ctx, options, nil)

[baywet]

Hi @jackhopner
Thanks for reaching out and for using the Go SDK.

At this very moment, the only use we make of the context is for middleware handlers options and we're using the default context of the request which is the default background context.

It should be easy to add a context parameter to all the layers and then either:

• Use what has been provided by the SDK user and derive from it with the values we want to add
• Use the default context (like we're doing today) should no value be provided.

This brings a couple of questions in terms of implementation and API surface :

What should happen when the SDK tries to add a context value and the key is already in use (unlikely but possible) ? My opinion is that it should return an error. Thoughts?

What should happen when the request gets cancelled ? With the current implementation, the http adapter will attempt to read the body, and get an error, which will be surfaced all the way up to the chained method API and to the consumer. I believe it's the right thing to do, people can then chose to read and handle the error or ignore it. Thoughts?

What should be the API surface? I'm reluctant to add yet another method (on top of Get and GetWithRequestConfigurationAndResponseHandler) as we already have challenges with the SDK size. Therefore I'm suggesting we reuse the existing method for which we could:

• rename to GetWithRequestConfigurationAndResponseHandlerAndContext like you suggested or keep the same name as it's starting to get long. Any guidance where the go community starts saying "this method name is too long?"
• add the parameter at the beginning or the end or on the request configuration parameter. The http.NewRequestWithContext function adds the parameter at the beginning (so before the body for a POST request?)

Appendix

Cancellation repro

package main

import (
	"context"
	"fmt"
	"log"
	"net/http"
	"time"
	"io/ioutil"
)

func main() {
	ctx, cancel := context.WithCancel(context.TODO())
	time.AfterFunc(5*time.Second, func() {
		cancel()
	})
	makeRequest(ctx)
}

func makeRequest (ctx context.Context) {
	req, err := http.NewRequestWithContext(ctx, "GET", "https://httpbin.org/range/2048?duration=8&chunk_size=256", nil)
	if err != nil {
		log.Fatal(err)
	}
	resp, err := http.DefaultClient.Do(req)
	if err != nil {
		log.Fatal(err)
	}
	if resp != nil {
		fmt.Println("got a response" + resp.Status)
		body, err := ioutil.ReadAll(resp.Body)
		if err != nil {
			log.Fatal(err)
		}
		fmt.Println("body len %v", len(body))
	}
}

returns

got a response200 OK
2022/05/26 08:39:18 context canceled
exit status 1

[ghost]

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

[jackhopner]

Thanks @baywet, sorry I missed your reply.

What should happen when the SDK tries to add a context value and the key is already in use (unlikely but possible) ? My opinion is that it should return an error. Thoughts?

I would suggest prefixing all keys with MSGRAPH_${KEY} and then returning an error on conflict, this should help reduce conflicts but still prevent any unexpected behavior.

What should happen when the request gets cancelled ? With the current implementation, the http adapter will attempt to read the body, and get an error, which will be surfaced all the way up to the chained method API and to the consumer. I believe it's the right thing to do, people can then chose to read and handle the error or ignore it. Thoughts?

If you pass the context when creating the HTTP request, when the context is cancelled the HTTP client will handle it and return an error of "context cancelled" (along with canceling the request). This means the HTTP adapter would never attempt to read the body and the client.Do(request) call would return an error itself.

What should be the API surface? I'm reluctant to add yet another method (on top of Get and GetWithRequestConfigurationAndResponseHandler) as we already have challenges with the SDK size. Therefore I'm suggesting we reuse the existing method for which we could:

Yeah fair the names are already quite long, I would suggest the following (both if possible):

• Add convenience GetWithContext, PostWithContext, DeleteWithContext functions which take context.Context as the first argument. This would align with the golang stdlib.
• If SDK size is an issue then, add RequestContext to *RequestConfiguration.

Hopefully that answers all your questions, let me know if you want me to take a stab at the implementation.

[baywet]

Thanks for the additional information so to recap:

• returning errors in case of options collision is ok
• returning errors in case of request/context cancellation is ok
• context parameter could be added to the request configuration class or as a parameter to the "long" method
• we aren't going to add additional methods due to package size
• we're probably not going to change existing method names
• we aren't going to add the context parameter to the short method

so effectively the decision that needs to be made it between

Parameter:

(similar to what dotnet is doing using optional parameters)

client.Me().Messages().PostWithRequestConfigurationAndResponseHandler(body, nil, nil, context)

or

Property on the request configuration.

configuration := import.NewMessagesPostRequestConfiguration()
configuration.Context = context
client.Me().Messages().PostWithRequestConfigurationAndResponseHandler(body, configuration, nil)

The short method staying as it is:

client.Me().Messages().Post(body)

I'll take that back to the PM @maisarissi and the architect @darrelmiller to get their input. Thanks a lot!

[darrelmiller]

My vote would be for a Context property on the RequestConfiguration

[maisarissi]

As Go doesn't support optional parameter nor does it support overload, adding a new parameter would mean that everyone would need to either pass a context or nil, as opposed to .NET which we can use overloads.

Let's go with Context property.

[baywet]

Thank you for providing input on this discussion, scheduled for July for now.

[mblaschke]

On the other side Azure SDK is providing clear design guidelines which also covers the context:
https://azure.github.io/azure-sdk/golang_introduction.html#cancellation

For Azure SDK the context is required for all API calls which is IMHO a good decision.

[baywet]

Thanks for the additional pointers here. It'd be coherent to align things with azure, maybe not as a first parameter though. Also, due to staff shortage on our end, this is most likely going to go to August, maybe September at this point.

[jackhopner]

Thanks for the additional pointers here. It'd be coherent to align things with azure, maybe not as a first parameter though. Also, due to staff shortage on our end, this is most likely going to go to August, maybe September at this point.

If it's going to be passed as a parameter then it really should be the first, this would be in line with how the stdlib and most other golang libraries work.

Also may be worth reading the link below as it warns against putting the context in a struct. (There's actually a golang linter which detects this - https://github.com/sivchari/containedctx)

https://go.dev/blog/context-and-structs

[rkodev]

@maisarissi cc @baywet @darrelmiller Given that this will be a breaking change, i.e the intentions is to change from

client.Me().Messages().PostWithRequestConfigurationAndResponseHandler(body, configuration, responseHandler)

to

client.Me().Messages().PostWithRequestConfigurationAndResponseHandler(context, body, configuration, responseHandler)

should we rename the methods from PostWithRequestConfigurationAndResponseHandler to simply be Post, this might get as closer to Azure SDK as well

[baywet]

We could rename it to post, but we already have a post method that only accepts the body (people complained about having to pass nil for a bunch of arguments).
The upside of reverting back to a single method is that it'd reduce our SDK size by a lot. The down side is that people would need to pass nil for the arguments they don't need.
We could alternatively name it something different (and shorter) like PostWithConfiguration.
If we keep both (post, and postwith) we also need to decide whether we add the context argument to the simple method.

[jackhopner]

We could rename it to post, but we already have a post method that only accepts the body (people complained about having to pass nil for a bunch of arguments). The upside of reverting back to a single method is that it'd reduce our SDK size by a lot. The down side is that people would need to pass nil for the arguments they don't need. We could alternatively name it something different (and shorter) like PostWithConfiguration. If we keep both (post, and postwith) we also need to decide whether we add the context argument to the simple method.

I'd personally recommend just having a single Post method, for a couple of reasons:

• Passing the context to any IO functions is good practice, forcing a consumer of the SDK to explicitly put nil (or the context) should hopefully get them passing context around more often.
• Smaller SDK

In regards to the other args such as configruation and responseHandler, I don't personally think having to pass nil here is the end of the world. You could also use varargs with an options type pattern, e.g.

type RequestOption struct {
    Configuration *ConfigurationType
    ResponseHandler *ResponseHandlerType
}

func Post(ctx context.Context, body BodyType ...opts RequestOption) {
    var config *ConfigurationType
    var respHandler *ResponseHandlerType

    for _, opt := range opts {
        if opt.Configuration != nil {
            config = opt.Configuration
        }...
    }...
}

Edit: Excuse the poor naming

[baywet]

Thanks for the input. Introducing yet another type on the executor method probably won't help us size wise. But we could move the response handler argument to a property on the existing request configuration type.
This way we'd have a single method with only 2 arguments (or 3 when a body is present)
Thoughts?

[maisarissi]

Hello everyone.
After reading https://pkg.go.dev/context I've realized that Contexts shouldn't be inside a struct type, what conflicts with what @darrelmiller and I shared a few comments above. Thanks to folks who shared their perspectives here :)

After talking to @darrelmiller, we agreed to proceed with the context as the first parameter of the function.

Thanks for the input. Introducing yet another type on the executor method probably won't help us size wise. But we could move the response handler argument to a property on the existing request configuration type. This way we'd have a single method with only 2 arguments (or 3 when a body is present) Thoughts?

and yes, let's add responseHandler as part of the request configuration.

As size has been an issue to build time and go docs generation let's have something similar to what @jackhopner shared by adding RequestConfiguration as optional and only one Post() method:

func Post(ctx context.Context, body BodyType ...opts RequestConfiguration) {
    var config *ConfigurationType

    for _, opt := range opts {
        if opt.Configuration != nil {
            config = opt.Configuration
        }...
    }...
}

[baywet]

@maisarissi examples 2 and 3 of your last answer are identical, can you update your message?

[maisarissi]

Just updated the comment above. Let's go with only one Post() method.

[baywet]

The only issue I have with that is that it leads people to think they can pass multiple request configuration instances. And forces us to check all entries (or we could check only the first one but that'd be ugly).
While this seems to be a common pattern in Go, examples seem to suggest that approach is used for multi-valued entries. Not single valued.

[rkodev]

FYI - This is the summary of the changes

• This is the final version of the code implement has this structure

func Post(ctx context.Context, body BodyType, opts RequestConfiguration) {
    var config *ConfigurationType

}

2. No overloads for the methods.