From 2007b9ff75d83207efda5e34c30d783cf5efe493 Mon Sep 17 00:00:00 2001 From: gagik Date: Thu, 6 Feb 2025 10:50:51 +0100 Subject: [PATCH 1/7] feat(logging): add option to set log location MONGOSH-1983 --- packages/cli-repl/src/cli-repl.spec.ts | 10 ++ packages/cli-repl/src/cli-repl.ts | 4 +- packages/e2e-tests/test/e2e.spec.ts | 125 ++++++++++++++++++++++++- packages/types/src/index.ts | 7 ++ 4 files changed, 141 insertions(+), 5 deletions(-) diff --git a/packages/cli-repl/src/cli-repl.spec.ts b/packages/cli-repl/src/cli-repl.spec.ts index 43ba0b6c0..43608be53 100644 --- a/packages/cli-repl/src/cli-repl.spec.ts +++ b/packages/cli-repl/src/cli-repl.spec.ts @@ -322,6 +322,7 @@ describe('CliRepl', function () { 'browser', 'updateURL', 'disableLogging', + 'logLocation', ] satisfies (keyof CliUserConfig)[]); }); @@ -1426,6 +1427,15 @@ describe('CliRepl', function () { ) ).to.have.lengthOf(1); }); + + it('can set the log location', async function () { + const testPath = path.join('./test', 'path'); + cliRepl.config.logLocation = testPath; + await cliRepl.start(await testServer.connectionString(), {}); + + expect(cliRepl.getConfig('logLocation')).is.true; + expect(cliRepl.logWriter?.logFilePath).equals(testPath); + }); }); it('times out fast', async function () { diff --git a/packages/cli-repl/src/cli-repl.ts b/packages/cli-repl/src/cli-repl.ts index 11c4076d6..908fbe5b0 100644 --- a/packages/cli-repl/src/cli-repl.ts +++ b/packages/cli-repl/src/cli-repl.ts @@ -256,7 +256,9 @@ export class CliRepl implements MongoshIOProvider { } this.logManager ??= new MongoLogManager({ - directory: this.shellHomeDirectory.localPath('.'), + directory: + (await this.getConfig('logLocation')) || + this.shellHomeDirectory.localPath('.'), retentionDays: 30, maxLogFileCount: +( process.env.MONGOSH_TEST_ONLY_MAX_LOG_FILE_COUNT || 100 diff --git a/packages/e2e-tests/test/e2e.spec.ts b/packages/e2e-tests/test/e2e.spec.ts index 8fd076cfb..3eda4e99e 100644 --- a/packages/e2e-tests/test/e2e.spec.ts +++ b/packages/e2e-tests/test/e2e.spec.ts @@ -15,7 +15,11 @@ import { promisify } from 'util'; import path from 'path'; import os from 'os'; import type { MongoLogEntryFromFile } from './repl-helpers'; -import { readReplLogFile, setTemporaryHomeDirectory } from './repl-helpers'; +import { + readReplLogFile, + setTemporaryHomeDirectory, + useTmpdir, +} from './repl-helpers'; import { bson } from '@mongosh/service-provider-core'; import type { Server as HTTPServer } from 'http'; import { createServer as createHTTPServer } from 'http'; @@ -1356,7 +1360,9 @@ describe('e2e', function () { let logBasePath: string; let historyPath: string; let readConfig: () => Promise; - let readLogFile: () => Promise; + let readLogFile: ( + customBasePath?: string + ) => Promise; let startTestShell: (...extraArgs: string[]) => Promise; beforeEach(function () { @@ -1393,11 +1399,16 @@ describe('e2e', function () { } readConfig = async () => EJSON.parse(await fs.readFile(configPath, 'utf8')); - readLogFile = async (): Promise => { + readLogFile = async ( + customBasePath?: string + ): Promise => { if (!shell.logId) { throw new Error('Shell does not have a logId associated with it'); } - const logPath = path.join(logBasePath, `${shell.logId}_log`); + const logPath = path.join( + customBasePath ?? logBasePath, + `${shell.logId}_log` + ); return readReplLogFile(logPath); }; startTestShell = async (...extraArgs: string[]) => { @@ -1557,6 +1568,112 @@ describe('e2e', function () { ).to.have.lengthOf(1); }); + describe('with custom log location', function () { + const customLogDir = useTmpdir(); + + it('fails with relative or invalid paths', async function () { + const globalConfig = path.join(homedir, 'globalconfig.conf'); + await fs.writeFile( + globalConfig, + `mongosh:\n logLocation: "./some-relative-path"` + ); + + shell = this.startTestShell({ + args: ['--nodb'], + env: { + ...env, + MONGOSH_GLOBAL_CONFIG_FILE_FOR_TESTING: globalConfig, + }, + forceTerminal: true, + }); + await shell.waitForPrompt(); + shell.assertContainsOutput('Ignoring config option "logLocation"'); + shell.assertContainsOutput( + 'must be a valid absolute path or empty' + ); + + expect( + await shell.executeLine( + 'config.set("logLocation", "[123123123123]")' + ) + ).contains( + 'Cannot set option "logLocation": logLocation must be a valid absolute path or empty' + ); + }); + + it('gets created according to logLocation, if set', async function () { + const globalConfig = path.join(homedir, 'globalconfig.conf'); + await fs.writeFile( + globalConfig, + `mongosh:\n logLocation: "${customLogDir.path}"` + ); + + shell = this.startTestShell({ + args: ['--nodb'], + env: { + ...env, + MONGOSH_GLOBAL_CONFIG_FILE_FOR_TESTING: globalConfig, + }, + forceTerminal: true, + }); + await shell.waitForPrompt(); + expect( + await shell.executeLine('config.get("logLocation")') + ).contains(customLogDir.path); + + try { + await readLogFile(); + expect.fail('expected to throw'); + } catch (error) { + expect((error as Error).message).includes( + 'no such file or directory' + ); + } + + expect( + (await readLogFile(customLogDir.path)).some( + (log) => log.attr?.input === 'config.get("logLocation")' + ) + ).is.true; + }); + + it('setting location while running mongosh does not have an immediate effect on logging', async function () { + expect( + await shell.executeLine('config.get("logLocation")') + ).does.not.contain(customLogDir.path); + const oldLogId = shell.logId; + + const oldLogEntries = await readLogFile(); + await shell.executeLine( + `config.set("logLocation", "${customLogDir.path}")` + ); + + await shell.waitForPrompt(); + expect( + await shell.executeLine('config.get("logLocation")') + ).contains(customLogDir.path); + + expect(shell.logId).equals(oldLogId); + + const currentLogEntries = await readLogFile(); + + try { + await readLogFile(customLogDir.path); + expect.fail('expected to throw'); + } catch (error) { + expect((error as Error).message).includes( + 'no such file or directory' + ); + } + expect( + currentLogEntries.some( + (log) => log.attr?.input === 'config.get("logLocation")' + ) + ).is.true; + expect(currentLogEntries.length - oldLogEntries.length).equals(2); + }); + }); + it('creates a log file that keeps track of session events', async function () { expect(await shell.executeLine('print(123 + 456)')).to.include('579'); const log = await readLogFile(); diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index bbe261779..13b3b412f 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -1,4 +1,5 @@ import type { ConnectEventMap } from '@mongodb-js/devtools-connect'; +import path from 'path'; export interface ApiEventArguments { pipeline?: any[]; @@ -507,6 +508,7 @@ export class CliUserConfig extends SnippetShellUserConfig { browser: undefined | false | string = undefined; updateURL = 'https://downloads.mongodb.com/compass/mongosh.json'; disableLogging = false; + logLocation: string | undefined = undefined; } export class CliUserConfigValidator extends SnippetShellUserConfigValidator { @@ -579,6 +581,11 @@ export class CliUserConfigValidator extends SnippetShellUserConfigValidator { return `${key} must be a valid URL or empty`; } return null; + case 'logLocation': + if (typeof value !== 'string' || !path.isAbsolute(value)) { + return `${key} must be a valid absolute path or empty`; + } + return null; default: return super.validate( key as keyof SnippetShellUserConfig, From ac83260ca4841303356938f1ce1fe7abfa1f43e7 Mon Sep 17 00:00:00 2001 From: gagik Date: Thu, 6 Feb 2025 14:27:03 +0100 Subject: [PATCH 2/7] fix(tests): update validation, await getConfig, and use more permissive test --- packages/cli-repl/src/cli-repl.spec.ts | 2 +- packages/e2e-tests/test/e2e.spec.ts | 4 +++- packages/types/src/index.ts | 5 ++++- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/packages/cli-repl/src/cli-repl.spec.ts b/packages/cli-repl/src/cli-repl.spec.ts index 43608be53..5cdcd3689 100644 --- a/packages/cli-repl/src/cli-repl.spec.ts +++ b/packages/cli-repl/src/cli-repl.spec.ts @@ -1433,7 +1433,7 @@ describe('CliRepl', function () { cliRepl.config.logLocation = testPath; await cliRepl.start(await testServer.connectionString(), {}); - expect(cliRepl.getConfig('logLocation')).is.true; + expect(await cliRepl.getConfig('logLocation')).equals('test/path'); expect(cliRepl.logWriter?.logFilePath).equals(testPath); }); }); diff --git a/packages/e2e-tests/test/e2e.spec.ts b/packages/e2e-tests/test/e2e.spec.ts index 3eda4e99e..6f0e5c917 100644 --- a/packages/e2e-tests/test/e2e.spec.ts +++ b/packages/e2e-tests/test/e2e.spec.ts @@ -1670,7 +1670,9 @@ describe('e2e', function () { (log) => log.attr?.input === 'config.get("logLocation")' ) ).is.true; - expect(currentLogEntries.length - oldLogEntries.length).equals(2); + expect(currentLogEntries.length).is.greaterThanOrEqual( + oldLogEntries.length + ); }); }); diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 13b3b412f..7dcc2c9db 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -582,7 +582,10 @@ export class CliUserConfigValidator extends SnippetShellUserConfigValidator { } return null; case 'logLocation': - if (typeof value !== 'string' || !path.isAbsolute(value)) { + if ( + value !== undefined && + (typeof value !== 'string' || !path.isAbsolute(value)) + ) { return `${key} must be a valid absolute path or empty`; } return null; From d138951a8ad083693687d579ee4b1ac04910d0b5 Mon Sep 17 00:00:00 2001 From: gagik Date: Thu, 6 Feb 2025 15:48:01 +0100 Subject: [PATCH 3/7] fix(tests): use the path variable for windows --- packages/cli-repl/src/cli-repl.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli-repl/src/cli-repl.spec.ts b/packages/cli-repl/src/cli-repl.spec.ts index 5cdcd3689..411ffa2b2 100644 --- a/packages/cli-repl/src/cli-repl.spec.ts +++ b/packages/cli-repl/src/cli-repl.spec.ts @@ -1433,7 +1433,7 @@ describe('CliRepl', function () { cliRepl.config.logLocation = testPath; await cliRepl.start(await testServer.connectionString(), {}); - expect(await cliRepl.getConfig('logLocation')).equals('test/path'); + expect(await cliRepl.getConfig('logLocation')).equals(testPath); expect(cliRepl.logWriter?.logFilePath).equals(testPath); }); }); From 3d8765c92ab56abf79d63bb2582fc4da3c16ac09 Mon Sep 17 00:00:00 2001 From: gagik Date: Thu, 6 Feb 2025 16:26:58 +0100 Subject: [PATCH 4/7] fix(tests): use temporary directory for custom log location --- packages/cli-repl/src/cli-repl.spec.ts | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/packages/cli-repl/src/cli-repl.spec.ts b/packages/cli-repl/src/cli-repl.spec.ts index 411ffa2b2..80321aeee 100644 --- a/packages/cli-repl/src/cli-repl.spec.ts +++ b/packages/cli-repl/src/cli-repl.spec.ts @@ -1428,13 +1428,20 @@ describe('CliRepl', function () { ).to.have.lengthOf(1); }); + const customLogLocation = useTmpdir(); it('can set the log location', async function () { - const testPath = path.join('./test', 'path'); - cliRepl.config.logLocation = testPath; + cliRepl.config.logLocation = customLogLocation.path; await cliRepl.start(await testServer.connectionString(), {}); - expect(await cliRepl.getConfig('logLocation')).equals(testPath); - expect(cliRepl.logWriter?.logFilePath).equals(testPath); + expect(await cliRepl.getConfig('logLocation')).equals( + customLogLocation.path + ); + expect(cliRepl.logWriter?.logFilePath).equals( + path.join( + customLogLocation.path, + (cliRepl.logWriter?.logId as string) + '_log' + ) + ); }); }); From 85453b9bf32bbdd3d02f62b90ece16559749f7fb Mon Sep 17 00:00:00 2001 From: gagik Date: Thu, 6 Feb 2025 22:26:32 +0100 Subject: [PATCH 5/7] wip: check what path sep is when using windows runner --- packages/types/src/index.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 7dcc2c9db..4eb2331ed 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -586,6 +586,13 @@ export class CliUserConfigValidator extends SnippetShellUserConfigValidator { value !== undefined && (typeof value !== 'string' || !path.isAbsolute(value)) ) { + // eslint-disable-next-line no-console + console.info( + path.sep, + path.win32 === path, + path.isAbsolute(value as string), + value + ); return `${key} must be a valid absolute path or empty`; } return null; From fba07f8cfd281eaa4852058d82fea18b99ae0015 Mon Sep 17 00:00:00 2001 From: gagik Date: Fri, 7 Feb 2025 10:18:18 +0100 Subject: [PATCH 6/7] fix(tests): try JSON.stringify --- packages/e2e-tests/test/e2e.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/e2e-tests/test/e2e.spec.ts b/packages/e2e-tests/test/e2e.spec.ts index 6f0e5c917..08f6a56e7 100644 --- a/packages/e2e-tests/test/e2e.spec.ts +++ b/packages/e2e-tests/test/e2e.spec.ts @@ -1605,7 +1605,7 @@ describe('e2e', function () { const globalConfig = path.join(homedir, 'globalconfig.conf'); await fs.writeFile( globalConfig, - `mongosh:\n logLocation: "${customLogDir.path}"` + `mongosh:\n logLocation: ${JSON.stringify(customLogDir.path)}` ); shell = this.startTestShell({ @@ -1645,7 +1645,7 @@ describe('e2e', function () { const oldLogEntries = await readLogFile(); await shell.executeLine( - `config.set("logLocation", "${customLogDir.path}")` + `config.set("logLocation", ${JSON.stringify(customLogDir.path)})` ); await shell.waitForPrompt(); From e7859a93b48a992a0bedde6dc446f565d953a3b2 Mon Sep 17 00:00:00 2001 From: gagik Date: Fri, 7 Feb 2025 11:58:51 +0100 Subject: [PATCH 7/7] Revert "wip: check what path sep is when using windows runner" This reverts commit 85453b9bf32bbdd3d02f62b90ece16559749f7fb. --- packages/types/src/index.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 4eb2331ed..7dcc2c9db 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -586,13 +586,6 @@ export class CliUserConfigValidator extends SnippetShellUserConfigValidator { value !== undefined && (typeof value !== 'string' || !path.isAbsolute(value)) ) { - // eslint-disable-next-line no-console - console.info( - path.sep, - path.win32 === path, - path.isAbsolute(value as string), - value - ); return `${key} must be a valid absolute path or empty`; } return null;