From bec1352ef7d2bd10a9baa0931a4568cced182b81 Mon Sep 17 00:00:00 2001 From: ryan Date: Wed, 2 Sep 2026 17:39:40 +0800 Subject: [PATCH] fix(logs): collect access logs regardless of plugin order Global Router.Use middleware is applied at HTTP Start instead of being snapshotted when each route is registered, so risk_control still wraps admin APIs that mount earlier. Access-log collection is enabled by default on SQLite/Postgres, not only ClickHouse. --- backend/core/extpoints/extpoints_test.go | 19 ++++++++- backend/core/extpoints/router.go | 15 ++++--- .../domain/risk_control/logstore/db_helper.go | 6 +-- backend/plugins/domain/risk_control/plugin.go | 4 +- .../domain/risk_control/plugin_test.go | 2 + backend/plugins/drivers/driver_http/plugin.go | 41 +++++++++++++------ backend/plugins/drivers/drivers_test.go | 35 ++++++++++++++++ 7 files changed, 94 insertions(+), 28 deletions(-) diff --git a/backend/core/extpoints/extpoints_test.go b/backend/core/extpoints/extpoints_test.go index fe684d75..d5612d7c 100644 --- a/backend/core/extpoints/extpoints_test.go +++ b/backend/core/extpoints/extpoints_test.go @@ -80,12 +80,12 @@ func TestRouterExtension(t *testing.T) { for _, route := range routes { if route.Method == "GET" && route.Path == "/api/v1/orders" { foundOrderGet = true - assert.Equal(t, []any{mGlobal, mAPI, "api_extra_middleware"}, route.Middlewares) + assert.Equal(t, []any{mAPI, "api_extra_middleware"}, route.Middlewares) assert.Equal(t, []any{hList}, route.Handlers) } if route.Method == "PUT" && route.Path == "/api/v1/admin/users/:id" { foundUserPut = true - assert.Equal(t, []any{mGlobal, mAPI, "api_extra_middleware", mAdmin}, route.Middlewares) + assert.Equal(t, []any{mAPI, "api_extra_middleware", mAdmin}, route.Middlewares) assert.Equal(t, []any{hUserPut}, route.Handlers) } } @@ -93,6 +93,21 @@ func TestRouterExtension(t *testing.T) { assert.True(t, foundUserPut) } +func TestRouterGlobalMiddlewareIsNotSnapshottedOntoRoutes(t *testing.T) { + r := extpoints.NewRouterRegistry() + r.GET("/before", "handler") + r.Use("late_global") + r.GET("/after", "handler") + + for _, route := range r.Routes() { + if len(route.Middlewares) != 0 { + t.Errorf("route %s %s Middlewares = %v, want none (globals live on Router.Middlewares)", + route.Method, route.Path, route.Middlewares) + } + } + assert.Equal(t, []any{"late_global"}, r.Middlewares()) +} + func TestRouterWhitelist(t *testing.T) { r := extpoints.NewRouterRegistry() require.NotNil(t, r) diff --git a/backend/core/extpoints/router.go b/backend/core/extpoints/router.go index 44258028..61e47262 100644 --- a/backend/core/extpoints/router.go +++ b/backend/core/extpoints/router.go @@ -95,11 +95,12 @@ func (r *RouterRegistry) addRoute(method, fullPath string, handlers ...any) Rout r.nextID++ rd := RouteDefinition{ - ID: r.nextID, - Method: strings.ToUpper(method), - Path: fullPath, - Handlers: handlers, - Middlewares: append([]any(nil), r.middlewares...), + ID: r.nextID, + Method: strings.ToUpper(method), + Path: fullPath, + Handlers: handlers, + // Global Router.Use middlewares are applied at HTTP Start from + // Router.Middlewares(), so late-registered plugins still wrap earlier routes. } r.routes = append(r.routes, rd) return rd @@ -241,9 +242,7 @@ func (g *RouterGroup) addRoute(method, fullPath string, handlers ...any) RouteDe g.registry.mu.Lock() defer g.registry.mu.Unlock() - allMiddlewares := make([]any, 0, len(g.registry.middlewares)+len(g.middlewares)) - allMiddlewares = append(allMiddlewares, g.registry.middlewares...) - allMiddlewares = append(allMiddlewares, g.middlewares...) + allMiddlewares := append([]any(nil), g.middlewares...) g.registry.nextID++ rd := RouteDefinition{ diff --git a/backend/plugins/domain/risk_control/logstore/db_helper.go b/backend/plugins/domain/risk_control/logstore/db_helper.go index af540ef9..ed33c591 100644 --- a/backend/plugins/domain/risk_control/logstore/db_helper.go +++ b/backend/plugins/domain/risk_control/logstore/db_helper.go @@ -42,10 +42,8 @@ func SetChDBForTest(db *gorm.DB) { } func getDB(ctx context.Context) *gorm.DB { - if c, ok := ctx.(*core.Context); ok && c != nil { - if s, err := core.Inject[contracts.DBService](c); err == nil && s != nil { - return s.DB(ctx) - } + if s, err := core.InjectFrom[contracts.DBService](ctx); err == nil && s != nil { + return s.DB(ctx) } dbMu.RLock() s := dbSvc diff --git a/backend/plugins/domain/risk_control/plugin.go b/backend/plugins/domain/risk_control/plugin.go index c879c381..dbe9e0ec 100644 --- a/backend/plugins/domain/risk_control/plugin.go +++ b/backend/plugins/domain/risk_control/plugin.go @@ -91,7 +91,9 @@ func (p *Plugin) Apply(ctx *core.Context) error { var dbCfg rcDBConfig _ = ctx.Config().Bind("database", &dbCfg) - SetAccessLogEnabled(chCfg.Enabled) + // Access logs persist on the active log database (SQLite / Postgres / ClickHouse). + // Collection is independent of ClickHouse being enabled. + SetAccessLogEnabled(true) logstore.SetDefaultDatabases(dbCfg.Enabled, chCfg.Enabled) core.Bind[contracts.DBService](ctx, logstore.SetDBService) diff --git a/backend/plugins/domain/risk_control/plugin_test.go b/backend/plugins/domain/risk_control/plugin_test.go index fbc37a6d..aa986b66 100644 --- a/backend/plugins/domain/risk_control/plugin_test.go +++ b/backend/plugins/domain/risk_control/plugin_test.go @@ -35,6 +35,8 @@ func TestRiskControlPluginUnit(t *testing.T) { setting, ok := ctx.Settings().Get("risk_control.enable_access_log") require.True(t, ok) assert.Equal(t, true, setting.Default) + assert.True(t, risk_control.IsAccessLogEnabled(), + "access log collection must be on even when ClickHouse is disabled") require.NoError(t, ctx.Dispose()) assert.False(t, customMWCalled) // not dispatched via gin engine here diff --git a/backend/plugins/drivers/driver_http/plugin.go b/backend/plugins/drivers/driver_http/plugin.go index fd3fb5dc..b323786c 100644 --- a/backend/plugins/drivers/driver_http/plugin.go +++ b/backend/plugins/drivers/driver_http/plugin.go @@ -252,27 +252,42 @@ func (p *Plugin) mountContextRoutes(ctx context.Context) error { } p.engine.Use(appContextMiddleware(ctx, p.coreCtx.Root())) SetWhitelist(p.coreCtx.Router().Whitelist()) + + globalMW, err := toGinHandlers(p.coreCtx.Router().Middlewares()) + if err != nil { + return fmt.Errorf("driver_http: invalid global middleware: %w", err) + } + for _, rd := range p.coreCtx.Router().Routes() { - allHandlers := make([]gin.HandlerFunc, 0, len(rd.Middlewares)+len(rd.Handlers)) - for _, m := range rd.Middlewares { - gh, err := toGinHandler(m) - if err != nil { - return fmt.Errorf("driver_http: invalid middleware for route %s %s: %w", rd.Method, rd.Path, err) - } - allHandlers = append(allHandlers, gh) + routeMW, convErr := toGinHandlers(rd.Middlewares) + if convErr != nil { + return fmt.Errorf("driver_http: invalid middleware for route %s %s: %w", rd.Method, rd.Path, convErr) } - for _, h := range rd.Handlers { - gh, err := toGinHandler(h) - if err != nil { - return fmt.Errorf("driver_http: invalid handler for route %s %s: %w", rd.Method, rd.Path, err) - } - allHandlers = append(allHandlers, gh) + handlers, convErr := toGinHandlers(rd.Handlers) + if convErr != nil { + return fmt.Errorf("driver_http: invalid handler for route %s %s: %w", rd.Method, rd.Path, convErr) } + allHandlers := make([]gin.HandlerFunc, 0, len(globalMW)+len(routeMW)+len(handlers)) + allHandlers = append(allHandlers, globalMW...) + allHandlers = append(allHandlers, routeMW...) + allHandlers = append(allHandlers, handlers...) p.engine.Handle(rd.Method, rd.Path, allHandlers...) } return nil } +func toGinHandlers(hs []any) ([]gin.HandlerFunc, error) { + out := make([]gin.HandlerFunc, 0, len(hs)) + for _, h := range hs { + gh, err := toGinHandler(h) + if err != nil { + return nil, err + } + out = append(out, gh) + } + return out, nil +} + //nolint:contextcheck // middleware must wrap the gin request context, not Start's ctx func appContextMiddleware(_ context.Context, appCtx *core.Context) gin.HandlerFunc { return func(c *gin.Context) { diff --git a/backend/plugins/drivers/drivers_test.go b/backend/plugins/drivers/drivers_test.go index 09149511..5b9c008a 100644 --- a/backend/plugins/drivers/drivers_test.go +++ b/backend/plugins/drivers/drivers_test.go @@ -152,6 +152,41 @@ func TestHTTPDriverLifecycle(t *testing.T) { require.NoError(t, err) } +func TestHTTPDriverAppliesGlobalMiddlewareRegisteredAfterRoutes(t *testing.T) { + ctx := core.NewContext(context.Background()) + ctx.Config().SetSource(core.NewMapSource(nil)) + require.NoError(t, ctx.Config().Resolve()) + + ctx.Router().GET("/early", func(c *gin.Context) { + c.Status(http.StatusOK) + }) + + var called atomic.Bool + ctx.Router().Use(func(c *gin.Context) { + called.Store(true) + c.Next() + }) + + httpPlugin := driver_http.New(driver_http.WithAddr("127.0.0.1:0")) + require.NoError(t, httpPlugin.Apply(ctx)) + d, ok := ctx.Driver(core.DriverTypeHTTP) + require.True(t, ok) + require.NoError(t, d.Start(context.Background())) + t.Cleanup(func() { + stopCtx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + _ = d.Stop(stopCtx) + }) + + resp, err := http.Get(fmt.Sprintf("http://%s/early", httpPlugin.Addr())) + require.NoError(t, err) + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) + if !called.Load() { + t.Fatal("Router.Use middleware registered after the route must still run at HTTP Start") + } +} + func TestAsynqWorkerDriverLifecycle(t *testing.T) { mr, err := miniredis.Run() require.NoError(t, err)