Merge pull request #4343 from hashicorp/bugfix/gh-4336-update-empty-kv

UI - Bugfix: Saving empty key/value pairs

ui-v2/app/adapters/kv.js

@@ -11,6 +11,7 @@ import { get } from '@ember/object';
 import { inject as service } from '@ember/service';
 
 import keyToArray from 'consul-ui/utils/keyToArray';
+import removeNull from 'consul-ui/utils/remove-null';
 
 import { PRIMARY_KEY, SLUG_KEY } from 'consul-ui/models/kv';
 import { FOREIGN_KEY as DATACENTER_KEY } from 'consul-ui/models/dc';
@@ -98,7 +99,7 @@ export default Adapter.extend({
           break;
         case this.isQueryRecord(url):
           response = {
-            ...response[0],
+            ...removeNull(response[0]),
             ...{
               [PRIMARY_KEY]: this.uidForURL(url),
             },

ui-v2/app/models/kv.js

@@ -13,7 +13,11 @@ export default Model.extend({
   [SLUG_KEY]: attr('string'),
   LockIndex: attr('number'),
   Flags: attr('number'),
-  Value: attr('string'),
+  // TODO: Consider defaulting all strings to '' because `typeof null !== 'string'`
+  // look into what other transformers do with `null` also
+  // preferably removeNull would be done in this layer also as if a property is `null`
+  // default Values don't kick in, which also explains `Tags` elsewhere
+  Value: attr('string'), //, {defaultValue: function() {return '';}}
   CreateIndex: attr('string'),
   ModifyIndex: attr('string'),
   Session: attr('string'),

ui-v2/app/utils/atob.js

@@ -1,6 +1,7 @@
 import TextEncoderLite from 'npm:text-encoder-lite';
 import base64js from 'npm:base64-js';
 export default function(str, encoding = 'utf-8') {
+  // str = String(str).trim();
   //decode
   const bytes = base64js.toByteArray(str);
   return new (TextDecoder || TextEncoderLite)(encoding).decode(bytes);

ui-v2/app/utils/remove-null.js

@@ -0,0 +1,9 @@
+export default function(obj) {
+  // non-recursive for the moment
+  return Object.keys(obj).reduce(function(prev, item, i, arr) {
+    if (obj[item] !== null) {
+      prev[item] = obj[item];
+    }
+    return prev;
+  }, {});
+}

ui-v2/app/validations/kv.js

@@ -1,5 +1,4 @@
 import { validatePresence, validateLength } from 'ember-changeset-validations/validators';
 export default {
   Key: [validatePresence(true), validateLength({ min: 1 })],
-  Value: validatePresence(true),
 };

ui-v2/tests/acceptance/dc/kvs/update.feature

@@ -1,7 +1,8 @@
 @setupApplicationTest
 Feature: dc / kvs / update: KV Update
-  Scenario: Update to [Name] change value to [Value]
+  Background:
     Given 1 datacenter model with the value "datacenter"
+  Scenario: Update to [Name] change value to [Value]
     And 1 kv model from yaml
     ---
       Key: [Name]
@@ -25,6 +26,54 @@ Feature: dc / kvs / update: KV Update
       | key-name                  | a value      |
       | folder/key-name           | a value      |
       --------------------------------------------
+  Scenario: Update to a key change value to '   '
+    And 1 kv model from yaml
+    ---
+      Key: key
+    ---
+    When I visit the kv page for yaml
+    ---
+      dc: datacenter
+      kv: key
+    ---
+    Then the url should be /datacenter/kv/key/edit
+    Then I fill in with yaml
+    ---
+      value: '   '
+    ---
+    And I submit
+    Then a PUT request is made to "/v1/kv/key?dc=datacenter" with the body "   "
+  Scenario: Update to a key change value to ''
+    And 1 kv model from yaml
+    ---
+      Key: key
+    ---
+    When I visit the kv page for yaml
+    ---
+      dc: datacenter
+      kv: key
+    ---
+    Then the url should be /datacenter/kv/key/edit
+    Then I fill in with yaml
+    ---
+      value: ''
+    ---
+    And I submit
+    Then a PUT request is made to "/v1/kv/key?dc=datacenter" with no body
+  Scenario: Update to a key when the value is empty
+    And 1 kv model from yaml
+    ---
+    Key: key
+    Value: ~
+    ---
+    When I visit the kv page for yaml
+    ---
+      dc: datacenter
+      kv: key
+    ---
+    Then the url should be /datacenter/kv/key/edit
+    And I submit
+    Then a PUT request is made to "/v1/kv/key?dc=datacenter" with no body
 @ignore
   Scenario: The feedback dialog says success or failure
     Then ok

ui-v2/tests/helpers/stub-super.js

@@ -38,6 +38,7 @@ export default function(obj, stub) {
         return _super;
       },
     });
+    // TODO: try/catch this?
     const actual = cb();
     Object.defineProperty(Object.getPrototypeOf(obj), '_super', {
       set: function(val) {

ui-v2/tests/helpers/type-to-url.js

@@ -1,34 +1,32 @@
 export default function(type) {
-  let url = null;
+  let requests = null;
   switch (type) {
     case 'dc':
-      url = ['/v1/catalog/datacenters'];
+      requests = ['/v1/catalog/datacenters'];
       break;
     case 'service':
-      url = ['/v1/internal/ui/services', '/v1/health/service/'];
+      requests = ['/v1/internal/ui/services', '/v1/health/service/'];
       break;
     case 'node':
-      url = ['/v1/internal/ui/nodes'];
+      requests = ['/v1/internal/ui/nodes'];
       break;
     case 'kv':
-      url = '/v1/kv/';
+      requests = ['/v1/kv/'];
       break;
     case 'acl':
-      url = ['/v1/acl/list'];
+      requests = ['/v1/acl/list'];
       break;
     case 'session':
-      url = ['/v1/session/node/'];
+      requests = ['/v1/session/node/'];
       break;
   }
-  return function(actual) {
+  // TODO: An instance of URL should come in here (instead of 2 args)
-    if (url === null) {
+  return function(url, method) {
+    if (requests === null) {
       return false;
     }
-    if (typeof url === 'string') {
+    return requests.some(function(item) {
-      return url === actual;
+      return method.toUpperCase() === 'GET' && url.indexOf(item) === 0;
-    }
-    return url.some(function(item) {
-      return actual.indexOf(item) === 0;
     });
   };
 }

ui-v2/tests/steps.js

@@ -218,12 +218,20 @@ export default function(assert) {
         );
         assert.equal(request.url, url, `Expected the request url to be ${url}, was ${request.url}`);
         const body = request.requestBody;
-        assert.equal(
+        assert.equal(body, data, `Expected the request body to be ${data}, was ${body}`);
-          body,
-          data,
-          `Expected the request body to be ${body}, was ${request.requestBody}`
-        );
       })
+      .then('a $method request is made to "$url" with no body', function(method, url) {
+        const request = api.server.history[api.server.history.length - 2];
+        assert.equal(
+          request.method,
+          method,
+          `Expected the request method to be ${method}, was ${request.method}`
+        );
+        assert.equal(request.url, url, `Expected the request url to be ${url}, was ${request.url}`);
+        const body = request.requestBody;
+        assert.equal(body, null, `Expected the request body to be null, was ${body}`);
+      })
+
       .then('a $method request is made to "$url"', function(method, url) {
         const request = api.server.history[api.server.history.length - 2];
         assert.equal(

ui-v2/tests/unit/adapters/kv-test.js

@@ -46,7 +46,7 @@ module('Unit | Adapter | kv', function(hooks) {
       const uid = {
         uid: JSON.stringify([dc, expected]),
       };
-      const actual = adapter.handleResponse(200, {}, uid, { url: url });
+      const actual = adapter.handleResponse(200, {}, [uid], { url: url });
       assert.deepEqual(actual, uid);
     });
   });

ui-v2/tests/unit/utils/atob-test.js

@@ -0,0 +1,67 @@
+import { module } from 'ember-qunit';
+import test from 'ember-sinon-qunit/test-support/test';
+import { skip } from 'qunit';
+import atob from 'consul-ui/utils/atob';
+module('Unit | Utils | atob', {});
+
+skip('it decodes non-strings properly', function(assert) {
+  [
+    {
+      test: '        ',
+      expected: '',
+    },
+    {
+      test: new String(),
+      expected: '',
+    },
+    {
+      test: new String('MTIzNA=='),
+      expected: '1234',
+    },
+    {
+      test: [],
+      expected: '',
+    },
+    {
+      test: ['   '],
+      expected: '',
+    },
+    {
+      test: new Array(),
+      expected: '',
+    },
+    {
+      test: ['MTIzNA=='],
+      expected: '1234',
+    },
+    {
+      test: null,
+      expected: '<27><>e',
+    },
+  ].forEach(function(item) {
+    const actual = atob(item.test);
+    assert.equal(actual, item.expected);
+  });
+});
+test('it decodes strings properly', function(assert) {
+  [
+    {
+      test: '',
+      expected: '',
+    },
+    {
+      test: 'MTIzNA==',
+      expected: '1234',
+    },
+  ].forEach(function(item) {
+    const actual = atob(item.test);
+    assert.equal(actual, item.expected);
+  });
+});
+test('throws when passed the wrong value', function(assert) {
+  [{}, ['MTIz', 'NA=='], new Number(), 'hi'].forEach(function(item) {
+    assert.throws(function() {
+      atob(item);
+    });
+  });
+});

ui-v2/tests/unit/utils/remove-null-test.js

@@ -0,0 +1,49 @@
+import removeNull from 'consul-ui/utils/remove-null';
+import { skip } from 'qunit';
+import { module, test } from 'qunit';
+
+module('Unit | Utility | remove null');
+
+test('it removes null valued properties shallowly', function(assert) {
+  [
+    {
+      test: {
+        Value: null,
+      },
+      expected: {},
+    },
+    {
+      test: {
+        Key: 'keyname',
+        Value: null,
+      },
+      expected: {
+        Key: 'keyname',
+      },
+    },
+    {
+      test: {
+        Key: 'keyname',
+        Value: '',
+      },
+      expected: {
+        Key: 'keyname',
+        Value: '',
+      },
+    },
+    {
+      test: {
+        Key: 'keyname',
+        Value: false,
+      },
+      expected: {
+        Key: 'keyname',
+        Value: false,
+      },
+    },
+  ].forEach(function(item) {
+    const actual = removeNull(item.test);
+    assert.deepEqual(actual, item.expected);
+  });
+});
+skip('it removes null valued properties deeply');

ui-v2/yarn.lock

@@ -70,8 +70,8 @@
     "@glimmer/di" "^0.2.0"
 
 "@hashicorp/api-double@^1.3.0":
-  version "1.3.1"
+  version "1.4.0"
-  resolved "https://registry.yarnpkg.com/@hashicorp/api-double/-/api-double-1.3.1.tgz#fd9d706674b934857a638459c2bb52d2f2809455"
+  resolved "https://registry.yarnpkg.com/@hashicorp/api-double/-/api-double-1.4.0.tgz#17ddad8e55370de0d24151a38c5f029bc207cafe"
   dependencies:
     "@gardenhq/o" "^8.0.1"
     "@gardenhq/tick-control" "^2.0.0"