Skip to content

Commit

Permalink
Merge pull request #881 from christophercr/feature/fix-http-undefined…
Browse files Browse the repository at this point in the history
…-correlation-id

fix(stark-core): refactor Http headers related API to prevent undefined and null header values to be added to Angular Http headers
  • Loading branch information
SuperITMan authored Nov 23, 2018
2 parents b84f8e5 + c728f9c commit 962e0fb
Show file tree
Hide file tree
Showing 9 changed files with 153 additions and 69 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ export interface StarkHttpBaseRequestBuilder<T extends StarkResource> {
/**
* Adds a header to the request
* @param name - Header name
* @param value - Header value
* @param value - Header value. In case of multiple values, they should be provided in an array.
* @returns The current builder
*/
setHeader(name: string, value: string): this;
setHeader(name: string, value: string | string[]): this;

/**
* Adds a query parameter to the request (if the parameter already exists it will be overwritten)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@ export abstract class StarkAbstractHttpBaseRequestBuilder<T extends StarkResourc
this.request = request;
}

public setHeader(name: string, value: string): this {
if (typeof value !== "undefined") {
public setHeader(name: string, value: string | string[]): this {
// in Angular, a header value can only be string or string[], not null/undefined (https://github.com/angular/angular/issues/18743)
if (name && typeof value !== "undefined" && value !== null) {
this.request.headers.set(name, value);
}
return this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,16 +55,16 @@ interface StarkHttpRequestBuilderSpecVariables<RequestBuilderType = StarkHttpBas
requestBuilder: RequestBuilderType;
}

function assertHeaders(requestHeaders: Map<string, string>, expectedHeaders: Map<string, string | undefined>): void {
function assertHeaders(requestHeaders: Map<string, string | string[]>, expectedHeaders: Map<string, string | string[] | undefined>): void {
expect(requestHeaders).toBeDefined();
expect(requestHeaders.size).toBe(expectedHeaders.size);

expectedHeaders.forEach((value?: string, header?: string) => {
expectedHeaders.forEach((value?: string | string[], header?: string) => {
expect(header).toBeDefined();
expect((<string>header).length).not.toBe(0);
expect(requestHeaders.has(<string>header)).toBe(true);
if (typeof value !== "undefined") {
expect(requestHeaders.get(<string>header)).toBe(value);
expect(requestHeaders.get(<string>header)).toEqual(value);
} else {
expect(requestHeaders.get(<string>header)).toBeUndefined();
}
Expand Down Expand Up @@ -123,9 +123,14 @@ function testSetHeader(beforeEachFn: () => StarkHttpRequestBuilderSpecVariables)
({ requestBuilder: builder } = beforeEachFn());
});

it("should add the header name/value only if the value is defined", () => {
it("should add the header name/value only if the name and value are defined and not null", () => {
builder.setHeader(StarkHttpHeaders.CONTENT_TYPE, contentTypeJSON);
builder.setHeader("invalidHeader", <any>undefined);
// tslint:disable-next-line:no-null-keyword
builder.setHeader("anotherInvalidHeader", <any>null);
builder.setHeader(<any>undefined, "dummy value");
// tslint:disable-next-line:no-null-keyword
builder.setHeader(<any>null, "another dummy value");

const request: StarkHttpRequest = builder.build();

Expand All @@ -134,17 +139,22 @@ function testSetHeader(beforeEachFn: () => StarkHttpRequestBuilderSpecVariables)

it("should add the header name/value and add it to existing ones", () => {
builder.setHeader(StarkHttpHeaders.CONTENT_TYPE, contentTypeJSON);
builder.setHeader(StarkHttpHeaders.ACCEPT_LANGUAGE, StarkLanguages.EN_US.isoCode);
builder.setHeader(StarkHttpHeaders.ACCEPT_LANGUAGE, [
StarkLanguages.EN_US.isoCode,
StarkLanguages.FR_BE.isoCode,
StarkLanguages.NL_BE.isoCode
]);

const request: StarkHttpRequest = builder.build();
const expectedHeaders: Map<string, string | string[]> = new Map<string, string | string[]>();
expectedHeaders.set(StarkHttpHeaders.CONTENT_TYPE, contentTypeJSON);
expectedHeaders.set(StarkHttpHeaders.ACCEPT_LANGUAGE, [
StarkLanguages.EN_US.isoCode,
StarkLanguages.FR_BE.isoCode,
StarkLanguages.NL_BE.isoCode
]);

assertHeaders(
request.headers,
new Map([
[StarkHttpHeaders.CONTENT_TYPE, contentTypeJSON],
[StarkHttpHeaders.ACCEPT_LANGUAGE, StarkLanguages.EN_US.isoCode]
])
);
assertHeaders(request.headers, expectedHeaders);
});
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ export interface StarkHttpRequest<P extends StarkResource = StarkResource> {
*/
fieldsToInclude?: string[];
/**
* Map containing the headers to be sent with the request.
* Map containing the headers to be sent with the request. Multiple values for a header are supported.
*/
headers: Map<string, string>;
headers: Map<string, string | string[]>;
/**
* Map containing the parameters that will be included as query parameters.
* The query parameters might be undefined values in case the allowUndefinedQueryParams option is enabled and passed to the corresponding builder.
Expand Down
59 changes: 29 additions & 30 deletions packages/stark-core/src/modules/http/services/http.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -501,11 +501,7 @@ describe("Service: StarkHttpService", () => {
let httpErrorResponse: Partial<HttpErrorResponse>;

beforeEach(() => {
({
starkHttpService: httpService,
httpMock: httpClientMock,
httpRequest: request
} = beforeEachFn());
({ starkHttpService: httpService, httpMock: httpClientMock, httpRequest: request } = beforeEachFn());

httpErrorResponse = {
status: StarkHttpStatusCodes.HTTP_400_BAD_REQUEST,
Expand Down Expand Up @@ -657,11 +653,7 @@ describe("Service: StarkHttpService", () => {
let httpService: HttpServiceHelper<MockResource>;

beforeEach(() => {
({
starkHttpService: httpService,
httpMock: httpClientMock,
httpRequest: request
} = beforeEachFn());
({ starkHttpService: httpService, httpMock: httpClientMock, httpRequest: request } = beforeEachFn());
});

it(
Expand Down Expand Up @@ -1100,11 +1092,7 @@ describe("Service: StarkHttpService", () => {
let httpResponse: Partial<HttpResponse<StarkResource>>;

beforeEach(() => {
({
starkHttpService: httpService,
httpMock: httpClientMock,
httpRequest: request
} = beforeEachFn());
({ starkHttpService: httpService, httpMock: httpClientMock, httpRequest: request } = beforeEachFn());

httpResponse = {
status: StarkHttpStatusCodes.HTTP_204_NO_CONTENT,
Expand Down Expand Up @@ -1444,7 +1432,7 @@ describe("Service: StarkHttpService", () => {
};
request.headers.set(dummyHeaderName, dummyHeaderValue);

const devAuthenticationHeaders: Map<string, string> = mockSessionService.devAuthenticationHeaders;
const devAuthenticationHeaders: Map<string, string | string[]> = mockSessionService.devAuthenticationHeaders;
expect(devAuthenticationHeaders.size).toBe(mockDevAuthHeaders.size);

const requestWithDevAuthHeaders: StarkHttpRequest = starkHttpService.addDevAuthenticationHeaders(request);
Expand All @@ -1453,28 +1441,28 @@ describe("Service: StarkHttpService", () => {
expect(requestWithDevAuthHeaders.headers.has(dummyHeaderName)).toBe(true);
expect(requestWithDevAuthHeaders.headers.get(dummyHeaderName)).toBe(dummyHeaderValue);

devAuthenticationHeaders.forEach((headerValue: string, header: string) => {
devAuthenticationHeaders.forEach((headerValue: string | string[], header: string) => {
expect(requestWithDevAuthHeaders.headers.has(header)).toBe(true);
expect(requestWithDevAuthHeaders.headers.get(header)).toBe(headerValue);
});
});
});

describe("addCorrelationIdentifierHeader", () => {
it("should get the correlationId from the Logging service and add it as a header to the current request headers", () => {
const dummyHeaderName: string = "some header";
const dummyHeaderValue: string = "some value";
const request: StarkHttpRequest<MockResource> = {
backend: mockBackend,
resourcePath: mockResourcePath,
headers: new Map<string, string>(),
queryParameters: new Map<string, string>(),
requestType: StarkHttpRequestType.DELETE,
item: mockResourceWithEtag,
serializer: mockResourceSerializer
};
request.headers.set(dummyHeaderName, dummyHeaderValue);
const dummyHeaderName: string = "some header";
const dummyHeaderValue: string = "some value";
const request: StarkHttpRequest<MockResource> = {
backend: mockBackend,
resourcePath: mockResourcePath,
headers: new Map<string, string>(),
queryParameters: new Map<string, string>(),
requestType: StarkHttpRequestType.DELETE,
item: mockResourceWithEtag,
serializer: mockResourceSerializer
};
request.headers.set(dummyHeaderName, dummyHeaderValue);

it("should get the correlationId from the Logging service and add it as a header to the current request headers", () => {
const correlationId: string = loggerMock.correlationId;
expect(correlationId).toBe(mockCorrelationId);

Expand All @@ -1486,6 +1474,17 @@ describe("Service: StarkHttpService", () => {
expect(requestWithCorrelationIdHeader.headers.has(loggerMock.correlationIdHttpHeaderName)).toBe(true);
expect(requestWithCorrelationIdHeader.headers.get(loggerMock.correlationIdHttpHeaderName)).toBe(correlationId);
});

it("should NOT add the correlationId header to the current request headers if the correlationId is not defined", () => {
(<any>loggerMock)["correlationId"] = undefined;

const requestWithCorrelationIdHeader: StarkHttpRequest<MockResource> = starkHttpService.addCorrelationIdentifierHeader(request);

expect(requestWithCorrelationIdHeader.headers.size).toBe(1); // just the custom header
expect(requestWithCorrelationIdHeader.headers.has(dummyHeaderName)).toBe(true);
expect(requestWithCorrelationIdHeader.headers.get(dummyHeaderName)).toBe(dummyHeaderValue);
expect(requestWithCorrelationIdHeader.headers.has(loggerMock.correlationIdHttpHeaderName)).toBe(false);
});
});
});

Expand Down
4 changes: 2 additions & 2 deletions packages/stark-core/src/modules/http/services/http.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ export class StarkHttpServiceImpl<P extends StarkResource> implements StarkHttpS
const requestCopy: StarkHttpRequest<P> = _cloneDeep(request);

// add the preAuthentication headers to the request headers
this.sessionService.devAuthenticationHeaders.forEach((value: string, header: string) => {
this.sessionService.devAuthenticationHeaders.forEach((value: string | string[], header: string) => {
requestCopy.headers.set(header, value);
});

Expand All @@ -173,7 +173,7 @@ export class StarkHttpServiceImpl<P extends StarkResource> implements StarkHttpS
* @returns The modified request object
*/
public addCorrelationIdentifierHeader(request: StarkHttpRequest<P>): StarkHttpRequest<P> {
if (this.logger.correlationIdHttpHeaderName && this.logger.correlationIdHttpHeaderName.length > 0) {
if (this.logger.correlationIdHttpHeaderName && this.logger.correlationIdHttpHeaderName.length > 0 && this.logger.correlationId) {
this.logger.debug(starkHttpServiceName + ": Adding correlation identifier header");
const requestCopy: StarkHttpRequest<P> = _cloneDeep(request);
requestCopy.headers.set(this.logger.correlationIdHttpHeaderName, this.logger.correlationId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export interface StarkSessionService {
* Authentication headers necessary for non-production environments
* @returns A Map containing the development authentication headers
*/
readonly devAuthenticationHeaders: Map<string, string>;
readonly devAuthenticationHeaders: Map<string, string | string[]>;

/**
* Returns the session's current user
Expand Down Expand Up @@ -74,5 +74,5 @@ export interface StarkSessionService {
* Add authentication headers to the session
* They are use by the http service to authenticate the user
*/
setDevAuthenticationHeaders(devAuthenticationHeaders: Map<string, string>): void;
setDevAuthenticationHeaders(devAuthenticationHeaders: Map<string, string | string[]>): void;
}
Original file line number Diff line number Diff line change
Expand Up @@ -674,7 +674,6 @@ describe("Service: StarkSessionService", () => {
mockKeepaliveService.request.calls.reset();

const expectedDevAuthHeaders: Map<string, string> = new Map<string, string>();
expectedDevAuthHeaders.set(mockCorrelationIdHeaderName, mockCorrelationId);
expectedDevAuthHeaders.set("usernameTestHeader", mockUser.username);
expectedDevAuthHeaders.set("firstnameTestHeader", mockUser.firstName);
expectedDevAuthHeaders.set("lastnameTestHeader", mockUser.lastName);
Expand All @@ -687,6 +686,45 @@ describe("Service: StarkSessionService", () => {
expect(mockKeepaliveService.interval).toHaveBeenCalledWith(appConfig.keepAliveInterval);
expect(mockKeepaliveService.request).toHaveBeenCalledTimes(1);

let mockHeadersObj: HttpHeaders = new HttpHeaders();
expectedDevAuthHeaders.forEach((value: string, key: string) => (mockHeadersObj = mockHeadersObj.set(key, value)));
mockHeadersObj = mockHeadersObj.set(mockCorrelationIdHeaderName, mockCorrelationId);

expect(mockKeepaliveService.request).toHaveBeenCalledWith(
new HttpRequest<void>("GET", <string>appConfig.keepAliveUrl, { headers: mockHeadersObj })
);
});

it("should NOT set the correlationId headers to the keepalive Http request if the correlation id is undefined", () => {
const sessionServiceHelper: SessionServiceHelper = new SessionServiceHelper(
mockStore,
mockLogger,
mockRoutingService,
appConfig,
mockIdleService,
mockInjectorService,
mockTranslateService,
mockSessionConfig
);

expect(sessionServiceHelper.keepalive).toBeDefined();
mockKeepaliveService.interval.calls.reset();
mockKeepaliveService.request.calls.reset();

const expectedDevAuthHeaders: Map<string, string> = new Map<string, string>();
expectedDevAuthHeaders.set("usernameTestHeader", mockUser.username);
expectedDevAuthHeaders.set("firstnameTestHeader", mockUser.firstName);
expectedDevAuthHeaders.set("lastnameTestHeader", mockUser.lastName);
expectedDevAuthHeaders.set("emailTestHeader", <string>mockUser.email);

(<any>mockLogger)["correlationId"] = undefined;
sessionServiceHelper.setDevAuthenticationHeaders(expectedDevAuthHeaders);
sessionServiceHelper.configureKeepaliveService();

expect(mockKeepaliveService.interval).toHaveBeenCalledTimes(1);
expect(mockKeepaliveService.interval).toHaveBeenCalledWith(appConfig.keepAliveInterval);
expect(mockKeepaliveService.request).toHaveBeenCalledTimes(1);

let mockHeadersObj: HttpHeaders = new HttpHeaders();
expectedDevAuthHeaders.forEach((value: string, key: string) => (mockHeadersObj = mockHeadersObj.set(key, value)));

Expand Down Expand Up @@ -882,41 +920,68 @@ describe("Service: StarkSessionService", () => {
});

describe("setDevAuthenticationHeaders", () => {
it("should construct the authentication headers based on the http headers that are passed", () => {
it("should construct the dev authentication headers based on the http headers that are passed", () => {
expect(sessionService["_devAuthenticationHeaders"]).toBe(<any>undefined);

const expectedDevAuthHeaders: Map<string, string> = new Map<string, string>();
expectedDevAuthHeaders.set(mockCorrelationIdHeaderName, mockCorrelationId);
expectedDevAuthHeaders.set("usernameTestHeader", mockUser.username);
expectedDevAuthHeaders.set("firstnameTestHeader", mockUser.firstName);
expectedDevAuthHeaders.set("lastnameTestHeader", mockUser.lastName);
expectedDevAuthHeaders.set("emailTestHeader", <string>mockUser.email);

sessionService.setDevAuthenticationHeaders(expectedDevAuthHeaders);
expect(sessionService["_devAuthenticationHeaders"]).toBeDefined();
expect(sessionService.devAuthenticationHeaders.size).toBe(5);
expect(sessionService.devAuthenticationHeaders.size).toBe(4);

expectedDevAuthHeaders.forEach((_value: string, key: string) => {
expectedDevAuthHeaders.forEach((value: string, key: string) => {
expect(sessionService.devAuthenticationHeaders.has(key)).toBe(true);
expect(sessionService.devAuthenticationHeaders.get(key)).toBe(_value);
expect(sessionService.devAuthenticationHeaders.get(key)).toBe(value);
});
});

it("should construct the dev authentication headers excluding those http headers with undefined or null names or values", () => {
expect(sessionService["_devAuthenticationHeaders"]).toBe(<any>undefined);

const expectedDevAuthHeaders: Map<string, string> = new Map<string, string>();
expectedDevAuthHeaders.set("usernameTestHeader", mockUser.username);
expectedDevAuthHeaders.set("firstnameTestHeader", mockUser.firstName);
expectedDevAuthHeaders.set("lastnameTestHeader", <any>undefined);
// tslint:disable-next-line:no-null-keyword
expectedDevAuthHeaders.set("emailTestHeader", <any>null);
expectedDevAuthHeaders.set(<any>undefined, "dummy value");
// tslint:disable-next-line:no-null-keyword
expectedDevAuthHeaders.set(<any>null, "another dummy value");

sessionService.setDevAuthenticationHeaders(expectedDevAuthHeaders);
expect(sessionService["_devAuthenticationHeaders"]).toBeDefined();
expect(sessionService.devAuthenticationHeaders.size).toBe(2);

expectedDevAuthHeaders.forEach((value: string, key: string) => {
if (key === "usernameTestHeader" || key === "firstnameTestHeader") {
expect(sessionService.devAuthenticationHeaders.has(key)).toBe(true);
expect(sessionService.devAuthenticationHeaders.get(key)).toBe(value);
} else {
expect(sessionService.devAuthenticationHeaders.has(key)).toBe(false);
}
});
});
});

describe("devAuthenticationHeaders", () => {
it("should return the pre-authentication headers if they were constructed", () => {
const expectedDevAuthHeaders: Map<string, string> = new Map<string, string>();
const expectedDevAuthHeaders: Map<string, string | string[]> = new Map<string, string | string[]>();
expectedDevAuthHeaders.set("usernameTestHeader", "doej");
expectedDevAuthHeaders.set("firstnameTestHeader", "john");
expectedDevAuthHeaders.set("lastTestHeader", "doe");
expectedDevAuthHeaders.set("emailTestHeader", ["[email protected]", "[email protected]"]);

sessionService.setInternalDevAuthenticationHeaders(expectedDevAuthHeaders);

const devAuthenticationHeaders: Map<string, string> = sessionService.devAuthenticationHeaders;
const devAuthenticationHeaders: Map<string, string | string[]> = sessionService.devAuthenticationHeaders;

expect(devAuthenticationHeaders.size).toBe(expectedDevAuthHeaders.size);

expectedDevAuthHeaders.forEach((value: string, header: string) => {
expectedDevAuthHeaders.forEach((value: string | string[], header: string) => {
expect(expectedDevAuthHeaders.has(header)).toBe(true);
expect(expectedDevAuthHeaders.get(header)).toBe(value);
});
Expand All @@ -926,7 +991,7 @@ describe("Service: StarkSessionService", () => {
/* tslint:disable-next-line:no-undefined-argument */
sessionService.setInternalDevAuthenticationHeaders(undefined);

const devAuthenticationHeaders: Map<string, string> = sessionService.devAuthenticationHeaders;
const devAuthenticationHeaders: Map<string, string | string[]> = sessionService.devAuthenticationHeaders;

expect(devAuthenticationHeaders.size).toBe(0);
});
Expand Down Expand Up @@ -991,7 +1056,7 @@ class SessionServiceHelper extends StarkSessionServiceImpl {
return of(undefined);
}

public setInternalDevAuthenticationHeaders(headers?: Map<string, string>): void {
public setInternalDevAuthenticationHeaders(headers?: Map<string, string | string[]>): void {
this._devAuthenticationHeaders = <any>headers;
}
}
Loading

0 comments on commit 962e0fb

Please sign in to comment.