Revert revert requestid contextkey

[benjajaja]

Description

Revert the revert of #2742, fix condition check, add a unit test that asserts that the condition is correct, and add a comment about why the default value is string instead of the recommended private-type-style.

See also the initial discussion and my latest comment in #2731 - there are several incorrect assumptions about how the context works or should work, which lead to the commit reversal.

Fixes #2356

Specific case example

• I am a developer of some app that uses fiber, and other libraries, which use context.Context.
• I follow the standard go library documentation's recommendation on context keys and use unexported types.
• I cannot use the requestid middleware because Revert ":bug: requestid.Config.ContextKey is interface{} (#2369)" #2742 has constrained the key to be a string.

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[ReneWerner87]

Hi, we have decided against other types for the context key.
From our point of view, there is no big real world benefit to allow other types for the context key and only adds complexity and errors without any real benefit.

String is sufficient as a type for all conceivable real cases that you might want to use in an application.

[benjajaja]

We're obviously not on the same page. Let's try to work this out, there are two separate issues going on here:

1 - This PR only aligns the requestid middleware back to fiber

The rest of fiber accepts any for context keys:

https://pkg.go.dev/github.com/gofiber/fiber/v2#Ctx.Locals
https://github.com/gofiber/fiber/blob/master/ctx.go#L970

2 - Context keys should be any

Please read the standard library documentation: https://pkg.go.dev/context#Context. Also check the official blog post on the matter: https://go.dev/blog/context. "Private unexported types" implies that functions that take context keys must type them as any. However, this PR is not the place to discuss this - if you really want to force context keys back to strings, then please make a PR that changes c.Locals API to take string instead of any, and argue your case there.

[nickajacks1]

IMO it's probably best to follow the standard Go guidance of using any.
It neither precludes fiber nor users from using strings, and it's less surprising for people used to working with Contexts.

[ReneWerner87]

Hello again,

I'd like to further explain our decision to explicitly set the data type as string.

While it's true that the underlying context in GoFiber allows any data type (interface{}), and the Locals also adhere to this flexible design, it's not always necessary or advantageous to pass this level of flexibility on to the end user of the middleware. Our approach follows the "Principle of Least Astonishment," which aims to create an API that behaves in a way most users would naturally expect.

By specifying the data type as string in the configuration, we aim to achieve several important objectives:

1. Clarity and Predictability: It provides the user with a clear understanding of the expected data type (a simple string). This reduces confusion and potential errors that might arise from misuse of the type flexibility offered by interface{}.

2. Simplicity and Reduced Complexity: Although GoFiber's core is powerful and flexible, exposing too much of this flexibility can be overwhelming or confusing for users. Limiting the data type to string simplifies the user experience, making the middleware more approachable.

3. Avoiding Unnecessary Overhead: Using a specific type like string can also help optimize performance, as the system does not need to constantly check and adapt to different data types.

For instance, consider a scenario where a user mistakenly passes a complex object as the ContextKey when a simple string identifier would suffice. This could lead to unnecessary processing and potential errors, which are avoided by enforcing a string type.

Currently, there have been no reported downsides to this approach in terms of functionality. It doesn't render the middleware non-functional, nor does the alternative of an unspecified type (interface{}) offer any significant real-world benefits in this context. Therefore, we prefer to keep the type specification as string for its clarity, simplicity, and alignment with the principles of sound software design.

I hope this clarifies our design choice. If you have any further questions or suggestions, feel free to share them.

[benjajaja]

Hello again,

I'd like to further explain our decision to explicitly set the data type as string.

While it's true that the underlying context in GoFiber allows any data type (interface{}), and the Locals also adhere to this flexible design, it's not always necessary or advantageous to pass this level of flexibility on to the end user of the middleware. Our approach follows the "Principle of Least Astonishment," which aims to create an API that behaves in a way most users would naturally expect.

You are breaking the principle you are quoting by constraining the context keys of middlewares to be string only. That is:

• I CAN use c.Locals() with an unexported type.
• I surprisingly CANNOT use an unexported type for the requestid middleware.
  To be clear: this contradicts both the general go guidelines on context keys, AND fiber's own context wrapper!

This PR changes this to make it less astonishing:

• I CAN use an unexported type for the requestid middleware.
• I CAN also use a string if I want.
• The default value still is requestid of type string.

By specifying the data type as string in the configuration, we aim to achieve several important objectives:

1. **Clarity and Predictability**: It provides the user with a clear understanding of the expected data type (a simple `string`). This reduces confusion and potential errors that might arise from misuse of the type flexibility offered by `interface{}`.

2. **Simplicity and Reduced Complexity**: Although GoFiber's core is powerful and flexible, exposing too much of this flexibility can be overwhelming or confusing for users. Limiting the data type to `string` simplifies the user experience, making the middleware more approachable.

3. **Avoiding Unnecessary Overhead**: Using a specific type like `string` can also help optimize performance, as the system does not need to constantly check and adapt to different data types.

All three points are moot:

1. It is unclear why only the middlewares are constrained to string, but not c.Locals otherwise.
2. Strings may be more simple, but are open to collisions, have you read https://pkg.go.dev/context#Context?
3. There is absolutely no overhead, on the contrary, a type alias to int would have less overhead than string in any case.

For instance, consider a scenario where a user mistakenly passes a complex object as the ContextKey when a simple string identifier would suffice. This could lead to unnecessary processing and potential errors, which are avoided by enforcing a string type.

Then go ahead an make a PR to change c.Locals to only take string as key, then. Taking this out here is absurd.

Currently, there have been no reported downsides to this approach in terms of functionality. It doesn't render the middleware non-functional, nor does the alternative of an unspecified type (interface{}) offer any significant real-world benefits in this context. Therefore, we prefer to keep the type specification as string for its clarity, simplicity, and alignment with the principles of sound software design.

Yes there is, per my description of this PR:

• I am a developer of some app that uses fiber, and other libraries, which use context.Context.
• I follow the standard go library documentation's recommendation on context keys and use unexported types.
• I cannot use the requestid middleware because Revert ":bug: requestid.Config.ContextKey is interface{} (#2369)" #2742 has constrained the key to be a string.

Additionally, a user that doesn't change the key, or changed it to a string is completely unaffected by this PR.

[ReneWerner87]

Hello,

Thank you for your feedback. Let me briefly address our decision:

1. External vs. Internal API: The decision to use string as the context key in our middleware configuration is a deliberate choice to keep the external API simple and user-friendly. While the internal API of GoFiber maintains the flexibility to handle interface{}, in the external API, we aim to reduce complexity for the end user. This separation ensures that while we offer flexibility and power internally, we also maintain ease of use and clarity in the parts of our software that interact directly with users. It's about striking a balance between providing powerful features and not overwhelming users with internal complexities that they may not need to deal with.

2. Unexported Types: Using unexported types as context keys, though technically feasible, could lead to accessibility issues and confusion. Strings, on the other hand, provide a clear and universally accessible solution.

Based on these considerations, we have decided to maintain the current implementation with string as the context key. This decision is aimed at balancing user-friendliness with technical flexibility.

[benjajaja]

1. **External vs. Internal API**: The decision to use `string` as the context key in our middleware configuration is a deliberate choice to keep the external API simple and user-friendly. While the internal API of GoFiber maintains the flexibility to handle `interface{}`, in the external API, we aim to reduce complexity for the end user. This separation ensures that while we offer flexibility and power internally, we also maintain ease of use and clarity in the parts of our software that interact directly with users. It's about striking a balance between providing powerful features and not overwhelming users with internal complexities that they may not need to deal with.

There is no such distinction. c.Locals is public/external just like the middleware config options.

2. **Unexported Types**: Using unexported types as context keys, though technically feasible, could lead to accessibility issues and confusion. Strings, on the other hand, provide a clear and universally accessible solution.

You have clearly not read https://pkg.go.dev/context#Context nor https://go.dev/blog/context. To summarize: strings will collide, unlike unexported types, so unexported types are safe and strings are not. And the whole point of configuring the context key of the requestid middleware is because it will be read back by the app through c.Locals or ctx.Value.

Based on these considerations, we have decided to maintain the current implementation with string as the context key. This decision is aimed at balancing user-friendliness with technical flexibility.

@ReneWerner87 who exactly is "we"?

[sixcolors]

Hello @benjajaja, thank you for raising your concerns and initiating this PR. Your technical insights are valuable, and I appreciate your dedication to improving GoFiber.

Given that @ReneWerner87 is a maintainer, his perspective holds significant weight. It appears there's also a potential consistency issue with other middleware packages config.ContextKey types, and it might be beneficial to explore a universal approach rather than proposing a PR for a single middleware package immediately following reversion PR #2742 and the #2731 discussion.

I suggest we open an issue or discussion where we can collaborate and work towards a solution that ensures consistency across the entire project. This way, we can address your concerns and maintain a uniform approach to enhance the overall user experience. Your contributions are important in achieving this goal, and I'm open to exploring your technical arguments further.

[sixcolors]

IMO it's probably best to follow the standard Go guidance of using any. It neither precludes fiber nor users from using strings, and it's less surprising for people used to working with Contexts.

GoFiber still supports go 1.17.... What approach would you recommend?

[benjajaja]

@sixcolors a patch for all middlewares is open here: #2751.

[ReneWerner87]

@ReneWerner87 who exactly is "we"?

-> the maintainer team + some of the contributors from the community

Can you please provide a (few) short examples of real world cases so that i can understand the more precise need for the change

[benjajaja]

Yes of course.

Example A, "using unexported types as per official guidelines":

This is some app that uses not only fiber but also other libraries, like for example for mqtt, job queues, whatever you have.
This app stores some values in context.
This app follows the official guidelines at https://pkg.go.dev/context#Context and uses an unexported type for keys to avoid any potential collisions within itself or any of its dependencies.

type contextKey string

const (
	UserID            = contextKey("user_id")
	TaskID            = contextKey("task_id")
	RequestId         = contextKey("request_id")
	// ...etc.
)

The app uses the provided fiber requestid middleware.

app.Use(requestid.New(requestid.Config{
	ContextKey: RequestId,
}))

If requestid.Config.ContextKey were typed to string instead of interface{}, then the app would not be able to use an unexported type for the context key.

The requestid may later be read back with something like this:

id := c.Locals(RequestId) // in a fiber handler / middleware / ...
id := ctx.Value(RequestId) // in a non-fiber "handler", e.g. a job goroutine or similar.

Example B, "using string against official guideline or legacy go version"

This app is just like the previous one, but uses strings as context keys for legacy reason or any other reason.

const (
	UserID            = "user_id"
	TaskID            = "task_id"
	RequestId         = "request_id"
	// ...etc.
)

The app can function just like the previous example.

---

It might be counterintuitive that the type interface{} here actually has the effect of being more typesafe than the tighter string, because it allows for values that are guaranteed to never collide.

[ReneWerner87]

okay, thanks for that
you have convinced me a little more

however, the probability of a collision is low

however, the security aspect that pjebs brings up here, this could be considered when building wrapper applications or plugins, for example, where the outer applications do not have the possibility of modification

valyala/fasthttp#1385

we will consider these aspects again and discuss the advantages and disadvantages of the current version compared to the new version

thanks for the insights and your effort

[sixcolors]

Some middleware use multiple context keys. Maybe middleware.ContextKeys.Key would be a good standard eg requestid.ContextKey.Id

[nickajacks1]

GoFiber still supports go 1.17.... What approach would you recommend?

any is just an alias for interface{}

[sixcolors]

Recomend we close this PR and work on #2751 as it includes this change.

[ReneWerner87]

We merged #2751
No need for this PR