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

net/http: expose matched pattern in Request #66405

Closed
sillydong opened this issue Mar 19, 2024 · 40 comments
Closed

net/http: expose matched pattern in Request #66405

sillydong opened this issue Mar 19, 2024 · 40 comments

Comments

@sillydong
Copy link
Contributor

sillydong commented Mar 19, 2024

Proposal Details

Go 1.22 has provided http.ServeMux with supporting of pattern match for requests. While I'm using it, I tried to get the matched pattern of that request. With help of *ServeMux.Handler, I can successfully get matched handler and pattern, but later when I run debug of the code, I found in *http.Request itself, there is a private pat which is exactly matched pattern info. I suggest we export the pat for public access or provide some function to allow user to get the matched pattern, so that we can avoid much logic to use *ServeMux.Handler to parse the request again. This reduces huge resource usage in high load services.

The matched pattern info is usually very useful when we do analysis for requests. We can group requests by their pattern and make the analysis more generic. This is a very high frequency usage scenario which makes reducing the resource usage very worthy.

sample code:

func TestServeMux(t *testing.T) {
	m := http.NewServeMux()
	m.Handle("GET /{name}", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		t.Log(r.PathValue("name"))
	}))
	http.ListenAndServe(":8999", m)
}

When we do request GET http://127.0.0.1:8999/aloha, we can see such info in debug console.

image

Update 2024/03/28:
As we talked in comments, @jba 's suggestion is a good solution #66405 (comment)

// Pattern returns the pattern that matched this request, if the request
// resulted from matching a pattern registered on a ServeMux.
// Otherwise, it returns the empty string.
func (*Request) Pattern() string

Update 2024/04/11:
After many discussions, the latest proposal can be found here #66405 (comment)
In short, add a Pattern field in http.Request to expose matched pattern info and can be set by third party routers.

Update 2024/05/13:
Final design: #66405 (comment)
The proposal is to add a Pattern string field to net/http.Request.
ServeMux.ServeHTTP will set the field to the pattern of the matching handler before dispatching to that handler when using the new Go 1.22 HTTP mux.
In Go 1.21 compatibility mode the field is unset.
The field is otherwise left untouched. Third-party routers may set it if they wish.

@gopherbot gopherbot added this to the Proposal milestone Mar 19, 2024
@sillydong sillydong changed the title proposal: net/http: export the pat iin http.Request to allow user directly access matched pattern proposal: net/http: export the pat in http.Request to allow user directly access matched pattern Mar 19, 2024
@seankhliao seankhliao changed the title proposal: net/http: export the pat in http.Request to allow user directly access matched pattern proposal: net/http: export the pattern in http.Request Mar 19, 2024
@seankhliao
Copy link
Member

see also #61551

@sillydong
Copy link
Contributor Author

Similar to #61551 but instead of setting pattern info in Context, I suggest we export it directly in http.Request where it already has that info and the pattern is actually corresponding to request itself instead of any Context.

@AlexanderYastrebov
Copy link
Contributor

Maybe the string pattern value could be made accessible via special name r.PathValue("*") (note also #64910)

@sillydong
Copy link
Contributor Author

sillydong commented Mar 22, 2024

We definitely need a way to get pattern info in lower cost instead of using *ServeMux.Handler to parse request again.

For now there are three suggestions for getting that

@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Mar 27, 2024
@ianlancetaylor
Copy link
Member

CC @neild @jba

@jba
Copy link
Contributor

jba commented Mar 27, 2024

I wouldn't be opposed to adding:

// Pattern returns the pattern that matched this request, if the request
// resulted from matching a pattern registered on a ServeMux.
// Otherwise, it returns the empty string.
func (*Request) Pattern() string

@jba
Copy link
Contributor

jba commented Mar 27, 2024

@sillydong, if you're OK with my suggestion above, please edit the top post to refer to my comment. You may also want to edit the issue title.

@sillydong sillydong changed the title proposal: net/http: export the pattern in http.Request proposal: net/http: add Pattern() string to *http.Request to expose pattern info of current request Mar 28, 2024
@DeedleFake
Copy link

What about custom mux implementations? Should they be able to store whatever pattern they used in the Request?

@neild
Copy link
Contributor

neild commented Mar 28, 2024

Request.PathValue already conveys information from the mux, without any way for custom mux implementations to set it.

Perhaps there should be some way for custom muxes to set Request.Pattern and Request.PathValue, but I think that can be a separate proposal. Adding Pattern doesn't seem to make things any worse for custom muxes than they are now.

@seankhliao
Copy link
Member

Request.PathValue already conveys information from the mux, without any way for custom mux implementations to set it.

it can be set with Request.SetPathValue

@neild
Copy link
Contributor

neild commented Mar 28, 2024

it can be set with Request.SetPathValue

Today I learned!

In that case, I agree that there should be a corresponding SetPattern, or Pattern should just be a field of Request.

@jba
Copy link
Contributor

jba commented Mar 28, 2024

I'm OK with a Pattern field.

@sillydong
Copy link
Contributor Author

To expose the Pattern field, we have to change private pattern to public Pattern, also the fields in pattern like str,method,host,segments,loc should be public, and this makes type segment should also be public. This is making things complicate. I suggest we provide Pattern()string and SetPattern(string) functions. In SetPattern(string) we can call existing parsePattern to parse the parameter and set to http.Request.pat. This can be much easier and cleaner.
@jba @neild what do you think?

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/574997 mentions this issue: net/http: add Pattern and SetPattern in Request to return matched pattern info

sillydong added a commit to sillydong/go that referenced this issue Apr 1, 2024
…tern info

SetPattern will parse string with parsePattern function. This allows third party frameworks set pattern info in requests.
Pattern will return matched pattern string if it is set.

Fixes golang#66405
@neild
Copy link
Contributor

neild commented Apr 1, 2024

To expose the Pattern field, we have to change private pattern to public Pattern, also the fields in pattern like str,method,host,segments,loc should be public, and this makes type segment should also be public.

Or we could just have:

type Request {
  // Pattern is the [ServeMux] pattern that matched the request, if any.
  Pattern string
}

That seems pretty simple.

If we do have a SetPattern, then we need to consider how to handle a pattern that doesn't parse. What if the user is using a mux with patterns that don't correspond to the ones http.ServeMux uses? That seems likely; I'd expect the entire reason someone would use an alternate mux is to get different pattern matching features.

@sillydong
Copy link
Contributor Author

sillydong commented Apr 2, 2024

@neild No. Pattern is actually a structured data. It contains segments and pattern string itself. Segments are used while getting path value. If we turn pattern into string, we lost everything.

I have created PR for Pattern()string and SetPattern(string)error, you can check it out.

@jba
Copy link
Contributor

jba commented Apr 2, 2024

@sillydong I think you're missing @neild's point. Your code for SetPattern parses the pattern according to http.ServeMux's rules, but someone else's pattern syntax may be entirely different. Since we can't know how other patterns work, the best we can do is use a string.

@sillydong
Copy link
Contributor Author

I think both you guys explained makes sense. But the problem is if we provide a function to set a string pattern, we have to create a new field to save that string info. That makes us not able to take advantage of Requst.pat. And also the Pattern function would have to return that string value instead of the real pattern string in Request.pat.str. This is kind of weird. I'm not quite sure about if this is a good way to handle the pattern info.

@jba
Copy link
Contributor

jba commented Apr 3, 2024

We would be duplicating information, that's true. Request.Pattern would be set from Request.pat.str. If we were just supporting retrieving the pattern, I probably would have a Pattern() string method instead that returns Request.pat.str. But since we are allowing setting it, and setting it to an arbitrary string that might not be a valid pattern, having a separate field makes more sense.

@jba
Copy link
Contributor

jba commented Apr 10, 2024

@sillydong My code was typed without testing, thanks for fixing it.
I agree that it should be the pattern of the innermost ServeMux. That's what Request.PathValue does, and it should be consistent.

Can I ask you once again to change the top post to reflect the current proposal, which is to have a Pattern field instead of a method.

Are there any unresolved objections to that?

@rsc
Copy link
Contributor

rsc commented Apr 10, 2024

FWIW, it is usually clearer to post the updated proposal in a new message, and then update the first comment to just say

"The latest proposal is <link to message>."

than to rewrite history. Rewriting history is confusing to anyone trying to get up to speed on the conversation.

@jba
Copy link
Contributor

jba commented Apr 10, 2024

This comment describes the current proposal.

Add a Pattern field of type string to net/http.Request.
ServeMux.ServeHTTP will set the field to the pattern of the matching handler before dispatching to that handler.
The field is otherwise left untouched. Third-party routers may set it if they wish.

@sillydong
Copy link
Contributor Author

sillydong commented Apr 14, 2024

Instead of checking r.pat, I took usage of the returned string of mux.findHandler(r). Easier and cleaner.

@rsc Proposal is updated and code change has been done. Please help push forward the progress. Thanks a lot. PR: golang.org/cl/574997

@rsc
Copy link
Contributor

rsc commented Apr 24, 2024

Have all remaining concerns about this proposal been addressed?

The proposal is to add a Pattern string field to net/http.Request.
ServeMux.ServeHTTP will set the field to the pattern of the matching handler before dispatching to that handler when using the new Go 1.22 HTTP mux.
In Go 1.21 compatibility mode the field is unset.
The field is otherwise left untouched. Third-party routers may set it if they wish.

@sillydong
Copy link
Contributor Author

In Go 1.21 compatibility mode the field is unset.

For this part, I think we can set matched pattern string to Pattern field because in Go 1.21 the mux is also able to run finding matched handler, it also returns matched pattern string.
image

@seankhliao
Copy link
Member

Does accepting this mean declining #61551 ?
And will http.StripPrefix modify Request.Pattern ?

@jba
Copy link
Contributor

jba commented Apr 25, 2024

Does accepting this mean declining #61551 ?

I think yes. I don't believe that proposal adds anything once we have Request.Pattern.

And will http.StripPrefix modify Request.Pattern ?

No. I don't even know what that would mean in some cases. For example, say the pattern is /{a}, the path is /foo, and the argument to StripPrefix is /f. That works today and changes the path to oo (it literally calls strings.TrimPrefix). What would it do to the pattern?

@seankhliao
Copy link
Member

So there's #64909 to teach StripPrefix about path patterns.

I was wondering about a more limited form where the pattern is processed the same as Request.URL.Path and RawPath now, so a literal match would be updated
(request /a/b, handle /a/ stripprefix /a/, pattern /b),
but a pattern match wouldn't
(request /a/b, handle /{x}/, stripprefix ???, pattern /{x}/).

@sillydong
Copy link
Contributor Author

In Go 1.21 compatibility mode the field is unset.

For this part, I think we can set matched pattern string to Pattern field because in Go 1.21 the mux is also able to run finding matched handler, it also returns matched pattern string. image

@rsc @jba Hi, what do you think of this update? I think it's fair enough to also update the pattern for mux121 since it does have the pattern info from findHandler, we should not ignore it.

@sillydong
Copy link
Contributor Author

Hi @rsc @jba , is there any update for this proposal? What do you think of also provide pattern info for Go 1.21 mux?

@rsc
Copy link
Contributor

rsc commented May 8, 2024

We were away last week. This issue will move to likely accept this week.

We should leave the old mux behavior unchanged. It is still accessible from a GODEBUG but we want people to move off it. We are not updating it, and eventually it may be removed. Adding new API to it gives the opposite impression.

@rsc
Copy link
Contributor

rsc commented May 8, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is to add a Pattern string field to net/http.Request.
ServeMux.ServeHTTP will set the field to the pattern of the matching handler before dispatching to that handler when using the new Go 1.22 HTTP mux.
In Go 1.21 compatibility mode the field is unset.
The field is otherwise left untouched. Third-party routers may set it if they wish.

@rsc rsc moved this from Active to Likely Accept in Proposals May 8, 2024
@rsc rsc moved this from Likely Accept to Accepted in Proposals May 15, 2024
@rsc
Copy link
Contributor

rsc commented May 15, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is to add a Pattern string field to net/http.Request.
ServeMux.ServeHTTP will set the field to the pattern of the matching handler before dispatching to that handler when using the new Go 1.22 HTTP mux.
In Go 1.21 compatibility mode the field is unset.
The field is otherwise left untouched. Third-party routers may set it if they wish.

@rsc rsc changed the title proposal: net/http: expose matched pattern in Request net/http: expose matched pattern in Request May 15, 2024
@rsc rsc modified the milestones: Proposal, Backlog May 15, 2024
@dmitshur dmitshur modified the milestones: Backlog, Go1.23 Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

Successfully merging a pull request may close this issue.

10 participants