fix: revert irrelevant changes of "wrong ICM is used with errors on S…

…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."
intershop · Nov 20, 2023 · a48c7ae · a48c7ae
1 parent 424595f
commit a48c7ae
Show file tree

Hide file tree

Showing 5 changed files with 105 additions and 112 deletions.
diff --git a/src/app/core/interceptors/icm-error-mapper.interceptor.spec.ts b/src/app/core/interceptors/icm-error-mapper.interceptor.spec.ts
@@ -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();
       },
     });
@@ -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();
       },
     });
@@ -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();
       },
     });
@@ -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();
       },
     });
@@ -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();
       },
     });
@@ -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();
       },
     });
@@ -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();
         },
       });
@@ -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,
           }

diff --git a/src/app/core/interceptors/icm-error-mapper.interceptor.ts b/src/app/core/interceptors/icm-error-mapper.interceptor.ts
@@ -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 = {
@@ -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') {
@@ -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) {
@@ -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>> {

diff --git a/src/app/core/store/core/configuration/configuration.effects.ts b/src/app/core/store/core/configuration/configuration.effects.ts
@@ -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'),

diff --git a/src/app/core/store/core/error/error.reducer.ts b/src/app/core/store/core/error/error.reducer.ts
@@ -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';
 
@@ -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,
     })
   )

diff --git a/src/app/core/utils/state-transfer/state-properties.service.ts b/src/app/core/utils/state-transfer/state-properties.service.ts
@@ -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),
@@ -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