Extend merge url params to remove null values

When new parameters are added to a URL some of them may be null.

This change prevents actual result of string `null` and instead removes
them so mergeUrlParams are more flexible.

app/assets/javascripts/lib/utils/url_utility.js

@@ -63,15 +63,22 @@ export function getParameterValues(sParam, url = window.location) {
   }, []);
 }
 
-// @param {Object} params - url keys and value to merge
-// @param {String} url
+/**
+ * Merges a URL to a set of params replacing value for
+ * those already present.
+ *
+ * Also removes `null` param values from the resulting URL.
+ *
+ * @param {Object} params - url keys and value to merge
+ * @param {String} url
+ */
 export function mergeUrlParams(params, url) {
   const re = /^([^?#]*)(\?[^#]*)?(.*)/;
   const merged = {};
-  const urlparts = url.match(re);
+  const [, fullpath, query, fragment] = url.match(re);
 
-  if (urlparts[2]) {
-    urlparts[2]
+  if (query) {
+    query
       .substr(1)
       .split('&')
       .forEach(part => {
@@ -84,11 +91,15 @@ export function mergeUrlParams(params, url) {
 
   Object.assign(merged, params);
 
-  const query = Object.keys(merged)
+  const newQuery = Object.keys(merged)
+    .filter(key => merged[key] !== null)
     .map(key => `${encodeURIComponent(key)}=${encodeURIComponent(merged[key])}`)
     .join('&');
 
-  return `${urlparts[1]}?${query}${urlparts[3]}`;
+  if (newQuery) {
+    return `${fullpath}?${newQuery}${fragment}`;
+  }
+  return `${fullpath}${fragment}`;
 }
 
 /**

spec/frontend/diffs/store/actions_spec.js

@@ -313,7 +313,7 @@ describe('DiffsStoreActions', () => {
 
     describe('fetchDiffFilesMeta', () => {
       it('should fetch diff meta information', done => {
-        const endpointMetadata = '/fetch/diffs_meta?';
+        const endpointMetadata = '/fetch/diffs_meta';
         const mock = new MockAdapter(axios);
         const data = { diff_files: [] };
         const res = { data };

spec/frontend/lib/utils/url_utility_spec.js

@@ -91,36 +91,75 @@ describe('URL utility', () => {
   });
 
   describe('mergeUrlParams', () => {
+    const { mergeUrlParams } = urlUtils;
+
     it('adds w', () => {
-      expect(urlUtils.mergeUrlParams({ w: 1 }, '#frag')).toBe('?w=1#frag');
-      expect(urlUtils.mergeUrlParams({ w: 1 }, '/path#frag')).toBe('/path?w=1#frag');
-      expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://host/path')).toBe('https://host/path?w=1');
-      expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://host/path#frag')).toBe(
-        'https://host/path?w=1#frag',
-      );
+      expect(mergeUrlParams({ w: 1 }, '#frag')).toBe('?w=1#frag');
+      expect(mergeUrlParams({ w: 1 }, '')).toBe('?w=1');
+      expect(mergeUrlParams({ w: 1 }, '/path#frag')).toBe('/path?w=1#frag');
+      expect(mergeUrlParams({ w: 1 }, 'https://host/path')).toBe('https://host/path?w=1');
+      expect(mergeUrlParams({ w: 1 }, 'https://host/path#frag')).toBe('https://host/path?w=1#frag');
+      expect(mergeUrlParams({ w: 1 }, 'https://h/p?k1=v1#frag')).toBe('https://h/p?k1=v1&w=1#frag');
+      expect(mergeUrlParams({ w: 'null' }, '')).toBe('?w=null');
+    });
 
-      expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://h/p?k1=v1#frag')).toBe(
-        'https://h/p?k1=v1&w=1#frag',
-      );
+    it('adds multiple params', () => {
+      expect(mergeUrlParams({ a: 1, b: 2, c: 3 }, '#frag')).toBe('?a=1&b=2&c=3#frag');
     });
 
     it('updates w', () => {
-      expect(urlUtils.mergeUrlParams({ w: 1 }, '?k1=v1&w=0#frag')).toBe('?k1=v1&w=1#frag');
+      expect(mergeUrlParams({ w: 2 }, '/path?w=1#frag')).toBe('/path?w=2#frag');
+      expect(mergeUrlParams({ w: 2 }, 'https://host/path?w=1')).toBe('https://host/path?w=2');
     });
 
-    it('adds multiple params', () => {
-      expect(urlUtils.mergeUrlParams({ a: 1, b: 2, c: 3 }, '#frag')).toBe('?a=1&b=2&c=3#frag');
+    it('removes null w', () => {
+      expect(mergeUrlParams({ w: null }, '?w=1#frag')).toBe('#frag');
+      expect(mergeUrlParams({ w: null }, '/path?w=1#frag')).toBe('/path#frag');
+      expect(mergeUrlParams({ w: null }, 'https://host/path?w=1')).toBe('https://host/path');
+      expect(mergeUrlParams({ w: null }, 'https://host/path?w=1#frag')).toBe(
+        'https://host/path#frag',
+      );
+      expect(mergeUrlParams({ w: null }, 'https://h/p?k1=v1&w=1#frag')).toBe(
+        'https://h/p?k1=v1#frag',
+      );
     });
 
-    it('adds and updates encoded params', () => {
-      expect(urlUtils.mergeUrlParams({ a: '&', q: '?' }, '?a=%23#frag')).toBe('?a=%26&q=%3F#frag');
+    it('adds and updates encoded param values', () => {
+      expect(mergeUrlParams({ foo: '&', q: '?' }, '?foo=%23#frag')).toBe('?foo=%26&q=%3F#frag');
+      expect(mergeUrlParams({ foo: 'a value' }, '')).toBe('?foo=a%20value');
+      expect(mergeUrlParams({ foo: 'a value' }, '?foo=1')).toBe('?foo=a%20value');
+    });
+
+    it('adds and updates encoded param names', () => {
+      expect(mergeUrlParams({ 'a name': 1 }, '')).toBe('?a%20name=1');
+      expect(mergeUrlParams({ 'a name': 2 }, '?a%20name=1')).toBe('?a%20name=2');
+      expect(mergeUrlParams({ 'a name': null }, '?a%20name=1')).toBe('');
     });
 
     it('treats "+" as "%20"', () => {
-      expect(urlUtils.mergeUrlParams({ ref: 'bogus' }, '?a=lorem+ipsum&ref=charlie')).toBe(
+      expect(mergeUrlParams({ ref: 'bogus' }, '?a=lorem+ipsum&ref=charlie')).toBe(
         '?a=lorem%20ipsum&ref=bogus',
       );
     });
+
+    it('treats question marks and slashes as part of the query', () => {
+      expect(mergeUrlParams({ ending: '!' }, '?ending=?&foo=bar')).toBe('?ending=!&foo=bar');
+      expect(mergeUrlParams({ ending: '!' }, 'https://host/path?ending=?&foo=bar')).toBe(
+        'https://host/path?ending=!&foo=bar',
+      );
+      expect(mergeUrlParams({ ending: '?' }, '?ending=!&foo=bar')).toBe('?ending=%3F&foo=bar');
+      expect(mergeUrlParams({ ending: '?' }, 'https://host/path?ending=!&foo=bar')).toBe(
+        'https://host/path?ending=%3F&foo=bar',
+      );
+      expect(mergeUrlParams({ ending: '!', op: '+' }, '?ending=?&op=/')).toBe('?ending=!&op=%2B');
+      expect(mergeUrlParams({ ending: '!', op: '+' }, 'https://host/path?ending=?&op=/')).toBe(
+        'https://host/path?ending=!&op=%2B',
+      );
+      expect(mergeUrlParams({ op: '+' }, '?op=/&foo=bar')).toBe('?op=%2B&foo=bar');
+      expect(mergeUrlParams({ op: '+' }, 'https://host/path?op=/&foo=bar')).toBe(
+        'https://host/path?op=%2B&foo=bar',
+      );
+    });
   });
 
   describe('removeParams', () => {