Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Params via Go 1.7 Contexts #147

Merged
merged 5 commits into from Feb 22, 2018
Merged

Conversation

@teepark
Copy link
Contributor

teepark commented Jun 16, 2016

With request-scoped contexts coming to net/http in go 1.7 there's no need for Handler and HandlerFunc to continue throwing away Params.

It may be a little early for this change, but I wanted to get your eyes on it and start the discussion.

@dobegor
Copy link

dobegor commented Jul 29, 2016

@julienschmidt Please consider merging this PR or implementing this functionality in some other way. Go 1.7 releases in a few weeks.

// ParamsKey is the request context key under which URL params are stored.
//
// This is only present from go 1.7.
const ParamsKey = "_httprouter_params"

This comment has been minimized.

Copy link
@bgentry

bgentry Aug 4, 2016

You may want to consider using an unexported type and key for this so that you're guaranteed to avoid collisions with other packages. See the userip example on the context blog post: https://blog.golang.org/context#TOC_3.2.

This comment has been minimized.

Copy link
@Thomasdezeeuw

Thomasdezeeuw Aug 4, 2016

It's even better to use a type, rather then a string. For example like in gRPC: https://github.com/grpc/grpc-go/blob/9e3a674ceba65708273cf1cd8e71a1bdce68107b/peer/peer.go#L54-L65.

This comment has been minimized.

Copy link
@bgentry

bgentry Aug 4, 2016

nice, that's even better 👏 Will do that from now on!

This comment has been minimized.

Copy link
@teepark

teepark Aug 11, 2016

Author Contributor

@bgentry @Thomasdezeeuw thanks, changed!

@bgentry
Copy link

bgentry commented Aug 4, 2016

With context as part of http.Request in Go 1.7, it might be worth considering the 3-argument httprouter.Handle interface to be a legacy implementation. This package could instead rely entirely on the http.Handler interface and make the params available via request.Context.

@teepark
Copy link
Contributor Author

teepark commented Aug 6, 2016

@bgentry agree, although the field is so cluttered with silly router benchmarks that the extra allocations required to shove it in the context may prevent de-facto standard status.

@julienschmidt julienschmidt added this to the v2 milestone Aug 10, 2016
@julienschmidt
Copy link
Owner

julienschmidt commented Aug 10, 2016

Thanks for the request.
It seems like a good idea for v2, since it is a breaking change.
I wasn't able to work on it lately but I hope to do so as soon as I'm done relocating.

If you have any ideas on improvements on the proposed approach or ideas for alternative approaches, please keep commenting.

@teepark
Copy link
Contributor Author

teepark commented Aug 11, 2016

@julienschmidt it's actually not a breaking change, that's one of the reasons I approached it this way. Most context-aware libraries out there want ctx as a first argument, but this leverages the context that's already attached to every http.Request in 1.7 (and changes nothing pre-1.7).

teepark added 3 commits Jun 16, 2016
Go 1.7 introduces request-scoped context, providing a place to hang
Params even in net/http standard Handlers attached with Router.Handler
or Router.HandlerFunc.

This provides an alternative implementation of Router.Handler which
attaches the params to the request context. It also includes a bit of
new public API: ParamsKey is the context key used, and
ParamsFromContext uses ParamsKey to get Params from a context.Context.

All of this is guarded by a "go1.7" build tag. With go 1.6 and older
none of the new API is available and Params continue to be thrown away
in Handler/HandlerFunc-attached endpoints.
@teepark teepark force-pushed the teepark:go17-std-handler branch from 30167f6 to 75ba051 Aug 11, 2016
@teepark
Copy link
Contributor Author

teepark commented Aug 29, 2016

@julienschmidt
ping, would you mind having another look at this? it's fully backwards-compatible, shouldn't need to wait for the v2 milestone.

@captncraig
Copy link

captncraig commented Oct 2, 2016

I love this. It should really simplify things for a lot of use cases. A couple questions though:

  1. Why does ParamsFromContext take a context, instead of just the request? Seems a tad easier for the consumer to use httprouter.ParamsFromContext(r) vs httprouter.ParamsFromContext(r.Context())
  2. How about an additional helper to get a named parameter in one shot? Something like httprouter.GetParam(r, "name") maybe?
@teepark
Copy link
Contributor Author

teepark commented Oct 4, 2016

@captncraig I'm a little concerned about confusion for people using the httprouter-specific interface.

ParamsFromContext(*http.Request) httprouter.Params or GetParam(*http.Request, string) string kind of looks like they can be used from any endpoint handler, but they would only work when attached by Handler or HandlerFunc.

This could be way off base, but it feels like scoping functions to the Context delineates the two more clearly. Like, yes there still exists r.Context() for endpoints attached with router.GET(), but it seems like you'd be more aware that "the context is where the params live" if you used the less-obvious Handler or HandlerFunc.

@lpar
Copy link

lpar commented Oct 12, 2016

Just noting that httprouter.GetParam(*http.Request, string) would be compatible with the fork at https://godoc.org/github.com/bouk/httprouter

@julienschmidt julienschmidt self-assigned this Oct 19, 2016
@julienschmidt julienschmidt changed the title Supports Params in net/http Handlers with go1.7+ Params via Go 1.7 Contexts Oct 20, 2016
@julienschmidt
Copy link
Owner

julienschmidt commented Oct 23, 2016

My current plan is to use the standard http.Handler signature in v2, which makes it a lot more convenient to deal with httprouter. E.g. one does not need to import httprouter everywhere anymore.
However, I am not sure how to support, if at all, Go versions older than 1.7.
Any opinions?

@mostafah
Copy link

mostafah commented Oct 23, 2016

It’s partly up to you in how you want to define the compatibility policy of the package, but me, as a user, understand that sometimes breaking changes are necessary and have no problem with that, specially for third-party packages.

BTW, I’d love net/http style handlers.

@Thomasdezeeuw
Copy link

Thomasdezeeuw commented Oct 23, 2016

Go officially supports 2 major versions, which means that in februari (Go 1.8) all supported Go verions have the context package. People that needs support for older versions can always us the v1 of httprouter. Just my 2 cents.

@lpar
Copy link

lpar commented Oct 23, 2016

I'd say people using an old (unsupported) Go version can continue to use httprouter 1.x.

The other issue is migrating people who have upgraded. The code changes aren't difficult, but they're annoying to have to perform. I wonder if there's a code rewriting library which would allow easy construction of an automated rewriting tool?

@Thomasdezeeuw
Copy link

Thomasdezeeuw commented Oct 23, 2016

@lpar maybe something like go fix can be used: https://blog.golang.org/introducing-gofix.

@teepark
Copy link
Contributor Author

teepark commented Oct 26, 2016

@julienschmidt I'm distilling these priorities from your last comment, anything important to add?

  1. standard http.Handler signature everywhere/only
  2. avoid the need to import httprouter for every handler
  3. gracefully support go <1.7

I want to push back a bit on 2, it has some significant drawbacks:

  • the context key will just have to be a standardized string ("params" maybe), with risk of collision -- see comments above about what we changed in this PR to avoid that
  • the params container type will have to be something available without import like map[string][]string, no option to add convenience methods like v1's Params
  • 1 & 2 together conflict with 3, since there wouldn't be a pre-1.7 way to pass parameters without providing an accessor function that you could swap out with build tags (but that would have to be imported)

In the meantime, could this PR be considered for v1 anyway? I understand v2 to be a backwards-compatibility-breaking release, but this PR only affects the second-class-citizen Handler/HandlerFunc API. Being able to use the GET, POST, etc convenience functions with the standard interface would still be a draw to v2, but with this change in v1 we could use the standard http.Handler interface with httprouter today.

@Thomasdezeeuw
Copy link

Thomasdezeeuw commented Oct 26, 2016

@teepark I'm not sure about @julienschmidt opinion. However most packages expose a FromContext function, which allows retrieval of a value from a context. Just like this code change does, so any handler that needs access to the params still needs to import the httprouter package.

// ParamsKey is the request context key under which URL params are stored.
//
// This is only present from go 1.7.
var ParamsKey = paramsKey{}

This comment has been minimized.

Copy link
@fgrosse

fgrosse Oct 28, 2016

Could we make this an unexported constant? I dont see a reason why the key would change and all access to it should go through ParamsFromContext anyway, right?

The idiomatic context example from [the go blog] also uses an unexported constant.

This comment has been minimized.

Copy link
@teepark

teepark Oct 28, 2016

Author Contributor

Yeah generally it's certainly best to minimize surface area.

The only semi-legitimate use case I can see for needing the key is to override the params in middleware or tests. Having the context key available allows working directly with the context, making that kind of direct manipulation possible.

Is that actually a use case worth supporting? ¯_(ツ)_/¯

This comment has been minimized.

Copy link
@Thomasdezeeuw
"net/http"
)

type paramsKey struct{}

This comment has been minimized.

Copy link
@fgrosse

fgrosse Oct 28, 2016

The official example implementation and documentation for context key types define this to be a type alias for int. I doubt there is any actual benefit defining the key like this. There is always only a single instance of this anyway so I suggest to make this as simple and idiomatic as possible.

This comment has been minimized.

Copy link
@teepark

teepark Oct 28, 2016

Author Contributor

I generally default to struct{} for sentinel values that don't need to carry any data, but whose only purpose is as a placeholder by identity.

I agree there's no practical difference, so by the same token I don't see any benefit to changing it -- even if int is more idiomatic (which I have reservations about), since this type isn't exported the user never interacts with it.

This comment has been minimized.

Copy link
@Thomasdezeeuw

Thomasdezeeuw Oct 28, 2016

@fgrosse their is a actual difference. There are two benefits to using the technique.

  1. The key can never be set outside the package, useful for overlapping key names.
  2. In the Context.Value an interface{} type is used as a key. An interface has two "values" in it, first the defined and then a pointer to the data (with an actual type), see http://research.swtch.com/interfaces. An as struct{} always has the same address (see https://play.golang.org/p/U-5LyKlCc0) it won't use extra memory for the actual data.

This comment has been minimized.

Copy link
@Thomasdezeeuw

This comment has been minimized.

Copy link
@fgrosse

fgrosse Oct 29, 2016

@Thomasdezeeuw I am aware of the benefits of defining a paramsKey type in the package. I am just arguing that there is no actual benefit of making this a struct{} instead of an int. You are just saving the size of an int times the number of instances of the type (here always exactly 1) which in this case is so small that it feels just a bit like cargo cult (no offense). I agree with @teepark that this is only a minor complaint but I found it is worth pointing out in a review and getting the opinion about this from other people.

The only downside I see with the struct{} approach is that this is arguably more confusing to beginners and it diverges from the canonical example and practice in other libraries.

This comment has been minimized.

Copy link
@julienschmidt

julienschmidt Oct 29, 2016

Owner

The benefit of struct{} is that it is guaranteed to be unique while an int could possible have a collision with another package

This comment has been minimized.

Copy link
@fgrosse

fgrosse Oct 29, 2016

As I already pointed out I am not questioning the existence of a paramsKey type in general but I propose to define this just like the canonical example in the context package documentation does:

// contextKey is an unexported type for context keys defined in this package.
// This prevents collisions with context keys defined in other packages.
type contextKey int

This will always prevent collisions with context keys from other packages since the map keys are just of type interface{} and comparison on those is defined to take the actual type into account:

From the Go Language Specification

Interface values are comparable. Two interface values are equal if they have identical dynamic types and equal dynamic values or if both have value nil.

I prepared this example to demonstrate this useful property: https://play.golang.org/p/T1UYUbmonh

Edit: replaced wrong copied part from spec with correct one

This comment has been minimized.

Copy link
@Thomasdezeeuw

Thomasdezeeuw Oct 29, 2016

Why allocate extra memory for an int when this is not needed? See https://play.golang.org/p/Nl61l_h_8-.

This comment has been minimized.

Copy link
@fgrosse

fgrosse Oct 29, 2016

Because this is extreme micro optimization that will only save you 4 bytes during the whole runtime of the program because the only value of this type is ever only allocated once during package initialisation. I do not think this is worth the disadvantages I mentioned earlier: maybe more confusing for new devs, deviates from the standard, it triggers more discussion than the whole topic is worth 😄

func(w http.ResponseWriter, req *http.Request, p Params) {
ctx := req.Context()
ctx = context.WithValue(ctx, ParamsKey, p)
req = req.WithContext(ctx)

This comment has been minimized.

Copy link
@fgrosse

fgrosse Oct 28, 2016

We should update the README.md to reflect the additional allocations this change introduces (with which I would be totally fine with)

Zero Garbage: The matching and dispatching process generates zero bytes of
garbage. In fact, the only heap allocations that are made, is by building the
slice of the key-value pairs for path parameters. If the request path contains
no parameters, not a single heap allocation is necessary.

This comment has been minimized.

Copy link
@teepark

teepark Oct 28, 2016

Author Contributor

Nice catch.

@mattes
Copy link

mattes commented Dec 26, 2016

I think this change would make httprouter incompatible with google app engine.

ref: googleapis/google-cloud-go#312

@teepark
Copy link
Contributor Author

teepark commented Dec 27, 2016

@mattes I don't have experience with Go on AppEngine, but if I'm reading this correctly it looks like the request object is a normal standard library request object.

I believe the issue you linked is about supporting standard lib context.Context in the appengine.* packages.

@mattes
Copy link

mattes commented Dec 28, 2016

mea culpa. just realize appengine is still in on go 1.6 ... which is the reason why it panicked when I tried to access request.Context(). this PR is using build tags for the different versions, so this should be fine.

Repository owner deleted a comment from vardius Feb 22, 2018
Repository owner deleted a comment from omeid Feb 22, 2018
Repository owner deleted a comment from donutloop Feb 22, 2018
Repository owner deleted a comment Feb 22, 2018
Repository owner deleted a comment from chancez Feb 22, 2018
Repository owner deleted a comment from ydnar Feb 22, 2018
Repository owner deleted a comment from mkjois Feb 22, 2018
Repository owner deleted a comment from ydnar Feb 22, 2018
Repository owner deleted a comment from bgentry Feb 22, 2018
Repository owner deleted a comment from ydnar Feb 22, 2018
Repository owner deleted a comment from omeid Feb 22, 2018
Repository owner deleted a comment from bwmarrin Feb 22, 2018
Repository owner deleted a comment from omeid Feb 22, 2018
Repository owner deleted a comment from fgrosse Feb 22, 2018
@julienschmidt
Copy link
Owner

julienschmidt commented Feb 22, 2018

First of all, sorry for the long waits. I didn't have much time for my side-projects lately and that GitHub emails folder remains mostly unchecked.

But PLEASE DO NOT use the issue tracker to ask for updates or other irrelevant comments. I doesn't help and just sends spam notifications to the 228 people currently watching this repository. I just removed a lot of comments. Please ONLY comment if you have to say something regarding the TECHNICAL approach.

Regarding the PR:
The changes seem sensible and should be fully backwards compatible. Thus, I am going to merge it now to not delay this long awaited feature any further.

Some minor adjustments should still be done later, e.g. the README should also be updated (the section about http.Handler and an example would be great).

In the v2 (whenever that is released), this should be the default approach.

@julienschmidt julienschmidt merged commit d767e59 into julienschmidt:master Feb 22, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mytototo
Copy link

mytototo commented Jun 15, 2018

Hi, I tried to use this new solution to retrieve route params but can't make it work (always nil).
Is it possible to add an example in the README with this new version using the context please ?

Thanks a lot.

@teepark
Copy link
Contributor Author

teepark commented Jun 15, 2018

@mytototo just on a hunch, did you maybe call ctx.WithValue() without overriding your ctx variable? WithValue doesn't change the context in place, it returns a new one with the value added.

@mytototo
Copy link

mytototo commented Jun 15, 2018

I thought we could use req.Context() to access the information of the request context. Isn't that the case ? Like used here: https://github.com/julienschmidt/httprouter/blob/master/params_go17.go#L36

@teepark
Copy link
Contributor Author

teepark commented Jun 15, 2018

Yes you're right, sorry it's been a while since I've been immersed in this change!

@mytototo
Copy link

mytototo commented Jun 16, 2018

I figured it out. I just needed to use HandlerFunc to have the context. Otherwise, we can't access it. I think it worth to have it written somewhere in the docs or examples. I find it a bit confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v2
Handler
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.