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: add a generic mechanism for storing arbitrary data in Request #60345

Open
aofei opened this issue May 22, 2023 · 3 comments
Labels
Milestone

Comments

@aofei
Copy link
Contributor

aofei commented May 22, 2023

There is a demand to hold arbitrary data in the request scope, such as passing data between multiple handlers or storing parsed path variables by an HTTP router (e.g., #60227). This proposal suggests a general mechanism for storing arbitrary data in the Request struct.

Background

Currently, the recommended approach for associating arbitrary data with a Request is to use Request.WithContext or Request.Clone along with context.WithValue:

req = req.WithContext(context.WithValue(userCtxKey, user))
// or
req = req.Clone(context.WithValue(userCtxKey, user))

The problem with this approach is that both methods create a copy of the Request (either shallow or deep), resulting in a performance hit. Additionally, users need to be cautious to avoid using the old Request.

It is worth mentioning that the primary purpose of Request.WithContext and Request.Clone is to enable users to control the entire lifetime of a request and its response: obtaining a connection, sending the request, and reading the response headers and body. Storing arbitrary data doesn't look like a good fit.

Before the context package was introduced (i.e., before Go 1.7), storing arbitrary data in Request was basically not possible. So as a workaround, HTTP routers or web frameworks designed alternative Handler variants, like func(ResponseWriter, *Request, map[string]string), to pass their parsed path variables along with ResponseWriter and Request. Or simply abandon Handler and introduce a Context struct of their own. The result is added complexity.

The #60227 also suggested a general mechanism to store arbitrary data in Request:

package http

type Request struct {
	...
	Vars map[string]any
	...
}

However, in my opinion, the usage of the exported map[string]any has the following problems:

  1. The user is required to perform a nil check before using it, which can be cumbersome.
  2. The presence of the exported map introduces the risk of data tampering by other packages.
  3. The use of string keys can lead to collisions, especially as it is also designed to store parsed path variables.

Proposal

I propose to add an unexported map[any]any field named "vaules" to the Request struct. This field is used to hold arbitrary data within the request scope. Additionally, we also need add two exported methods to the Request struct: func(key any) (value any, ok bool) as the getter and func(key, value any) as the setter for Request.values.

Implementation

This proposal will introduce the following API changes to the net/http package:

package http

type Request struct {
	...
	values map[any]any
	...
}

// Value returns the value set in the r for the given key, or nil if no value is
// present. The ok result indicates whether value was found in the r.
func (r *Request) Value(key any) (value any, ok bool) { ... }

// Set sets the value to the r for the given key.
//
// The provided key must be comparable and should not be of type string or any
// other built-in type to avoid collisions between packages. Users should define
// their own types for keys.
func (r *Request) SetValue(key, value any) { ... }
@aofei aofei added the Proposal label May 22, 2023
@gopherbot gopherbot added this to the Proposal milestone May 22, 2023
@seankhliao
Copy link
Member

I don't see how this is any safer or more convenient than changing Request.Vars to map[any]any and adding the same disclaimer?

@ianlancetaylor
Copy link
Contributor

CC @neild @bradfitz

@AlexanderYastrebov
Copy link
Contributor

The problem with this approach is that both methods create a copy of the Request (either shallow or deep), resulting in a performance hit.

#48811 Request.SetContext was declined due to

Request is explicitly immutable. That's why we have Request.WithContext.

Does this proposal make request mutable? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

5 participants