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

Feature Request: SetUserValue key changed to interface{} #1385

Closed
pjebs opened this issue Sep 25, 2022 · 6 comments
Closed

Feature Request: SetUserValue key changed to interface{} #1385

pjebs opened this issue Sep 25, 2022 · 6 comments

Comments

@pjebs
Copy link
Contributor

pjebs commented Sep 25, 2022

Currently SetUserValue accepts only a string for a key.
This is a limitation.

It would be great if the key accepted an interface{} similar to context.Context: https://pkg.go.dev/context#WithValue

The rationale for the (arguably backward compatible change) is that if it accepts interface{}, I can make the key into an unexported internal type.

That way, outside packages can't tamper with my user value.

In the fiber framework, the fiber context (which gives access to fasthttp.uservalue) is passed down middlewares. Some of these middleware are external packages I have no control over.

I need to protect some of my user values so they don't get tampered with. If the type of key is an unexported internal type, they can't modify it.

This approach is heavily used by context.Context.

type internalKey string

ctx.SetUserValue( internalKey("my key"), 22)
@erikdubbelboer
Copy link
Collaborator

I'm not sure what to think of this. It will cause extra allocations. But I guess anyone who cares about that wouldn't be using SetUserValue anyways.

I don't have time to add support for this at the moment, so a pull request would be very nice.

@pjebs
Copy link
Contributor Author

pjebs commented Sep 27, 2022

In the context.WithValue documentation it states:

The provided key must be comparable and should not be of type string or any other built-in type to avoid collisions between packages using context. Users of WithValue should define their own types for keys. To avoid allocating when assigning to an interface{}, context keys often have concrete type struct{}. Alternatively, exported context key variables' static type should be a pointer or interface.

With regards to your concern about extra allocations, can you help me understand what they are saying?

Then I will issue a PR.

@erikdubbelboer
Copy link
Collaborator

They are saying that with context if you do:

type internalKey struct{}

ctx.SetUserValue( internalKey{}, 22)

it won't allocate because an empty struct has no size so it fits within the interface{} without having to allocate extra memory.

While doing:

type internalKey string

ctx.SetUserValue( internalKey("my key"), 22)

would cause an extra allocation since string is too big to fit into the interface{}. So an extra string will get allocated on the heap and the interface{} will point to that.

@pjebs
Copy link
Contributor Author

pjebs commented Sep 30, 2022

@erikdubbelboer Thanks for explaining

@pjebs
Copy link
Contributor Author

pjebs commented Oct 7, 2022

@erikdubbelboer Any idea when new version will be tagged?

@erikdubbelboer
Copy link
Collaborator

I'll tag one on Monday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants