feat: add MiddlewareGraphHook to handle controller middleware depende… #361
Conversation
…ncies - Add MiddlewareGraphHook to build injection relationships for controller middlewares - Support both class-level and method-level @Middleware annotations - Add test fixture app to verify middleware module references - Export ControllerInfoUtil and MethodInfoUtil from @eggjs/tegg 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a public re-export MethodInfoUtil; registers a middlewareGraphHook with GlobalGraph during configDidLoad; implements middlewareGraphHook to inject middleware proto nodes from AOP metadata into the global graph; and adds test fixtures and tests exercising class- and method-level middleware AOP behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Egg App
participant Hook as ControllerAppBootHook
participant GG as GlobalGraph
participant MGH as middlewareGraphHook
participant ModuleNodes as Module/Controller ProtoNodes
participant AOP as AOP Metadata
App->>Hook: configDidLoad()
Hook->>GG: registerBuildHook(middlewareGraphHook)
Note right of GG `#f0f4c3`: middlewareGraphHook registered
GG->>MGH: invoke during graph build
MGH->>ModuleNodes: iterate module & controller proto nodes
loop per controller
MGH->>AOP: read controller/method AOP metadata
AOP-->>MGH: middleware classes list
MGH->>ModuleNodes: resolve middleware proto nodes by class
ModuleNodes-->>MGH: proto node(s) or undefined
alt found
MGH->>GG: addInject(middleware proto node)
end
end
Note right of GG `#e8f5e9`: middleware proto nodes injected into graph
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (1)
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 |
Summary of ChangesHello @killagu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves how controller middlewares are handled within the application's dependency injection system. By introducing a dedicated graph hook, it ensures that middlewares, whether defined at the class or method level, are correctly integrated and their dependencies are resolved, leading to a more robust and predictable middleware execution flow. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a MiddlewareGraphHook to automatically manage dependencies for controller middlewares, which is a great enhancement for the framework. The implementation correctly handles both class-level and method-level middlewares, and the addition of a comprehensive test suite is commendable. I've identified a potential performance issue in the middleware discovery logic and a flaky test case. My review includes suggestions to optimize the code and improve test reliability.
| function findMiddlewareProtoNodes(globalGraph: GlobalGraph, protoNode: GraphNode<ProtoNode, ProtoDependencyMeta>) { | ||
| const proto = protoNode.val.proto; | ||
| if (!ClassProtoDescriptor.isClassProtoDescriptor(proto)) { | ||
| return; | ||
| } | ||
|
|
||
| const middlewareClazzSet = new Set<EggProtoImplClass<IAdvice>>(); | ||
|
|
||
| // Get AOP middlewares from controller class | ||
| const controllerAopMiddlewares = ControllerInfoUtil.getControllerAopMiddlewares(proto.clazz); | ||
| if (controllerAopMiddlewares && controllerAopMiddlewares.length > 0) { | ||
| for (const middlewareClazz of controllerAopMiddlewares) { | ||
| middlewareClazzSet.add(middlewareClazz); | ||
| } | ||
| } | ||
|
|
||
| // Get AOP middlewares from controller methods | ||
| const methods = MethodInfoUtil.getMethods(proto.clazz); | ||
| for (const methodName of methods) { | ||
| const methodAopMiddlewares = MethodInfoUtil.getMethodAopMiddlewares(proto.clazz, methodName); | ||
| if (methodAopMiddlewares && methodAopMiddlewares.length > 0) { | ||
| for (const middlewareClazz of methodAopMiddlewares) { | ||
| middlewareClazzSet.add(middlewareClazz); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (middlewareClazzSet.size === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const result: GraphNode<ProtoNode, ProtoDependencyMeta>[] = []; | ||
| for (const middlewareClazz of middlewareClazzSet) { | ||
| // Find the proto node for this middleware class | ||
| const middlewareProtoNode = findProtoNodeByClass(globalGraph, middlewareClazz); | ||
| if (middlewareProtoNode) { | ||
| result.push(middlewareProtoNode); | ||
| } | ||
| } | ||
|
|
||
| return result.length > 0 ? result : undefined; | ||
| } |
There was a problem hiding this comment.
The function findMiddlewareProtoNodes has a few areas for improvement:
- Inconsistent Return Type: It can return
undefinedor an array. It would be cleaner to always return an array (GraphNode[]), which would be empty if no middlewares are found. This simplifies the calling code by removing the need for a null check. - Redundant Checks: The
if (array && array.length > 0)pattern is partially redundant, as afor...ofloop won't execute on an empty array. This can be simplified.
I've provided a suggestion that addresses these points to make the function more robust and easier to read.
function findMiddlewareProtoNodes(globalGraph: GlobalGraph, protoNode: GraphNode<ProtoNode, ProtoDependencyMeta>): GraphNode<ProtoNode, ProtoDependencyMeta>[] {
const proto = protoNode.val.proto;
if (!ClassProtoDescriptor.isClassProtoDescriptor(proto)) {
return [];
}
const middlewareClazzSet = new Set<EggProtoImplClass<IAdvice>>();
// Get AOP middlewares from controller class
const controllerAopMiddlewares = ControllerInfoUtil.getControllerAopMiddlewares(proto.clazz);
if (controllerAopMiddlewares) {
for (const middlewareClazz of controllerAopMiddlewares) {
middlewareClazzSet.add(middlewareClazz);
}
}
// Get AOP middlewares from controller methods
const methods = MethodInfoUtil.getMethods(proto.clazz);
for (const methodName of methods) {
const methodAopMiddlewares = MethodInfoUtil.getMethodAopMiddlewares(proto.clazz, methodName);
if (methodAopMiddlewares) {
for (const middlewareClazz of methodAopMiddlewares) {
middlewareClazzSet.add(middlewareClazz);
}
}
}
if (middlewareClazzSet.size === 0) {
return [];
}
const result: GraphNode<ProtoNode, ProtoDependencyMeta>[] = [];
for (const middlewareClazz of middlewareClazzSet) {
// Find the proto node for this middleware class
const middlewareProtoNode = findProtoNodeByClass(globalGraph, middlewareClazz);
if (middlewareProtoNode) {
result.push(middlewareProtoNode);
}
}
return result;
}| function findProtoNodeByClass( | ||
| globalGraph: GlobalGraph, | ||
| clazz: EggProtoImplClass, | ||
| ): GraphNode<ProtoNode, ProtoDependencyMeta> | undefined { | ||
| for (const protoNode of globalGraph.protoGraph.nodes.values()) { | ||
| const proto = protoNode.val.proto; | ||
| if (ClassProtoDescriptor.isClassProtoDescriptor(proto) && proto.clazz === clazz) { | ||
| return protoNode; | ||
| } | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
The findProtoNodeByClass function iterates over all prototypes in globalGraph.protoGraph for each middleware it needs to find. This can cause performance degradation in larger applications with many prototypes, as it results in a complexity of O(N) for each lookup.
To optimize this, consider creating a Map<EggProtoImplClass, GraphNode<ProtoNode, ProtoDependencyMeta>> of all prototypes once at the beginning of the middlewareGraphHook function. This would allow findProtoNodeByClass to perform an O(1) lookup, significantly improving performance.
| import path from 'path'; | ||
| import assert from 'assert'; | ||
|
|
||
| describe('plugin/controller/test/http/middleware-graph-hook.test.ts', () => { |
There was a problem hiding this comment.
The describe block name plugin/controller/test/http/middleware-graph-hook.test.ts is inconsistent with the test file name middleware-aop.test.ts. To avoid confusion, it's best to keep these consistent. Consider renaming the describe block to better reflect the file's name or the feature being tested.
| describe('plugin/controller/test/http/middleware-graph-hook.test.ts', () => { | |
| describe('plugin/controller/test/http/middleware-aop.test.ts', () => { |
| beforeEach(() => { | ||
| mm(process.env, 'EGG_TYPESCRIPT', true); | ||
| }); |
There was a problem hiding this comment.
The beforeEach hook sets EGG_TYPESCRIPT to true, but this is already done in the before hook. This makes the setup in beforeEach redundant. You can safely remove this beforeEach block.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
plugin/controller/app.ts (1)
4-4: Guard assumptions aroundGlobalGraph.instanceinconfigDidLoadRegistering the
middlewareGraphHookinconfigDidLoadis the right place conceptually, but usingGlobalGraph.instance!hard-codes the assumption that the global graph has already been created by the time this hook runs. If that ordering ever changes (e.g., plugin load order tweaks or reuse in a different context), this will throw at startup.Consider either:
- Adding a defensive check with a clear error message if
GlobalGraph.instanceis undefined, or- Documenting/centralizing the initialization order so this assumption is explicit and test-covered.
Also applies to: 15-15, 140-142
plugin/controller/test/http/middleware-aop.test.ts (1)
5-30: Tighten test bootstrapping to avoid redundant mocksThe test logic and expectations look solid, but the lifecycle can be simplified:
EGG_TYPESCRIPTis mocked in bothbeforeEachandbefore, whilemm.restore()inafterEachresets all mocks. In practice, only one of these is needed; keeping it inbefore(before app creation) is usually sufficient.- The
describetitle referencesmiddleware-graph-hook.test.tswhile the file name ismiddleware-aop.test.ts, which can be slightly confusing when scanning test output.Consider removing the redundant
beforeEachmock and aligning thedescribestring with the filename.plugin/controller/lib/MiddlewareGraphHook.ts (1)
11-83: Middleware graph hook is conceptually correct; consider edge cases and minor refinementsThe hook correctly discovers AOP middlewares from controller classes/methods and wires them into the graph, and the use of a
Setavoids duplicate edges per controller–middleware pair. A few nuances worth calling out:
- Module dependency cycles: Because
addInjectalso adds module-level edges, a controller in module A depending on middleware in module B, where B already depends on A (via regular injects), will now form a cycle and can causemoduleGraph.loopPath()to fail the build. If cross-module advice depends back on controllers/services, this may surface as new “recursive deps” errors. It’s worth confirming whether such patterns are expected and, if so, whether middleware edges should perhaps be treated differently (e.g., separate edge type or optional in module topological sort).- Proto selection by class only:
findProtoNodeByClasspicks the first proto whoseclazzmatches. In a future where advice protos might be multi-instance or qualified, this could become ambiguous. If you ever introduce multiple protos for the same advice class, you may want a more specific selection strategy (e.g., by qualifiers or module).- Minor cleanup: The pre-check
const middlewareModuleNode = globalGraph.findModuleNode(...)is only used as a presence guard;addInjectre-resolves the module internally. This is fine for now, but if you care about perf in large graphs, you could either:
- Let
addInjecthandle the missing-module case entirely, or- Extend
addInject’s API in the future to reuse the resolved module node.Functionally this looks good for the scenarios covered by the new tests; the above are mostly robustness concerns for more complex module graphs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
core/controller-decorator/index.ts(1 hunks)plugin/controller/app.ts(3 hunks)plugin/controller/lib/MiddlewareGraphHook.ts(1 hunks)plugin/controller/test/fixtures/apps/middleware-graph-app/app/router.ts(1 hunks)plugin/controller/test/fixtures/apps/middleware-graph-app/config/config.default.js(1 hunks)plugin/controller/test/fixtures/apps/middleware-graph-app/config/module.json(1 hunks)plugin/controller/test/fixtures/apps/middleware-graph-app/config/plugin.js(1 hunks)plugin/controller/test/fixtures/apps/middleware-graph-app/modules/advice-module/advice/AnotherAdvice.ts(1 hunks)plugin/controller/test/fixtures/apps/middleware-graph-app/modules/advice-module/advice/TestAdvice.ts(1 hunks)plugin/controller/test/fixtures/apps/middleware-graph-app/modules/advice-module/package.json(1 hunks)plugin/controller/test/fixtures/apps/middleware-graph-app/modules/controller-module/TestController.ts(1 hunks)plugin/controller/test/fixtures/apps/middleware-graph-app/modules/controller-module/package.json(1 hunks)plugin/controller/test/fixtures/apps/middleware-graph-app/package.json(1 hunks)plugin/controller/test/http/middleware-aop.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
plugin/controller/test/fixtures/apps/middleware-graph-app/modules/controller-module/TestController.ts (1)
core/controller-decorator/src/decorator/http/HTTPMethod.ts (1)
HTTPMethod(7-23)
plugin/controller/app.ts (2)
core/metadata/src/model/graph/GlobalGraph.ts (1)
GlobalGraph(38-272)plugin/controller/lib/MiddlewareGraphHook.ts (1)
middlewareGraphHook(11-27)
plugin/controller/test/fixtures/apps/middleware-graph-app/modules/advice-module/advice/AnotherAdvice.ts (1)
plugin/controller/test/fixtures/apps/middleware-graph-app/modules/advice-module/advice/TestAdvice.ts (1)
Advice(8-19)
plugin/controller/test/fixtures/apps/middleware-graph-app/modules/advice-module/advice/TestAdvice.ts (1)
plugin/controller/test/fixtures/apps/middleware-graph-app/modules/advice-module/advice/AnotherAdvice.ts (1)
Advice(8-19)
plugin/controller/lib/MiddlewareGraphHook.ts (6)
core/metadata/src/model/graph/GlobalGraph.ts (2)
GlobalGraph(38-272)proto(143-159)core/common-util/src/Graph.ts (1)
GraphNode(10-54)core/metadata/src/model/ProtoDescriptor/ClassProtoDescriptor.ts (1)
ClassProtoDescriptor(10-38)core/types/core-decorator/model/EggPrototypeInfo.ts (1)
EggProtoImplClass(5-5)core/controller-decorator/src/util/ControllerInfoUtil.ts (1)
ControllerInfoUtil(14-76)core/controller-decorator/src/util/MethodInfoUtil.ts (1)
MethodInfoUtil(24-116)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (typescript)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (16)
🔇 Additional comments (13)
plugin/controller/test/fixtures/apps/middleware-graph-app/package.json (1)
1-3: LGTM! Minimal package.json appropriate for test fixture.The minimal manifest correctly identifies the test fixture app.
core/controller-decorator/index.ts (1)
24-24: LGTM! Export follows established pattern.The new export of MethodInfoUtil is consistent with the existing ControllerInfoUtil export on Line 23.
plugin/controller/test/fixtures/apps/middleware-graph-app/config/config.default.js (1)
1-13: LGTM! Standard Egg.js configuration for test fixture.The configuration structure is correct and appropriate for a test environment.
plugin/controller/test/fixtures/apps/middleware-graph-app/modules/controller-module/package.json (1)
1-6: LGTM! Valid Egg module manifest.The module configuration correctly identifies the controller-module for the test fixture.
plugin/controller/test/fixtures/apps/middleware-graph-app/app/router.ts (1)
1-3: LGTM! Valid placeholder router.The empty router file correctly satisfies Egg.js requirements for the test fixture.
plugin/controller/test/fixtures/apps/middleware-graph-app/modules/advice-module/package.json (1)
1-6: LGTM! Valid Egg module manifest.The module configuration correctly identifies the advice-module for the test fixture.
plugin/controller/test/fixtures/apps/middleware-graph-app/config/module.json (1)
1-8: LGTM! Module registry correctly configured.The module paths correctly reference both the advice-module and controller-module for the middleware graph test fixture.
plugin/controller/test/fixtures/apps/middleware-graph-app/modules/advice-module/advice/AnotherAdvice.ts (2)
1-6: LGTM! Imports are correct.All necessary imports for the AOP advice implementation are present.
8-18: LGTM! Advice implementation follows established pattern.The implementation correctly wraps the next() call and mutates the response body, matching the pattern used in TestAdvice. The unused
_ctxparameter is properly prefixed with an underscore.plugin/controller/test/fixtures/apps/middleware-graph-app/config/plugin.js (1)
1-21: Fixture plugin config looks consistentThe four plugin entries are well-structured and match the expected Egg/tegg plugin wiring pattern for this test fixture; no issues from a test-config perspective.
plugin/controller/test/fixtures/apps/middleware-graph-app/modules/controller-module/TestController.ts (1)
1-35: Controller and middleware wiring aligns with test scenariosThe HTTPController, HTTPMethod, and Middleware decorations are consistent with the tests: class-level middleware on
/class-middlewareand additional method-level middleware on/method-middleware. Import paths also look correct relative to the advice module layout.plugin/controller/test/fixtures/apps/middleware-graph-app/modules/advice-module/advice/TestAdvice.ts (2)
1-6: LGTM: Imports are correct.All imports are necessary and properly sourced from the @eggjs/tegg packages.
8-19: LGTM: Clean advice implementation for testing.The implementation correctly follows the
IAdviceinterface and mirrors the pattern established inAnotherAdvice.ts. The body null-check prevents mutation errors, and the distinct property name (adviceApplied) enables test verification of class-level middleware application.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
plugin/controller/test/mcp/mcpCluster.test.ts (1)
41-49:as anyin notification parsing reduces type-safety but is acceptable in testsCasting
itemtoanyis a pragmatic way to placate the type system here, but it hides real shape mismatches. If you want to keep some safety, consider narrowing on the concrete non‑text variant (e.g., by checking a specificitem.typethat is known to carrydata) or asserting just thatitemhas adatafield instead of the full object cast.plugin/controller/test/mcp/mcp.test.ts (1)
41-49: Sameas anypattern as cluster tests; consider a narrower assertionThis mirrors the cluster test helper and keeps runtime behavior, but using
(item as any).datadrops type guarantees. If you later extend MCP content types, a more explicit branch (e.g., checking for a known binary type or'data' in item) would make failures easier to catch while preserving most of this convenience.plugin/controller/test/mcp/helper.test.ts (1)
108-111: Omittingtoolscapability relies on SDK defaults; consider making intent explicitCommenting out
tools: {}means this test now depends on the MCP client’s default capabilities behavior. That’s probably fine given the current SDK, but it’s a bit implicit: if the SDK later requires explicit tool capabilities, this test will start failing in a non-obvious way. A brief comment explaining whytoolsis intentionally omitted, or an assertion around the negotiated capabilities, would make this more robust.If you haven’t already, please double‑check the current MCP client docs to confirm that omitting
capabilities.toolsis the recommended pattern and won’t disable tool calls in future versions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugin/controller/test/mcp/helper.test.ts(1 hunks)plugin/controller/test/mcp/mcp.test.ts(1 hunks)plugin/controller/test/mcp/mcpCluster.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (typescript)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (18)
…ncies
🤖 Generated with Claude Code
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
New Features
Tests
Chores