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

proposal: net/http: StripPrefix for path patterns #64909

Closed
egtann opened this issue Dec 31, 2023 · 8 comments
Closed

proposal: net/http: StripPrefix for path patterns #64909

egtann opened this issue Dec 31, 2023 · 8 comments
Labels
Milestone

Comments

@egtann
Copy link

egtann commented Dec 31, 2023

Proposal Details

With the upcoming wildcard prefixes arriving in the stdlib Mux in Go 1.22, it makes sense to revisit some functions in the stdlib to ensure they're compatible with the change.

http.StripPrefix does not currently support the newly added wildcards. This means using any wildcard in a path will break routing when used with http.StripPrefix. As one example:

r.Handle("/{username}/*", http.StripPrefix("/{username}", userHandler)) // Does not work

There are two clear paths forward:

  • Add a new function which supports wildcards, StripWildcardPrefix, or
  • Modify the existing StripPrefix to support wildcards

I propose we modify http.StripPrefix, since the performance in stripping wildcards will not have any impact on existing, non-wildcard paths.

Either option is better than exists today, with every project that wants both wildcards and StripPrefix needing to roll their own implementation, despite there being a StripPrefix in the standard library.

I've created an example implementation with a few tests and benchmarks here: https://github.com/egtann/strip-wildcard-prefix

If no action is taken, the documentation for http.StripPrefix should still be updated to clearly state that it does not strip wildcards.

Updates

We can add support for this to http.StripPrefix without any impact for existing, non-wildcard paths by simply using the original http.StripPrefix implementation if the prefix has no wildcards (which is known at initialization, not when routing). I've adjusted the text above to clarify.

@gopherbot gopherbot added this to the Proposal milestone Dec 31, 2023
@seankhliao
Copy link
Member

seankhliao commented Dec 31, 2023

if you strip the wildcard, should the value still be available in pathvalue?

and if you're already using wildcards, why does your handler care about the path?

@egtann
Copy link
Author

egtann commented Dec 31, 2023

The existing behavior of http.StripPrefix copies the entire Request and only modifies the path. I think maintaining the existing behavior in this version results in fewer surprises, so yes, the stripped wildcard should still be available in PathValue. It's also very useful to have access to it in sub-handlers.

@seankhliao seankhliao changed the title proposal: net/http: Strip Wildcard Prefixes proposal: net/http: StripPrefix for path patterns Dec 31, 2023
@adonovan
Copy link
Member

adonovan commented Jan 2, 2024

@jba

@jba
Copy link
Contributor

jba commented Jan 2, 2024

I'm with @seankhliao on this. I don't see the usefulness. If we did do it, we'd have to keep access to the stripped wildcard values, as @egtann points out, but that puts the http.Request into an odd state—wildcard values with no corresponding wildcard in the path. (Admittedly, you could also get to that state with SetPathValue.)

@egtann, can you motivate this with two or three examples?

@egtann
Copy link
Author

egtann commented Jan 2, 2024

Sure, the big use-case is prefixing a path with some customer-specific identifier. Apps which allow a user to access different company accounts with a single login may employ a similar approach, so the URL itself is tied to a particular company when shared.

Here are some large (non-Go) examples. Shopify does this:

/store/{storeSlug}/...

Each handler needs access to that slug to do its job; it can't query the database for "all products" unless it knows "for which store?"

So if I hypothetically had Shopify routing set up in Go, it might use a top-level router to apply auth centrally, then have many sub-routers attached at different paths:

r.Handle("/store/{storeSlug}", http.StripPrefix("/store/{storeSlug}", storeHandler))
r.Handle("/settings", http.StripPrefix("/settings", userSettingsHandler))

// Then inside storeHandler:
sh.Handle("/settings", http.StripPrefix("/settings", storeSettingsHandler))
...

Gmail uses the same pattern in its URLs:

/mail/u/{intToIdentifyTheUser}/...

There are some workarounds that come to mind:

  • Build a middleware which tosses things into context.Values.
  • Pass the entire path to every sub-handler, and don't strip it at all.
  • Roll your own replacement function for StripPrefix supporting this behavior.

All of these workarounds are fine. It just seemed like a surprising limitation of http.StripPrefix to need to build around given wildcard support is being added to the stdlib in other areas.

As just my own experience (without ever having looked into the StripPrefix implementation before), I naively expected it to strip a prefix in this way, and it took a while debugging to uncover the reason why my routes were responding with 404s. I've been coding professionally in Go for many years, so while it was a face-palm moment for me, I have to imagine I won't be the only one to hit this after wildcards land in 1.22.

@jba
Copy link
Contributor

jba commented Jan 2, 2024

Thanks for the examples, and for explaining exactly what your concern is.

I think the workarounds are fine, especially the second—don't use StripPrefix here at all. It seems to create more work than whatever benefit it provides.

But your point about expectations is well taken. We should add doc to StripPrefix that explains that it doesn't work with wildcards. It already says "the prefix must match exactly," so perhaps a bit more verbiage at that point would suffice.

@rsc
Copy link
Contributor

rsc commented May 8, 2024

StripPrefix is about constant string prefixes, not about paths and patterns. Leaving it alone is the right path forward.

@rsc
Copy link
Contributor

rsc commented May 8, 2024

This proposal has been declined as infeasible.
— rsc for the proposal review group

@rsc rsc closed this as completed May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Declined
Development

No branches or pull requests

6 participants