Skip to content

feat: [#970] implement withoutMiddleware for routes#275

Open
u-wlkjyy wants to merge 1 commit into
goravel:masterfrom
u-wlkjyy:feat/without-middleware
Open

feat: [#970] implement withoutMiddleware for routes#275
u-wlkjyy wants to merge 1 commit into
goravel:masterfrom
u-wlkjyy:feat/without-middleware

Conversation

@u-wlkjyy

Copy link
Copy Markdown

Description

Implements WithoutMiddleware for the fiber driver, mirroring goravel/gin#229. This resolves #970 for fiber.

Changes

  • group.go: WithoutMiddleware method on Group — excludes middleware by reflect.Type at registration time. Middleware/WithoutMiddleware are symmetric (inline constructors). excludeMiddlewares filters the middleware list before registration.
  • utils.go: isSameMiddleware — compares reflect.TypeOf (dereferencing pointers), matching struct-based middleware across instances. Closure-based func(Context) middleware share a single type and are a documented limitation.
  • action.go: Action.WithoutMiddleware — stores exclusions in Info.ExcludedMiddleware.
  • Tests: TestWithoutMiddleware in GroupTestSuite, TestIsSameMiddleware unit test.

Dependencies

Requires the framework PR goravel/framework#1497 to be merged first. A replace directive in go.mod points to the framework PR branch until then.

Fixes #970.

- Group.WithoutMiddleware: excludes middleware by reflect.Type at
  registration time (isSameMiddleware in utils.go)
- Action.WithoutMiddleware: stores exclusions in Info.ExcludedMiddleware
- group.go: Middleware/WithoutMiddleware symmetric, inline constructors
- tests: TestWithoutMiddleware in GroupTestSuite, TestIsSameMiddleware
- replace directive points to framework PR branch until merged
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.13559% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.26%. Comparing base (80773b6) to head (8068786).
⚠️ Report is 173 commits behind head on master.

Files with missing lines Patch % Lines
action.go 0.00% 5 Missing ⚠️
group.go 95.45% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #275      +/-   ##
==========================================
- Coverage   79.09%   76.26%   -2.83%     
==========================================
  Files          10       15       +5     
  Lines         885     1361     +476     
==========================================
+ Hits          700     1038     +338     
- Misses        159      261     +102     
- Partials       26       62      +36     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@goravel-coder goravel-coder left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Action.WithoutMiddleware is a no-op + missing test

🔴 Critical: Action.WithoutMiddleware never takes effect

The primary use case from the framework contract doesn't work in the fiber driver:

router.Get("/api/webhook", Handler).WithoutMiddleware(ThrottleMiddleware)
// ↑ ThrottleMiddleware STILL RUNS — exclusion is stored but never checked

Root cause: The fiber driver filters excluded middleware at registration time in Group.getMiddlewares()Group.excludeMiddlewares(), which only reads Group.excludedMiddlewares. But Action.WithoutMiddleware writes to a different field — routes[path][method].ExcludedMiddleware — which the fiber request pipeline never reads.

The middleware chain is already registered with fiber before WithoutMiddleware can be called on the returned Action:

func (r *Group) Get(path string, handler contractshttp.HandlerFunc) contractsroute.Action {
    first, rest := fiberHandlerArgs(r.getMiddlewares(handler)) // chain built HERE
    r.instance.Get(r.getFiberFullPath(path), first, rest...)    // registered HERE
    return NewAction(...)                                        // Action returned AFTER
}

So Action.WithoutMiddleware mutates routes[path][method].ExcludedMiddleware after the fiber route is already registered with the full middleware chain. Nothing consults that field at request time.

Contrast with gin: The gin driver (goravel/gin#229) handles this correctly — middlewareToGinHandler checks routeInfo.ExcludedMiddleware on every request:

// gin's middlewareToGinHandler — fiber has no equivalent
routeInfo := context.Request().Info()
if routeInfo.ExcludedMiddleware != nil {
    for _, excluded := range routeInfo.ExcludedMiddleware {
        if isSameMiddleware(excluded, middleware) {
            c.Next()
            return
        }
    }
}

Fiber's middlewareToFiberHandler skips this check entirely:

func middlewareToFiberHandler(middleware httpcontract.Middleware) fiber.Handler {
    return func(c fiber.Ctx) error {
        context := NewContext(c)
        defer releaseContext(context)
        middleware(context) // no ExcludedMiddleware check
        return nil
    }
}

This is possible in fiber because ContextRequest.Info() already looks up routes[originPath][method] at request time (context_request.go:340), so the excluded list is reachable from inside a fiber handler.

Suggested fix

Add request-time filtering in middlewareToFiberHandler, mirroring gin:

func middlewareToFiberHandler(middleware httpcontract.Middleware) fiber.Handler {
    return func(c fiber.Ctx) error {
        context := NewContext(c)
        defer releaseContext(context)

        routeInfo := context.Request().Info()
        for _, excluded := range routeInfo.ExcludedMiddleware {
            if isSameMiddleware(excluded, middleware) {
                if err := c.Next(); err != nil {
                    return err
                }
                return nil
            }
        }

        middleware(context)
        return nil
    }
}

This makes both Group.WithoutMiddleware (registration-time) and Action.WithoutMiddleware (request-time) work. The Group-level path can stay as-is since it filters before registration; the Action-level path needs the request-time check.


🟠 Major: Missing Action.WithoutMiddleware test (the bug above is hidden by it)

The gin PR added both TestAction_WithoutMiddleware and TestAction_WithoutMiddleware_Integration. The fiber PR only added TestWithoutMiddleware for the Group-level path:

// group_test.go — only exercises Group.WithoutMiddleware
s.route.Middleware(mw).WithoutMiddleware(mw).Get("/without", func(ctx contractshttp.Context) contractshttp.Response {
    return ctx.Response().Json(http.StatusOK, contractshttp.Json{
        "mw": ctx.Value("mw"),
    })
})
s.assert("GET", "/without", http.StatusOK, `{"mw":null}`)

The Action-level path — the one broken above — has no test:

// Missing: something like
s.route.Middleware(mw).Get("/without-action", handler).WithoutMiddleware(mw)
s.assert("GET", "/without-action", http.StatusOK, `{"mw":null}`)

Adding this test would have caught issue #1 immediately, since mw would be "applied" instead of null.

The codecov report already flags this gap: action.go is at 0.00% patch coverage with 5 missing lines — exactly the WithoutMiddleware method body.

Suggested test

Add to GroupTestSuite (or an ActionTestSuite matching the gin PR's structure):

func (s *GroupTestSuite) TestActionWithoutMiddleware() {
    mw := func(ctx contractshttp.Context) {
        ctx.WithValue("mw", "applied")
        ctx.Request().Next()
    }

    s.route.Middleware(mw).Get("/without-action", func(ctx contractshttp.Context) contractshttp.Response {
        return ctx.Response().Json(http.StatusOK, contractshttp.Json{
            "mw": ctx.Value("mw"),
        })
    }).WithoutMiddleware(mw)

    s.assert("GET", "/without-action", http.StatusOK, `{"mw":null}`)
}

This currently fails (returns {"mw":"applied"}) and will pass once issue #1 is fixed.


Summary

# Severity Issue Status
1 🔴 Critical Action.WithoutMiddleware is a no-op — ExcludedMiddleware stored but never read at request time Blocks merge
2 🟠 Major No test for Action.WithoutMiddleware — hides the bug above; action.go at 0% patch coverage Blocks merge

Both are fixable with ~15 lines. Recommend Request changes until Action.WithoutMiddleware works end-to-end and has a covering test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants