From fb4e32878db2e69ab5ed91ab98c198ebc85f1dd4 Mon Sep 17 00:00:00 2001 From: IainSAP <46536134+IainSAP@users.noreply.github.com> Date: Thu, 13 Feb 2025 17:34:14 +0000 Subject: [PATCH] fix(`@sap-ux/odata-service-inquirer`): No `log` method when unparseaeble metadata results in unhandled exception (#2895) * fix entity-helper.ts There is no 'log' method on the passed ILogWrapper interface yet and so calling this will cause a runtime exception. Changing for now to error as it should be. Will follow-up TBI to add `log` to `ILogWrapper` as passed from YUI. * Create witty-flies-drive.md * Update witty-flies-drive.md * Update questions.test.ts Update test * Adds logger compatibility between `@vscode-logging/logger` and `@sap-ux/logger` https://github.com/SAP/open-ux-tools/issues/2896 * Updates cset * Adds tests to cover --- .changeset/flat-cougars-sleep.md | 7 + packages/fiori-generator-shared/package.json | 3 +- .../src/logging/logWrapper.ts | 105 ++++++++++++-- .../test/logWrapper.test.ts | 88 ----------- .../test/logging/logWrapper.test.ts | 137 ++++++++++++++++++ .../telemetryHelper.test.ts | 0 .../test/{test => telemetry}/utils.test.ts | 0 packages/fiori-generator-shared/tsconfig.json | 3 + .../src/prompts/edmx/entity-helper.ts | 2 +- .../test/unit/prompts/edmx/questions.test.ts | 2 +- pnpm-lock.yaml | 3 + 11 files changed, 247 insertions(+), 103 deletions(-) create mode 100644 .changeset/flat-cougars-sleep.md delete mode 100644 packages/fiori-generator-shared/test/logWrapper.test.ts create mode 100644 packages/fiori-generator-shared/test/logging/logWrapper.test.ts rename packages/fiori-generator-shared/test/{test => telemetry}/telemetryHelper.test.ts (100%) rename packages/fiori-generator-shared/test/{test => telemetry}/utils.test.ts (100%) diff --git a/.changeset/flat-cougars-sleep.md b/.changeset/flat-cougars-sleep.md new file mode 100644 index 0000000000..06ebc5155f --- /dev/null +++ b/.changeset/flat-cougars-sleep.md @@ -0,0 +1,7 @@ +--- +'@sap-ux/fiori-generator-shared': minor +"@sap-ux/odata-service-inquirer": patch +--- + +Adds interoperability between `@vscode-logging/logger` and `@sap-ux/logger` to prevent crashes where non-implemented log functions were being called. +Fix entity-helper.ts to log error at correct level. diff --git a/packages/fiori-generator-shared/package.json b/packages/fiori-generator-shared/package.json index 1bb246f8fb..5109643628 100644 --- a/packages/fiori-generator-shared/package.json +++ b/packages/fiori-generator-shared/package.json @@ -44,7 +44,8 @@ "@types/mem-fs": "1.1.2", "@types/semver": "7.5.2", "@types/vscode": "1.73.1", - "@types/yeoman-environment": "2.10.11" + "@types/yeoman-environment": "2.10.11", + "@sap-ux/logger": "workspace:*" }, "engines": { "node": ">=18.x" diff --git a/packages/fiori-generator-shared/src/logging/logWrapper.ts b/packages/fiori-generator-shared/src/logging/logWrapper.ts index befbd0664a..50d9431291 100644 --- a/packages/fiori-generator-shared/src/logging/logWrapper.ts +++ b/packages/fiori-generator-shared/src/logging/logWrapper.ts @@ -1,8 +1,9 @@ +import type { Log, Logger as SapUxLogger, Transport } from '@sap-ux/logger'; import type { getExtensionLoggerOpts, + IChildLogger as ILogWrapper, IVSCodeExtLogger, - LogLevel, - IChildLogger as ILogWrapper + LogLevel } from '@vscode-logging/logger'; import { getExtensionLogger } from '@vscode-logging/logger'; import { format } from 'logform'; @@ -38,7 +39,31 @@ export const DefaultLogger: LogWrapper = { console.trace(msg); }, getChildLogger: () => DefaultLogger, - getLogLevel: () => 'off' + getLogLevel: () => 'off', + /** + * Limited compatibility with `@sap-ux/logger` + * + * @param data + */ + log: function (data: string | Log): void { + console.log(data instanceof Object ? data.message : data); + }, + add: function (): SapUxLogger { + console.warn('Log method `add(transport)` not implemented.'); + return this; + }, + remove: function (): SapUxLogger { + console.warn('Log method `remove(transport)` not implemented.'); + return this; + }, + transports: function (): Transport[] { + console.warn('Logger method `transports()` not implemented.'); + return []; + }, + child: function (): SapUxLogger { + console.warn('Log method `remove(transport)` not implemented. Returning current logger.'); + return this; + } }; const LOG_LEVEL_KEYS: Record = { @@ -71,9 +96,9 @@ export function createCLILogger(logName: string, logLevel: LogLevel = 'off'): IL /** * Log to vscode extension logger and yeoman logger simultaneously. * This allows use of Application Wizard log config and log file use but still have a single output channel for - * App Gen logging. + * generator logging. */ -export class LogWrapper implements ILogWrapper { +export class LogWrapper implements ILogWrapper, SapUxLogger { private static _vscodeLogger: ILogWrapper; private static _yoLogger: Logger | undefined; private static _logLevel: LogLevel; @@ -110,7 +135,11 @@ export class LogWrapper implements ILogWrapper { LogWrapper._vscodeLogger?.debug(t('debug.loggingConfigured', { logLevel: LogWrapper._logLevel })); } - static readonly logAtLevel = (level: LogLevel, message: string, ...args: any[]) => { + static readonly logAtLevel = (level: LogLevel, message: string | object, ...args: any[]) => { + if (typeof message === 'object') { + message = JSON.stringify(message); + } + if (LogWrapper._vscodeLogger && level !== 'off') { LogWrapper._vscodeLogger[level](message, ...args); } @@ -146,7 +175,7 @@ export class LogWrapper implements ILogWrapper { * @param msg - message to log * @param {...any} args - additional arguments */ - error(msg: string, ...args: any[]): void { + error(msg: string | object, ...args: any[]): void { LogWrapper.logAtLevel('error', msg, ...args); } /** @@ -155,7 +184,7 @@ export class LogWrapper implements ILogWrapper { * @param msg - message to log * @param {...any} args - additional arguments */ - warn(msg: string, ...args: any[]): void { + warn(msg: string | object, ...args: any[]): void { LogWrapper.logAtLevel('warn', msg, ...args); } /** @@ -164,7 +193,7 @@ export class LogWrapper implements ILogWrapper { * @param msg - message to log * @param {...any} args - additional arguments */ - info(msg: string, ...args: any[]): void { + info(msg: string | object, ...args: any[]): void { LogWrapper.logAtLevel('info', msg, ...args); } /** @@ -173,7 +202,7 @@ export class LogWrapper implements ILogWrapper { * @param msg - message to log * @param {...any} args - additional arguments */ - debug(msg: string, ...args: any[]): void { + debug(msg: string | object, ...args: any[]): void { LogWrapper.logAtLevel('debug', msg, ...args); } /** @@ -191,7 +220,7 @@ export class LogWrapper implements ILogWrapper { * * @param msg - message to log */ - public static log(msg: string): void { + public static log(msg: string | object): void { LogWrapper.logAtLevel('info', msg); } @@ -204,7 +233,59 @@ export class LogWrapper implements ILogWrapper { return LogWrapper._logLevel; } + /** + * Not implemented method, added to support limited interoperability with @sap-ux/logger. + * + * @returns {ILogWrapper} - the current logger + */ getChildLogger(/* opts: { label: string } */): ILogWrapper { - throw new Error(t('error.methodNotImplemented')); + LogWrapper.logAtLevel(`trace`, 'Log method `getChildLogger()` not implemented. Returning current logger.'); + return this; + } + + /** + * Limited compatibility with `@sap-ux/logger` to use log() method. + * + * @param data + */ + log(data: string | Log): void { + // LogLevel is not supported in this implementation + LogWrapper.logAtLevel('info', (data as Log).message ?? data); + } + /** + * Not implemented method, added to support limited interoperability with @sap-ux/logger. + * + * @returns {ILogWrapper} - the current logger + */ + add(): SapUxLogger { + LogWrapper.logAtLevel(`warn`, 'Log method `add(transport)` not implemented.'); + return this; + } + /** + * Not implemented method, added to support limited interoperability with @sap-ux/logger. + * + * @returns {ILogWrapper} - the current logger + */ + remove(): SapUxLogger { + LogWrapper.logAtLevel(`warn`, 'Log method `remove(transport)` not implemented.'); + return this; + } + /** + * Added to support limited interoperability with @sap-ux/logger. + * + * @returns {ILogWrapper} - the current logger transports + */ + transports(): Transport[] { + LogWrapper.logAtLevel(`warn`, 'Log method `transports()` not implemented.'); + return []; + } + /** + * Not implemented method, added to support limited interoperability with @sap-ux/logger. + * + * @returns {ILogWrapper} - the current logger + */ + child(): SapUxLogger { + LogWrapper.logAtLevel(`warn`, 'Log method `child(options)` not implemented. Returning current logger.'); + return this; } } diff --git a/packages/fiori-generator-shared/test/logWrapper.test.ts b/packages/fiori-generator-shared/test/logWrapper.test.ts deleted file mode 100644 index ff334bf959..0000000000 --- a/packages/fiori-generator-shared/test/logWrapper.test.ts +++ /dev/null @@ -1,88 +0,0 @@ -import { DefaultLogger, createCLILogger, LogWrapper } from '../src/logging/logWrapper'; -import type { Logger } from 'yeoman-environment'; -import type { IVSCodeExtLogger } from '@vscode-logging/logger'; - -describe('Test logWrapper', () => { - test('Test logWrapper functions ', () => { - const loggerProps = ['fatal', 'error', 'warn', 'info', 'debug', 'trace', 'getChildLogger', 'getLogLevel']; - expect(Object.keys(DefaultLogger)).toEqual(loggerProps); - - expect(Object.keys(DefaultLogger.getChildLogger())).toStrictEqual(loggerProps); - - const consoleErrorSpy = jest.spyOn(console, 'error'); - const consoleLogSpy = jest.spyOn(console, 'log'); - const consoleWarnSpy = jest.spyOn(console, 'warn'); - const consoleTraceSpy = jest.spyOn(console, 'trace'); - - DefaultLogger.fatal('Fatal'); - expect(consoleLogSpy).toHaveBeenCalledWith('Fatal'); - expect(DefaultLogger.warn('warn')).toBeUndefined(); - expect(DefaultLogger.info('info')).toBeUndefined(); - expect(DefaultLogger.trace('trace')).toBeUndefined(); - - DefaultLogger.error('Error'); - expect(consoleErrorSpy).toHaveBeenCalledWith('Error'); - - const logWrapper = new LogWrapper('TestLogger', {} as Logger, 'info'); - - LogWrapper['_yoLogger'] = undefined as any; - LogWrapper['logAtLevel']('info', 'Info message'); - expect(consoleErrorSpy).toHaveBeenCalledWith('LogWrapper is not initialised'); - - console.log = jest.fn(); - //@ts-expect-error - private member access - LogWrapper['_yoLogger'] = DefaultLogger.info; - LogWrapper['logAtLevel']('info', 'Info message'); - //@ts-expect-error - private member access - let consoleMsg = console.log.mock.calls[0][0]; - expect(consoleMsg).toEqual(expect.stringContaining(`Info message`)); - - const testLogger = createCLILogger('TestLogger', 'info'); - testLogger.info('Test msg'); - - const consoleSpy = jest.spyOn(console, 'log'); - consoleMsg = consoleSpy.mock.calls[0][0]; - expect(consoleMsg).toEqual(expect.stringContaining(`INFO: Info message`)); - - logWrapper.fatal('Fatal'); - expect(consoleErrorSpy).toHaveBeenCalled(); - - logWrapper.error('Error'); - expect(consoleErrorSpy).toHaveBeenCalled(); - - logWrapper.warn('Warn'); - expect(consoleWarnSpy).toHaveBeenCalled(); - - logWrapper.info('Info'); - expect(consoleLogSpy).toHaveBeenCalled(); - - logWrapper.debug('Debug'); - expect(consoleLogSpy).toHaveBeenCalled(); - - logWrapper.trace('Trace'); - expect(consoleTraceSpy).toHaveBeenCalled(); - - expect(LogWrapper.log).toBeDefined(); - LogWrapper.log('A message'); - expect(consoleLogSpy).toHaveBeenCalled(); - - try { - logWrapper.getChildLogger(); - fail(`logWrapper.getChildLogger() should have thrown error but didn't`); - } catch (error) { - expect(error.message).toBeDefined(); - } - }); - - test('should log with extension logger', () => { - const mockLogger = { - debug: jest.fn() - }; - const mockExtensionLogger = { - getChildLogger: () => mockLogger - } as unknown as IVSCodeExtLogger; - - new LogWrapper('ExtensionLogger', {} as Logger, 'info', mockExtensionLogger); - expect(mockLogger.debug).toHaveBeenCalledWith('Logging has been configured at log level: info'); - }); -}); diff --git a/packages/fiori-generator-shared/test/logging/logWrapper.test.ts b/packages/fiori-generator-shared/test/logging/logWrapper.test.ts new file mode 100644 index 0000000000..761fa8f5b7 --- /dev/null +++ b/packages/fiori-generator-shared/test/logging/logWrapper.test.ts @@ -0,0 +1,137 @@ +import type { ChildLoggerOptions, Logger as SapUxLogger, Transport } from '@sap-ux/logger'; +import type { IVSCodeExtLogger } from '@vscode-logging/logger'; +import type { Logger as YoLogger } from 'yeoman-environment'; +import { createCLILogger, DefaultLogger, LogWrapper } from '../../src/logging/logWrapper'; + +describe('Test logWrapper', () => { + afterEach(() => { + jest.restoreAllMocks(); + // Reset static values + LogWrapper['_yoLogger'] = undefined as any; + LogWrapper['_vscodeLogger'] = undefined as any; + LogWrapper['_logLevel'] = undefined as any; + }); + test('Test logWrapper functions ', () => { + const consoleErrorSpy = jest.spyOn(console, 'error'); + const consoleLogSpy = jest.spyOn(console, 'log'); + const consoleWarnSpy = jest.spyOn(console, 'warn'); + const consoleTraceSpy = jest.spyOn(console, 'trace'); + + // DefaultLogger tests + DefaultLogger.fatal('fatal'); + expect(consoleLogSpy).toHaveBeenLastCalledWith('fatal'); + DefaultLogger.warn('warn'); + expect(consoleWarnSpy).toHaveBeenLastCalledWith('warn'); + DefaultLogger.info('info'); + expect(consoleLogSpy).toHaveBeenLastCalledWith('info'); + DefaultLogger.trace('trace'); + expect(consoleTraceSpy).toHaveBeenLastCalledWith('trace'); + DefaultLogger.error('error'); + expect(consoleErrorSpy).toHaveBeenLastCalledWith('error'); + DefaultLogger.log('log'); + expect(consoleLogSpy).toHaveBeenLastCalledWith('log'); + // Log level is ignored as log levels dont align between @sap-ux/logger and @vscode-logging/logger + DefaultLogger.log({ message: 'testLogObjectMsg', level: 0 }); + expect(consoleLogSpy).toHaveBeenLastCalledWith('testLogObjectMsg'); + + // LogWrapper tests + const logWrapper = new LogWrapper('TestLogger', {} as YoLogger, 'info'); + + LogWrapper['_yoLogger'] = undefined as any; + LogWrapper['logAtLevel']('info', 'Info message'); + expect(consoleErrorSpy).toHaveBeenCalledWith('LogWrapper is not initialised'); + + console.log = jest.fn(); + //@ts-expect-error - private member access + LogWrapper['_yoLogger'] = DefaultLogger.info; + LogWrapper['logAtLevel']('info', 'Info message'); + //@ts-expect-error - private member access + let consoleMsg = console.log.mock.calls[0][0]; + expect(consoleMsg).toEqual(expect.stringContaining(`Info message`)); + + const testLogger = createCLILogger('TestLogger', 'info'); + testLogger.info('Test msg'); + + const consoleSpy = jest.spyOn(console, 'log'); + consoleMsg = consoleSpy.mock.calls[0][0]; + expect(consoleMsg).toEqual(expect.stringContaining(`INFO: Info message`)); + + logWrapper.fatal('Fatal'); + expect(consoleErrorSpy).toHaveBeenCalled(); + + logWrapper.error('Error'); + expect(consoleErrorSpy).toHaveBeenCalled(); + + logWrapper.warn('Warn'); + expect(consoleWarnSpy).toHaveBeenCalled(); + + logWrapper.info('Info'); + expect(consoleLogSpy).toHaveBeenCalled(); + + logWrapper.debug('Debug'); + expect(consoleLogSpy).toHaveBeenCalled(); + + logWrapper.trace('Trace'); + expect(consoleTraceSpy).toHaveBeenCalled(); + + logWrapper.log('Log'); + expect(consoleLogSpy).toHaveBeenCalled(); + + expect(LogWrapper.log).toBeDefined(); + LogWrapper.log('A message'); + expect(consoleLogSpy).toHaveBeenCalled(); + + try { + logWrapper.getChildLogger(); + fail(`logWrapper.getChildLogger() should have thrown error but didn't`); + } catch (error) { + expect(error.message).toBeDefined(); + } + }); + + test('should log with extension logger', () => { + const mockLogger = { + debug: jest.fn() + }; + const mockExtensionLogger = { + getChildLogger: () => mockLogger + } as unknown as IVSCodeExtLogger; + + new LogWrapper('ExtensionLogger', {} as YoLogger, 'info', mockExtensionLogger); + expect(mockLogger.debug).toHaveBeenCalledWith('Logging has been configured at log level: info'); + }); + + test('should be compatible with `@sap-ux/logger`', () => { + const consoleLogSpy = jest.spyOn(console, 'log'); + + (DefaultLogger as SapUxLogger).log('log'); + expect(consoleLogSpy).toHaveBeenLastCalledWith('log'); + expect((DefaultLogger as SapUxLogger).add({} as Transport)).toMatchObject(DefaultLogger); + expect((DefaultLogger as SapUxLogger).remove({} as Transport)).toMatchObject(DefaultLogger); + expect((DefaultLogger as SapUxLogger).transports()).toEqual([]); + expect((DefaultLogger as SapUxLogger).child({} as ChildLoggerOptions)).toMatchObject(DefaultLogger); + + // LogWrapper tests + const yoLoggerSpy = jest.fn() as unknown as YoLogger; + const logWrapper = new LogWrapper('TestLogger', yoLoggerSpy, 'info'); + logWrapper.log({ message: 'testLogObjectMsg', level: 1 }); + expect(yoLoggerSpy).toHaveBeenLastCalledWith(expect.stringContaining('testLogObjectMsg')); + + logWrapper.add(); + expect(yoLoggerSpy).toHaveBeenLastCalledWith( + expect.stringContaining('Log method `add(transport)` not implemented.') + ); + logWrapper.remove(); + expect(yoLoggerSpy).toHaveBeenLastCalledWith( + expect.stringContaining('Log method `remove(transport)` not implemented.') + ); + logWrapper.transports(); + expect(yoLoggerSpy).toHaveBeenLastCalledWith( + expect.stringContaining('Log method `transports()` not implemented.') + ); + logWrapper.child(); + expect(yoLoggerSpy).toHaveBeenLastCalledWith( + expect.stringContaining('Log method `child(options)` not implemented.') + ); + }); +}); diff --git a/packages/fiori-generator-shared/test/test/telemetryHelper.test.ts b/packages/fiori-generator-shared/test/telemetry/telemetryHelper.test.ts similarity index 100% rename from packages/fiori-generator-shared/test/test/telemetryHelper.test.ts rename to packages/fiori-generator-shared/test/telemetry/telemetryHelper.test.ts diff --git a/packages/fiori-generator-shared/test/test/utils.test.ts b/packages/fiori-generator-shared/test/telemetry/utils.test.ts similarity index 100% rename from packages/fiori-generator-shared/test/test/utils.test.ts rename to packages/fiori-generator-shared/test/telemetry/utils.test.ts diff --git a/packages/fiori-generator-shared/tsconfig.json b/packages/fiori-generator-shared/tsconfig.json index 3a278bb83f..085c618e2e 100644 --- a/packages/fiori-generator-shared/tsconfig.json +++ b/packages/fiori-generator-shared/tsconfig.json @@ -12,6 +12,9 @@ { "path": "../btp-utils" }, + { + "path": "../logger" + }, { "path": "../project-access" }, diff --git a/packages/odata-service-inquirer/src/prompts/edmx/entity-helper.ts b/packages/odata-service-inquirer/src/prompts/edmx/entity-helper.ts index c58d5627c5..d791f48716 100644 --- a/packages/odata-service-inquirer/src/prompts/edmx/entity-helper.ts +++ b/packages/odata-service-inquirer/src/prompts/edmx/entity-helper.ts @@ -101,7 +101,7 @@ export function getEntityChoices( } }); } catch (err) { - LoggerHelper.logger.log(t('errors.unparseableMetadata', { error: err.message })); + LoggerHelper.logger.error(t('errors.unparseableMetadata', { error: err.message })); } return { diff --git a/packages/odata-service-inquirer/test/unit/prompts/edmx/questions.test.ts b/packages/odata-service-inquirer/test/unit/prompts/edmx/questions.test.ts index f1388670c8..5367cc1d5c 100644 --- a/packages/odata-service-inquirer/test/unit/prompts/edmx/questions.test.ts +++ b/packages/odata-service-inquirer/test/unit/prompts/edmx/questions.test.ts @@ -42,7 +42,7 @@ describe('Test entity prompts', () => { expect(questions).toMatchSnapshot(); // Invalid metadata - const errorLogSpy = jest.spyOn(LoggerHelper.logger, 'log'); + const errorLogSpy = jest.spyOn(LoggerHelper.logger, 'error'); questions = getEntitySelectionQuestions('{}', 'lrop'); expect(questions).toEqual([]); expect(errorLogSpy).toBeCalledWith(expect.stringMatching('Unable to parse entities')); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 293bf5c506..41f51b34c4 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1987,6 +1987,9 @@ importers: specifier: 7.5.4 version: 7.5.4 devDependencies: + '@sap-ux/logger': + specifier: workspace:* + version: link:../logger '@types/mem-fs': specifier: 1.1.2 version: 1.1.2