search

GoogleCloudPlatform / go-endpoints

Cloud Endpoints for Go
https://go-endpoints.appspot.com
Apache License 2.0
255 stars 56 forks source link

Add an extension mechanism to decorate contexts #126

Closed campoy closed 8 years ago

campoy commented 8 years ago

User request by mail an extension mechanism to be able to use https://github.com/golang/appengine/blob/master/appengine.go#L66 on all endpoints.

This is the simplest change that allows a solution I could imagine.

campoy commented 8 years ago

Hey @crhym3, what do you think about this change?

danjacques commented 8 years ago

That looks similar in purpose to this: https://github.com/GoogleCloudPlatform/go-endpoints/pull/125

One difference is that #125 allows the Context setup function to know which service/method its being initialized for, enabling it to install middleware into the Context based on the method.

campoy commented 8 years ago

The change I'm proposing you enables your solution on a higher layer, keeping the framework as simple as possible.

"Do less. Enable more"

On the other hand I like the idea of having the ContextDecorator as a field of the server rather than a global variable.

x1ddos commented 8 years ago

Yeah, sounds good. Maybe the two, #125 and #126, can be combined? e.g. use #126 but as a field of the server.

danjacques commented 8 years ago

I'd really like the ability for the callback to know which service/method it's working on behalf of. Can we include that from #125?

campoy commented 8 years ago

Changed the code to provide the hook as part of Server.

Let's merge this and if we really need the info about what service is being called we can create a new PR.

danjacques commented 8 years ago

Hmm, could you update the callback to include the service/method?

ContextCallback func(context.Context, *ServiceInfo, *MethodInfo) context.Context

This is probably the cleanest way all around. If not, I can create a separate PR to modify the signature to this, but it seems redundant to introduce a callback and then immediately propose updating it.

x1ddos commented 8 years ago

LGTM

@danjacques I think I agree with @campoy - it's just easier to reason about smaller-sized PRs, one little functionality at a time. It would be also nice if the callback PR had a sample use case showing the advantage of including such functionality in the package.