From 0b0818279806d36a0b7c17a8496e82ec97910def Mon Sep 17 00:00:00 2001 From: Josh Kelley Date: Thu, 21 Mar 2024 11:39:42 -0400 Subject: [PATCH] fix(node): Time zone handling for `cron` (#11225) The `cron` package uses `timeZone`, while Sentry internally uses `timezone`. This caused Sentry cron jobs to be incorrectly upserted with no time zone information. Because of the use of the `...(timeZone ? { timeZone } : undefined)` idiom, TypeScript type checking did not catch the mistake. I kept `cron`'s `timeZone` capitalization within Sentry's instrumentation and changed from the `...(timeZone ? { timeZone } : undefined)` idiom so TypeScript could catch mistakes of this sort. (Passing an undefined `timezone` appears to be okay, because Sentry's `node-cron` instrumentation does it.) --- packages/node-experimental/src/cron/cron.ts | 4 ++-- packages/node-experimental/test/cron.test.ts | 21 +++++++++++++------- packages/node/src/cron/cron.ts | 4 ++-- packages/node/test/cron.test.ts | 21 +++++++++++++------- 4 files changed, 32 insertions(+), 18 deletions(-) diff --git a/packages/node-experimental/src/cron/cron.ts b/packages/node-experimental/src/cron/cron.ts index 2540d82e736b..8b6fc324a7a6 100644 --- a/packages/node-experimental/src/cron/cron.ts +++ b/packages/node-experimental/src/cron/cron.ts @@ -100,7 +100,7 @@ export function instrumentCron(lib: T & CronJobConstructor, monitorSlug: stri }, { schedule: { type: 'crontab', value: cronString }, - ...(timeZone ? { timeZone } : {}), + timezone: timeZone || undefined, }, ); } @@ -132,7 +132,7 @@ export function instrumentCron(lib: T & CronJobConstructor, monitorSlug: stri }, { schedule: { type: 'crontab', value: cronString }, - ...(timeZone ? { timeZone } : {}), + timezone: timeZone || undefined, }, ); }; diff --git a/packages/node-experimental/test/cron.test.ts b/packages/node-experimental/test/cron.test.ts index eee6d4a66711..d068280a41e0 100644 --- a/packages/node-experimental/test/cron.test.ts +++ b/packages/node-experimental/test/cron.test.ts @@ -53,13 +53,20 @@ describe('cron check-ins', () => { const CronJobWithCheckIn = cron.instrumentCron(CronJobMock, 'my-cron-job'); - new CronJobWithCheckIn('* * * Jan,Sep Sun', () => { - expect(withMonitorSpy).toHaveBeenCalledTimes(1); - expect(withMonitorSpy).toHaveBeenLastCalledWith('my-cron-job', expect.anything(), { - schedule: { type: 'crontab', value: '* * * 1,9 0' }, - }); - done(); - }); + new CronJobWithCheckIn( + '* * * Jan,Sep Sun', + () => { + expect(withMonitorSpy).toHaveBeenCalledTimes(1); + expect(withMonitorSpy).toHaveBeenLastCalledWith('my-cron-job', expect.anything(), { + schedule: { type: 'crontab', value: '* * * 1,9 0' }, + timezone: 'America/Los_Angeles', + }); + done(); + }, + undefined, + true, + 'America/Los_Angeles', + ); }); test('CronJob.from()', done => { diff --git a/packages/node/src/cron/cron.ts b/packages/node/src/cron/cron.ts index 2540d82e736b..8b6fc324a7a6 100644 --- a/packages/node/src/cron/cron.ts +++ b/packages/node/src/cron/cron.ts @@ -100,7 +100,7 @@ export function instrumentCron(lib: T & CronJobConstructor, monitorSlug: stri }, { schedule: { type: 'crontab', value: cronString }, - ...(timeZone ? { timeZone } : {}), + timezone: timeZone || undefined, }, ); } @@ -132,7 +132,7 @@ export function instrumentCron(lib: T & CronJobConstructor, monitorSlug: stri }, { schedule: { type: 'crontab', value: cronString }, - ...(timeZone ? { timeZone } : {}), + timezone: timeZone || undefined, }, ); }; diff --git a/packages/node/test/cron.test.ts b/packages/node/test/cron.test.ts index eee6d4a66711..d068280a41e0 100644 --- a/packages/node/test/cron.test.ts +++ b/packages/node/test/cron.test.ts @@ -53,13 +53,20 @@ describe('cron check-ins', () => { const CronJobWithCheckIn = cron.instrumentCron(CronJobMock, 'my-cron-job'); - new CronJobWithCheckIn('* * * Jan,Sep Sun', () => { - expect(withMonitorSpy).toHaveBeenCalledTimes(1); - expect(withMonitorSpy).toHaveBeenLastCalledWith('my-cron-job', expect.anything(), { - schedule: { type: 'crontab', value: '* * * 1,9 0' }, - }); - done(); - }); + new CronJobWithCheckIn( + '* * * Jan,Sep Sun', + () => { + expect(withMonitorSpy).toHaveBeenCalledTimes(1); + expect(withMonitorSpy).toHaveBeenLastCalledWith('my-cron-job', expect.anything(), { + schedule: { type: 'crontab', value: '* * * 1,9 0' }, + timezone: 'America/Los_Angeles', + }); + done(); + }, + undefined, + true, + 'America/Los_Angeles', + ); }); test('CronJob.from()', done => {