Skip to content

Commit 3f5237c

Browse files
shekhar-ruddershekhar-rudder
committed
chore: review comments 1
1 parent 425464a commit 3f5237c

File tree

8 files changed

+662
-548
lines changed

8 files changed

+662
-548
lines changed

‎src/v0/destinations/fb_custom_audience/config.ts‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,8 @@ const subTypeFields: readonly string[] = [
9393
'DATA_FILE',
9494
];
9595

96+
const DESTINATION = 'fb_custom_audience';
97+
9698
const USER_ADD = 'add';
9799
const USER_DELETE = 'remove';
98100
// https://developers.facebook.com/docs/marketing-api/audiences/guides/custom-audiences/
@@ -102,15 +104,6 @@ and error method we found that 65000 bytes is the maximum payload allowed size b
102104
*/
103105
const DEFAULT_MAX_PAYLOAD_SIZE = 60000; // bytes
104106

105-
/**
106-
* Returns whether hashing validation is enabled.
107-
* Can be enabled via env var FB_CUSTOM_AUDIENCE_HASHING_VALIDATION_ENABLED=true.
108-
* Defaults to false (disabled).
109-
*/
110-
function isHashingValidationEnabled(): boolean {
111-
return process.env.FB_CUSTOM_AUDIENCE_HASHING_VALIDATION_ENABLED === 'true';
112-
}
113-
114107
/**
115108
* Returns the maximum payload size in bytes for FB Custom Audience batching.
116109
* Can be overridden per workspace via env var FB_CUSTOM_AUDIENCE_MAX_PAYLOAD_SIZE_<WORKSPACE_ID>,
@@ -134,6 +127,7 @@ function getMaxPayloadSize(workspaceId: string): number {
134127
}
135128

136129
export {
130+
DESTINATION,
137131
ENDPOINT_PATH,
138132
getEndPoint,
139133
schemaFields,
@@ -143,5 +137,4 @@ export {
143137
typeFields,
144138
subTypeFields,
145139
getMaxPayloadSize,
146-
isHashingValidationEnabled,
147140
};

‎src/v0/destinations/fb_custom_audience/recordTransform.ts‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,6 @@ const processRecord = (
5353
disableFormat: boolean | undefined,
5454
): { dataElement: unknown[]; metadata: Metadata } => {
5555
const fields = record.message.fields!;
56-
const { workspaceId } = record.metadata;
57-
const destinationId = record.destination.ID;
5856
let dataElement: unknown[] = [];
5957
let nullUserData = true;
6058

@@ -71,8 +69,8 @@ const processRecord = (
7169
isHashRequired,
7270
eachProperty,
7371
updatedProperty,
74-
workspaceId,
75-
destinationId,
72+
record.metadata.workspaceId,
73+
record.destination.ID,
7674
);
7775

7876
if (dataElement[dataElement.length - 1]) {

‎src/v0/destinations/fb_custom_audience/util.test.ts‎

Lines changed: 87 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ jest.mock('../../../util/stats', () => ({
66

77
import stats from '../../../util/stats';
88
import type { WrappedResponse } from './types';
9-
109
const basePayload = {
1110
responseField: {
1211
access_token: 'ABC',
@@ -51,6 +50,9 @@ const baseResponse = {
5150
method: '',
5251
};
5352

53+
const TEST_WORKSPACE_ID = 'ws-1';
54+
const TEST_DESTINATION_ID = 'dest-1';
55+
5456
describe('FB_custom_audience utils test', () => {
5557
describe('getDataSource function tests', () => {
5658
it('Should return empty datasource if type and subType are both NA', () => {
@@ -108,12 +110,12 @@ describe('FB_custom_audience utils test', () => {
108110
expected: ['59107c750fd5ee2758d1988f2bf12d9f110439221ebdb7997e70d6a2c1c5afda'],
109111
},
110112
{
111-
name: 'Should not hash field if isHashRequired is set to false (pre-hashed input is passed through as-is)',
113+
name: 'Should not hash field if isHashRequired is set to false',
112114
initialData: [],
113115
isHashRequired: false,
114116
field: 'FN',
115-
value: '59107c750fd5ee2758d1988f2bf12d9f110439221ebdb7997e70d6a2c1c5afda',
116-
expected: ['59107c750fd5ee2758d1988f2bf12d9f110439221ebdb7997e70d6a2c1c5afda'],
117+
value: 'some-name',
118+
expected: ['some-name'],
117119
},
118120
{
119121
name: 'Should not hash MADID and just pass value',
@@ -212,8 +214,8 @@ describe('FB_custom_audience utils test', () => {
212214
isHashRequired,
213215
field,
214216
value,
215-
'ws-1',
216-
'dest-1',
217+
TEST_WORKSPACE_ID,
218+
TEST_DESTINATION_ID,
217219
);
218220
expect(result).toEqual(expected);
219221
});
@@ -229,87 +231,121 @@ describe('FB_custom_audience utils test', () => {
229231
});
230232

231233
afterEach(() => {
232-
delete process.env.FB_CUSTOM_AUDIENCE_HASHING_VALIDATION_ENABLED;
234+
delete process.env.AUDIENCE_HASHING_VALIDATION_ENABLED;
233235
});
234236

235237
it('Hashing ON + pre-hashed value → emits metric and throws when validation enabled', () => {
236-
process.env.FB_CUSTOM_AUDIENCE_HASHING_VALIDATION_ENABLED = 'true';
238+
process.env.AUDIENCE_HASHING_VALIDATION_ENABLED = 'true';
237239
expect(() =>
238-
getUpdatedDataElement([], true, 'EMAIL', hashedValue, 'ws-1', 'dest-1'),
240+
getUpdatedDataElement(
241+
[],
242+
true,
243+
'EMAIL',
244+
hashedValue,
245+
TEST_WORKSPACE_ID,
246+
TEST_DESTINATION_ID,
247+
),
239248
).toThrow(
240249
'Hashing is enabled but the value for field EMAIL appears to already be hashed. Either disable hashing or send unhashed data.',
241250
);
242-
expect(mockStatsIncrement).toHaveBeenCalledWith(
243-
'fb_custom_audience_hashing_inconsistency',
244-
{
245-
propertyName: 'EMAIL',
246-
type: 'hashed_when_hash_enabled',
247-
workspaceId: 'ws-1',
248-
destinationId: 'dest-1',
249-
},
250-
);
251+
expect(mockStatsIncrement).toHaveBeenCalledWith('audience_hashing_inconsistency', {
252+
propertyName: 'EMAIL',
253+
type: 'hashed_when_hash_enabled',
254+
workspaceId: 'ws-1',
255+
destinationId: 'dest-1',
256+
destType: 'fb_custom_audience',
257+
});
251258
});
252259

253260
it('Hashing ON + plaintext value → no error, no metric', () => {
254261
expect(() =>
255-
getUpdatedDataElement([], true, 'EMAIL', plaintextEmail, 'ws-1', 'dest-1'),
262+
getUpdatedDataElement(
263+
[],
264+
true,
265+
'EMAIL',
266+
plaintextEmail,
267+
TEST_WORKSPACE_ID,
268+
TEST_DESTINATION_ID,
269+
),
256270
).not.toThrow();
257271
expect(mockStatsIncrement).not.toHaveBeenCalled();
258272
});
259273

260274
it('Hashing OFF + plaintext value → emits metric and throws when validation enabled', () => {
261-
process.env.FB_CUSTOM_AUDIENCE_HASHING_VALIDATION_ENABLED = 'true';
275+
process.env.AUDIENCE_HASHING_VALIDATION_ENABLED = 'true';
262276
expect(() =>
263-
getUpdatedDataElement([], false, 'EMAIL', plaintextEmail, 'ws-1', 'dest-1'),
277+
getUpdatedDataElement(
278+
[],
279+
false,
280+
'EMAIL',
281+
plaintextEmail,
282+
TEST_WORKSPACE_ID,
283+
TEST_DESTINATION_ID,
284+
),
264285
).toThrow(
265286
'Hashing is disabled but the value for field EMAIL appears to be unhashed. Either enable hashing or send pre-hashed data.',
266287
);
267-
expect(mockStatsIncrement).toHaveBeenCalledWith(
268-
'fb_custom_audience_hashing_inconsistency',
269-
{
270-
propertyName: 'EMAIL',
271-
type: 'unhashed_when_hash_disabled',
272-
workspaceId: 'ws-1',
273-
destinationId: 'dest-1',
274-
},
275-
);
288+
expect(mockStatsIncrement).toHaveBeenCalledWith('audience_hashing_inconsistency', {
289+
propertyName: 'EMAIL',
290+
type: 'unhashed_when_hash_disabled',
291+
workspaceId: 'ws-1',
292+
destinationId: 'dest-1',
293+
destType: 'fb_custom_audience',
294+
});
276295
});
277296

278297
it('Hashing OFF + 64-char hex value → no error, no metric', () => {
279298
expect(() =>
280-
getUpdatedDataElement([], false, 'EMAIL', hashedValue, 'ws-1', 'dest-1'),
299+
getUpdatedDataElement(
300+
[],
301+
false,
302+
'EMAIL',
303+
hashedValue,
304+
TEST_WORKSPACE_ID,
305+
TEST_DESTINATION_ID,
306+
),
281307
).not.toThrow();
282308
expect(mockStatsIncrement).not.toHaveBeenCalled();
283309
});
284310

285311
it('Validation disabled (default) + hashing ON + pre-hashed value → emits metric but no throw', () => {
286312
expect(() =>
287-
getUpdatedDataElement([], true, 'EMAIL', hashedValue, 'ws-1', 'dest-1'),
313+
getUpdatedDataElement(
314+
[],
315+
true,
316+
'EMAIL',
317+
hashedValue,
318+
TEST_WORKSPACE_ID,
319+
TEST_DESTINATION_ID,
320+
),
288321
).not.toThrow();
289-
expect(mockStatsIncrement).toHaveBeenCalledWith(
290-
'fb_custom_audience_hashing_inconsistency',
291-
{
292-
propertyName: 'EMAIL',
293-
type: 'hashed_when_hash_enabled',
294-
workspaceId: 'ws-1',
295-
destinationId: 'dest-1',
296-
},
297-
);
322+
expect(mockStatsIncrement).toHaveBeenCalledWith('audience_hashing_inconsistency', {
323+
propertyName: 'EMAIL',
324+
type: 'hashed_when_hash_enabled',
325+
workspaceId: 'ws-1',
326+
destinationId: 'dest-1',
327+
destType: 'fb_custom_audience',
328+
});
298329
});
299330

300331
it('Validation disabled (default) + hashing OFF + plaintext value → emits metric but no throw', () => {
301332
expect(() =>
302-
getUpdatedDataElement([], false, 'EMAIL', plaintextEmail, 'ws-1', 'dest-1'),
333+
getUpdatedDataElement(
334+
[],
335+
false,
336+
'EMAIL',
337+
plaintextEmail,
338+
TEST_WORKSPACE_ID,
339+
TEST_DESTINATION_ID,
340+
),
303341
).not.toThrow();
304-
expect(mockStatsIncrement).toHaveBeenCalledWith(
305-
'fb_custom_audience_hashing_inconsistency',
306-
{
307-
propertyName: 'EMAIL',
308-
type: 'unhashed_when_hash_disabled',
309-
workspaceId: 'ws-1',
310-
destinationId: 'dest-1',
311-
},
312-
);
342+
expect(mockStatsIncrement).toHaveBeenCalledWith('audience_hashing_inconsistency', {
343+
propertyName: 'EMAIL',
344+
type: 'unhashed_when_hash_disabled',
345+
workspaceId: 'ws-1',
346+
destinationId: 'dest-1',
347+
destType: 'fb_custom_audience',
348+
});
313349
});
314350
});
315351
});

‎src/v0/destinations/fb_custom_audience/util.ts‎

Lines changed: 9 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -15,53 +15,15 @@ import type {
1515
FbRecordMessage,
1616
WrappedResponse,
1717
} from './types';
18-
import { typeFields, subTypeFields, getEndPoint, isHashingValidationEnabled } from './config';
18+
import { typeFields, subTypeFields, getEndPoint, DESTINATION } from './config';
1919
import {
2020
defaultRequestConfig,
2121
defaultPostRequestConfig,
2222
defaultDeleteRequestConfig,
2323
} from '../../util';
2424
import stats from '../../../util/stats';
2525
import * as config from './config';
26-
27-
const HASHED_VALUE_REGEX = /^[\da-f]{64}$/;
28-
29-
const validateHashingConsistency = (
30-
propertyName: string,
31-
normalizedValue: string,
32-
isHashRequired: boolean,
33-
workspaceId: string,
34-
destinationId: string,
35-
): void => {
36-
if (!normalizedValue) return;
37-
const isAlreadyHashed = HASHED_VALUE_REGEX.test(normalizedValue);
38-
if (isHashRequired && isAlreadyHashed) {
39-
stats.increment('fb_custom_audience_hashing_inconsistency', {
40-
propertyName,
41-
type: 'hashed_when_hash_enabled',
42-
workspaceId,
43-
destinationId,
44-
});
45-
if (isHashingValidationEnabled()) {
46-
throw new InstrumentationError(
47-
`Hashing is enabled but the value for field ${propertyName} appears to already be hashed. Either disable hashing or send unhashed data.`,
48-
);
49-
}
50-
}
51-
if (!isHashRequired && !isAlreadyHashed) {
52-
stats.increment('fb_custom_audience_hashing_inconsistency', {
53-
propertyName,
54-
type: 'unhashed_when_hash_disabled',
55-
workspaceId,
56-
destinationId,
57-
});
58-
if (isHashingValidationEnabled()) {
59-
throw new InstrumentationError(
60-
`Hashing is disabled but the value for field ${propertyName} appears to be unhashed. Either enable hashing or send pre-hashed data.`,
61-
);
62-
}
63-
}
64-
};
26+
import { validateHashingConsistency } from '../../util/audienceUtils';
6527

6628
/**
6729
* Example payload ={
@@ -197,6 +159,12 @@ const getUpdatedDataElement = (
197159
workspaceId: string,
198160
destinationId: string,
199161
): unknown[] => {
162+
const destination = {
163+
workspaceId,
164+
id: destinationId,
165+
type: DESTINATION,
166+
config: { isHashRequired },
167+
};
200168
// Normalize undefined/null to empty string
201169
const normalizedValue = propertyValue ?? '';
202170

@@ -222,13 +190,7 @@ const getUpdatedDataElement = (
222190
const shouldHash = isHashRequired && isHashableField;
223191

224192
if (isHashableField) {
225-
validateHashingConsistency(
226-
propertyName,
227-
String(normalizedValue),
228-
isHashRequired,
229-
workspaceId,
230-
destinationId,
231-
);
193+
validateHashingConsistency(propertyName, String(normalizedValue), destination);
232194
}
233195

234196
if (shouldHash) {

0 commit comments

Comments
 (0)