refactor and add test cases

onlyjackfrost · onlyjackfrost · commit cf2b6ffa11cc · 2023-09-11T14:46:37.000+08:00
diff --git a/packages/core/src/lib/cache-layer/cacheLayerLoader.ts b/packages/core/src/lib/cache-layer/cacheLayerLoader.ts
@@ -4,7 +4,6 @@ import * as moment from 'moment';
 import { inject, injectable, interfaces } from 'inversify';
 import { TYPES } from '@vulcan-sql/core/types';
 import {
-  IActivityLogger,
   CacheLayerInfo,
   ICacheLayerOptions,
   cacheProfileName,
diff --git a/packages/core/src/lib/cache-layer/cacheLayerRefresher.ts b/packages/core/src/lib/cache-layer/cacheLayerRefresher.ts
@@ -3,7 +3,11 @@ import { uniq } from 'lodash';
 import { ToadScheduler, SimpleIntervalJob, AsyncTask } from 'toad-scheduler';
 import { inject, injectable, multiInject } from 'inversify';
 import { TYPES } from '@vulcan-sql/core/types';
-import { APISchema, IActivityLogger } from '@vulcan-sql/core/models';
+import {
+  APISchema,
+  CacheLayerInfo,
+  IActivityLogger,
+} from '@vulcan-sql/core/models';
 import { ConfigurationError } from '../utils/errors';
 import { ICacheLayerLoader } from './cacheLayerLoader';
 import { getLogger } from '../utils';
@@ -49,15 +53,13 @@ export class CacheLayerRefresher implements ICacheLayerRefresher {
     // check if the index name is duplicated more than one API schemas
     this.checkDuplicateIndex(schemas);
     // traverse each cache table of each schema
-    const activityLogger = this.getActivityLogger();
     await Promise.all(
       schemas.map(async (schema) => {
         // skip the schema by return if not set the cache
         if (!schema.cache) return;
-        const { urlPath } = schema;
         return await Promise.all(
           schema.cache.map(async (cache) => {
-            const { cacheTableName, profile, refreshTime, sql } = cache;
+            const { cacheTableName, profile, refreshTime } = cache;
             // replace the '/' tp '_' to avoid the file path issue for templateSource
             const templateName = schema.templateSource.replace('/', '_');
             // If refresh time is set, use the scheduler to schedule the load task for refresh
@@ -68,59 +70,14 @@ export class CacheLayerRefresher implements ICacheLayerRefresher {
               const refreshJob = new SimpleIntervalJob(
                 { milliseconds, runImmediately },
                 new AsyncTask(workerId, async () => {
-                  // load data the to cache storage
-                  let refreshResult = RefreshResult.SUCCESS;
-                  const now = moment.utc().format('YYYY-MM-DD HH:mm:ss');
-                  try {
-                    // get the current time in format of UTC
-                    await this.cacheLoader.load(templateName, cache);
-                  } catch (error: any) {
-                    refreshResult = RefreshResult.FAILED;
-                    this.logger.debug(`Failed to refresh cache: ${error}`);
-                  } finally {
-                    // send activity log
-                    const content = {
-                      logTime: now,
-                      urlPath,
-                      sql,
-                      refreshResult,
-                    };
-                    if (activityLogger)
-                      activityLogger.log(content).catch((err: any) => {
-                        this.logger.debug(
-                          `Failed to log activity after refreshing cache: ${err}`
-                        );
-                      });
-                  }
+                  await this.sendActivityLogAfterLoad(schema, cache);
                 }),
                 { preventOverrun: true, id: workerId }
               );
               // add the job to schedule cache refresh task
               this.scheduler.addIntervalJob(refreshJob);
             } else {
-              let refreshResult = RefreshResult.SUCCESS;
-              const now = moment.utc().format('YYYY-MM-DD HH:mm:ss');
-              try {
-                // get the current time in format of UTC
-                await this.cacheLoader.load(templateName, cache);
-              } catch (error: any) {
-                refreshResult = RefreshResult.FAILED;
-                this.logger.debug(`Failed to refresh cache: ${error}`);
-              } finally {
-                // send activity log
-                const content = {
-                  logTime: now,
-                  urlPath,
-                  sql,
-                  refreshResult,
-                };
-                if (activityLogger)
-                  activityLogger.log(content).catch((err: any) => {
-                    this.logger.debug(
-                      `Failed to log activity after refreshing cache: ${err}`
-                    );
-                  });
-              }
+              await this.sendActivityLogAfterLoad(schema, cache);
             }
           })
         );
@@ -135,12 +92,42 @@ export class CacheLayerRefresher implements ICacheLayerRefresher {
     this.scheduler.stop();
   }
 
-  private getActivityLogger(): IActivityLogger | undefined {
-    const activityLogger = this.activityLoggers.find((logger) =>
-      logger.isEnabled()
-    );
+  private async sendActivityLogAfterLoad(
+    schema: APISchema,
+    cache: CacheLayerInfo
+  ) {
+    const { urlPath } = schema;
+    const { sql } = cache;
+    // if fn is not a function, return
+    let refreshResult = RefreshResult.SUCCESS;
+    const now = moment.utc().format('YYYY-MM-DD HH:mm:ss');
+    const templateName = schema.templateSource.replace('/', '_');
+    try {
+      // get the current time in format of UTC
+      await this.cacheLoader.load(templateName, cache);
+    } catch (error: any) {
+      refreshResult = RefreshResult.FAILED;
+      this.logger.debug(`Failed to refresh cache: ${error}`);
+    } finally {
+      // send activity log
+      const content = {
+        logTime: now,
+        urlPath,
+        sql,
+        refreshResult,
+      };
+      const activityLoggers = this.getActivityLoggers();
+      for (const activityLogger of activityLoggers)
+        activityLogger.log(content).catch((err: any) => {
+          this.logger.debug(
+            `Failed to log activity after refreshing cache: ${err}`
+          );
+        });
+    }
+  }
 
-    return activityLogger;
+  private getActivityLoggers(): IActivityLogger[] {
+    return this.activityLoggers.filter((logger) => logger.isEnabled());
   }
 
   private checkDuplicateCacheTableName(schemas: APISchema[]) {
diff --git a/packages/core/src/lib/loggers/httpLogger.ts b/packages/core/src/lib/loggers/httpLogger.ts
@@ -33,6 +33,7 @@ export class HttpLogger extends BaseActivityLogger<HttpLoggerConfig> {
     try {
       // get connection info from option and use axios to send a post requet to the endpoint
       await this.sendActivityLog(url, payload, headers);
+      this.logger.debug(`Activity log sent`);
     } catch (err) {
       this.logger.debug(
         `Failed to send activity log to http logger, url: ${url}`
@@ -43,7 +44,7 @@ export class HttpLogger extends BaseActivityLogger<HttpLoggerConfig> {
 
   protected sendActivityLog = async (
     url: string,
-    payload: JSON,
+    payload: any,
     headers: AxiosRequestHeaders | undefined
   ): Promise<void> => {
     await axios.post(url, payload, {
diff --git a/packages/core/src/models/extensions/logger.ts b/packages/core/src/models/extensions/logger.ts
@@ -1,6 +1,7 @@
 import { ExtensionBase } from './base';
 import { TYPES } from '@vulcan-sql/core/types';
 import { VulcanExtension } from './decorators';
+import { isEmpty } from 'lodash';
 
 export enum ActivityLoggerType {
   HTTP_LOGGER = 'http-logger',
@@ -20,8 +21,10 @@ export abstract class BaseActivityLogger<ActivityLoggerTypeOption>
 
   public isEnabled(): boolean {
     const config = this.getConfig();
-    if (!config) return false;
-    if (config.enabled === true) return true;
+    if (!config || isEmpty(config)) return false;
+    if (!config.enabled) return false;
+    if (!config['options']) return false;
+    if (config['options'][this.getExtensionId()!]) return true;
     else return false;
   }
 
diff --git a/packages/core/test/cache-layer/cacheLayerRefresher.spec.ts b/packages/core/test/cache-layer/cacheLayerRefresher.spec.ts
@@ -423,4 +423,53 @@ describe('Test cache layer refresher', () => {
     },
     100 * 1000
   );
+  // should not send activity log when logger is not enabled
+  it('should not send activity log when logger is not enabled', async () => {
+    const schemas: Array<APISchema> = [
+      {
+        ...sinon.stubInterface<APISchema>(),
+        templateSource: 'template-1',
+        profiles: [profiles[0].name, profiles[1].name],
+        cache: [
+          {
+            cacheTableName: 'orders',
+            sql: sinon.default.stub() as any,
+            profile: profiles[0].name,
+          },
+          {
+            cacheTableName: 'products',
+            sql: sinon.default.stub() as any,
+            profile: profiles[1].name,
+          },
+        ] as Array<CacheLayerInfo>,
+      },
+      {
+        ...sinon.stubInterface<APISchema>(),
+        templateSource: 'template-2',
+        profiles: [profiles[2].name],
+        cache: [
+          {
+            cacheTableName: 'users',
+            sql: sinon.default.stub() as any,
+            profile: profiles[2].name,
+          },
+        ] as Array<CacheLayerInfo>,
+      },
+    ];
+    const mockLogger = new HttpLogger(
+      {
+        enabled: false,
+      },
+      'http-logger'
+    );
+    mockLogger.isEnabled = jest.fn().mockReturnValue(false);
+    // Act
+    const loader = new CacheLayerLoader(options, stubFactory as any);
+    const refresher = new CacheLayerRefresher(loader, [mockLogger]);
+    await refresher.start(schemas);
+
+    // Assert
+    expect(mockLogger.log).toHaveBeenCalledTimes(0);
+    refresher.stop();
+  });
 });
diff --git a/packages/core/test/httplogger.spec.ts b/packages/core/test/httplogger.spec.ts
@@ -74,4 +74,30 @@ describe('Activity logs', () => {
     sinon.stub(httpLogger, 'sendActivityLog').throws();
     await expect(httpLogger.log({})).rejects.toThrow();
   });
+
+  // isEnabled should return false when logger is disabled
+  it.each([
+    {}, // empty config
+    {
+      enabled: false, // not enabled
+    },
+    {
+      enabled: false, // not enabled but has logger
+      options: {
+        'http-logger': { connection: { host: 'localhost', port: 80 } },
+      },
+    },
+    {
+      enabled: true, // enabled but do not have http-logger config
+      options: {
+        'non-http-logger': {},
+      },
+    },
+  ])(
+    'isEnabled should return false when logger is disabled',
+    async (config) => {
+      const httpLogger = createMockHttpLogger(config);
+      expect(httpLogger.isEnabled()).toBe(false);
+    }
+  );
 });
diff --git a/packages/core/test/utils/url.spec.ts b/packages/core/test/utils/url.spec.ts
@@ -1,4 +1,3 @@
-import { get } from 'lodash';
 import { getUrl, ConnectionConfig } from '../../src/lib/utils/url';
 
 describe('url util functions', () => {
diff --git a/packages/serve/src/lib/middleware/activityLogMiddleware.ts b/packages/serve/src/lib/middleware/activityLogMiddleware.ts
@@ -42,20 +42,20 @@ export class ActivityLogMiddleware extends BuiltInMiddleware<IActivityLoggerOpti
     const body = context.response.body as any;
     const error = body?.message;
     const user = context.state.user;
+    const activityLog = {
+      logTime,
+      duration,
+      method: context.request.method,
+      url: context.request.originalUrl,
+      ip: context.request.ip,
+      header: context.request.header,
+      params: context.params,
+      query: context.request.query,
+      status: context.response.status,
+      error,
+      user,
+    };
     for (const activityLogger of Object.values(this.activityLoggerMap)) {
-      const activityLog = {
-        logTime,
-        duration,
-        method: context.request.method,
-        url: context.request.originalUrl,
-        ip: context.request.ip,
-        header: context.request.header,
-        params: context.params,
-        query: context.request.query,
-        status: context.response.status,
-        error,
-        user,
-      };
       activityLogger.log(activityLog).catch((e) => {
         logger.debug(`Error when logging activity: ${e}`);
       });
diff --git a/packages/serve/src/lib/middleware/index.ts b/packages/serve/src/lib/middleware/index.ts
@@ -7,6 +7,7 @@ export * from './response-format';
 export * from './enforceHttpsMiddleware';
 export * from './docRouterMiddleware';
 export * from './errorHandlerMIddleware';
+export * from './activityLogMiddleware';
 
 import { CorsMiddleware } from './corsMiddleware';
 import {
diff --git a/packages/serve/test/middlewares/built-in-middlewares/activityLogMiddleware.spec.ts b/packages/serve/test/middlewares/built-in-middlewares/activityLogMiddleware.spec.ts
@@ -174,4 +174,69 @@ describe('Test activity log middlewares', () => {
     expect(actual[0].error).toEqual(expected.error);
     expect(actual[0].user).toEqual(expected.user);
   });
+  // should not log when logger is disabled
+  it('should not log when logger is disabled', async () => {
+    // Arrange
+    const ctx: KoaContext = {
+      ...sinon.stubInterface<KoaContext>(),
+
+      params: {
+        uuid: faker.datatype.uuid(),
+      },
+      state: {
+        user: {
+          name: faker.name.firstName(),
+          attr: {
+            email: faker.internet.email(),
+            id: faker.datatype.uuid(),
+          },
+        },
+      },
+      request: {
+        ...sinon.stubInterface<Request>(),
+        ip: faker.internet.ip(),
+        method: faker.internet.httpMethod(),
+        originalUrl: faker.internet.url(),
+        header: {
+          ...sinon.stubInterface<IncomingHttpHeaders>(),
+          'X-Agent': 'test-normal-client',
+        },
+        query: {
+          ...sinon.stubInterface<ParsedUrlQuery>(),
+          sortby: 'name',
+        },
+      },
+      response: {
+        ...sinon.stubInterface<Response>(),
+        status: 200,
+        length: faker.datatype.number({ min: 100, max: 100000 }),
+        body: {
+          result: 'OK',
+        },
+      },
+    };
+
+    const expected = {
+      method: ctx.request.method,
+      url: ctx.request.originalUrl,
+      status: ctx.response.status,
+      headers: ctx.request.headers,
+      error: undefined,
+      ip: ctx.request.ip,
+      params: ctx.params,
+      user: ctx.state.user,
+    };
+    // Act
+    const middleware = new ActivityLogMiddleware(
+      { ...extensionConfig, enabled: false },
+      '',
+      [mockLogger]
+    );
+    await middleware.activate();
+    await middleware.handle(ctx, async () => Promise.resolve());
+
+    // Assert
+    const logMock = mockLogger.log as jest.Mock;
+    expect(logMock).not.toHaveBeenCalled();
+  });
 });

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,3 @@`
`1`		`-import { get } from 'lodash';`
`2`	`1`	`import { getUrl, ConnectionConfig } from '../../src/lib/utils/url';`
`3`	`2`
`4`	`3`	`describe('url util functions', () => {`