From bde4264f7e99ab75d5c5b6e622a53fcd8d3a41d8 Mon Sep 17 00:00:00 2001 From: Dan Skinner Date: Fri, 11 Aug 2023 14:47:34 +0100 Subject: [PATCH 1/2] startSession should no longer clone the client --- packages/plugin-express/src/express.js | 8 +-- packages/plugin-koa/src/koa.js | 16 +++--- packages/plugin-koa/test/koa.test.ts | 13 ++--- packages/plugin-restify/src/restify.js | 8 +-- packages/plugin-server-session/session.js | 11 ++-- .../test/session.test.ts | 50 +++++-------------- 6 files changed, 44 insertions(+), 62 deletions(-) diff --git a/packages/plugin-express/src/express.js b/packages/plugin-express/src/express.js index 06388728ad..bbe1ad903e 100644 --- a/packages/plugin-express/src/express.js +++ b/packages/plugin-express/src/express.js @@ -13,9 +13,11 @@ module.exports = { name: 'express', load: client => { const requestHandler = (req, res, next) => { - // Get a client to be scoped to this request. If sessions are enabled, use the - // resumeSession() call to get a session client, otherwise, clone the existing client. - const requestClient = client._config.autoTrackSessions ? client.resumeSession() : clone(client) + // clone the client to be scoped to this request. If sessions are enabled, start one + const requestClient = clone(client) + if (requestClient._config.autoTrackSessions) { + requestClient.startSession() + } // attach it to the request req.bugsnag = requestClient diff --git a/packages/plugin-koa/src/koa.js b/packages/plugin-koa/src/koa.js index e90011652a..84aff13a3a 100644 --- a/packages/plugin-koa/src/koa.js +++ b/packages/plugin-koa/src/koa.js @@ -14,9 +14,11 @@ module.exports = { name: 'koa', load: client => { const requestHandler = async (ctx, next) => { - // Get a client to be scoped to this request. If sessions are enabled, use the - // resumeSession() call to get a session client, otherwise, clone the existing client. - const requestClient = client._config.autoTrackSessions ? client.resumeSession() : clone(client) + // clone the client to be scoped to this request. If sessions are enabled, start one + const requestClient = clone(client) + if (requestClient._config.autoTrackSessions) { + requestClient.startSession() + } ctx.bugsnag = requestClient @@ -35,9 +37,11 @@ module.exports = { } requestHandler.v1 = function * (next) { - // Get a client to be scoped to this request. If sessions are enabled, use the - // resumeSession() call to get a session client, otherwise, clone the existing client. - const requestClient = client._config.autoTrackSessions ? client.resumeSession() : clone(client) + // clone the client to be scoped to this request. If sessions are enabled, start one + const requestClient = clone(client) + if (requestClient._config.autoTrackSessions) { + requestClient.startSession() + } this.bugsnag = requestClient diff --git a/packages/plugin-koa/test/koa.test.ts b/packages/plugin-koa/test/koa.test.ts index cd78d07d0e..a4b1e1fe6c 100644 --- a/packages/plugin-koa/test/koa.test.ts +++ b/packages/plugin-koa/test/koa.test.ts @@ -34,7 +34,7 @@ describe('plugin: koa', () => { }) describe('requestHandler', () => { - it('should start a session and attach a client to the context', async () => { + it('should clone the client, start a session and attach the cloned client to the context', async () => { const client = new Client({ apiKey: 'api_key', plugins: [plugin] }) const startSession = jest.fn().mockReturnValue(client) @@ -58,10 +58,11 @@ describe('plugin: koa', () => { expect(client._logger.warn).not.toHaveBeenCalled() expect(client._logger.error).not.toHaveBeenCalled() - expect(startSession).not.toHaveBeenCalled() + expect(startSession).toHaveBeenCalledTimes(1) expect(pauseSession).not.toHaveBeenCalled() - expect(resumeSession).toHaveBeenCalledTimes(1) - expect(context.bugsnag).toBe(client) + expect(resumeSession).not.toHaveBeenCalled() + expect(context.bugsnag).toStrictEqual(expect.any(Client)) + expect(context.bugsnag).not.toBe(client) expect(client._clientContext.run).toHaveBeenCalledWith(expect.any(Client), next) }) @@ -114,7 +115,7 @@ describe('plugin: koa', () => { expect(client._clientContext.run).toHaveBeenCalledWith(expect.any(Client), next) const event: Event = await new Promise(resolve => { - client.notify(new Error('abc'), noop, (_, event) => resolve(event as Event)) + (context.bugsnag as Client).notify(new Error('abc'), noop, (_, event) => resolve(event as Event)) }) expect(client._logger.warn).not.toHaveBeenCalled() @@ -183,7 +184,7 @@ describe('plugin: koa', () => { expect(client._clientContext.run).toHaveBeenCalledWith(expect.any(Client), next) const event: Event = await new Promise(resolve => { - client.notify(new Error('abc'), noop, (_, event) => resolve(event as Event)) + (context.bugsnag as Client).notify(new Error('abc'), noop, (_, event) => resolve(event as Event)) }) expect(client._logger.warn).not.toHaveBeenCalled() diff --git a/packages/plugin-restify/src/restify.js b/packages/plugin-restify/src/restify.js index a85ef9999a..ab071f9878 100644 --- a/packages/plugin-restify/src/restify.js +++ b/packages/plugin-restify/src/restify.js @@ -13,9 +13,11 @@ module.exports = { name: 'restify', load: client => { const requestHandler = (req, res, next) => { - // Get a client to be scoped to this request. If sessions are enabled, use the - // resumeSession() call to get a session client, otherwise, clone the existing client. - const requestClient = client._config.autoTrackSessions ? client.resumeSession() : clone(client) + // clone the client to be scoped to this request. If sessions are enabled, start one + const requestClient = clone(client) + if (requestClient._config.autoTrackSessions) { + requestClient.startSession() + } // attach it to the request req.bugsnag = requestClient diff --git a/packages/plugin-server-session/session.js b/packages/plugin-server-session/session.js index 75234833e0..7932aee8db 100644 --- a/packages/plugin-server-session/session.js +++ b/packages/plugin-server-session/session.js @@ -1,5 +1,4 @@ const intRange = require('@bugsnag/core/lib/validators/int-range') -const clone = require('@bugsnag/core/lib/clone-client') const SessionTracker = require('./tracker') const Backoff = require('backo') const runSyncCallbacks = require('@bugsnag/core/lib/sync-callback-runner') @@ -11,11 +10,10 @@ module.exports = { sessionTracker.start() client._sessionDelegate = { startSession: (client, session) => { - const sessionClient = clone(client) - sessionClient._session = session - sessionClient._pausedSession = null - sessionTracker.track(sessionClient._session) - return sessionClient + client._session = session + client._pausedSession = null + sessionTracker.track(client._session) + return client }, pauseSession: (client) => { client._pausedSession = client._session @@ -31,7 +29,6 @@ module.exports = { if (client._pausedSession) { client._session = client._pausedSession client._pausedSession = null - return client } diff --git a/packages/plugin-server-session/test/session.test.ts b/packages/plugin-server-session/test/session.test.ts index c1d4cb48f0..16400cb2f0 100644 --- a/packages/plugin-server-session/test/session.test.ts +++ b/packages/plugin-server-session/test/session.test.ts @@ -96,30 +96,6 @@ describe('plugin: server sessions', () => { c.startSession() }) - it('should clone properties that shouldn’t be mutated on the original client', () => { - class TrackerMock extends EventEmitter { - start () {} - stop () {} - track () {} - } - Tracker.mockImplementation(() => new TrackerMock() as any) - - const c = new Client({ apiKey: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' }, undefined, [plugin]) - - c.leaveBreadcrumb('tick') - c._metadata = { datetime: { tz: 'GMT+1' } } - - const sessionClient = c.startSession() - - sessionClient.leaveBreadcrumb('tock') - sessionClient.addMetadata('other', { widgetsAdded: 'cat,dog,mouse' }) - - expect(c._breadcrumbs.length).toBe(1) - expect(Object.keys(c._metadata).length).toBe(1) - expect(sessionClient._breadcrumbs.length).toBe(2) - expect(Object.keys(sessionClient._metadata).length).toBe(2) - }) - it('should support pausing/resuming sessions', () => { class TrackerMock extends EventEmitter { start () {} @@ -130,28 +106,28 @@ describe('plugin: server sessions', () => { const c = new Client({ apiKey: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' }, undefined, [plugin]) - // start a session and get its id - const sessionClient = c.startSession() - const sid0 = (sessionClient._session as Session).id + // start a session and get its idc.startSession() + c.startSession() + const sid0 = (c._session as Session).id // ensure pausing the session clears the client._session property - sessionClient.pauseSession() - const s1 = sessionClient._session - const psid1 = (sessionClient._pausedSession as Session).id + c.pauseSession() + const s1 = c._session + const psid1 = (c._pausedSession as Session).id expect(s1).toBe(null) expect(psid1).toBe(sid0) // ensure resuming the session gets back the original session (not a new one) - sessionClient.resumeSession() - const sid2 = (sessionClient._session as Session).id + c.resumeSession() + const sid2 = (c._session as Session).id expect(sid2).toBe(sid0) // ensure resumeSession() starts a new one when no paused session exists - sessionClient._session = null - sessionClient._pausedSession = null - const resumedClient = sessionClient.resumeSession() - expect(resumedClient._session).toBeTruthy() - const sid3 = (resumedClient._session as Session).id + c._session = null + c._pausedSession = null + c.resumeSession() + expect(c._session).toBeTruthy() + const sid3 = (c._session as unknown as Session).id expect(sid3).not.toBe(sid0) }) }) From cdde0fd63653179a16faf03dbde5c10c936a5e63 Mon Sep 17 00:00:00 2001 From: djskinner Date: Wed, 30 Aug 2023 10:03:45 +0100 Subject: [PATCH 2/2] Update packages/plugin-server-session/test/session.test.ts Co-authored-by: Joe Haines --- packages/plugin-server-session/test/session.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/plugin-server-session/test/session.test.ts b/packages/plugin-server-session/test/session.test.ts index 16400cb2f0..71b8443692 100644 --- a/packages/plugin-server-session/test/session.test.ts +++ b/packages/plugin-server-session/test/session.test.ts @@ -106,7 +106,7 @@ describe('plugin: server sessions', () => { const c = new Client({ apiKey: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' }, undefined, [plugin]) - // start a session and get its idc.startSession() + // start a session and get its id c.startSession() const sid0 = (c._session as Session).id