-
Notifications
You must be signed in to change notification settings - Fork 45
feat: add MiddlewareGraphHook to handle controller middleware depende… #361
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| import { GraphNode } from '@eggjs/tegg-common-util'; | ||
| import { | ||
| ClassProtoDescriptor, | ||
| GlobalGraph, | ||
| ProtoDependencyMeta, | ||
| ProtoNode, | ||
| } from '@eggjs/tegg-metadata'; | ||
| import { EggProtoImplClass, IAdvice } from '@eggjs/tegg-types'; | ||
| import { ControllerInfoUtil, MethodInfoUtil } from '@eggjs/tegg'; | ||
|
|
||
| export function middlewareGraphHook(globalGraph: GlobalGraph) { | ||
| for (const moduleNode of globalGraph.moduleGraph.nodes.values()) { | ||
| for (const controllerProtoNode of moduleNode.val.protos) { | ||
| const middlewareProtoNodes = findMiddlewareProtoNodes(globalGraph, controllerProtoNode); | ||
| if (!middlewareProtoNodes) continue; | ||
| for (const middlewareProtoNode of middlewareProtoNodes) { | ||
| const middlewareModuleNode = globalGraph.findModuleNode(middlewareProtoNode.val.proto.instanceModuleName); | ||
| if (!middlewareModuleNode) continue; | ||
| globalGraph.addInject( | ||
| moduleNode, | ||
| controllerProtoNode, | ||
| middlewareProtoNode, | ||
| middlewareProtoNode.val.proto.name); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| 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; | ||
| } | ||
|
|
||
| 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; | ||
| } | ||
|
Comment on lines
+72
to
+83
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The To optimize this, consider creating a |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| export default () => { | ||
| // This file is required by egg | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| 'use strict'; | ||
|
|
||
| module.exports = function() { | ||
| const config = { | ||
| keys: 'test key', | ||
| security: { | ||
| csrf: { | ||
| ignoreJSON: false, | ||
| }, | ||
| }, | ||
| }; | ||
| return config; | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| [ | ||
| { | ||
| "path": "../modules/advice-module" | ||
| }, | ||
| { | ||
| "path": "../modules/controller-module" | ||
| } | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| 'use strict'; | ||
|
|
||
| exports.tracer = { | ||
| package: 'egg-tracer', | ||
| enable: true, | ||
| }; | ||
|
|
||
| exports.tegg = { | ||
| package: '@eggjs/tegg-plugin', | ||
| enable: true, | ||
| }; | ||
|
|
||
| exports.teggConfig = { | ||
| package: '@eggjs/tegg-config', | ||
| enable: true, | ||
| }; | ||
|
|
||
| exports.aopModule = { | ||
| package: '@eggjs/tegg-aop-plugin', | ||
| enable: true, | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import { | ||
| Advice, | ||
| AdviceContext, | ||
| IAdvice, | ||
| } from '@eggjs/tegg/aop'; | ||
| import { AccessLevel } from '@eggjs/tegg-types'; | ||
|
|
||
| @Advice({ | ||
| accessLevel: AccessLevel.PUBLIC, | ||
| }) | ||
| export class AnotherAdvice implements IAdvice { | ||
| async around(_ctx: AdviceContext, next: () => Promise<any>) { | ||
| const body = await next(); | ||
| if (body) { | ||
| body.anotherAdviceApplied = true; | ||
| } | ||
| return body; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import { | ||
| Advice, | ||
| AdviceContext, | ||
| IAdvice, | ||
| } from '@eggjs/tegg/aop'; | ||
| import { AccessLevel } from '@eggjs/tegg-types'; | ||
|
|
||
| @Advice({ | ||
| accessLevel: AccessLevel.PUBLIC, | ||
| }) | ||
| export class TestAdvice implements IAdvice { | ||
| async around(_ctx: AdviceContext, next: () => Promise<any>) { | ||
| const body = await next(); | ||
| if (body) { | ||
| body.adviceApplied = true; | ||
| } | ||
| return body; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "name": "advice-module", | ||
| "eggModule": { | ||
| "name": "advice-module" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| import { | ||
| HTTPController, | ||
| HTTPMethod, | ||
| HTTPMethodEnum, | ||
| Middleware, | ||
| } from '@eggjs/tegg'; | ||
| import { TestAdvice } from '../advice-module/advice/TestAdvice'; | ||
| import { AnotherAdvice } from '../advice-module/advice/AnotherAdvice'; | ||
|
|
||
| @HTTPController({ | ||
| path: '/test', | ||
| }) | ||
| @Middleware(TestAdvice) | ||
| export class TestController { | ||
| @HTTPMethod({ | ||
| method: HTTPMethodEnum.GET, | ||
| path: '/class-middleware', | ||
| }) | ||
| async classMiddleware() { | ||
| return { | ||
| message: 'class middleware', | ||
| }; | ||
| } | ||
|
|
||
| @HTTPMethod({ | ||
| method: HTTPMethodEnum.GET, | ||
| path: '/method-middleware', | ||
| }) | ||
| @Middleware(AnotherAdvice) | ||
| async methodMiddleware() { | ||
| return { | ||
| message: 'method middleware', | ||
| }; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "name": "controller-module", | ||
| "eggModule": { | ||
| "name": "controller-module" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| { | ||
| "name": "middleware-graph-app" | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,61 @@ | ||||||
| import mm from 'egg-mock'; | ||||||
| import path from 'path'; | ||||||
| import assert from 'assert'; | ||||||
|
|
||||||
| describe('plugin/controller/test/http/middleware-graph-hook.test.ts', () => { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Suggested change
|
||||||
| let app; | ||||||
|
|
||||||
| beforeEach(() => { | ||||||
| mm(process.env, 'EGG_TYPESCRIPT', true); | ||||||
| }); | ||||||
|
Comment on lines
+8
to
+10
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
|
|
||||||
| afterEach(() => { | ||||||
| mm.restore(); | ||||||
| }); | ||||||
|
|
||||||
| before(async () => { | ||||||
| mm(process.env, 'EGG_TYPESCRIPT', true); | ||||||
| mm(process, 'cwd', () => { | ||||||
| return path.join(__dirname, '../..'); | ||||||
| }); | ||||||
| app = mm.app({ | ||||||
| baseDir: path.join(__dirname, '../fixtures/apps/middleware-graph-app'), | ||||||
| framework: require.resolve('egg'), | ||||||
| }); | ||||||
| await app.ready(); | ||||||
| }); | ||||||
|
|
||||||
| after(() => { | ||||||
| return app.close(); | ||||||
| }); | ||||||
|
|
||||||
| it('should add module reference for class level middleware', async () => { | ||||||
| assert.deepStrictEqual(app.moduleReferences.map(t => t.name), [ | ||||||
| 'advice-module', | ||||||
| 'controller-module', | ||||||
| ]); | ||||||
|
killagu marked this conversation as resolved.
|
||||||
| }); | ||||||
|
|
||||||
| it('class level middleware should work', async () => { | ||||||
| app.mockCsrf(); | ||||||
| const res = await app.httpRequest() | ||||||
| .get('/test/class-middleware') | ||||||
| .expect(200); | ||||||
| assert.deepStrictEqual(res.body, { | ||||||
| message: 'class middleware', | ||||||
| adviceApplied: true, | ||||||
| }); | ||||||
| }); | ||||||
|
|
||||||
| it('method level middleware should work', async () => { | ||||||
| app.mockCsrf(); | ||||||
| const res = await app.httpRequest() | ||||||
| .get('/test/method-middleware') | ||||||
| .expect(200); | ||||||
| assert.deepStrictEqual(res.body, { | ||||||
| message: 'method middleware', | ||||||
| adviceApplied: true, | ||||||
| anotherAdviceApplied: true, | ||||||
| }); | ||||||
| }); | ||||||
| }); | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function
findMiddlewareProtoNodeshas a few areas for improvement: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.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.