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

📝 [Proposal]: Add Support for Context in RequestID Middleware #3175

Closed
3 tasks done
zhangyongding opened this issue Oct 22, 2024 · 9 comments · Fixed by #3200
Closed
3 tasks done

📝 [Proposal]: Add Support for Context in RequestID Middleware #3175

zhangyongding opened this issue Oct 22, 2024 · 9 comments · Fixed by #3200

Comments

@zhangyongding
Copy link

zhangyongding commented Oct 22, 2024

Convenient to get the value of requestid from UserContext

// New creates a new middleware handler
func New(config ...Config) fiber.Handler {
	// Set default config
	cfg := configDefault(config...)

	// Return new handler
	return func(c fiber.Ctx) error {
		// Don't execute middleware if Next returns true
		if cfg.Next != nil && cfg.Next(c) {
			return c.Next()
		}
		// Get id from request, else we generate one
		rid := c.Get(cfg.Header)
		if rid == "" {
			rid = cfg.Generator()
		}

		// Set new id to response header
		c.Set(cfg.Header, rid)

		// Add the request ID to locals
		c.Locals(requestIDKey, rid)

		// Add the request ID to UserContext
		ctx := context.WithValue(c.UserContext(), cfg.Header, rid)
		c.SetUserContext(ctx)

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

Alignment with Express API

N/a

HTTP RFC Standards Compliance

N/a

API Stability

N/a

Feature Examples

N/a

Checklist:

  • I agree to follow Fiber's Code of Conduct.
  • I have searched for existing issues that describe my proposal before opening this one.
  • I understand that a proposal that does not meet these guidelines may be closed without explanation.
@ReneWerner87
Copy link
Member

can you describe what advantages it has over the value we already have in the locals?

@zhangyongding
Copy link
Author

Helps to include RequestID when obtaining UserContext, hoping to still carry RequestID when using UserContext as context in some scenarios

@ReneWerner87
Copy link
Member

what do you think we should expand the context in all middlewares where we also fill the locals?
@gaby @efectn

i wouldn't do it only in the requestid mw, because then it wouldn't be consistent

@JIeJaitt
Copy link
Contributor

@ReneWerner87 @gaby @efectn I think requestid, keyauth, csrf, session middleware may need this function more, cache, compress, logger, limiter, favicon, pprof, recover, redirect, rewrite, etag, cors and helmet, idempotency, earlydata, encryptcookie may not be so need this function. Maybe we can add those with higher needs first.

@ReneWerner87
Copy link
Member

ok can you support us with a PR

@JIeJaitt
Copy link
Contributor

@ReneWerner87 I'm very happy to finish it. I will finish this PR in the next few days.

@gaby gaby added this to v3 Nov 12, 2024
@gaby gaby moved this to In Progress in v3 Nov 12, 2024
@gaby gaby added this to the v3 milestone Nov 12, 2024
@gaby gaby changed the title 📝 [Proposal]: Add support for UserContext in requestid middleware 📝 [Proposal]: Add Support for Context in RequestID Middleware Nov 12, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in v3 Nov 15, 2024
@ReneWerner87
Copy link
Member

@ReneWerner87 @gaby @efectn I think requestid, keyauth, csrf, session middleware may need this function more, cache, compress, logger, limiter, favicon, pprof, recover, redirect, rewrite, etag, cors and helmet, idempotency, earlydata, encryptcookie may not be so need this function. Maybe we can add those with higher needs first.

Now we can start with the other middlewares.

@ReneWerner87
Copy link
Member

@coderabbitai create an issue report for the other middlewares with a task list to check off

Copy link
Contributor

coderabbitai bot commented Nov 21, 2024

Tip

For best results, initiate chat on the files or code changes.

@ReneWerner87, I've created the issue report as requested: Add Context Support to Additional Middlewares.

🔗 Analysis chain

@ReneWerner87, creating the issue report for the other middlewares with a task list as per your request.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Create a GitHub issue to add context support to additional middlewares
gh issue create \
  --assignee ReneWerner87 \
  --title "Add Context Support to Additional Middlewares" \
  --body "Based on the discussion in [Issue #3175](https://github.com/gofiber/fiber/issues/3175), we need to extend context support to other middlewares for consistency.

Middlewares to update:
- [ ] keyauth
- [ ] csrf
- [ ] session

This will ensure that relevant context can be passed through the \`UserContext\` in these middlewares.

Requester: @ReneWerner87"

Length of output: 502

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

Successfully merging a pull request may close this issue.

4 participants