fix: don't constrain middlewares' context-keys to strings 🐛

[benjajaja]

Description

context recommends using "unexported type" as context keys to avoid collisions https://pkg.go.dev/github.com/gofiber/fiber/v2#Ctx.Locals.

The official go blog also recommends this https://go.dev/blog/context.

fiber.Ctx.Locals(key any, value any) correctly allows consumers to use unexported types or e.g. strings.

But some fiber middlewares constrain their context-keys to string in their "default config structs", making it impossible to use unexported types.

This PR removes the string constraint from all middleware default config structs, allowing to now use unexported types as per the official guidelines. However the default value is still a string, so it's not a breaking change, and anyone still using strings as context keys is not affected.

Depends on 2750

Fixes #2356

Type of change

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

Checklist:

• I have performed a self-review of my own code
• New and existing unit tests pass locally with my changes

[ReneWerner87]

Hello,

Thank you for your contribution and the pull request. Before considering changes regarding the configuration of Context* data types across middlewares, I would like to refer to a recently posted comment where I explained the reasons for maintaining the string data type in our current configuration.

We are committed to ensuring a clear and simple interface for our users, and believe that using the string type is most beneficial in this regard. Therefore, we will not be adopting changes that propose a shift to interface{} for the time being, until we have had a thorough discussion on this matter.

Please feel free to contribute your thoughts and arguments in the discussion on the linked comment. Your feedback is very valuable to us, and we want to ensure all perspectives are considered before making a final decision.

Thank you for your understanding and support.

[sixcolors]

This PR introduces flexibility to context keys without causing breaking changes, as the default values remain the same, ensuring backward compatibility.

It is a step in the right direction of reducing potential for collisions, but it doesn't fully resolve the issue. A Breaking change solution could be #2684 (comment)

@benjajaja what do you think?

[sixcolors]

LGTM!

[ReneWerner87]

#2750 is included ?

we had another discussion and took our time and decided to adopt your approach

[sixcolors]

I would note that there is a minor breaking change

cfg.ContextKey != nil will not behave the same if a app dev had explicitly set cfg.ContextKey = “”

[sixcolors]

I would note that there is a minor breaking change

cfg.ContextKey != nil will not behave the same if a app dev had explicitly set cfg.ContextKey = “”

If we wanted this to be non-breaking we could add a utility function:

func IsZeroValue(x interface{}) bool {
    return reflect.DeepEqual(x, reflect.Zero(reflect.TypeOf(x)).Interface())
}

and check if !utils.IsZeroValue(cfg.ContextKey) {…} instead of the != nil

This won’t work.

[ReneWerner87]

Good idea, could you prepare this change

[nickajacks1]

Wouldn't checking ZeroValue break a few cases, such as intentionally using 0 or struct{}?
I'd think a nil check more accurately determines when the user didn't pass a value and we need to set a default.

[sixcolors]

Wouldn't checking ZeroValue break a few cases, such as intentionally using 0 or struct{}? I'd think a nil check more accurately determines when the user didn't pass a value and we need to set a default.

@nickajacks1 thanks you are right, that won’t work. We would have to check that it is nil or (string and "") to make that work and still be fully backwards compatible.

Should we keep it as just != nil or add the string zero value alternative?

[benjajaja]

Wouldn't checking ZeroValue break a few cases, such as intentionally using 0 or struct{}? I'd think a nil check more accurately determines when the user didn't pass a value and we need to set a default.

Yes, it is common to use iota generated "enums" as context keys, and they start from 0. Also, reflection seems overkill.

The breaking change is an edge case IMO. Under normal circumstances, the user either specifies their own context key, or omits it from the config struct.