fix(controller): register middlewareGraphHook before global graph build#6020
Open
gxkl wants to merge 2 commits into
Open
fix(controller): register middlewareGraphHook before global graph build#6020gxkl wants to merge 2 commits into
gxkl wants to merge 2 commits into
Conversation
middlewareGraphHook was registered in configDidLoad via GlobalGraph.instanceFor(bag)?.registerBuildHook(), but the per-app GlobalGraph is only created inside moduleHandler.init() during didLoad, so the optional chain silently no-oped and cross-module controller middleware inject edges were never woven into the graph. In the standalone tegg repo the graph was created synchronously in the EggModuleLoader constructor (tegg plugin configDidLoad), which ran before the controller plugin's configDidLoad, so registering on GlobalGraph.instance there worked. The migration moved graph creation into the async load() path and changed the non-null assertion into an optional chain, masking the regression. Route the registration through moduleHandler.registerGlobalGraphBuildHook() instead — hooks buffered there are flushed onto the graph right after creation, before build() runs (the same pattern the aop plugin uses for its crosscut/pointcut graph hooks). Add a cross-module middleware fixture module and a regression test that asserts the inject edges exist in the built graph. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughReworks ChangesMiddleware Graph Hook Registration and Validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ControllerAppBootHook
participant ModuleHandler
participant GlobalGraph
participant MiddlewareGraphTest
ControllerAppBootHook->>ModuleHandler: registerGlobalGraphBuildHook(middlewareGraphHook)
ModuleHandler->>ModuleHandler: init()
ModuleHandler->>GlobalGraph: flush buffered hook
MiddlewareGraphTest->>GlobalGraph: findInjectProto(crossModuleMiddlewareController)
GlobalGraph-->>MiddlewareGraphTest: inject edges to CountAdvice and FooMethodAdvice
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates ControllerAppBootHook to buffer the middlewareGraphHook on moduleHandler during configDidLoad rather than registering it directly on GlobalGraph, which is not yet initialized at that stage. This ensures cross-module controller middleware inject edges are correctly woven. A new test controller and integration tests have been added to verify this behavior. The reviewer suggests using .ts extensions instead of .js in import paths within the new test controller to maintain consistency with the repository's conventions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+3
to
+4
| import { CountAdvice } from '../multi-module-common/advice/CountAdvice.js'; | ||
| import { FooMethodAdvice } from '../multi-module-common/advice/FooMethodAdvice.js'; |
Contributor
There was a problem hiding this comment.
To maintain consistency with the existing convention across source and test files in this repository, please use .ts extensions instead of .js in import paths for TypeScript source files.
Suggested change
| import { CountAdvice } from '../multi-module-common/advice/CountAdvice.js'; | |
| import { FooMethodAdvice } from '../multi-module-common/advice/FooMethodAdvice.js'; | |
| import { CountAdvice } from '../multi-module-common/advice/CountAdvice.ts'; | |
| import { FooMethodAdvice } from '../multi-module-common/advice/FooMethodAdvice.ts'; |
References
- In this repository, use
.tsextensions in import/export paths for TypeScript source files to maintain consistency with the existing convention across source and test files.
Deploying egg with
|
| Latest commit: |
78116fb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e8110055.egg-cci.pages.dev |
| Branch Preview URL: | https://fix-controller-middleware-gr.egg-cci.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #6020 +/- ##
==========================================
+ Coverage 81.95% 82.18% +0.23%
==========================================
Files 678 678
Lines 20800 20800
Branches 4147 4147
==========================================
+ Hits 17047 17095 +48
+ Misses 3235 3199 -36
+ Partials 518 506 -12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a tegg controller-plugin regression where middlewareGraphHook was registered before the per-app GlobalGraph existed, causing cross-module controller middleware inject edges to never be woven into the graph. The change routes hook registration through moduleHandler.registerGlobalGraphBuildHook(), ensuring the hook is applied immediately after graph creation and before build() runs.
Changes:
- Register
middlewareGraphHookviaapp.moduleHandler.registerGlobalGraphBuildHook(...)instead ofGlobalGraph.instanceFor(...)?.... - Add a new fixture module (
multi-module-controller) that references middleware advices from another module to exercise cross-module weaving. - Add a regression test that asserts the expected inject edges exist in the built
GlobalGraph, plus request-level behavior checks.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tegg/plugin/controller/src/app.ts | Register the graph build hook through moduleHandler so it’s applied before the global graph build runs. |
| tegg/plugin/controller/test/http/middleware-graph.test.ts | New regression test verifying cross-module middleware inject edges are woven during graph build and still work at request time. |
| tegg/plugin/controller/test/fixtures/apps/controller-app/config/module.json | Enables the new fixture module in the controller-app fixture’s module list. |
| tegg/plugin/controller/test/fixtures/apps/controller-app/modules/multi-module-controller/package.json | Declares the new fixture module for tegg module loading. |
| tegg/plugin/controller/test/fixtures/apps/controller-app/modules/multi-module-controller/CrossModuleMiddlewareController.ts | New controller using cross-module @Middleware advices to reproduce the regression. |
|
Dependency limit exceeded — report not shown. This pull request scan exceeded the 10,000-dependency limit applied to this scan, so the results are incomplete and may be inaccurate. To avoid reporting false positives, Socket has not posted a report. Upgrade your plan to raise the dependency limit and get complete reports, or view the partial scan in the dashboard. Socket is always free for open source. If this is a non-commercial open source project, contact us to request a free Team account. |
Deploying egg-v3 with
|
| Latest commit: |
78116fb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bd8859c8.egg-v3.pages.dev |
| Branch Preview URL: | https://fix-controller-middleware-gr.egg-v3.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tegg/plugin/controller/test/http/middleware-graph.test.ts (1)
1-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding multi-app regression coverage.
This PR fixes a lifecycle/graph-build-timing bug in hook registration. As per coding guidelines, "When changing loader, runtime, lifecycle, or eventbus behavior, add or update multi-app regression coverage (for example tests like
MultiApp.test.ts) to verify two concurrent apps do not cross-talk." The current tests validate a single app's graph but don't confirm the buffered-hook approach is correctly scoped per-app when two apps run concurrently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tegg/plugin/controller/test/http/middleware-graph.test.ts` around lines 1 - 70, Add multi-app regression coverage for the buffered hook registration change: the current `middleware-graph.test.ts` only verifies one `MockApplication` and does not prove `middlewareGraphHook` stays scoped per app when two apps initialize concurrently. Extend the tests around `GlobalGraph.instanceFor`, `mm.app`, and `app.ready()` to spin up a second app (similar to `MultiApp` coverage) and assert each app’s graph and middleware injection edges remain isolated with no cross-talk.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tegg/plugin/controller/test/http/middleware-graph.test.ts`:
- Around line 1-70: Add multi-app regression coverage for the buffered hook
registration change: the current `middleware-graph.test.ts` only verifies one
`MockApplication` and does not prove `middlewareGraphHook` stays scoped per app
when two apps initialize concurrently. Extend the tests around
`GlobalGraph.instanceFor`, `mm.app`, and `app.ready()` to spin up a second app
(similar to `MultiApp` coverage) and assert each app’s graph and middleware
injection edges remain isolated with no cross-talk.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8458a456-56b3-468f-8b74-0f4690c4134e
📒 Files selected for processing (5)
tegg/plugin/controller/src/app.tstegg/plugin/controller/test/fixtures/apps/controller-app/config/module.jsontegg/plugin/controller/test/fixtures/apps/controller-app/modules/multi-module-controller/CrossModuleMiddlewareController.tstegg/plugin/controller/test/fixtures/apps/controller-app/modules/multi-module-controller/package.jsontegg/plugin/controller/test/http/middleware-graph.test.ts
…phHook Two concurrent apps loading the same cross-module middleware fixture modules: each app must get its own GlobalGraph with its own woven middleware inject edges (per-app scoping of the buffered hook), with no proto sharing across apps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Addressed review feedback in 78116fb:
🤖 Generated with Claude Code |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tegg/plugin/controller/test/http/middleware-graph-multi-app.test.ts`:
- Around line 31-55: The test setup in middleware-graph-multi-app.test.ts can
leak app1 if app2.ready() fails before entering the cleanup block. Move the app
creation and Promise.all(app1.ready(), app2.ready()) inside the try so the
finally always closes any app that booted partially. Keep the existing
GlobalGraph.instanceFor, findControllerProto, and findCountAdviceInject
assertions unchanged after the apps are ready.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e8a7205-185b-41d3-b6d3-bd9b7affeaa4
📒 Files selected for processing (11)
tegg/plugin/controller/test/fixtures/apps/controller-app-multi-b/app/controller/README.mdtegg/plugin/controller/test/fixtures/apps/controller-app-multi-b/config/config.default.tstegg/plugin/controller/test/fixtures/apps/controller-app-multi-b/config/module.jsontegg/plugin/controller/test/fixtures/apps/controller-app-multi-b/config/plugin.tstegg/plugin/controller/test/fixtures/apps/controller-app-multi-b/package.jsontegg/plugin/controller/test/fixtures/apps/controller-app-multi-c/app/controller/README.mdtegg/plugin/controller/test/fixtures/apps/controller-app-multi-c/config/config.default.tstegg/plugin/controller/test/fixtures/apps/controller-app-multi-c/config/module.jsontegg/plugin/controller/test/fixtures/apps/controller-app-multi-c/config/plugin.tstegg/plugin/controller/test/fixtures/apps/controller-app-multi-c/package.jsontegg/plugin/controller/test/http/middleware-graph-multi-app.test.ts
✅ Files skipped from review due to trivial changes (8)
- tegg/plugin/controller/test/fixtures/apps/controller-app-multi-c/config/module.json
- tegg/plugin/controller/test/fixtures/apps/controller-app-multi-b/config/module.json
- tegg/plugin/controller/test/fixtures/apps/controller-app-multi-c/package.json
- tegg/plugin/controller/test/fixtures/apps/controller-app-multi-c/config/config.default.ts
- tegg/plugin/controller/test/fixtures/apps/controller-app-multi-b/app/controller/README.md
- tegg/plugin/controller/test/fixtures/apps/controller-app-multi-b/config/plugin.ts
- tegg/plugin/controller/test/fixtures/apps/controller-app-multi-c/app/controller/README.md
- tegg/plugin/controller/test/fixtures/apps/controller-app-multi-b/package.json
Comment on lines
+31
to
+55
| const app1 = mm.app({ baseDir: getFixtures('apps/controller-app-multi-b') }); | ||
| const app2 = mm.app({ baseDir: getFixtures('apps/controller-app-multi-c') }); | ||
| await Promise.all([app1.ready(), app2.ready()]); | ||
| try { | ||
| const graph1 = GlobalGraph.instanceFor((app1 as any)._teggScopeBag); | ||
| const graph2 = GlobalGraph.instanceFor((app2 as any)._teggScopeBag); | ||
| assert(graph1); | ||
| assert(graph2); | ||
| assert.notStrictEqual(graph1, graph2, 'each app must have its own GlobalGraph'); | ||
|
|
||
| for (const graph of [graph1!, graph2!]) { | ||
| const controllerProto = findControllerProto(graph); | ||
| assert(controllerProto, 'controller proto should exist in each app graph'); | ||
| const countAdviceProto = findCountAdviceInject(graph, controllerProto!); | ||
| assert(countAdviceProto, 'middleware inject edge should be woven in each app graph'); | ||
| assert.equal(countAdviceProto!.instanceModuleName, 'multi-module-common'); | ||
| } | ||
|
|
||
| // The protos behind the edges are per-app instances, not shared. | ||
| const proto1 = findCountAdviceInject(graph1!, findControllerProto(graph1!)!); | ||
| const proto2 = findCountAdviceInject(graph2!, findControllerProto(graph2!)!); | ||
| assert.notStrictEqual(proto1, proto2, 'advice protos must not be shared across apps'); | ||
| } finally { | ||
| await Promise.all([app1.close(), app2.close()]); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Potential app leak if one app fails to boot.
app1/app2 are created and ready()-awaited before the try block starts. If app2.ready() rejects while app1.ready() succeeds, Promise.all throws before entering try, so app1 is never closed in finally. Move app creation/awaiting inside the try to guarantee cleanup on partial boot failure.
🧹 Proposed fix
- const app1 = mm.app({ baseDir: getFixtures('apps/controller-app-multi-b') });
- const app2 = mm.app({ baseDir: getFixtures('apps/controller-app-multi-c') });
- await Promise.all([app1.ready(), app2.ready()]);
- try {
+ const app1 = mm.app({ baseDir: getFixtures('apps/controller-app-multi-b') });
+ const app2 = mm.app({ baseDir: getFixtures('apps/controller-app-multi-c') });
+ try {
+ await Promise.all([app1.ready(), app2.ready()]);
const graph1 = GlobalGraph.instanceFor((app1 as any)._teggScopeBag);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const app1 = mm.app({ baseDir: getFixtures('apps/controller-app-multi-b') }); | |
| const app2 = mm.app({ baseDir: getFixtures('apps/controller-app-multi-c') }); | |
| await Promise.all([app1.ready(), app2.ready()]); | |
| try { | |
| const graph1 = GlobalGraph.instanceFor((app1 as any)._teggScopeBag); | |
| const graph2 = GlobalGraph.instanceFor((app2 as any)._teggScopeBag); | |
| assert(graph1); | |
| assert(graph2); | |
| assert.notStrictEqual(graph1, graph2, 'each app must have its own GlobalGraph'); | |
| for (const graph of [graph1!, graph2!]) { | |
| const controllerProto = findControllerProto(graph); | |
| assert(controllerProto, 'controller proto should exist in each app graph'); | |
| const countAdviceProto = findCountAdviceInject(graph, controllerProto!); | |
| assert(countAdviceProto, 'middleware inject edge should be woven in each app graph'); | |
| assert.equal(countAdviceProto!.instanceModuleName, 'multi-module-common'); | |
| } | |
| // The protos behind the edges are per-app instances, not shared. | |
| const proto1 = findCountAdviceInject(graph1!, findControllerProto(graph1!)!); | |
| const proto2 = findCountAdviceInject(graph2!, findControllerProto(graph2!)!); | |
| assert.notStrictEqual(proto1, proto2, 'advice protos must not be shared across apps'); | |
| } finally { | |
| await Promise.all([app1.close(), app2.close()]); | |
| } | |
| const app1 = mm.app({ baseDir: getFixtures('apps/controller-app-multi-b') }); | |
| const app2 = mm.app({ baseDir: getFixtures('apps/controller-app-multi-c') }); | |
| try { | |
| await Promise.all([app1.ready(), app2.ready()]); | |
| const graph1 = GlobalGraph.instanceFor((app1 as any)._teggScopeBag); | |
| const graph2 = GlobalGraph.instanceFor((app2 as any)._teggScopeBag); | |
| assert(graph1); | |
| assert(graph2); | |
| assert.notStrictEqual(graph1, graph2, 'each app must have its own GlobalGraph'); | |
| for (const graph of [graph1!, graph2!]) { | |
| const controllerProto = findControllerProto(graph); | |
| assert(controllerProto, 'controller proto should exist in each app graph'); | |
| const countAdviceProto = findCountAdviceInject(graph, controllerProto!); | |
| assert(countAdviceProto, 'middleware inject edge should be woven in each app graph'); | |
| assert.equal(countAdviceProto!.instanceModuleName, 'multi-module-common'); | |
| } | |
| // The protos behind the edges are per-app instances, not shared. | |
| const proto1 = findCountAdviceInject(graph1!, findControllerProto(graph1!)!); | |
| const proto2 = findCountAdviceInject(graph2!, findControllerProto(graph2!)!); | |
| assert.notStrictEqual(proto1, proto2, 'advice protos must not be shared across apps'); | |
| } finally { | |
| await Promise.all([app1.close(), app2.close()]); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tegg/plugin/controller/test/http/middleware-graph-multi-app.test.ts` around
lines 31 - 55, The test setup in middleware-graph-multi-app.test.ts can leak
app1 if app2.ready() fails before entering the cleanup block. Move the app
creation and Promise.all(app1.ready(), app2.ready()) inside the try so the
finally always closes any app that booted partially. Keep the existing
GlobalGraph.instanceFor, findControllerProto, and findCountAdviceInject
assertions unchanged after the apps are ready.
killagu
reviewed
Jul 4, 2026
| // it is flushed onto the graph right after creation, before build() runs. | ||
| // moduleHandler is created in the tegg plugin's configDidLoad, which runs | ||
| // before ours (teggController declares a dependency on tegg). | ||
| this.app.moduleHandler.registerGlobalGraphBuildHook(middlewareGraphHook); |
Contributor
There was a problem hiding this comment.
都要重构为 GlobalGraph.instanceFor 了，需要支持 multi app 的情况
Contributor
Author
There was a problem hiding this comment.
this.app.moduleHandler 这里提供的方法就是 app 维度的？
就是 GlobalGraph.instanceFor(this.app._teggScopeBag) 是在 didLoad 里 moduleHandler.init() 才有值的，这里永远是 undefined
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
middlewareGraphHookwas registered in the controller plugin'sconfigDidLoadviaGlobalGraph.instanceFor(bag)?.registerBuildHook(...). But the per-appGlobalGraphis only created insidemoduleHandler.init()duringdidLoad, so at that pointinstanceForreturnsundefinedand the optional chain silently no-ops — cross-module controller middleware inject edges were never woven into the graph.This is a migration regression: in the standalone tegg repo the graph was created synchronously in the
EggModuleLoaderconstructor (tegg pluginconfigDidLoad, which runs before the controller plugin's), so registering onGlobalGraph.instance!there worked. The migration moved graph creation into the asyncload()path and changed the non-null assertion into an optional chain, which masked the breakage.Change
Route the registration through
moduleHandler.registerGlobalGraphBuildHook()— hooks buffered there are flushed onto the graph right after creation, beforebuild()runs. This is the same pattern the aop plugin uses for its crosscut/pointcut graph hooks (plugin/aop/src/app.ts).Tests
multi-module-controllerwith a controller using cross-module@Middlewareadvices (CountAdvice/FooMethodAdvicefrommulti-module-common).test/http/middleware-graph.test.tsasserts the inject edges exist in the built graph (fails before the fix — the request-level behavior tests pass either way because middleware resolution is lazy at request time, which is exactly why this regression was silent).@eggjs/controller-plugin+@eggjs/aop-pluginsuites: 16 files / 71 tests green.🤖 Generated with Claude Code
Summary by CodeRabbit