Change to Storage interface to accept context interface

[keithballdotnet]

I would like to modify the storage interface to accept an interface for context information.

i.e.

// GetClient loads the client by id (client_id)
GetClient(context interface{}, id string) (Client, error)

It would also require changes to the response to accept a Context to be included in the response that could be passed into the storage calls.

// Server response
type Response struct {
    Context            interface{}

This would be a breaking change. Would this be considered for a merge?

[liggitt]

at first glance, I'm not a fan of this. Are there other ways to accomplish what you need the context for?

[keithballdotnet]

Not really. As go doesn't have a way of getting a goroutine id it's not possible have something on one side of the library and get it the other side of the library.

The lack of go routine id means the go answer to this is the context package.
https://godoc.org/golang.org/x/net/context

I made the context to be interface{} so that can be upto the developer what that context would be.

Any alternatives without making breaking changes would be very welcome.

[liggitt]

I'm curious what information you would need to plumb through in a context... shouldn't the client id, token id, etc, be sufficient?

[keithballdotnet]

The target is for contextual logging where we have 100s of calls per second, which means it's important to track errors with a request id and potentially other contextual information.

It is not desired to have access tokens/refresh tokens in clear text in the logs.

Potentially too, it could be that the storage understands the context and behaves differently dependant on that context.

[liggitt]

In general, I think plumbing a slush bucket context through interfaces isn't a great approach. Where does the line get drawn? Which interfaces should include a context and which shouldn't? Why do none of the golang interfaces include a context, if this is the recommended design pattern?

[keithballdotnet]

Well, you might not see it as a great approach. It is though a common pattern in web stuff.

See grpc example:
https://github.com/grpc/grpc-go/blob/master/examples/helloworld/helloworld/helloworld.pb.go

Also
http://www.gorillatoolkit.org/pkg/context
https://github.com/rcrowley/go-tigertonic/blob/master/context.go

As discussed before how google requires it's calls to be made:
http://blog.golang.org/context/google/google.go

[RangelReale]

Another way that you could look to do this is, Response has a Storage field, that you could possibly modify a property or put another Storage entirely, maybe with your context in it. All HandleXXX functions always use the storage on the Response struct, never directly on Server.

[aeneasr]

This would support stores to abort a request (e.g. RESTful request) if the token request was aborted due to an error or similar. The Go Team wanted to add a context param to all the SQL query commands in Go 1.5, but it seems like they didn't finish it in time. So even if this isn't relevant for SQL queries right now, it will be in the future.

I myself am not a huge fan of context variables because the scope is kinda blurry and requires a lot of boilerplate code. But it's coming and nobody can stop it, so better go with it.

[keithballdotnet]

@RangelReale while it would be possible, it's not a very attractive solution to recreate the storage each time with a new context. Hmmm.

[itsjamie]

If you do plan to implement this, I would recommend that you use context.Context rather than interface{}. It's an interface, so you could fit any type onto it.

Also, with the changes coming to database/sql to support Context timeouts it makes even more sense.

[vvakame]

Hi, I want to use this library on GoogleAppEngine & Datastore.
I want to use Datastore to storage backend.
appengine's context generate by *http.Request. (*1 *2)
Don't share ctx between requests...
Datastore API required context argument to do everything! reading data... writing data... and others.

[openshift-bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[ptman]

/remove-lifecycle stale

But really, it seems this whole project is stale. Is it no longer maintained?

[openshift-bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[ptman]

/remove-lifecycle stale

[ptman]

/lifecycle frozen