From 7ac53b8506bcb78b10e1835b58d77578c24e62d7 Mon Sep 17 00:00:00 2001 From: achettyiitr Date: Tue, 26 Dec 2023 17:09:13 +0530 Subject: [PATCH 1/4] fix: use JSON.stringify instead of lodash.toString --- .gitignore | 3 ++ src/warehouse/config/helpers.js | 6 +++- src/warehouse/index.js | 3 +- src/warehouse/util.js | 9 +++-- test/__tests__/data/warehouse/events.js | 32 +++++++++++++++-- test/__tests__/warehouse.test.js | 46 ++++++++++++++++++++++++- 6 files changed, 88 insertions(+), 11 deletions(-) diff --git a/.gitignore b/.gitignore index ab1fc2a840..24d37f6354 100644 --- a/.gitignore +++ b/.gitignore @@ -132,3 +132,6 @@ dist # Others **/.DS_Store + + +.idea \ No newline at end of file diff --git a/src/warehouse/config/helpers.js b/src/warehouse/config/helpers.js index ecc7d382b9..f8d502c204 100644 --- a/src/warehouse/config/helpers.js +++ b/src/warehouse/config/helpers.js @@ -6,7 +6,10 @@ const isNull = (x) => { }; const isBlank = (value) => { - return _.isEmpty(_.toString(value)); + if (isNull(value) || value === '') { + return true; + } + return _.isEmpty(JSON.stringify(value)) }; const getFirstValidValue = (message, props) => { @@ -27,6 +30,7 @@ function isDataLakeProvider(provider) { module.exports = { isNull, + isBlank, getFirstValidValue, isDataLakeProvider, }; diff --git a/src/warehouse/index.js b/src/warehouse/index.js index c12239c440..c6c7f460ee 100644 --- a/src/warehouse/index.js +++ b/src/warehouse/index.js @@ -5,7 +5,6 @@ const { v4: uuidv4 } = require('uuid'); const { isObject, - isBlank, isValidJsonPathKey, isValidLegacyJsonPathKey, keysFromJsonPaths, @@ -24,7 +23,7 @@ const whPageColumnMappingRules = require('./config/WHPageConfig.js'); const whScreenColumnMappingRules = require('./config/WHScreenConfig.js'); const whGroupColumnMappingRules = require('./config/WHGroupConfig.js'); const whAliasColumnMappingRules = require('./config/WHAliasConfig.js'); -const { isDataLakeProvider } = require('./config/helpers'); +const {isDataLakeProvider, isBlank} = require('./config/helpers'); const { InstrumentationError } = require('../v0/util/errorTypes'); const whExtractEventTableColumnMappingRules = require('./config/WHExtractEventTableConfig.js'); diff --git a/src/warehouse/util.js b/src/warehouse/util.js index 91b49039f1..056b6e96b4 100644 --- a/src/warehouse/util.js +++ b/src/warehouse/util.js @@ -4,6 +4,7 @@ const get = require('get-value'); const v0 = require('./v0/util'); const v1 = require('./v1/util'); const { PlatformError, InstrumentationError } = require('../v0/util/errorTypes'); +const {isBlank} = require('./config/helpers'); const minTimeInMs = Date.parse('0001-01-01T00:00:00Z'); const maxTimeInMs = Date.parse('9999-12-31T23:59:59.999Z'); @@ -22,10 +23,6 @@ const isValidLegacyJsonPathKey = (eventType, key, level, jsonKeys = {}) => { return eventType === 'track' && jsonKeys[key] === level; }; -const isBlank = (value) => { - return _.isEmpty(_.toString(value)); -}; - /* This function takes in an array of json paths and returns an object with keys as the json path and value as the position of the key in the json path Example: @@ -97,6 +94,9 @@ const timestampRegex = new RegExp( ); function validTimestamp(input) { + if (typeof input !== 'string') { + return false; + } if (timestampRegex.test(input)) { // check if date value lies in between min time and max time. if not then it's not a valid timestamp const d = new Date(input); @@ -147,7 +147,6 @@ const getRecordIDForExtract = (message) => { module.exports = { isObject, - isBlank, isValidJsonPathKey, isValidLegacyJsonPathKey, keysFromJsonPaths, diff --git a/test/__tests__/data/warehouse/events.js b/test/__tests__/data/warehouse/events.js index 20a6ce89b3..6a740f4601 100644 --- a/test/__tests__/data/warehouse/events.js +++ b/test/__tests__/data/warehouse/events.js @@ -60,7 +60,17 @@ const sampleEvents = { originalTimestamp: "2020-01-24T06:29:02.364Z", properties: { currency: "USD", - revenue: 50 + revenue: 50, + stack: { + history: { + errorDetails: [ + { + "message": "Cannot set headers after they are sent to the client", + "toString": "[function]" + } + ] + } + } }, receivedAt: "2020-01-24T11:59:02.403+05:30", request_ip: "[::1]:53708", @@ -180,6 +190,12 @@ const sampleEvents = { data: { currency: "USD", revenue: 50, + stack_history_error_details: [ + { + "message": "Cannot set headers after they are sent to the client", + "toString": "[function]" + } + ], context_app_build: "1.0.0", context_app_name: "RudderLabs JavaScript SDK", context_app_namespace: "com.rudderlabs.javascript", @@ -314,6 +330,12 @@ const sampleEvents = { data: { CURRENCY: "USD", REVENUE: 50, + STACK_HISTORY_ERROR_DETAILS: [ + { + "message": "Cannot set headers after they are sent to the client", + "toString": "[function]" + } + ], CONTEXT_APP_BUILD: "1.0.0", CONTEXT_APP_NAME: "RudderLabs JavaScript SDK", CONTEXT_APP_NAMESPACE: "com.rudderlabs.javascript", @@ -340,7 +362,7 @@ const sampleEvents = { RECEIVED_AT: "2020-01-24T06:29:02.403Z", ORIGINAL_TIMESTAMP: "2020-01-24T06:29:02.364Z", CHANNEL: "web", - EVENT: "button_clicked" + EVENT: "button_clicked", } } ], @@ -448,6 +470,12 @@ const sampleEvents = { data: { currency: "USD", revenue: 50, + stack_history_error_details: [ + { + "message": "Cannot set headers after they are sent to the client", + "toString": "[function]" + } + ], context_app_build: "1.0.0", context_app_name: "RudderLabs JavaScript SDK", context_app_namespace: "com.rudderlabs.javascript", diff --git a/test/__tests__/warehouse.test.js b/test/__tests__/warehouse.test.js index 045bab35a6..1dbaabb03e 100644 --- a/test/__tests__/warehouse.test.js +++ b/test/__tests__/warehouse.test.js @@ -20,6 +20,7 @@ const { const { validTimestamp } = require("../../src/warehouse/util.js"); +const {isBlank} = require("../../src/warehouse/config/helpers.js"); const version = "v0"; const integrations = [ @@ -1138,12 +1139,55 @@ describe("validTimestamp", () => { { input: '05:23:59.244Z', expected: false, + }, + { + input: {abc: 123}, + expected: false, + }, + { + input: { + toString: '2023-06-14T05:23:59.244Z' + }, + expected: false, } ] for (const testCase of testCases) { - it(`should return ${testCase.expected} for ${testCase.input}`, () => { + it(`should return ${testCase.expected} for ${JSON.stringify(testCase.input)}`, () => { expect(validTimestamp(testCase.input)).toEqual(testCase.expected); }); } }); + +describe("isBlank", () => { + const testCases = [ + { + input: null, + expected: true + }, + { + input: "", + expected: true + }, + { + input: "test", + expected: false + }, + { + input: 1634762544, + expected: false + }, + { + input: { + toString: '2023-06-14T05:23:59.244Z' + }, + expected: false, + }, + ] + + for (const testCase of testCases) { + it(`should return ${testCase.expected} for ${JSON.stringify(testCase.input)}`, () => { + expect(isBlank(testCase.input)).toEqual(testCase.expected); + }); + } +}) \ No newline at end of file From 2901805ff4b32c8194b4812656b596f0306a9388 Mon Sep 17 00:00:00 2001 From: achettyiitr Date: Wed, 27 Dec 2023 11:38:10 +0530 Subject: [PATCH 2/4] chore: some more changes --- src/warehouse/config/helpers.js | 7 ++++--- test/__tests__/data/warehouse/events.js | 2 -- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/warehouse/config/helpers.js b/src/warehouse/config/helpers.js index f8d502c204..81b88e6986 100644 --- a/src/warehouse/config/helpers.js +++ b/src/warehouse/config/helpers.js @@ -6,10 +6,11 @@ const isNull = (x) => { }; const isBlank = (value) => { - if (isNull(value) || value === '') { - return true; + try { + return _.isEmpty(_.toString(value)); + } catch (e) { + return false; } - return _.isEmpty(JSON.stringify(value)) }; const getFirstValidValue = (message, props) => { diff --git a/test/__tests__/data/warehouse/events.js b/test/__tests__/data/warehouse/events.js index 6a740f4601..ef9cc21096 100644 --- a/test/__tests__/data/warehouse/events.js +++ b/test/__tests__/data/warehouse/events.js @@ -2081,7 +2081,6 @@ const sampleEvents = { id: "string", user_id: "string", received_at: "datetime", - event: "string" }, receivedAt: "2020-01-24T11:59:02.403+05:30" }, @@ -2149,7 +2148,6 @@ const sampleEvents = { id: "string", user_id: "string", received_at: "datetime", - event: "string", boolean_property: "boolean", }, receivedAt: "2020-01-24T11:59:02.403+05:30" From f1b64cd484133d786b5546751ba678d98542f29f Mon Sep 17 00:00:00 2001 From: achettyiitr Date: Wed, 27 Dec 2023 11:42:25 +0530 Subject: [PATCH 3/4] chore: some more changes --- test/__tests__/warehouse.test.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/__tests__/warehouse.test.js b/test/__tests__/warehouse.test.js index 1dbaabb03e..16f3404826 100644 --- a/test/__tests__/warehouse.test.js +++ b/test/__tests__/warehouse.test.js @@ -1152,8 +1152,9 @@ describe("validTimestamp", () => { } ] - for (const testCase of testCases) { - it(`should return ${testCase.expected} for ${JSON.stringify(testCase.input)}`, () => { + for (let i = 0; i < testCases.length; i++) { + const testCase = testCases[i]; + it(`should return ${testCase.expected} for testcase ${i + 1}`, () => { expect(validTimestamp(testCase.input)).toEqual(testCase.expected); }); } @@ -1185,8 +1186,9 @@ describe("isBlank", () => { }, ] - for (const testCase of testCases) { - it(`should return ${testCase.expected} for ${JSON.stringify(testCase.input)}`, () => { + for (let i = 0; i < testCases.length; i++) { + const testCase = testCases[i]; + it(`should return ${testCase.expected} for testcase ${i + 1}`, () => { expect(isBlank(testCase.input)).toEqual(testCase.expected); }); } From c437f1f6ad6008b646f30300fc02a288a164f820 Mon Sep 17 00:00:00 2001 From: achettyiitr Date: Wed, 3 Jan 2024 18:40:21 +0530 Subject: [PATCH 4/4] chore: review comments --- src/warehouse/config/helpers.js | 2 +- test/__tests__/warehouse.test.js | 52 +++++++++++++++++++++----------- 2 files changed, 36 insertions(+), 18 deletions(-) diff --git a/src/warehouse/config/helpers.js b/src/warehouse/config/helpers.js index c51fb3a43d..ef00d7ee73 100644 --- a/src/warehouse/config/helpers.js +++ b/src/warehouse/config/helpers.js @@ -10,7 +10,7 @@ const isBlank = (value) => { try { return _.isEmpty(_.toString(value)); } catch (e) { - logger.error(`Error in isBlank: ${e}`); + logger.error(`Error in isBlank: ${e.message}`); return false; } }; diff --git a/test/__tests__/warehouse.test.js b/test/__tests__/warehouse.test.js index 16f3404826..772e59e65a 100644 --- a/test/__tests__/warehouse.test.js +++ b/test/__tests__/warehouse.test.js @@ -1097,99 +1097,117 @@ describe("Integration options", () => { describe("validTimestamp", () => { const testCases = [ { + name: "undefined input should return false", input: undefined, expected: false, }, { + name: "negative year and time input should return false #1", input: '-0001-11-30T00:00:00+0000', expected: false, }, { + name: "negative year and time input should return false #2", input: '-2023-06-14T05:23:59.244Z', expected: false, }, { + name: "negative year and time input should return false #3", + input: '-1900-06-14T05:23:59.244Z', + expected: false, + }, + { + name: "positive year and time input should return false", input: '+2023-06-14T05:23:59.244Z', expected: false, }, { + name: "valid timestamp input should return true", input: '2023-06-14T05:23:59.244Z', expected: true, }, { - input: '-1900-06-14T05:23:59.244Z', - expected: false, - }, - { + name: "non-date string input should return false", input: 'abc', expected: false, }, { - input: '%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216Windows%u2216win%u002ein', + name: "malicious string input should return false", + input: '%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216%u002e%u002e%u2216Windows%u2216win%u002ein', expected: false, }, { + name: "empty string input should return false", input: '', expected: false, }, { + name: "valid date input should return true", input: '2023-06-14', expected: true, }, { + name: "time-only input should return false", input: '05:23:59.244Z', expected: false, }, { - input: {abc: 123}, + name: "non-string input should return false", + input: { abc: 123 }, expected: false, }, { + name: "object with toString method input should return false", input: { toString: '2023-06-14T05:23:59.244Z' }, expected: false, - } - ] + }, + ]; - for (let i = 0; i < testCases.length; i++) { - const testCase = testCases[i]; - it(`should return ${testCase.expected} for testcase ${i + 1}`, () => { + for (const testCase of testCases) { + it(`should return ${testCase.expected} for ${testCase.name}`, () => { expect(validTimestamp(testCase.input)).toEqual(testCase.expected); }); } }); + + describe("isBlank", () => { const testCases = [ { + name: "null", input: null, expected: true }, { + name: "empty string", input: "", expected: true }, { + name: "non-empty string", input: "test", expected: false }, { + name: "numeric value", input: 1634762544, expected: false }, { + name: "object with toString property", input: { toString: '2023-06-14T05:23:59.244Z' }, - expected: false, + expected: false }, - ] + ]; - for (let i = 0; i < testCases.length; i++) { - const testCase = testCases[i]; - it(`should return ${testCase.expected} for testcase ${i + 1}`, () => { + for (const testCase of testCases) { + it(`should return ${testCase.expected} for ${testCase.name}`, () => { expect(isBlank(testCase.input)).toEqual(testCase.expected); }); } -}) \ No newline at end of file +}); \ No newline at end of file