📝 [Proposal]: Standardization of Middleware Context Key Prevent Collisions (Breaking Change)

[sixcolors]

Issue Description

The middleware in GoFiber sets string values to ctx.Locals using non-standardized default context keys, and since these defaults are non-zero values this forces their existence. This behavior can lead to conflicts and confusion for users. Furthermore, the specificity of these context keys, i.e., their purpose and potential conflicts, should be addressed.

Additionally, the presence of dots or dashes in context keys can cause issues when using PassLocalsToViews, which injects key-value pairs into various template engines. Even template engines that correctly handle dots and dashes may cause fiber to feel hacked together and not polished.

Below are some examples of middleware and their non-standardized default context keys:

• BasicAuth:

  • ContextUsername: "username"
  • ContextPassword: "password"

• CSRF:

  • SessionKey: "fiber.csrf.token"
  • HandlerContextKey: "fiber.csrf.handler"

• Keyauth:

  • ContextKey: "token"

• RequestID:

  • ContextKey: "requestid"

• Paseto:

  • ContextKey: "auth-token"

• JWT:

  • ContextKey: "user"

Key Specificity and Conflicts

The default context keys provided by the middleware lack specificity, making it unclear what each key is for. This lack of clarity can lead to conflicts when multiple middleware components use similar or identical context keys for different purposes. Additionally, these keys are used to set string values to ctx.Locals, which can lead to unintended side effects if conflicts occur.

Key Specificity Example

For instance, it's not immediately clear what "ContextUsername" and "ContextPassword" refer to in the BasicAuth middleware, and what kind of string values they set in ctx.Locals. Providing more specific key names can enhance user understanding.

Potential Conflicts

Additionally, the non-standardized default context keys across middleware components can result in unintended conflicts when setting values in ctx.Locals. If multiple middleware components use the same key name for different purposes, it can lead to unexpected behavior and errors in ctx.Locals.

Dots and Dashes in Context Keys

The use of dots or dashes in context keys, while generally accepted in many scenarios, can pose challenges when using PassLocalsToViews. This middleware injects key-value pairs into various template engines, including HTML, Django, Pug, Mustache, etc. These template engines may not handle dots and dashes correctly or may make it cumbersome to access keys with such characters.

Proposed Solution

The proposed solution should aim to address both key specificity and potential conflicts when setting string values to ctx.Locals. You may consider one of the following approaches:

### Context Key Formatting
Consider adopting a standardized naming convention for context keys to enhance consistency and clarity. Here are some suggestions:

• Choose a default camelCase, PascalCase or snake_case for all middlware context keys.
• Prefix context keys with "fiber" and middleware name (e.g., fiberCsrfToken or fiberBasicAuthUsername) or just the middleware name (e.g., csrfToken or basicAuthUsername).

Follow convention from context by using unexported key type to avoid collisions. See also Go Concurrency Patterns: Context.

Breaking Change

The proposed solution represents a breaking change. This means that implementing these changes in GoFiber would require adjustments by users. Here's the context and reasons behind this breaking change:

Context

The current non-standardized context key names, such as "user" or "token," can lead to confusion and conflicts, making it challenging for users to understand their purpose and behaviour. By using functions following the naming convention pkg.*FromContext(ctx) (eg basicauth.UsernameFromContext(ctx), basicauth.PasswordFromContext(ctx)) we can improve clarity and consistency. In the case where the package will return only one value use pkg.FromContext(ctx) (eg jwt.FromContext(ctx)).

Rationale

1. Enhanced User Experience: Standardized ways to access values from middleware. This leads to an improved overall user experience when working with GoFiber middleware.

2. Reduced Conflicts: Using unexported types for key values in packages eliminates the potential for collisions.

3. Consistency: A consistent approach to accessing values simplifies the customization process for users.

4. Clarity in Documentation: Standardized functions also enhance the clarity of documentation. Documentation can provide clear examples and guidance using standardized naming conventions, making it more accessible to users.

Impact on Users

This is a breaking change for users that access values stored in the context.

Migration Plan

Documentation requires an update and the change should be mentioned in the release notes for v3.

By implementing this breaking change, GoFiber would provide a more polished and user-friendly experience and offer better consistency throughout the middleware ecosystem.

Additional Context (optional)

#2682

gofiber/template#307

Checklist:

• I agree to follow Fiber's Code of Conduct.
• I have checked for existing issues that describe my suggestion prior to opening this one.
• I understand that improperly formatted feature requests may be closed without explanation.

[nickajacks1]

Encouraging users to access the default context keys using constants defined in the middleware packages may reduce the impact of such changes.

[nickajacks1]

Prefix context keys with "fiber" and middleware name (e.g., fiberCsrfToken or fiberBasicAuthUsername) or just the middleware name (e.g., csrfToken or basicAuthUsername).

Though unlikely to happen in the first place, a collision with a third party middleware (especially net/http middlewares using adaptor) is less likely if we prefix with fiber.

[sixcolors]

@benjajaja #2750 #2751 potential solution to this proposal middleware/whatever.ContextKey.Key?

for example:

// middleware package
package basicauth

import (
    "github.com/gofiber/fiber/v2"
)

// unexportedKeyType is an unexported type for the context key.
type unexportedKeyType int

// contextKeys struct contains all context keys used by the middleware.
type contextKeys struct {
    Username   unexportedKeyType
    Password    unexportedKeyType
    // Add more context keys as needed
}

// ContextKeys is an instance of contextKeys.
var ContextKeys = contextKeys{
    Username:   0,
    Password: 1,
    // Initialize more keys as needed
}

// New creates a new middleware handler
func New(config Config) fiber.Handler {
     return func(c *fiber.Ctx) error {
        // ...

        // Set values in the context using the keys from the struct.
        c.Locals(Username, username)

        // Set the handler function in the context
        c.Locals(ContextKeys.Password, password)

        // ...
        return c.Next()
    }
}

Changes would be breaking still but the collision issue would be resolved.

Thoughts?

[benjajaja]

@nickajacks1 adding prefixes is still stringly-typed and unsafe, yet it is also a breaking change. Let's just change to an unexported type which will never, ever collide. It's the officially "blessed" way of golang now.

@sixcolors I agree that this should be the future, thank you for opening this issue! Overall this would nudge users in the right direction, which is what we should strive for.

The biggest challenge is that string-typed context keys are probably being used via same-string-value, which will NOT be detectable at compile-time. For example, the basicauth middleware uses "username" and "password" as keys, and probably quite a few projects out there using fiber simply read back the values with c.Locals("username"), instead of the more typesafe c.Locals(basicauth.ConfigDefault.ContextUsername). BTW this is another reason/example to use unexported types!

It seems like v3 is happening - this would probably be the best moment to introduce this breaking change.

@sixcolors in you example, the contextKeys struct is redundant, or at least not something common in go, you can just use const Username etc.

// middleware package
package basicauth

import (
    "github.com/gofiber/fiber/v2"
)

// basicauthKey is an unexported type for the context key.
type basicauthKey int

const (
    UsernameKey basicauthKey = iota
    PasswordKey
)

// Config defines the config for middleware.
type Config struct {
    ...
    // ContextUsername is the key to store the username in Locals
    //
    // Optional. Default: basicauth.UsernameKey
    ContextUsername interface{}

    // ContextPassword is the key to store the password in Locals
    //
    // Optional. Default: basicauth.PasswordKey
    ContextPassword interface{}
}

// ConfigDefault is the default config
var ConfigDefault = Config{
    ...
    ContextUsername: UsernameKey,
    ContextPassword: PasswordKey,
}

Then the users can read the values back with c.Locals(basicauth.UsernameKey) and so forth. But they cannot rely on some magic values. They can also provide their own keys, given that Config.ContextUsername is now interface{}.

[sixcolors]

@benjajaja fair enough, your version is probably better, I just liked the expressiveness of middleware.ContextKeys.Key

[nickajacks1]

@nickajacks1 adding prefixes is still stringly-typed and unsafe, yet it is also a breaking change. Let's just change to an unexported type which will never, ever collide. It's the officially "blessed" way of golang now.

agreed

[sixcolors]

Following the example for https://pkg.go.dev/context it seems that it's best to provide a function in the package to access values from ctx. Will do it that way in PR.