hide query params with empty values

workaround for emberjs/ember.js#18683
aaxelb · Jan 17, 2020 · 97fd330 · 97fd330
1 parent 021b014
commit 97fd330
Show file tree

Hide file tree

Showing 9 changed files with 140 additions and 16 deletions.
diff --git a/app/application/route.ts b/app/application/route.ts
@@ -26,12 +26,6 @@ export default class ApplicationRoute extends Route.extend(
     @service i18n!: I18N;
     @service currentUser!: CurrentUser;
 
-    queryParams = {
-        viewOnlyToken: {
-            refreshModel: false,
-        },
-    };
-
     afterModel() {
         const i18n = this.get('i18n');
         const availableLocales: [string] = i18n.get('locales').toArray();

diff --git a/app/locations/auto.ts b/app/locations/auto.ts
@@ -1,7 +1,7 @@
 import AutoLocation from '@ember/routing/auto-location';
-import GuidLocationMixin from 'ember-osf-web/locations/guid-mixin';
+import CleanUrlLocationMixin from 'ember-osf-web/locations/clean-url-mixin';
 
 // Sadly the `auto` location must be overriden, otherwise
 // ember-cli's server doesn't allow visiting specific URLs
-export default class GuidAutoLocation extends AutoLocation.extend(GuidLocationMixin) {
+export default class CleanUrlAutoLocation extends AutoLocation.extend(CleanUrlLocationMixin) {
 }
diff --git a/app/locations/guid-mixin.ts → app/locations/clean-url-mixin.ts b/app/locations/guid-mixin.ts → app/locations/clean-url-mixin.ts
diff --git a/app/locations/history.ts b/app/locations/history.ts
@@ -4,7 +4,7 @@ import { inject as service } from '@ember/service';
 import { waitForQueue } from 'ember-concurrency';
 import { task } from 'ember-concurrency-decorators';
 
-import GuidLocationMixin from 'ember-osf-web/locations/guid-mixin';
+import CleanUrlLocationMixin from 'ember-osf-web/locations/clean-url-mixin';
 import OsfRouterService from 'ember-osf-web/services/osf-router';
 import Ready from 'ember-osf-web/services/ready';
 import scrollTo from 'ember-osf-web/utils/scroll-to';
@@ -17,7 +17,7 @@ function splitFragment(url: string): [string, string?] {
 }
 
 // Add support for scrolling to elements according to the URL's #fragment.
-export default class FragmentHistoryLocation extends HistoryLocation.extend(GuidLocationMixin) {
+export default class FragmentHistoryLocation extends HistoryLocation.extend(CleanUrlLocationMixin) {
     @service ready!: Ready;
     @service osfRouter!: OsfRouterService;
 

diff --git a/app/locations/none.ts b/app/locations/none.ts
@@ -1,9 +1,9 @@
 import NoneLocation from '@ember/routing/none-location';
-import GuidLocationMixin from 'ember-osf-web/locations/guid-mixin';
+import CleanUrlLocationMixin from 'ember-osf-web/locations/clean-url-mixin';
 
 // Sadly the `none` location must be overriden, otherwise
 // ember-test-helper's helpers will leave --segments left over in the URL
-export default class GuidNoneLocation extends NoneLocation.extend(GuidLocationMixin) {
+export default class CleanUrlNoneLocation extends NoneLocation.extend(CleanUrlLocationMixin) {
     path!: string;
 
     // Opt out of URL "cleaning" when in tests.

diff --git a/app/utils/clean-url.ts b/app/utils/clean-url.ts
@@ -1,12 +1,20 @@
 import { assert } from '@ember/debug';
+import { filterQueryParams } from './url-parts';
+
 /**
- * Remove the --guidtype portions of a URL that are only used for guid routing
- * and should never appear in a real URL.
+ * Given a URL, remove cruft that should not be visible to the user:
+ *   - `/--foo/` path segments (only used for guid/engine routing)
+ *   - trailing slash at the end of the path
+ *   - query params with empty values (workaround for a bug in ember:
+ *     https://github.com/emberjs/ember.js/issues/18683)
+ *     NOTE: this may cause problems if a query param has a non-empty default
+ *           value but may be set to an empty value.
+ *
  */
 export default function cleanURL(url: string) {
     assert(`cleanURL expects a path starting with '/', got '${url}'`, url.startsWith('/'));
 
-    const cleanedURL = url
+    const cleanedURL = filterQueryParams(url, (_, value) => Boolean(value))
         .replace(/(?:^|\/)--[^/?]+/g, '') // remove '--foo' segments
         .replace(/\/(?=$|[?#])/, ''); // remove trailing slash
 

diff --git a/app/utils/url-parts.ts b/app/utils/url-parts.ts
@@ -51,6 +51,29 @@ export function addQueryParam(url: string, key: string, value: string): string {
     });
 }
 
+export function filterQueryParams(
+    url: string,
+    filterFn: (key: string, value: string) => boolean,
+): string {
+    const { path, queryString, fragment } = splitUrl(url);
+
+    const queryParams = deserializeQueryString(queryString || '');
+
+    const filteredQueryParams: Record<string, string> = {};
+
+    Object.entries(queryParams)
+        .filter(([key, value]) => filterFn(key, value))
+        .forEach(([key, value]) => {
+            filteredQueryParams[key] = value;
+        });
+
+    return joinUrl({
+        path,
+        queryString: param(filteredQueryParams),
+        fragment,
+    });
+}
+
 export function addPathSegment(url: string, segment: string): string {
     const { path, queryString, fragment } = splitUrl(url);
     return joinUrl({

diff --git a/tests/unit/utils/clean-url-test.ts b/tests/unit/utils/clean-url-test.ts
@@ -114,6 +114,26 @@ const TEST_CASES = [{
     input: '/--user/abcd/?query=param#frag',
     cleanOutput: '/abcd?query=param#frag',
     notFoundOutput: 'abcd#frag',
+}, {
+    input: '/?aoeu=',
+    cleanOutput: '/',
+    notFoundOutput: '',
+}, {
+    input: '/AttheEnd/--registries/?query=',
+    cleanOutput: '/AttheEnd',
+    notFoundOutput: 'AttheEnd',
+}, {
+    input: '/normal--url/?query=param&empty=',
+    cleanOutput: '/normal--url?query=param',
+    notFoundOutput: 'normal--url',
+}, {
+    input: '/--user/abcd/?some-empty=#frag',
+    cleanOutput: '/abcd#frag',
+    notFoundOutput: 'abcd#frag',
+}, {
+    input: '/--user/abcd/?blah=&blee=&query=param#frag',
+    cleanOutput: '/abcd?query=param#frag',
+    notFoundOutput: 'abcd#frag',
 }];
 
 module('Unit | Utility | cleanURL', () => {

diff --git a/tests/unit/utils/url-parts-test.ts b/tests/unit/utils/url-parts-test.ts
@@ -1,6 +1,13 @@
-import { addPathSegment, addQueryParam, joinUrl, splitUrl } from 'ember-osf-web/utils/url-parts';
 import { module, test } from 'qunit';
 
+import {
+    addPathSegment,
+    addQueryParam,
+    filterQueryParams,
+    joinUrl,
+    splitUrl,
+} from 'ember-osf-web/utils/url-parts';
+
 module('Unit | Utility | url-parts', () => {
     test('splitUrl and joinUrl', assert => {
         const testCases = [
@@ -140,4 +147,76 @@ module('Unit | Utility | url-parts', () => {
             assert.equal(actual, testCase.expected, 'addPathSegment added a path segment');
         }
     });
+
+    test('filterQueryParams', assert => {
+        const testCases: Array<{
+            initial: string,
+            filterFn: (k: string, v: string) => boolean,
+            expected: string,
+        }> = [
+            {
+                initial: 'https://osf.io/',
+                filterFn: () => true,
+                expected: 'https://osf.io/',
+            },
+            {
+                initial: 'https://osf.io/?blah=blee&bloo=blah&blee=blee',
+                filterFn: () => true,
+                expected: 'https://osf.io/?blah=blee&bloo=blah&blee=blee',
+            },
+            {
+                initial: 'https://osf.io/?blah=blee&bloo=blah&blee=blee',
+                filterFn: () => false,
+                expected: 'https://osf.io/',
+            },
+            {
+                initial: 'https://osf.io/?blah=blee&bloo=blah&blee=blee',
+                filterFn: (k, v) => k === v,
+                expected: 'https://osf.io/?blah=blee&bloo=blah',
+            },
+            {
+                initial: 'https://osf.io/?blah=blah&bloo=blah&blee=blee',
+                filterFn: (k, v) => k !== v,
+                expected: 'https://osf.io/?bloo=blah',
+            },
+            {
+                initial: 'https://osf.io/?blah=blah&bloo=blah&blee=blee#plus-fragment',
+                filterFn: (k, v) => k !== v,
+                expected: 'https://osf.io/?bloo=blah#plus-fragment',
+            },
+            {
+                initial: '/just/a/path?blah=blee&bloo=blah&blee=blee',
+                filterFn: () => true,
+                expected: '/just/a/path?blah=blee&bloo=blah&blee=blee',
+            },
+            {
+                initial: '/just/a/path?blah=blee&bloo=blah&blee=blee',
+                filterFn: () => false,
+                expected: '/just/a/path',
+            },
+            {
+                initial: '/just/a/path?blah=blee&bloo=blah&blee=blee',
+                filterFn: (k, v) => k === v,
+                expected: '/just/a/path?blah=blee&bloo=blah',
+            },
+            {
+                initial: '/just/a/path?blah=blee&bloo=blah&blee=blee#plus-fragment',
+                filterFn: () => false,
+                expected: '/just/a/path#plus-fragment',
+            },
+            {
+                initial: '/just/a/path?blah=blee&bloo=blah&blee=blee#plus-fragment',
+                filterFn: (k, v) => k === v,
+                expected: '/just/a/path?blah=blee&bloo=blah#plus-fragment',
+            },
+        ];
+
+        for (const testCase of testCases) {
+            const actual = filterQueryParams(
+                testCase.initial,
+                testCase.filterFn,
+            );
+            assert.equal(actual, testCase.expected, 'filterQueryParams filtered query params');
+        }
+    });
 });