fix(requestid): fix condition for setting default value of ContextKey…

[gopkg-dev]

… in configDefault function

The condition for setting the default value of ContextKey in the configDefault function has been fixed. Previously, it only checked if cfg.ContextKey was an empty string, but now it also checks if it is nil. This ensures that the default value is set correctly even if cfg.ContextKey is not explicitly provided.

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Explain the details for making this change. What existing problem does the pull request solve?

Fixes # (issue)

⚠️ For changes specific to v3, please switch to the v3 Pull Request Template.

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Checklist:

• For new functionalities I follow the inspiration of the express js framework and built them similar in usage
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation - /docs/ directory for https://docs.gofiber.io/
• 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
• If new dependencies exist, I have checked that they are really necessary and agreed with the maintainers/community (we want to have as few dependencies as possible)
• I tried to make my code as fast as possible with as few allocations as possible
• For new code I have written benchmarks so that they can be analyzed and improved

Commit formatting:

Use emojis on commit messages so it provides an easy way of identifying the purpose or intention of a commit. Check out the emoji cheatsheet here: https://gitmoji.carloscuesta.me/

[welcome]

Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

[nickajacks1]

ContextKey will only ever be "" if the user explicitly sets it to that value. In that case, overriding it to be "requestid" might be surprising.
Though I suppose changing it to accept "" now might break someone's code out there.

[ReneWerner87]

should be fine, check this #2731 (comment)

[ReneWerner87]

thanks for the fix

due to the underlying locals customization of fasthttp, we have changed the type to interface to also allow numbers etc, this customization is correct

[ReneWerner87]

@gaby @efectn @gopkg-dev @Z3NTL3 @nickajacks1 @sixcolors
should we rather revert this pull request instead of this customization? meanwhile my opinion has changed and normally there is no reason for other types instead of strings if this should be a key
#2369

@benjajaja sorry for that, but i think it would be better to go back to the previous state

[nickajacks1]

I feel like I've seen context keys as types/global vars before, something like

type contextKey int
const (
    contextKeyRequestId contextKey = iota
    contextKeySession
    // etc...
)

For this PR, if it was me, I'd probably keep it as an interface, do a nil check and leave it at that.
But even so, it's probably perfectly valid for Fiber to only support strings if that's a desired direction.
Cf. #2684

[sixcolors]

Will review tonight or tomorrow night.

[Z3NTL3]

I have looked on GoDoc and I've seen that ContextKey is an inteface{} type and not a string. So this change would be valid.

But there is one more thing, I've looked into the middleware code. And I've seen there is no appropiate type checking. Because an interface{} type in Go can be anything, when the users passes an object thats very off, like a struct or map it will put this object as a key to c.Locals which will result in unexpected behaviour and leak something bad off. There should be a check with reflect package to check if the given ContextKey is a string or int, anything else should set configuration to default.

https://github.com/valyala/fasthttp/blob/master/userdata.go#L14
This check here for setting, it's not enough in my opinion, Fiber uses this of fasthttp under the hood for c.Locals
This will leak something bad in the memory when the key is anything else than int or string

[ReneWerner87]

@Z3NTL3 whats your opinion on this #2731 (comment)

[Z3NTL3]

@Z3NTL3 whats your opinion on this #2731 (comment)

revert

[sixcolors]

Revert. String zero value used for default is behavior elsewhere too.

And follow up discussions can be held to adopt a universal approach when agreed upon.

[gopkg-dev]

If the ContextKey parameter is not passed, it defaults to nil. This can cause the default configuration to lose its effect and prevent the use of the default ContextKey value. Therefore, I believe this is a bug. Currently, I am using it as follows:

package main

import (
	"log"

	"github.com/gofiber/fiber/v2"
	"github.com/gofiber/fiber/v2/middleware/requestid"
)

func main() {
	app := fiber.New()

	// Current implementation
	app.Use(requestid.New(requestid.Config{
		ContextKey: requestid.ConfigDefault.ContextKey,
		Generator:  util.NewXID,
	}))
	
	// Expected behavior
	app.Use(requestid.New(requestid.Config{
		Generator:  util.NewXID,
	}))

	log.Fatal(app.Listen(":3000"))
}

[Z3NTL3]

@gopkg-dev

When viewing the GoDoc we can see ContextKey is an interface{}, which has to be again a string for the following reason:

I've looked into the middleware source. And I've seen there is no appropiate type checking. Because an ``interface{} type in Go can be anything, when the users passes an object thats very off, like a struct or map it will put this object as a key to c.Locals which will result in unexpected behaviour and leak something bad off. There should be a check with reflect package to check if the given ContextKey is a string or int, anything else should set configuration to default.

The reason for this is - Explained

https://github.com/valyala/fasthttp/blob/master/userdata.go#L14
This is used by c.Locals (also the middleware for setting ContextKey)
It will leak something bad in the memory when the key is anything else than int or string, so passing an interface{} is very much illegal

So its better to revert it back to be a string instead of an interface, to avoid overloads in different validating parts and memory leaks. In that matter, your PR becomes sadly said invalid. Idk what the maintainers will do.

[Z3NTL3]

@gopkg-dev @ReneWerner87

ContextKey is set using:

https://github.com/valyala/fasthttp/blob/master/userdata.go#L14.

When we examine this method from fasthttp which is also being used byc.Locals under the hood

We can see that this method converts anything to a byte representation. So when we allow an interface the user may pass anything. When a struct or map is passed for example then it will be represented like:

In request Context object representation:
ContextKey: string= "struct{Test bool} {Test true}"

So this is the ContextKey, a string including the byte representation of the struct

Thats so illegal, the performance will tear down so hard. Some very illegal behaviour.

For those reasons, reverting the ContextKey back to be a string is valid.

[efectn]

@gopkg-dev @ReneWerner87

ContextKey is set using:

https://github.com/valyala/fasthttp/blob/master/userdata.go#L14.

When we examine this method from fasthttp which is also being used byc.Locals under the hood

We can see that this method converts anything to a byte representation. So when we allow an interface the user may pas anything. When a struct or map is passed for example then it will be represented like:

In request Context object representation:
ContextKey: string= "struct{Test bool} {Test true}"

So this is the ContextKey, a string including the byte representation of the struct

Thats so illegal, the performance will tear down so hard. Some very illegal behaviour.

For those reasons, reverting the ContextKey back to be a string is valid.

I do agree. We should keep parameters/fields as type-safe as possible.

[ReneWerner87]

closed because of #2742

[benjajaja]

@gopkg-dev @ReneWerner87

ContextKey is set using:

https://github.com/valyala/fasthttp/blob/master/userdata.go#L14.

When we examine this method from fasthttp which is also being used byc.Locals under the hood

We can see that this method converts anything to a byte representation.
[...]

This is not correct, at all: *userData.Set(k,v) ONLY converts keys of type []byte to string (because comparing []bytes behind any would panic on line 22).

The std package context says the following about keys:

A key can be any type that supports equality; packages should define keys as an unexported type to avoid collisions.

So context keys should never be restricted to strings by libraries, and both fiber and fasthttp correctly follow this and allow any for their abstractions over context.Context.

---

The requestid middleware cannot not be an exception to the any key rule. If you make this exception, you disallow the key type that is recommended by the standard library: type key int.

---

The revert should be reverted, but the condition should be changed to a simple nil check, and the default value should not be of an unexported type.

[ReneWerner87]

Just because the methodic which is used inside supports all types, this does not have to be used outside for the key
How useful is it, for example, to use a struct as a key, which is also possible there?
Keys are usually strings or ints, but in this case strings make more sense and if numbers are to be used, this can also be done as a string number
By allowing only one type from the outside, we prevent possible errors or incorrect use