Skip to content

Commit

Permalink
fix: revert irrelevant changes of "wrong ICM is used with errors on S…
Browse files Browse the repository at this point in the history 
…SR" (#1533)

fix: revert irrelevant changes of "wrong ICM is used when configured ICM throws errors on SSR (#1519)"

* reverted: icmBaseURL no longer uses environment.ts fallback in production mode
* reverted: make usage of default/fallback configuration in environment.ts configurable for getting state properties
* refactored: errors should only contain properties of defined HttpError interface - this is now handled at the error interceptor
* kept: do not log an error that can't be stringified, it floods the log with irrelevant information

BREAKING CHANGES: No longer a breaking change regarding "No longer falls back to `environment.ts` configured `icmBaseURL` in production mode."
  • Loading branch information
@shauke
shauke committed Nov 20, 2023
1 parent 424595f commit a48c7ae
Show file tree
Hide file tree
Showing 5 changed files with 105 additions and 112 deletions.
155 changes: 78 additions & 77 deletions src/app/core/interceptors/icm-error-mapper.interceptor.spec.ts
View file
Expand Up @@ -33,12 +33,12 @@ describe('Icm Error Mapper Interceptor', () => {
next: fail,
error: error => {
expect(error).toMatchInlineSnapshot(`
{
"message": "Unauthorized",
"name": "HttpErrorResponse",
"status": 401,
}
`);
{
"message": "Unauthorized",
"name": "HttpErrorResponse",
"status": 401,
}
`);
done();
},
});
Expand All @@ -51,12 +51,13 @@ describe('Icm Error Mapper Interceptor', () => {
next: fail,
error: error => {
expect(error).toMatchInlineSnapshot(`
{
"message": "Http failure response for some: 400 Bad Request",
"name": "HttpErrorResponse",
"status": 400,
}
`);
{
"code": "Bad Request",
"message": "Http failure response for some: 400 Bad Request",
"name": "HttpErrorResponse",
"status": 400,
}
`);
done();
},
});
Expand All @@ -73,35 +74,35 @@ describe('Icm Error Mapper Interceptor', () => {
next: fail,
error: error => {
expect(error).toMatchInlineSnapshot(`
{
"errors": [
{
"causes": [
{
"code": "basket.promotion_code.add_code_promotion_code_not_found.error",
"message": "The promotion code could not be found.",
"paths": [
"$.code",
],
},
{
"code": "some.other.error",
"message": "Some other error.",
"paths": [
"$.code",
],
},
],
"code": "basket.promotion_code.add_not_successful.error",
"message": "The promotion code could not be added.",
"status": "422",
},
],
"message": "The promotion code could not be added. The promotion code could not be found. Some other error.",
"name": "HttpErrorResponse",
"status": 422,
}
`);
{
"errors": [
{
"causes": [
{
"code": "basket.promotion_code.add_code_promotion_code_not_found.error",
"message": "The promotion code could not be found.",
"paths": [
"$.code",
],
},
{
"code": "some.other.error",
"message": "Some other error.",
"paths": [
"$.code",
],
},
],
"code": "basket.promotion_code.add_not_successful.error",
"message": "The promotion code could not be added.",
"status": "422",
},
],
"message": "The promotion code could not be added. The promotion code could not be found. Some other error.",
"name": "HttpErrorResponse",
"status": 422,
}
`);
done();
},
});
Expand Down Expand Up @@ -137,21 +138,21 @@ describe('Icm Error Mapper Interceptor', () => {
next: fail,
error: error => {
expect(error).toMatchInlineSnapshot(`
{
"errors": [
{
"code": "basket.add_line_item_not_successful.error",
"message": "The product could not be added to your cart.",
"paths": [
"$[0]",
],
"status": "422",
},
],
"name": "HttpErrorResponse",
"status": 422,
}
`);
{
"errors": [
{
"code": "basket.add_line_item_not_successful.error",
"message": "The product could not be added to your cart.",
"paths": [
"$[0]",
],
"status": "422",
},
],
"name": "HttpErrorResponse",
"status": 422,
}
`);
done();
},
});
Expand All @@ -176,12 +177,12 @@ describe('Icm Error Mapper Interceptor', () => {
next: fail,
error: error => {
expect(error).toMatchInlineSnapshot(`
{
"code": "customer.credentials.login.not_unique.error",
"name": "HttpErrorResponse",
"status": 409,
}
`);
{
"code": "customer.credentials.login.not_unique.error",
"name": "HttpErrorResponse",
"status": 409,
}
`);
done();
},
});
Expand All @@ -198,12 +199,12 @@ describe('Icm Error Mapper Interceptor', () => {
next: fail,
error: error => {
expect(error).toMatchInlineSnapshot(`
{
"message": "Bad Request (The following attributes are missing: email,preferredLanguage)",
"name": "HttpErrorResponse",
"status": 400,
}
`);
{
"message": "Bad Request (The following attributes are missing: email,preferredLanguage)",
"name": "HttpErrorResponse",
"status": 400,
}
`);
done();
},
});
Expand Down Expand Up @@ -232,12 +233,12 @@ describe('Icm Error Mapper Interceptor', () => {
next: fail,
error: error => {
expect(error).toMatchInlineSnapshot(`
{
"message": "Bad Request (The following attributes are invalid: email,preferredLanguage){"email":"asdf@.","preferredLanguage":"ASDF"}",
"name": "HttpErrorResponse",
"status": 400,
}
`);
{
"message": "Bad Request (The following attributes are invalid: email,preferredLanguage){"email":"asdf@.","preferredLanguage":"ASDF"}",
"name": "HttpErrorResponse",
"status": 400,
}
`);
done();
},
});
Expand All @@ -255,14 +256,14 @@ describe('Icm Error Mapper Interceptor', () => {
});
});

it('should convert other error responses directly for a fallback', done => {
it('should convert other error responses to simplified format', done => {
http.get('some').subscribe({
next: fail,
error: error => {
expect(error).toMatchInlineSnapshot(`
{
"error": "some other format",
"key": "value",
"code": "Bad Request",
"message": "Http failure response for some: 400 Bad Request",
"name": "HttpErrorResponse",
"status": 400,
}
Expand Down
38 changes: 22 additions & 16 deletions src/app/core/interceptors/icm-error-mapper.interceptor.ts
View file
Expand Up @@ -22,7 +22,6 @@ export class ICMErrorMapperInterceptor implements HttpInterceptor {
// eslint-disable-next-line complexity
private mapError(httpError: HttpErrorResponse, request: HttpRequest<unknown>): HttpError {
const specialHandlers = this.injector.get<SpecialHttpErrorHandler[]>(SPECIAL_HTTP_ERROR_HANDLER, []);

const specialHandler = specialHandlers.find(handler => handler.test(httpError, request));

const responseError: HttpError = {
Expand All @@ -37,6 +36,7 @@ export class ICMErrorMapperInterceptor implements HttpInterceptor {
if (httpError.headers?.get('error-type') === 'error-missing-attributes') {
return { ...responseError, message: httpError.error };
}

if (httpError.headers?.get('error-type') === 'error-invalid-attributes') {
let message = httpError.error;
if (typeof request.body === 'object') {
Expand All @@ -47,30 +47,33 @@ export class ICMErrorMapperInterceptor implements HttpInterceptor {
message,
};
}

if (httpError.headers?.get('error-key')) {
return {
...responseError,
code: httpError.headers.get('error-key'),
};
}

if (typeof httpError.error === 'string') {
return {
...responseError,
message: httpError.error,
};
}

if (typeof httpError.error === 'object' && httpError.error) {
const errors: {
code: string;
message: string;
causes?: {
if (typeof httpError.error === 'object') {
// handle error objects with multiple errors
if (httpError.error?.errors?.length) {
const errors: {
code: string;
message: string;
paths: string[];
}[];
}[] = httpError.error?.errors;
if (errors?.length) {
causes?: {
code: string;
message: string;
paths: string[];
}[];
}[] = httpError.error.errors;
if (errors.length === 1) {
const error = errors[0];
if (error.causes?.length) {
Expand All @@ -85,15 +88,18 @@ export class ICMErrorMapperInterceptor implements HttpInterceptor {
...responseError,
errors: httpError.error?.errors,
};
} else {
return {
...responseError,
...httpError.error,
};
}

// handle all other error responses with error object
return {
...responseError,
code: httpError.error?.code || httpError.statusText,
message: httpError.error?.message || httpError.message,
};
}

return pick(httpError, 'status', 'name', 'message');
// handle error responses without error object
return pick(httpError, 'name', 'status', 'message');
}

intercept(req: HttpRequest<unknown>, next: HttpHandler): Observable<HttpEvent<unknown>> {
Expand Down
4 changes: 1 addition & 3 deletions src/app/core/store/core/configuration/configuration.effects.ts
View file
Expand Up @@ -65,9 +65,7 @@ export class ConfigurationEffects {
ofType(ROOT_EFFECTS_INIT),
take(1),
concatLatestFrom(() => [
this.stateProperties.getStateOrEnvOrDefault<string>('ICM_BASE_URL', 'icmBaseURL', {
disableDefault: PRODUCTION_MODE,
}),
this.stateProperties.getStateOrEnvOrDefault<string>('ICM_BASE_URL', 'icmBaseURL'),
this.stateProperties.getStateOrEnvOrDefault<string>('ICM_SERVER', 'icmServer'),
this.stateProperties.getStateOrEnvOrDefault<string>('ICM_SERVER_STATIC', 'icmServerStatic'),
this.stateProperties.getStateOrEnvOrDefault<string>('ICM_CHANNEL', 'icmChannel'),
Expand Down
6 changes: 1 addition & 5 deletions src/app/core/store/core/error/error.reducer.ts
View file
@@ -1,5 +1,4 @@
import { createReducer, on } from '@ngrx/store';
import { pick } from 'lodash-es';

import { HttpError } from 'ish-core/models/http-error/http-error.model';

Expand All @@ -24,10 +23,7 @@ export const errorReducer = createReducer(
serverConfigError,
(state, action): ErrorState => ({
...state,
current:
typeof action.payload.error === 'object'
? pick(action.payload.error, 'code', 'errors', 'message', 'name', 'status')
: action.payload.error,
current: action.payload.error,
type: action.type,
})
)
Expand Down
14 changes: 3 additions & 11 deletions src/app/core/utils/state-transfer/state-properties.service.ts
View file
Expand Up @@ -30,15 +30,9 @@ export class StatePropertiesService {
constructor(private store: Store) {}

/**
* Retrieve property from first set property of server state, system environment or environment.ts (default)
* optional the fallback to default can be disabled
* e.g. for production environments where there should not be a fallback for the system environment configuration
* Retrieve property from first set property of server state, system environment or environment.ts
*/
getStateOrEnvOrDefault<T>(
envKey: string,
envPropKey: keyof Environment,
options?: { disableDefault: boolean }
): Observable<T> {
getStateOrEnvOrDefault<T>(envKey: string, envPropKey: keyof Environment): Observable<T> {
return this.store.pipe(
select(getConfigurationState),
mapToProperty(envPropKey as keyof ConfigurationType),
Expand All @@ -47,10 +41,8 @@ export class StatePropertiesService {
return value;
} else if (SSR && process.env[envKey]) {
return process.env[envKey];
} else if (!options?.disableDefault) {
return environment[envPropKey];
} else {
return;
return environment[envPropKey];
}
}),
SSR
Expand Down

0 comments on commit a48c7ae

Please sign in to comment.