From b29546e578baf4dcce63fa226e1e66dd7f9e6e4d Mon Sep 17 00:00:00 2001 From: John Cowen Date: Thu, 5 Jul 2018 13:35:06 +0100 Subject: [PATCH] Looking into atob functionality, consequence of Value: null The Consul API can pass through `Value: null` which does not get cast to a string by ember-data. This snowballs into problems with `atob` which then tried to decode `null`. There are 2 problems here. 1. `Value` should never be `null` - I've added a removeNull function to shallowly loop though props and remove properties that are `null`, for the moment this is only on single KV JSON responses - therefore `Value` will never be `null` which is the root of the problem 2. `atob` doesn't quite follow the `window.atob` API in that the `window.atob` API casts everything down to a string first, therefore it will try to decode `null` > `'null'` > `crazy unicode thing`. - I've commented in a fix for this, but whilst this shouldn't be causing anymore problems in our UI (now that `Value` is never `null`), I'll uncomment it in another future release. Tests are already written for it which more closely follow `window.atob` but skipped for now (next commit) --- ui-v2/app/adapters/kv.js | 3 +- ui-v2/app/utils/atob.js | 1 + ui-v2/app/utils/remove-null.js | 9 +++ ui-v2/tests/helpers/stub-super.js | 1 + ui-v2/tests/unit/adapters/kv-test.js | 2 +- ui-v2/tests/unit/utils/atob-test.js | 67 ++++++++++++++++++++++ ui-v2/tests/unit/utils/remove-null-test.js | 49 ++++++++++++++++ 7 files changed, 130 insertions(+), 2 deletions(-) create mode 100644 ui-v2/app/utils/remove-null.js create mode 100644 ui-v2/tests/unit/utils/atob-test.js create mode 100644 ui-v2/tests/unit/utils/remove-null-test.js diff --git a/ui-v2/app/adapters/kv.js b/ui-v2/app/adapters/kv.js index 309d467889..4f21239b7c 100644 --- a/ui-v2/app/adapters/kv.js +++ b/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), }, diff --git a/ui-v2/app/utils/atob.js b/ui-v2/app/utils/atob.js index edac64c916..237a4b5d4a 100644 --- a/ui-v2/app/utils/atob.js +++ b/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); diff --git a/ui-v2/app/utils/remove-null.js b/ui-v2/app/utils/remove-null.js new file mode 100644 index 0000000000..c624365ea9 --- /dev/null +++ b/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) { + return { ...prev, ...{ [item]: obj[item] } }; + } + return prev; + }, {}); +} diff --git a/ui-v2/tests/helpers/stub-super.js b/ui-v2/tests/helpers/stub-super.js index af19a48164..a10a2a37cf 100644 --- a/ui-v2/tests/helpers/stub-super.js +++ b/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) { diff --git a/ui-v2/tests/unit/adapters/kv-test.js b/ui-v2/tests/unit/adapters/kv-test.js index c0f03b1746..a1bcb35556 100644 --- a/ui-v2/tests/unit/adapters/kv-test.js +++ b/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); }); }); diff --git a/ui-v2/tests/unit/utils/atob-test.js b/ui-v2/tests/unit/utils/atob-test.js new file mode 100644 index 0000000000..a0d8ce8952 --- /dev/null +++ b/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: '��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); + }); + }); +}); diff --git a/ui-v2/tests/unit/utils/remove-null-test.js b/ui-v2/tests/unit/utils/remove-null-test.js new file mode 100644 index 0000000000..c887deb595 --- /dev/null +++ b/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');