go icon indicating copy to clipboard operation
go copied to clipboard

proposal: net/http: expose matched pattern in Request

Open sillydong opened this issue 11 months ago • 36 comments

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 https://github.com/golang/go/issues/66405#issuecomment-2023331350

// 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 https://github.com/golang/go/issues/66405#issuecomment-2048393929 In short, add a Pattern field in http.Request to expose matched pattern info and can be set by third party routers.

sillydong avatar Mar 19 '24 17:03 sillydong

see also #61551

seankhliao avatar Mar 19 '24 17:03 seankhliao

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.

sillydong avatar Mar 19 '24 23:03 sillydong

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

AlexanderYastrebov avatar Mar 21 '24 21:03 AlexanderYastrebov

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

  • [x] expose pat property in request directly or provide a function Pattern to get pattern string
  • [ ] let servemux set matched pattern info in context, people get pattern from context by fixed key https://github.com/golang/go/issues/61551
  • [ ] use a special name r.PathValue("*") to get pattern

sillydong avatar Mar 22 '24 08:03 sillydong

CC @neild @jba

ianlancetaylor avatar Mar 27 '24 16:03 ianlancetaylor

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 avatar Mar 27 '24 17:03 jba

@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.

jba avatar Mar 27 '24 18:03 jba

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

DeedleFake avatar Mar 28 '24 13:03 DeedleFake

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.

neild avatar Mar 28 '24 14:03 neild

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

seankhliao avatar Mar 28 '24 14:03 seankhliao

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.

neild avatar Mar 28 '24 14:03 neild

I'm OK with a Pattern field.

jba avatar Mar 28 '24 16:03 jba

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?

sillydong avatar Mar 30 '24 01:03 sillydong

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

gopherbot avatar Mar 30 '24 14:03 gopherbot

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.

neild avatar Apr 01 '24 16:04 neild

@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.

sillydong avatar Apr 02 '24 12:04 sillydong

@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.

jba avatar Apr 02 '24 14:04 jba

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.

sillydong avatar Apr 03 '24 14:04 sillydong

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 avatar Apr 03 '24 16:04 jba

I thought about all possible solutions, if we go with ability of allowing third-party frameworks set pattern, we have to provide a simple extra field to save it. And if it's empty, we still return Request.pat.str.

sillydong avatar Apr 07 '24 13:04 sillydong

I thought about all possible solutions, if we go with ability of allowing third-party frameworks set pattern, we have to provide a simple extra field to save it. And if it's empty, we still return Request.pat.str.

Maybe do

	} else {
		h, _, r.pat, r.matches = mux.findHandler(r)
		if r.pat != nil {
			r.Pattern = r.pat.str
		}
	}

around here https://github.com/golang/go/blob/20f052c83c380b8b3700b7aca93017178a692d78/src/net/http/server.go#L2684-L2686 to avoid adding methods.

AlexanderYastrebov avatar Apr 08 '24 11:04 AlexanderYastrebov

@AlexanderYastrebov coorect, a single string field to save the pattern string. I will update the PR in this way.

sillydong avatar Apr 09 '24 03:04 sillydong

A ServeMux can itself be used as a handler. What should the value of r.Pattern be in this case:

mux := http.NewServeMux()
http.Handle("/foo", mux)
mux.HandleFunc("/bar", func(w http.ResponseWriter, r *http.Request) { /* r.Pattern? */})

jba avatar Apr 09 '24 18:04 jba

When I first see this code, I would say the r.Pattern should be /bar since it's inside mux, but when I wrote this code and run ListenAndServe, I found it always returns 404, no matter I request /foo or /foo/bar. The reason is ServeMux cannot match the right pattern. If we run with the following code, we can request with /foo successfully and r.Pattern is /

mux := http.NewServeMux()
http.Handle("/foo", mux)
mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { /* r.Pattern? */})

sillydong avatar Apr 10 '24 14:04 sillydong

I believe the correct snippet should be

http.Handle("/foo/", mux)

seankhliao avatar Apr 10 '24 15:04 seankhliao

This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group

rsc avatar Apr 10 '24 18:04 rsc

@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?

jba avatar Apr 10 '24 19:04 jba

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.

rsc avatar Apr 10 '24 20:04 rsc

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.

jba avatar Apr 10 '24 20:04 jba

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

sillydong avatar Apr 14 '24 16:04 sillydong