fix: remove matrix parameters from URL to prevent accumulation (#976)

Co-authored-by: max.kless@googlemail.com <max.kless@googlemail.com>

.eslintrc.json

@@ -402,6 +402,7 @@
               "^.*/(<name>)(/src/app)?/store/\\1-store(\\.module)?(<theme>)?\\.ts",
               // ngrx router-store
               "^.*/src/app/core/store/core/router/router\\.(operators|serializer)(<theme>)?\\.ts",
+              "^.*/src/app/core/routing/\\.serializer()?\\.ts",
               // allow only app related content directly in src/app
               "^.*/src/app/app[\\w\\.\\-]+\\.ts$",
               // application shell

src/app/app.module.ts

@@ -5,7 +5,7 @@ import { UrlSerializer } from '@angular/router';
 
 import { COOKIE_CONSENT_VERSION } from 'ish-core/configurations/state-keys';
 import { CoreModule } from 'ish-core/core.module';
-import { CustomUrlSerializer } from 'ish-core/utils/custom-url-serializer';
+import { PWAUrlSerializer } from 'ish-core/routing/pwa-url.serializer';
 
 import { environment } from '../environments/environment';
 
@@ -35,7 +35,7 @@ import { ShellModule } from './shell/shell.module';
   ],
   /* eslint-disable @angular-eslint/sort-ngmodule-metadata-arrays */
   bootstrap: [AppComponent],
-  providers: [{ provide: UrlSerializer, useClass: CustomUrlSerializer }],
+  providers: [{ provide: UrlSerializer, useClass: PWAUrlSerializer }],
 })
 export class AppModule {
   constructor(transferState: TransferState) {

src/app/core/routing/category/category.route.spec.ts

@@ -136,7 +136,7 @@ describe('Category Route', () => {
   describe('additional URL params', () => {
     it('should ignore additional URL params when supplied', () => {
       const category = createCategoryView(categoryTree([specials, topSeller, limitedOffer]), limitedOffer.uniqueId);
-      expect(matchCategoryRoute(wrap(`${generateCategoryUrl(category)};lang=de_DE;redirect=1`))).toMatchInlineSnapshot(`
+      expect(matchCategoryRoute(wrap(`${generateCategoryUrl(category)};lang=de_DE`))).toMatchInlineSnapshot(`
         Object {
           "categoryUniqueId": "Specials.TopSeller.LimitedOffer",
         }

src/app/core/routing/product/product.route.spec.ts

@@ -299,8 +299,7 @@ describe('Product Route', () => {
       const category = createCategoryView(categoryTree([specials, topSeller, limitedOffer]), limitedOffer.uniqueId);
       const product = createProductView({ sku: 'A', name: 'Das neue Surface Pro 7' } as Product);
 
-      expect(matchProductRoute(wrap(`${generateProductUrl(product, category)};lang=de_DE;redirect=1`)))
-        .toMatchInlineSnapshot(`
+      expect(matchProductRoute(wrap(`${generateProductUrl(product, category)};lang=de_DE`))).toMatchInlineSnapshot(`
         Object {
           "categoryUniqueId": "Specials.TopSeller.LimitedOffer",
           "sku": "A",

src/app/core/routing/pwa-url.serializer.spec.ts

@@ -0,0 +1,37 @@
+import { TestBed } from '@angular/core/testing';
+import { UrlSerializer } from '@angular/router';
+
+import { PWAUrlSerializer } from './pwa-url.serializer';
+
+describe('Pwa Url Serializer', () => {
+  let urlSerializer: UrlSerializer;
+
+  beforeEach(() => {
+    TestBed.configureTestingModule({ providers: [{ provide: UrlSerializer, useClass: PWAUrlSerializer }] });
+    urlSerializer = TestBed.inject(UrlSerializer);
+  });
+
+  it('should deal with parenthesis in parameter names', () => {
+    const tree = urlSerializer.parse('/path/a/b/c?param=(value)');
+    const result = urlSerializer.serialize(tree);
+    expect(result).toMatchInlineSnapshot(`"/path/a/b/c?param=(value)"`);
+  });
+
+  it('should deal with parenthesis in path segment names', () => {
+    const tree = urlSerializer.parse('/path/a/(segmentname)');
+    const result = urlSerializer.serialize(tree);
+    expect(result).toMatchInlineSnapshot(`"/path/a/(segmentname)"`);
+  });
+
+  it('should remove matrix parameters from URL', () => {
+    const tree = urlSerializer.parse('/path/a/b/c;matrix=value');
+    const result = urlSerializer.serialize(tree);
+    expect(result).toMatchInlineSnapshot(`"/path/a/b/c"`);
+  });
+
+  it('should remove matrix parameters from and keep query params in URL', () => {
+    const tree = urlSerializer.parse('/path/a/b/c;matrix=value?test=dummy&foo=bar');
+    const result = urlSerializer.serialize(tree);
+    expect(result).toMatchInlineSnapshot(`"/path/a/b/c?test=dummy&foo=bar"`);
+  });
+});

src/app/core/utils/custom-url-serializer.ts → src/app/core/routing/pwa-url.serializer.ts

@@ -1,11 +1,11 @@
 import { DefaultUrlSerializer, UrlSerializer, UrlTree } from '@angular/router';
 
 /**
- * Custom serializer for allowing parenthesis in URLs
+ * Custom serializer for allowing parenthesis in URLs and removing matrix parameters
  *
  * taken from: https://github.com/angular/angular/issues/10280#issuecomment-309129784
  */
-export class CustomUrlSerializer implements UrlSerializer {
+export class PWAUrlSerializer implements UrlSerializer {
   private defaultUrlSerializer: DefaultUrlSerializer = new DefaultUrlSerializer();
 
   parse(url: string): UrlTree {
@@ -14,6 +14,14 @@ export class CustomUrlSerializer implements UrlSerializer {
   }
 
   serialize(tree: UrlTree): string {
-    return this.defaultUrlSerializer.serialize(tree).replace(/%28/g, '(').replace(/%29/g, ')');
+    return (
+      this.defaultUrlSerializer
+        .serialize(tree)
+        // display parenthesis unencoded in URL
+        .replace(/%28/g, '(')
+        .replace(/%29/g, ')')
+        // remove matrix parameters
+        .replace(/;[^?]*/, '')
+    );
   }
 }

src/app/core/store/core/configuration/configuration.integration.spec.ts

@@ -2,14 +2,14 @@ import { Location } from '@angular/common';
 import { Component } from '@angular/core';
 import { TestBed, fakeAsync, tick } from '@angular/core/testing';
 import { BrowserTransferStateModule } from '@angular/platform-browser';
-import { Router } from '@angular/router';
+import { Router, UrlSerializer } from '@angular/router';
 import { RouterTestingModule } from '@angular/router/testing';
 import { TranslateModule } from '@ngx-translate/core';
 import { EMPTY } from 'rxjs';
 import { instance, mock, when } from 'ts-mockito';
 
 import { configurationMeta } from 'ish-core/configurations/configuration.meta';
-import { ConfigurationGuard } from 'ish-core/guards/configuration.guard';
+import { PWAUrlSerializer } from 'ish-core/routing/pwa-url.serializer';
 import { ConfigurationService } from 'ish-core/services/configuration/configuration.service';
 import { LocalizationsService } from 'ish-core/services/localizations/localizations.service';
 import { applyConfiguration, getFeatures, getRestEndpoint } from 'ish-core/store/core/configuration';
@@ -41,14 +41,13 @@ describe('Configuration Integration', () => {
           [ConfigurationEffects],
           [configurationMeta]
         ),
-        RouterTestingModule.withRoutes([
-          { path: 'home', component: DummyComponent, canActivate: [ConfigurationGuard] },
-        ]),
+        RouterTestingModule.withRoutes([{ path: 'home', component: DummyComponent }]),
         TranslateModule.forRoot(),
       ],
       providers: [
         provideStoreSnapshots(),
         { provide: LocalizationsService, useFactory: () => instance(mock(LocalizationsService)) },
+        { provide: UrlSerializer, useClass: PWAUrlSerializer },
       ],
     });
 
@@ -78,33 +77,26 @@ describe('Configuration Integration', () => {
   it('should set imported channel to state', fakeAsync(() => {
     router.navigateByUrl('/home;channel=site');
     tick(500);
-    expect(location.path()).toMatchInlineSnapshot(`"/home;channel=site"`);
+    expect(location.path()).toMatchInlineSnapshot(`"/home"`);
     expect(getRestEndpoint(store$.state)).toMatchInlineSnapshot(`"http://example.org/INTERSHOP/rest/WFS/site/rest"`);
   }));
 
   it('should set imported channel and application to state', fakeAsync(() => {
     router.navigateByUrl('/home;channel=site;application=app');
     tick(500);
-    expect(location.path()).toMatchInlineSnapshot(`"/home;channel=site;application=app"`);
-    expect(getRestEndpoint(store$.state)).toMatchInlineSnapshot(`"http://example.org/INTERSHOP/rest/WFS/site/app"`);
-  }));
-
-  it('should set imported channel to state and redirect if requested', fakeAsync(() => {
-    router.navigateByUrl('/home;channel=site;redirect=1');
-    tick(500);
     expect(location.path()).toMatchInlineSnapshot(`"/home"`);
-    expect(getRestEndpoint(store$.state)).toMatchInlineSnapshot(`"http://example.org/INTERSHOP/rest/WFS/site/rest"`);
+    expect(getRestEndpoint(store$.state)).toMatchInlineSnapshot(`"http://example.org/INTERSHOP/rest/WFS/site/app"`);
   }));
 
-  it('should preserve query parameters when redirecting', fakeAsync(() => {
-    router.navigateByUrl('/home;channel=site;redirect=1?foo=bar&test=hello');
+  it('should preserve query parameters when setting state', fakeAsync(() => {
+    router.navigateByUrl('/home;channel=site?foo=bar&test=hello');
     tick(500);
     expect(location.path()).toMatchInlineSnapshot(`"/home?foo=bar&test=hello"`);
     expect(getRestEndpoint(store$.state)).toMatchInlineSnapshot(`"http://example.org/INTERSHOP/rest/WFS/site/rest"`);
   }));
 
   it('should set imported features to state', fakeAsync(() => {
-    router.navigateByUrl('/home;features=a,b,c;redirect=1');
+    router.navigateByUrl('/home;features=a,b,c');
     tick(500);
     expect(location.path()).toMatchInlineSnapshot(`"/home"`);
     expect(getFeatures(store$.state)).toMatchInlineSnapshot(`
@@ -118,15 +110,15 @@ describe('Configuration Integration', () => {
 
   it('should unset features if "none" was provided', fakeAsync(() => {
     store$.dispatch(applyConfiguration({ features: ['a', 'b', 'c'] }));
-    router.navigateByUrl('/home;features=none;redirect=1');
+    router.navigateByUrl('/home;features=none');
     tick(500);
     expect(location.path()).toMatchInlineSnapshot(`"/home"`);
     expect(getFeatures(store$.state)).toMatchInlineSnapshot(`Array []`);
   }));
 
   it('should not set features if "default" was provided', fakeAsync(() => {
     store$.dispatch(applyConfiguration({ features: ['a', 'b', 'c'] }));
-    router.navigateByUrl('/home;features=default;redirect=1');
+    router.navigateByUrl('/home;features=default');
     tick(500);
     expect(location.path()).toMatchInlineSnapshot(`"/home"`);
     expect(getFeatures(store$.state)).toMatchInlineSnapshot(`
@@ -139,7 +131,7 @@ describe('Configuration Integration', () => {
   }));
 
   it('should set imported locale to state', fakeAsync(() => {
-    router.navigateByUrl('/home;redirect=1;lang=de_DE');
+    router.navigateByUrl('/home;lang=de_DE');
     tick(500);
     expect(location.path()).toMatchInlineSnapshot(`"/home"`);
     expect(getCurrentLocale(store$.state)).toMatchInlineSnapshot(`"de_DE"`);

src/app/core/store/core/core-store.module.ts

@@ -1,14 +1,11 @@
 import { NgModule, Type } from '@angular/core';
-import { Router } from '@angular/router';
 import { EffectsModule } from '@ngrx/effects';
 import { StoreRouterConnectingModule, routerReducer } from '@ngrx/router-store';
 import { ActionReducerMap, MetaReducer, StoreModule } from '@ngrx/store';
 import { pick } from 'lodash-es';
 
 import { configurationMeta } from 'ish-core/configurations/configuration.meta';
 import { ngrxStateTransferMeta } from 'ish-core/configurations/ngrx-state-transfer';
-import { ConfigurationGuard } from 'ish-core/guards/configuration.guard';
-import { addGlobalGuard } from 'ish-core/utils/routing';
 
 import { ConfigurationEffects } from './configuration/configuration.effects';
 import { configurationReducer } from './configuration/configuration.reducer';
@@ -56,10 +53,6 @@ const coreMetaReducers: MetaReducer<CoreState>[] = [
   ],
 })
 export class CoreStoreModule {
-  constructor(router: Router) {
-    addGlobalGuard(router, ConfigurationGuard);
-  }
-
   /**
    * Instantiate {@link CoreStoreModule} for testing.
    *

src/hybrid/default-url-mapping-table.ts

@@ -1,6 +1,6 @@
 const ICM_CONFIG_MATCH = `^/INTERSHOP/web/WFS/(?<channel>[\\w-]+)/(?<lang>[\\w-]+)/(?<application>[\\w-]+)/[\\w-]+`;
 
-const PWA_CONFIG_BUILD = ';channel=$<channel>;lang=$<lang>;application=$<application>;redirect=1';
+const PWA_CONFIG_BUILD = ';channel=$<channel>;lang=$<lang>;application=$<application>';
 
 interface HybridMappingEntry {
   /** ID for grouping */