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

Allow scope access in handler #40

Merged
merged 3 commits into from
Oct 13, 2024
Merged

Conversation

tigerwill90
Copy link
Owner

@tigerwill90 tigerwill90 commented Oct 10, 2024

Add Scope Awareness to Middleware and Handlers

This PR introduces scope awareness to middleware and handlers, allowing them to behave differently depending on the context in which they are executed. For instance, consider a middleware that handles path inconsistencies like /foo////bar, which cleans up the path and redirects the client to the correct path if it exists. Such middleware would be useful only in the NoRoute or NoMethod handlers, where requests that don't directly match any route are processed.

Currently, there's a risk that users might mistakenly apply this middleware to all routes, when it should be limited to certain scopes, like:

fox.WithMiddlewareFor(fox.NoMethodHandler | fox.NoRouteHandler, redirectFixedPath)

Example:

The following example demonstrates how to restrict middleware execution to specific scopes:

allowedScope := fox.NoMethodHandler | fox.NoRouteHandler
redirectFixedPath := fox.MiddlewareFunc(func(next fox.HandlerFunc) fox.HandlerFunc {
    return func(c fox.Context) {
        if c.Scope()&allowedScope == 0 {
            log.Println("not in the right scope")
            next(c)
            return
        }

        req := c.Request()
        target := req.URL.Path
        cleanedPath := fox.CleanPath(target)

        if cleanedPath == target {
            next(c)
            return
        }

        req.URL.Path = cleanedPath
        route, cc, tsr := c.Fox().Lookup(c.Writer(), req)
        if route != nil {
            defer cc.Close()

            code := http.StatusMovedPermanently
            if req.Method != http.MethodGet {
                code = http.StatusPermanentRedirect
            }

            if !tsr || route.IgnoreTrailingSlashEnabled() {
                if err := c.Redirect(code, cleanedPath); err != nil {
                    panic(err)
                }
                return
            }

            if route.RedirectTrailingSlashEnabled() {
                if err := c.Redirect(code, fox.FixTrailingSlash(cleanedPath)); err != nil {
                    panic(err)
                }
                return
            }
        }

        req.URL.Path = target
        next(c)
    }
})

f := fox.New(
    // WARNING: Applying middleware globally, including for all scopes.
    fox.WithMiddleware(redirectFixedPath),
)

f.MustHandle(http.MethodGet, "/hello/{name}", func(c fox.Context) {
    _ = c.String(200, "Hello %s\n", c.Param("name"))
})

http.ListenAndServe(":8080", f)

Without scope-awareness, the middleware could mistakenly be applied to all routes, causing unnecessary redirection behavior in cases where it's not needed.

Solution:

With access to the scope inside the handler (Context.Scope), middleware can prevent misapplication by checking the scope and logging an error if it's executed in an unintended context:

allowedScope := fox.NoMethodHandler | fox.NoRouteHandler
if c.Scope()&allowedScope == 0 {
    log.Println("not in the right scope")
    next(c)
    return
}

Benefits:

In addition to preventing misapplication, this feature allows middleware like OpenTelemetry to adjust behavior based on context, such as customizing the span name in traces depending on whether the handler is being executed for a route, NoRoute, or NoMethod handler.

Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 88.46154% with 6 lines in your changes missing coverage. Please review.

Project coverage is 89.02%. Comparing base (1032196) to head (954a867).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
internal/iterutil/iterutil.go 60.00% 3 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
+ Coverage   88.72%   89.02%   +0.30%     
==========================================
  Files          16       17       +1     
  Lines        2048     2068      +20     
==========================================
+ Hits         1817     1841      +24     
+ Misses        172      169       -3     
+ Partials       59       58       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tigerwill90 tigerwill90 marked this pull request as ready for review October 10, 2024 20:29
@tigerwill90 tigerwill90 self-assigned this Oct 10, 2024
@tigerwill90 tigerwill90 added the enhancement New feature or request label Oct 10, 2024
@tigerwill90 tigerwill90 merged commit 58d4554 into master Oct 13, 2024
7 of 8 checks passed
@tigerwill90 tigerwill90 deleted the feat/allow-scope-access branch October 13, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants