Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: improve headers parsing #243

Merged
merged 7 commits into from
Jan 31, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions src/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ test('Should perform an initial fetch as POST', async () => {
expect(request.method).toBe('POST');
expect(body.context.appName).toBe('webAsPOST');
expect(request.headers).toMatchObject({
'Content-Type': 'application/json',
'content-type': 'application/json',
});
});

Expand Down Expand Up @@ -650,7 +650,7 @@ test('Should abort previous request', async () => {
client.updateContext({ userId: '456' }); // abort 2
await client.updateContext({ userId: '789' });

expect(abortSpy).toBeCalledTimes(2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how are they different?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one is deprecated. just a rename in a library - no real difference

expect(abortSpy).toHaveBeenCalledTimes(2);
abortSpy.mockRestore();
});

Expand Down Expand Up @@ -690,7 +690,7 @@ test('Should run without abort controller', async () => {
client.updateContext({ userId: '456' });
await client.updateContext({ userId: '789' });

expect(abortSpy).toBeCalledTimes(0);
expect(abortSpy).toHaveBeenCalledTimes(0);
abortSpy.mockRestore();
});

Expand Down Expand Up @@ -783,7 +783,7 @@ test('Should include etag in second request', async () => {

expect(firstRequest.headers).toMatchObject({});
expect(secondRequest.headers).toMatchObject({
'If-None-Match': etag,
'if-none-match': etag,
});
});

Expand All @@ -805,7 +805,7 @@ test('Should add clientKey as Authorization header', async () => {
const request = getTypeSafeRequest(fetchMock);

expect(request.headers).toMatchObject({
Authorization: 'some123key',
authorization: 'some123key',
});
});

Expand Down Expand Up @@ -1037,7 +1037,7 @@ test('Updating context should wait on asynchronous start', async () => {
userId: '123',
});

expect(fetchMock).toBeCalledTimes(2);
expect(fetchMock).toHaveBeenCalledTimes(2);
});

test('Should not replace sessionId when updating context', async () => {
Expand Down Expand Up @@ -1254,7 +1254,7 @@ test('Initializing client twice should show a console warning', async () => {
await client.start();
await client.start();
// Expect console.error to be called once before start runs.
expect(console.error).toBeCalledTimes(2);
expect(console.error).toHaveBeenCalledTimes(2);
});

test('Should pass under custom header clientKey', async () => {
Expand Down Expand Up @@ -1356,7 +1356,7 @@ test('Should pass custom headers', async () => {
});
});

test('Should add `x-unleash` headers', async () => {
test('Should add unleash identification headers', async () => {
fetchMock.mockResponses(
[JSON.stringify(data), { status: 200 }],
[JSON.stringify(data), { status: 200 }]
Expand All @@ -1382,13 +1382,13 @@ test('Should add `x-unleash` headers', async () => {

const expectedHeaders = {
// will be replaced at build time with the actual version
'x-unleash-sdk': 'unleash-js@__VERSION__',
'x-unleash-connection-id': expect.stringMatching(uuidFormat),
'x-unleash-appname': appName,
'unleash-sdk': 'unleash-client-js:__VERSION__',
'unleash-connection-id': expect.stringMatching(uuidFormat),
'unleash-appname': appName,
};

const getConnectionId = (request: any) =>
request.headers['x-unleash-connection-id'];
request.headers['unleash-connection-id'];

expect(featureRequest.headers).toMatchObject(expectedHeaders);
expect(metricsRequest.headers).toMatchObject(expectedHeaders);
Expand Down
30 changes: 10 additions & 20 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,9 @@ import LocalStorageProvider from './storage-provider-local';
import EventsHandler from './events-handler';
import {
computeContextHashValue,
notNullOrUndefined,
parseHeaders,
urlWithContextAsQuery,
} from './util';
import { sdkVersion } from './version';
import { uuidv4 } from './uuidv4';

const DEFINED_FIELDS = [
Expand Down Expand Up @@ -470,24 +469,15 @@ export class UnleashClient extends TinyEmitter {
}

private getHeaders() {
const isPOST = this.usePOSTrequests;
const headers = {
[this.headerName]: this.clientKey,
Accept: 'application/json',
'x-unleash-sdk': sdkVersion,
'x-unleash-connection-id': this.connectionId,
'x-unleash-appname': this.context.appName,
};
if (isPOST) {
headers['Content-Type'] = 'application/json';
}
if (this.etag) {
headers['If-None-Match'] = this.etag;
}
Object.entries(this.customHeaders)
.filter(notNullOrUndefined)
.forEach(([name, value]) => (headers[name] = value));
return headers;
return parseHeaders({
clientKey: this.clientKey,
connectionId: this.connectionId,
appName: this.context.appName,
customHeaders: this.customHeaders,
headerName: this.headerName,
etag: this.etag,
isPost: this.usePOSTrequests,
});
}

private async storeToggles(toggles: IToggle[]): Promise<void> {
Expand Down
35 changes: 33 additions & 2 deletions src/metrics.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ test('should send metrics with custom auth header', async () => {

expect(fetchMock.mock.calls.length).toEqual(1);
expect(requestBody.headers).toMatchObject({
NotAuthorization: '123',
notauthorization: '123',
});
});

Expand Down Expand Up @@ -271,7 +271,7 @@ describe('Custom headers for metrics', () => {

test('Custom headers should override preset headers', async () => {
const customHeaders = {
Authorization: 'definitely-not-the-client-key',
authorization: 'definitely-not-the-client-key',
};

const requestBody = await runMetrics(customHeaders);
Expand Down Expand Up @@ -303,4 +303,35 @@ describe('Custom headers for metrics', () => {
expect(requestBody.headers).not.toMatchObject(customHeaders);
}
);

test('Should use case-insensitive headers', () => {
const metrics = new Metrics({
onError: console.error,
appName: 'test',
metricsInterval: 5,
disableMetrics: false,
url: 'http://localhost:3000',
clientKey: '123',
fetch: fetchMock,
headerName: 'Authorization',
customHeaders: {
'Custom-Header': '123',
'custom-header': '456',
'unleash-APPname': 'override',
'unleash-connection-id': 'override',
},
connectionId: '123',
metricsIntervalInitial: 2,
});

metrics.count('foo', true);
metrics.sendMetrics();

const requestBody = getTypeSafeRequest(fetchMock);
expect(requestBody.headers).toMatchObject({
'custom-header': '456',
'unleash-appname': 'override',
'unleash-connection-id': '123',
});
});
});
25 changes: 9 additions & 16 deletions src/metrics.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// Simplified version of: https://github.com/Unleash/unleash-client-node/blob/main/src/metrics.ts

import { notNullOrUndefined } from './util';
import { sdkVersion } from './version';
import { parseHeaders } from './util';

export interface MetricsOptions {
onError: OnError;
Expand Down Expand Up @@ -122,20 +121,14 @@ export default class Metrics {
}

private getHeaders() {
const headers = {
[this.headerName]: this.clientKey,
Accept: 'application/json',
'Content-Type': 'application/json',
'x-unleash-sdk': sdkVersion,
'x-unleash-connection-id': this.connectionId,
'x-unleash-appname': this.appName,
};

Object.entries(this.customHeaders)
.filter(notNullOrUndefined)
.forEach(([name, value]) => (headers[name] = value));

return headers;
return parseHeaders({
clientKey: this.clientKey,
appName: this.appName,
connectionId: this.connectionId,
customHeaders: this.customHeaders,
headerName: this.headerName,
isPost: true,
});
}

public async sendMetrics(): Promise<void> {
Expand Down
117 changes: 117 additions & 0 deletions src/util.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
computeContextHashValue,
contextString,
urlWithContextAsQuery,
parseHeaders,
} from './util';

test('should not add paramters to URL', async () => {
Expand Down Expand Up @@ -119,3 +120,119 @@ describe('computeContextHashValue', () => {
);
});
});

describe('parseHeaders', () => {
const clientKey = 'test-client-key';
const appName = 'appName';
const connectionId = '1234-5678';

test('should set basic headers correctly', () => {
const result = parseHeaders({
clientKey,
appName,
connectionId,
});

expect(result).toEqual({
authorization: clientKey,
'unleash-sdk': 'unleash-client-js:__VERSION__',
'unleash-appname': appName,
accept: 'application/json',
'unleash-connection-id': connectionId,
});
});

test('should set custom client key header name', () => {
const result = parseHeaders({
clientKey,
appName,
connectionId,
headerName: 'auth',
});

expect(result).toMatchObject({ auth: clientKey });
expect(Object.keys(result)).not.toContain('authorization');
});

test('should add custom headers', () => {
const customHeaders = {
'custom-header': 'custom-value',
'unleash-connection-id': 'should-not-be-overwritten',
'unleash-appname': 'new-app-name',
};
const result = parseHeaders({
clientKey,
appName,
connectionId,
customHeaders,
});

expect(Object.keys(result)).toHaveLength(6);
expect(result).toMatchObject({
'custom-header': 'custom-value',
'unleash-connection-id': connectionId,
'unleash-appname': 'new-app-name',
});
});

test('should include etag if provided', () => {
const etag = '12345';
const result = parseHeaders({
clientKey,
appName,
connectionId,
etag,
});

expect(result['if-none-match']).toBe(etag);
});

test('should handle custom headers in a case-insensitive way and normalize them', () => {
const customHeaders = {
'custom-HEADER': 'custom-value-1',
'Custom-Header': 'custom-value-2',
};
const result = parseHeaders({
clientKey,
appName,
connectionId,
customHeaders,
});

expect(result).toMatchObject({
'custom-header': 'custom-value-2',
});
});

test('should ignore null or undefined custom headers', () => {
const customHeaders = {
header1: 'value1',
header2: null as any,
header3: undefined as any,
};
const result = parseHeaders({
clientKey,
appName,
connectionId,
customHeaders,
});

expect(result).toEqual(
expect.not.objectContaining({
header2: expect.anything(),
header3: expect.anything(),
})
);
});

test('should include content-type for POST requests', () => {
const result = parseHeaders({
clientKey,
appName,
connectionId,
isPost: true,
});

expect(result['content-type']).toBe('application/json');
});
});
Loading