-
Notifications
You must be signed in to change notification settings - Fork 45
Feature: Implementing built-in middleware and middleware loader #17
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
Conversation
oscar60310
left a comment
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.
Other part LGTM.
| }; | ||
| } | ||
| public async handle(context: KoaRouterContext, next: RouteMiddlewareNext) { | ||
| if (!this.enabled) await next(); |
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.
NIT: Maybe we can do an early return here to avoid some codes being executed accidentally.
| if (!this.enabled) await next(); | |
| if (!this.enabled) return next(); |
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.
Thanks for reviewing and suggesting, it has been changed to return 😄 .
| // if header or query location not found request id, set default to uuid | ||
| const requestId = | ||
| (fieldIn === FieldInType.HEADER | ||
| ? (request.header[name] as string) |
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.
I remember node http server “lower casing” the headers, but I'm not sure :D
https://medium.com/@andrelimamail/http-node-server-lower-casing-headers-365764218527
| ? (request.header[name] as string) | |
| ? (request.header[name.toLowerCase()] as string) |
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.
Thanks for reviewing and suggesting, it has been changed to return 😄 .
| : (request.query[name] as string)) || uuid.v4(); | ||
|
|
||
| // keep request id for logger used | ||
| await asyncReqIdStorage.enterWith({ requestId }); |
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.
Nice to set requestId for synchronous executions, but because it is still Experimental feature, can we use .run function instead?
https://nodejs.org/api/async_context.html#asynclocalstorageenterwithstore

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.
Thanks for reviewing and suggesting, it has been changed to return 😄 .
packages/core/src/models/artifact.ts
Outdated
| CURSOR = 'cursor', | ||
| OFFSET = 'offset', | ||
| KEYSET = 'keyset', | ||
| } | ||
|
|
||
| export enum FieldInType { | ||
| QUERY = 'QUERY', | ||
| HEADER = 'HEADER', | ||
| PATH = 'PATH', | ||
| QUERY = 'query', | ||
| HEADER = 'header', | ||
| PATH = 'path', | ||
| } | ||
|
|
||
| export enum FieldDataType { | ||
| BOOLEAN = 'BOOLEAN', | ||
| NUMBER = 'NUMBER', | ||
| STRING = 'STRING', | ||
| BOOLEAN = 'boolean', | ||
| NUMBER = 'number', | ||
| STRING = 'string', |
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.
We've just created some middleware to normalize these values XD
https://github.com/Canner/vulcan/pull/16/files#diff-b597fbd22f8a9d7fcff5148d1a0ab79ba84c58844bd02b00c66d36c99b5b974cR23
I can update my code if you decided to use lower cases.
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.
Thanks for reviewing and pointing, I change back to uppercase, so you don't need to update 😃
| async ({ name, expected }) => { | ||
| // Arrange | ||
| const folderPath = path.resolve(__dirname, 'test-custom-validators'); | ||
| const folderPath = path.join(__dirname, 'test-custom-validators'); |
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.
There was no difference between these functions here, may I ask why we changed it?
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.
Thanks for reviewing and pointing, I just want to use join more easily understand, because resolve is used to like cd, but in this case, join could do the same thing by joining the path, and no need to think the cd path in mind and check.
| this.logger.info(`request: query = ${JSON.stringify(query)}`); | ||
| this.logger.info(`request: params = ${JSON.stringify(params)}.`); | ||
| await next(); | ||
| this.logger.info(`response: body = ${JSON.stringify(response.body)}`); |
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.
NIT: The response body of our API server might be huge, we can let users to set what data they want to record in config in the future.
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.
Thanks for reviewing and suggesting, currently I add the TODO comment to mention me to enhance the part after the feature almost ready in our phase 1 plan 😃
| // export non-default | ||
| export { BaseRouteMiddleware, RouteMiddlewareNext } from './middleware'; | ||
| export * from './loader'; | ||
| export * from './built-in-middlewares'; |
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.
NIT
We used dynamic import in the middleware loader:
export const loadBuiltIn = async () => {
// built-in middleware folder
const builtInFolder = path.join(__dirname, 'built-in-middlewares');
// read built-in middlewares in index.ts, the content is an array middleware class
return (
(await defaultImport<ClassType<BaseRouteMiddleware>[]>(builtInFolder)) || []
);
};It sometimes means we don't want to load the code unit we need them, re-exporting them here causes these codes to be loaded when the application starts, we might remove the export here to fix the issue.
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.
Thanks for reviewing and pointing, and you are right, it will be used if someone calls it when the application has started and use it again, so I move the loadBuiltIn to loader.spec.ts because it was originally used for test only 😃 .
If I misunderstand your point and suggestion, please let me know~
| } | ||
|
|
||
| export default class IPTypeValidator implements IValidator { | ||
| export class IPTypeValidator implements IValidator { |
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.
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.
c064f8c to
cac3583
Compare
|
Hi @oscar60310, I have fixed it, please check it, thank you for your reviewing ~ |
cac3583 to
1d011d6
Compare
49029d1 to
c50a648
Compare
…sts. - add tslog package and create "getLogger" by "LoggerFactory". - refactor "PaginationMode", "FieldInType", "FieldDataType" value to lower case. - fix "transform" method in "PaginationTransformer".
…eading index file through read folder. - add "ModuleLoader" for loader exended. - fix "getLogger" to support update options for existed logger in map. - refactor "DateTypeValidator", "IntegerTypeValidator", "StringTypeValidator", "UUIDTypeValidator" for removing default. - add index.ts in "data-type-validators" for export default. - refactor validator test cases.
- add built-in middlewares: "CORSMiddleware", "RequestIdMiddleware", "AuditLoggingMiddleware" - add middleware loader to load middleware module. - add middleware related test cases.
…eware in server. - refactor "ModuleLoader" class to "defaultImport" function. - rename "Type" to "ClassType". - fix checking options in middleware. - set middleware and add test cases in vulcan app server.
…ructure. - add "BuiltInMiddleware" for built in middleware extend. - support "RateLimitMiddleware". - refactor "MiddlewareConfig" for enabling or disabling middleware.
…ag to prevent detecting class .ts file coverage in test/ - restore upper case for enum type in artifact, including "PaginationMode", "FieldInType", "FieldDataType". - add ignore tag in "IPTypeValidator" class in test/ to prevent detecting unneccesary code.
…e to get request-id, and using run method for async local storage. - make next() return directly when "enabled" is false in middleware. - fix for converting to lowercase to get request-id in "RequestdMiddleware" and setup default value if name or field not existed. - fix to use run method for async local storage to prevent using experimental method. - move the "loadBuiltIn" method to "load.spec.ts" for only test used.
1d011d6 to
4be9a2f
Compare


Description
This PR makes 4 things:
1. Provider logger through
getLoggerUsing the tslog to implement
getLoggermethod and get logger by each package nameLoggingScopeenum. We use thetslogis that the plugin could help us to log the data to different places byattachTransportto implement code easily, so we could integrate tografana, also thetslogcould update setting after the logger generated not likelog4js.2. refactor the validator loader supporting the dynamic module import
After the last meeting suggestion, change the original lookup of each file way to import module (seeking the
index.tsdefault) way and load validators.Here is the built-in validators:
Below is the custom validators by user-defined:
3. add built-in middleware and middleware loader which supports the dynamic module import
Support 3 components in the built-in middleware currently:
koa-cors)koa2-ratelimit)dispayRequestId, it will get the request-id from the above Request Id middleware.Below is the built-in module:
And below is the custom middleware by user-defined:
How To Test / Expected Results
For the test result, please see the below test cases that passed the unit test:
The core package
The serve package
Commit Message
ef09460 - feat(core,serve): add logger, refactor schema type, add pagination tests.
tslogpackage and creategetLoggerbyLoggerFactory.PaginationMode,FieldInType,FieldDataTypevalue to lower case.transformmethod inPaginationTransformer.e81916e - feat(core): refactor validator loader for supporting load module by reading index file through read folder.
ModuleLoaderfor loader extended.getLoggerto support update options for the existing logger in the map.DateTypeValidator,IntegerTypeValidator,StringTypeValidator,UUIDTypeValidatorfor removing default.data-type-validatorsfor export default.096f2a5 - feat(serve): add built-in middleware, and loader
CORSMiddleware,RequestIdMiddleware,AuditLoggingMiddleware7951238 - feat(core, serve): refactor validator and middleware loader, add middleware in server.
ModuleLoaderclass todefaultImportfunction.ClassType.b1e883b - feat(serve): add rate limit middleware, refactor middleware config structure.
BuiltInMiddlewarefor built in middleware extend.RateLimitMiddleware.MiddlewareConfigfor enabling or disabling middleware.70c1b13 - fix(core): restore upper case for enum type in the artifact, add ignore tag to prevent detecting class .ts file coverage in test/
PaginationMode,FieldInType,FieldDataType.IPTypeValidatorclass in test/ to prevent detecting unnecessary code.1d011d6 - fix(serve): fix for returning next() directly, converting to lowercase to get request-id, and using the run method for async local storage.
enabledis false in middleware.RequestdMiddlewareand setup default value if name or field does not exist.loadBuiltInmethod toloader.spec.tsfor only the test used.