Skip to content

Commit 26fc5d6

Browse files
sunitaprajapati89sunitaprajapati89
authored
fix: The proxy URL is being altered in version 2.20.4, resulting in invalid URL (#1058)
* fix: The proxy URL is being altered in version 2.20.4, resulting in an invalid URL * fix: handled the case of appending '/b' * refactor-removed console statements * refactor: refactored getEndpoint() to check if baseURL ends with /b * refactor: added new test cases for getEndpoint() * refactor: added validateURL() method, new test cases for SegmentDestination * refactor: added getEndpointForSettings and related test cases * refactor: updated fetchSettings.test.ts * fix: commit on order to run e2e tests on CI * refactor: added test cases for http, add review point changes * refactor: merged if/else statements * refactor: removed unnecessary comments * refactor: updated regex * refactor: removed write key from e2e * refactor: removed redundant statement * refactor: Instead ofd throw error, returned default values in getEndpoints() & getEndpointForSettings() * refactor: refactored test cases
1 parent a13fc91 commit 26fc5d6

File tree

6 files changed

+353
-32
lines changed

6 files changed

+353
-32
lines changed

‎packages/core/src/__tests__/internal/fetchSettings.test.ts

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -281,4 +281,158 @@ describe('internal #getSettings', () => {
281281
expect(setSettingsSpy).not.toHaveBeenCalled();
282282
}
283283
);
284+
describe('getEndpointForSettings', () => {
285+
it.each([
286+
['example.com/v1/', 'https://example.com/v1/'],
287+
['https://example.com/v1/', 'https://example.com/v1/'],
288+
['http://example.com/v1/', 'http://example.com/v1/'],
289+
])(
290+
'should append projects/key/settings if proxy end with / and useSegmentEndpoint is true',
291+
(cdnProxy, expectedBaseURL) => {
292+
const config = {
293+
...clientArgs.config,
294+
useSegmentEndpoints: true,
295+
cdnProxy: cdnProxy,
296+
};
297+
const anotherClient = new SegmentClient({
298+
...clientArgs,
299+
config,
300+
});
301+
const spy = jest.spyOn(
302+
Object.getPrototypeOf(anotherClient),
303+
'getEndpointForSettings'
304+
);
305+
expect(anotherClient['getEndpointForSettings']()).toBe(
306+
`${expectedBaseURL}projects/${config.writeKey}/settings`
307+
);
308+
expect(spy).toHaveBeenCalled();
309+
}
310+
);
311+
it.each([
312+
['example.com/v1/projects/', 'https://example.com/v1/projects/'],
313+
['https://example.com/v1/projects/', 'https://example.com/v1/projects/'],
314+
['http://example.com/v1/projects/', 'http://example.com/v1/projects/'],
315+
])(
316+
'should append projects/writeKey/settings if proxy ends with projects/ and useSegmentEndpoint is true',
317+
(cdnProxy, expectedBaseURL) => {
318+
const config = {
319+
...clientArgs.config,
320+
useSegmentEndpoints: true,
321+
cdnProxy: cdnProxy,
322+
};
323+
const anotherClient = new SegmentClient({
324+
...clientArgs,
325+
config,
326+
});
327+
328+
const spy = jest.spyOn(
329+
Object.getPrototypeOf(anotherClient),
330+
'getEndpointForSettings'
331+
);
332+
expect(anotherClient['getEndpointForSettings']()).toBe(
333+
`${expectedBaseURL}projects/${config.writeKey}/settings`
334+
);
335+
expect(spy).toHaveBeenCalled();
336+
}
337+
);
338+
it.each([
339+
['example.com/v1/projects', 'https://example.com/v1/projects'],
340+
['https://example.com/v1/projects', 'https://example.com/v1/projects'],
341+
['http://example.com/v1/projects', 'http://example.com/v1/projects'],
342+
])(
343+
'should append /projects/writeKey/settings if proxy ends with /projects and useSegmentEndpoint is true',
344+
(cdnProxy, expectedBaseURL) => {
345+
const config = {
346+
...clientArgs.config,
347+
useSegmentEndpoints: true,
348+
cdnProxy: cdnProxy,
349+
};
350+
const anotherClient = new SegmentClient({
351+
...clientArgs,
352+
config,
353+
});
354+
355+
const spy = jest.spyOn(
356+
Object.getPrototypeOf(anotherClient),
357+
'getEndpointForSettings'
358+
);
359+
expect(anotherClient['getEndpointForSettings']()).toBe(
360+
`${expectedBaseURL}/projects/${config.writeKey}/settings`
361+
);
362+
expect(spy).toHaveBeenCalled();
363+
}
364+
);
365+
it.each([
366+
['example.com/v1?params=xx'],
367+
['https://example.com/v1?params=xx'],
368+
['http://example.com/v1?params=xx'],
369+
])(
370+
'should throw an error if proxy comes with query params and useSegmentEndpoint is true',
371+
(cdnProxy) => {
372+
const config = {
373+
...clientArgs.config,
374+
useSegmentEndpoints: true,
375+
cdnProxy: cdnProxy,
376+
};
377+
const anotherClient = new SegmentClient({
378+
...clientArgs,
379+
config,
380+
});
381+
382+
const spy = jest.spyOn(
383+
Object.getPrototypeOf(anotherClient),
384+
'getEndpointForSettings'
385+
);
386+
// Expect the private method to throw an error
387+
expect(anotherClient['getEndpointForSettings']()).toBe(
388+
`${settingsCDN}/${config.writeKey}/settings`
389+
);
390+
expect(spy).toHaveBeenCalled();
391+
}
392+
);
393+
it.each([
394+
['example.com/v1/', false],
395+
['example.com/v1/projects/', false],
396+
['example.com/v1/projects', false],
397+
['example.com/v1?params=xx', false],
398+
])(
399+
'should always return identical result if proxy is provided and useSegmentEndpoints is false',
400+
(cdnProxy, useSegmentEndpoints) => {
401+
const config = {
402+
...clientArgs.config,
403+
useSegmentEndpoints: useSegmentEndpoints,
404+
cdnProxy: cdnProxy,
405+
};
406+
const anotherClient = new SegmentClient({
407+
...clientArgs,
408+
config,
409+
});
410+
const spy = jest.spyOn(
411+
Object.getPrototypeOf(anotherClient),
412+
'getEndpointForSettings'
413+
);
414+
const expected = `https://${cdnProxy}`;
415+
expect(anotherClient['getEndpointForSettings']()).toBe(expected);
416+
expect(spy).toHaveBeenCalled();
417+
}
418+
);
419+
it('No cdn proxy provided, should return default settings CDN', () => {
420+
const config = {
421+
...clientArgs.config,
422+
useSegmentEndpoints: true, // No matter in this case
423+
};
424+
const anotherClient = new SegmentClient({
425+
...clientArgs,
426+
config,
427+
});
428+
const spy = jest.spyOn(
429+
Object.getPrototypeOf(anotherClient),
430+
'getEndpointForSettings'
431+
);
432+
expect(anotherClient['getEndpointForSettings']()).toBe(
433+
`${settingsCDN}/${config.writeKey}/settings`
434+
);
435+
expect(spy).toHaveBeenCalled();
436+
});
437+
});
284438
});

‎packages/core/src/__tests__/util.test.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,7 @@ describe('getURL function', () => {
172172
});
173173

174174
it('should return the root URL when the path is empty', () => {
175-
expect(getURL('www.example.com', '')).toBe('https://www.example.com/');
175+
expect(getURL('www.example.com', '')).toBe('https://www.example.com');
176176
});
177177

178178
it('should handle query parameters correctly in the URL path', () => {
@@ -188,13 +188,13 @@ describe('getURL function', () => {
188188
});
189189

190190
// Negative Test Cases
191-
it('should handle empty host gracefully', () => {
192-
expect(getURL('', '/home')).toBe('https:///home');
191+
it('should throw an error for empty host', () => {
192+
expect(() => getURL('', '/home')).toThrow('Invalid URL has been passed');
193193
});
194194

195-
it('should handle invalid characters in the host', () => {
196-
expect(getURL('invalid host.com', '/path')).toBe(
197-
'https://invalid host.com/path'
195+
it('should throw an error for invalid characters in the host', () => {
196+
expect(() => getURL('invalid host.com', '/path')).toThrow(
197+
'Invalid URL has been passed'
198198
);
199199
});
200200
});

‎packages/core/src/analytics.ts

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -320,22 +320,37 @@ export class SegmentClient {
320320

321321
return map;
322322
}
323-
324-
async fetchSettings() {
325-
const settingsPrefix: string = this.config.cdnProxy ?? settingsCDN;
323+
private getEndpointForSettings(): string {
324+
let settingsPrefix = '';
325+
let settingsEndpoint = '';
326326
const hasProxy = !!(this.config?.cdnProxy ?? '');
327327
const useSegmentEndpoints = Boolean(this.config?.useSegmentEndpoints);
328-
let settingsEndpoint = '';
328+
329329
if (hasProxy) {
330+
settingsPrefix = this.config.cdnProxy ?? '';
330331
if (useSegmentEndpoints) {
331-
settingsEndpoint = `/projects/${this.config.writeKey}/settings`;
332-
} else {
333-
settingsEndpoint = '';
332+
const isCdnProxyEndsWithSlash = settingsPrefix.endsWith('/');
333+
settingsEndpoint = isCdnProxyEndsWithSlash
334+
? `projects/${this.config.writeKey}/settings`
335+
: `/projects/${this.config.writeKey}/settings`;
334336
}
335337
} else {
338+
settingsPrefix = settingsCDN;
336339
settingsEndpoint = `/${this.config.writeKey}/settings`;
337340
}
338-
const settingsURL = getURL(settingsPrefix, settingsEndpoint);
341+
try {
342+
return getURL(settingsPrefix, settingsEndpoint);
343+
} catch (error) {
344+
console.error(
345+
'Error in getEndpointForSettings:',
346+
`fallback to ${settingsCDN}/${this.config.writeKey}/settings`
347+
);
348+
return `${settingsCDN}/${this.config.writeKey}/settings`;
349+
}
350+
}
351+
352+
async fetchSettings() {
353+
const settingsURL = this.getEndpointForSettings();
339354
try {
340355
const res = await fetch(settingsURL, {
341356
headers: {

‎packages/core/src/plugins/SegmentDestination.ts

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -92,17 +92,24 @@ export class SegmentDestination extends DestinationPlugin {
9292
const config = this.analytics?.getConfig();
9393
const hasProxy = !!(config?.proxy ?? '');
9494
const useSegmentEndpoints = Boolean(config?.useSegmentEndpoints);
95-
95+
let baseURL = '';
9696
let endpoint = '';
97-
9897
if (hasProxy) {
99-
endpoint = useSegmentEndpoints ? '/b' : '';
98+
//baseURL is always config?.proxy if hasProxy
99+
baseURL = config?.proxy ?? '';
100+
if (useSegmentEndpoints) {
101+
const isProxyEndsWithSlash = baseURL.endsWith('/');
102+
endpoint = isProxyEndsWithSlash ? 'b' : '/b';
103+
}
100104
} else {
101-
endpoint = '/b'; // If no proxy, always append '/b'
105+
baseURL = this.apiHost ?? defaultApiHost;
106+
}
107+
try {
108+
return getURL(baseURL, endpoint);
109+
} catch (error) {
110+
console.error('Error in getEndpoint:', `fallback to ${defaultApiHost}`);
111+
return defaultApiHost;
102112
}
103-
104-
const baseURL = config?.proxy ?? this.apiHost ?? defaultApiHost;
105-
return getURL(baseURL, endpoint);
106113
}
107114
configure(analytics: SegmentClient): void {
108115
super.configure(analytics);
@@ -128,7 +135,7 @@ export class SegmentDestination extends DestinationPlugin {
128135
segmentSettings?.apiHost !== null
129136
) {
130137
//assign the api host from segment settings (domain/v1)
131-
this.apiHost = segmentSettings.apiHost;
138+
this.apiHost = `https://${segmentSettings.apiHost}/b`;
132139
}
133140
this.settingsResolve();
134141
}

0 commit comments

Comments
 (0)