Skip to content
This repository has been archived by the owner on Mar 31, 2024. It is now read-only.

Simplify HashingStore interface. #18

Merged
merged 1 commit into from
Aug 24, 2016
Merged

Simplify HashingStore interface. #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/ui/public/state_management/__tests__/state.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,13 @@ import { encode as encodeRison } from 'rison-node';
import 'ui/private';
import Notifier from 'ui/notify/notifier';
import StateManagementStateProvider from 'ui/state_management/state';
import { unhashQueryString } from 'ui/state_management/state_hashing';
import { HashingStore } from 'ui/state_management/state_storage';
import {
unhashQueryString,
} from 'ui/state_management/state_hashing';
import {
createStorageHash,
HashingStore,
} from 'ui/state_management/state_storage';
import StubBrowserStorage from 'test_utils/stub_browser_storage';
import EventsProvider from 'ui/events';

Expand All @@ -32,7 +37,7 @@ describe('State Management', function () {
const { param, initial, storeInHash } = (opts || {});
sinon.stub(config, 'get').withArgs('state:storeInSessionStorage').returns(!!storeInHash);
const store = new StubBrowserStorage();
const hashingStore = new HashingStore({ store });
const hashingStore = new HashingStore(createStorageHash, store);
const state = new State(param, initial, { hashingStore, notifier });

const getUnhashedSearch = state => {
Expand Down
25 changes: 14 additions & 11 deletions src/ui/public/state_management/state.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import Notifier from 'ui/notify/notifier';
import KbnUrlProvider from 'ui/url';

import {
createStorageHash,
HashingStore,
LazyLruStore,
} from './state_storage';
Expand All @@ -21,34 +22,36 @@ export default function StateProvider(Private, $rootScope, $location, config) {
function State(urlParam, defaults, { hashingStore, notifier } = {}) {
State.Super.call(this);

let self = this;
self.setDefaults(defaults);
self._urlParam = urlParam || '_s';
this.setDefaults(defaults);
this._urlParam = urlParam || '_s';
this._notifier = notifier || new Notifier();
self._hashingStore = hashingStore || new HashingStore({
store: new LazyLruStore({

this._hashingStore = hashingStore || (() => {
const lazyLruStore = new LazyLruStore({
id: `${this._urlParam}:state`,
store: window.sessionStorage,
maxItems: MAX_BROWSER_HISTORY
})
});
});

return new HashingStore(createStorageHash, lazyLruStore);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can simplify the constructor to the point where it doesn't require a config object.

})();

// When the URL updates we need to fetch the values from the URL
self._cleanUpListeners = _.partial(_.callEach, [
this._cleanUpListeners = _.partial(_.callEach, [
// partial route update, no app reload
$rootScope.$on('$routeUpdate', function () {
$rootScope.$on('$routeUpdate', () => {
self.fetch();
}),

// beginning of full route update, new app will be initialized before
// $routeChangeSuccess or $routeChangeError
$rootScope.$on('$routeChangeStart', function () {
$rootScope.$on('$routeChangeStart', () => {
if (!self._persistAcrossApps) {
self.destroy();
}
}),

$rootScope.$on('$routeChangeSuccess', function () {
$rootScope.$on('$routeChangeSuccess', () => {
if (self._persistAcrossApps) {
self.fetch();
}
Expand Down
36 changes: 17 additions & 19 deletions src/ui/public/state_management/state_storage/_tests__/hashing_store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,22 @@
import expect from 'expect.js';
import sinon from 'sinon';
import { encode as encodeRison } from 'rison-node';

import StubBrowserStorage from 'test_utils/stub_browser_storage';
import { HashingStore } from 'ui/state_management/state_storage';
import {
createStorageHash,
HashingStore,
} from 'ui/state_management/state_storage';

const setup = ({ createHash } = {}) => {
const setup = createStorageHash => {
const store = new StubBrowserStorage();
const hashingStore = new HashingStore({ store, createHash });
const hashingStore = new HashingStore(createStorageHash, store);
return { store, hashingStore };
};

describe('Hashing Store', () => {
describe('#hashAndSetItem', () => {
it('adds a value to the store and returns its hash', () => {
const { hashingStore, store } = setup();
const { hashingStore, store } = setup(createStorageHash);
const val = { foo: 'bar' };
const hash = hashingStore.hashAndSetItem(val);
expect(hash).to.be.a('string');
Expand All @@ -23,7 +25,7 @@ describe('Hashing Store', () => {
});

it('json encodes the values it stores', () => {
const { hashingStore, store } = setup();
const { hashingStore, store } = setup(createStorageHash);
const val = { toJSON() { return 1; } };
const hash = hashingStore.hashAndSetItem(val);
expect(hashingStore.getItemAtHash(hash)).to.eql(1);
Expand All @@ -32,9 +34,7 @@ describe('Hashing Store', () => {
it('addresses values with a short hash', () => {
const val = { foo: 'bar' };
const longHash = 'longlonglonglonglonglonglonglonglonglonghash';
const { hashingStore } = setup({
createHash: () => longHash
});
const { hashingStore } = setup(() => longHash);

const hash = hashingStore.hashAndSetItem(val);
expect(hash.length < longHash.length).to.be.ok();
Expand All @@ -57,11 +57,9 @@ describe('Hashing Store', () => {
];

const matchVal = json => f => JSON.stringify(f.val) === json;
const { hashingStore } = setup({
createHash: val => {
const fixture = fixtures.find(matchVal(val));
return fixture.hash;
}
const { hashingStore } = setup(val => {
const fixture = fixtures.find(matchVal(val));
return fixture.hash;
});

const hash1 = hashingStore.hashAndSetItem(fixtures[0].val);
Expand All @@ -73,7 +71,7 @@ describe('Hashing Store', () => {
});

it('bubbles up the error if the store fails to hashAndSetItem', () => {
const { store, hashingStore } = setup();
const { store, hashingStore } = setup(createStorageHash);
const err = new Error();
sinon.stub(store, 'setItem').throws(err);
expect(() => {
Expand All @@ -84,14 +82,14 @@ describe('Hashing Store', () => {

describe('#getItemAtHash', () => {
it('reads a value from the store by its hash', () => {
const { hashingStore } = setup();
const { hashingStore } = setup(createStorageHash);
const val = { foo: 'bar' };
const hash = hashingStore.hashAndSetItem(val);
expect(hashingStore.getItemAtHash(hash)).to.eql(val);
});

it('returns null when the value is not in the store', () => {
const { hashingStore } = setup();
const { hashingStore } = setup(createStorageHash);
const val = { foo: 'bar' };
const hash = hashingStore.hashAndSetItem(val);
expect(hashingStore.getItemAtHash(`${hash} break`)).to.be(null);
Expand All @@ -100,7 +98,7 @@ describe('Hashing Store', () => {

describe('#isHash', () => {
it('can identify values that look like hashes', () => {
const { hashingStore } = setup();
const { hashingStore } = setup(createStorageHash);
const val = { foo: 'bar' };
const hash = hashingStore.hashAndSetItem(val);
expect(hashingStore.isHash(hash)).to.be(true);
Expand All @@ -118,7 +116,7 @@ describe('Hashing Store', () => {

tests.forEach(([type, val]) => {
it(`is not fooled by rison ${type} "${val}"`, () => {
const { hashingStore } = setup();
const { hashingStore } = setup(createStorageHash);
const rison = encodeRison(val);
expect(hashingStore.isHash(rison)).to.be(false);
});
Expand Down
5 changes: 5 additions & 0 deletions src/ui/public/state_management/state_storage/create_storage_hash.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { Sha256 } from 'ui/crypto';

export default function createStorageHash(json) {
return new Sha256().update(json, 'utf8').digest('hex');
}
27 changes: 5 additions & 22 deletions src/ui/public/state_management/state_storage/hashing_store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,4 @@
import angular from 'angular';
import { sortBy } from 'lodash';
import { Sha256 } from 'ui/crypto';

import StubBrowserStorage from 'test_utils/stub_browser_storage';
import { LazyLruStore } from './lazy_lru_store';

/**
* The HashingStore is a wrapper around a browser store object
Expand All @@ -12,9 +7,9 @@ import { LazyLruStore } from './lazy_lru_store';
* at a later time.
*/
class HashingStore {
constructor({ store, createHash, maxItems } = {}) {
this._store = store || window.sessionStorage;
if (createHash) this._createHash = createHash;
constructor(createStorageHash, store) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why get rid of the config object?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think config objects are great when dealing with a ton of arguments (more than 3), since they add more meaning for each argument when invoking the method.

On the other hand, there's also some meaning in the order in which arguments are passed (in this case, the createStoreHash function is used to set items in the store, so it comes first). And config objects require additional syntax in their creation and in their consumption.

Since this constructor only needs two arguments, I don't think this is an appropriate case for a config object. The readability benefits of the config object through named parameters don't exist any more and the additional syntax required to use the config object becomes noise, in my opinion.

Also, personally, config objects stand out to me as yellow flags that indicate there may be a way to simplify the code and user fewer arguments. So by removing the config object here, I've removed that yellow flag for myself.

this._createStorageHash = createStorageHash;
this._store = store;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove the defaults to make the dependencies explicit.

}

/**
Expand Down Expand Up @@ -49,25 +44,13 @@ class HashingStore {
* @return {string} the hash of the value
*/
hashAndSetItem(object) {
// The object may contain Angular $$ properties, so let's ignore them.
const json = angular.toJson(object);
const hash = this._getShortHash(json);
this._store.setItem(hash, json);
return hash;
}

// private api

/**
* calculate the full hash of a json object
*
* @private
* @param {string} json
* @return {string} hash
*/
_createHash(json) {
return new Sha256().update(json, 'utf8').digest('hex');
}

/**
* Calculate the full hash for a json blob and then shorten in until
* it until it doesn't collide with other short hashes in the store
Expand All @@ -77,7 +60,7 @@ class HashingStore {
* @param {string} shortHash
*/
_getShortHash(json) {
const fullHash = `${HashingStore.HASH_TAG}${this._createHash(json)}`;
const fullHash = `${HashingStore.HASH_TAG}${this._createStorageHash(json)}`;

let short;
for (let i = 7; i < fullHash.length; i++) {
Expand Down
4 changes: 4 additions & 0 deletions src/ui/public/state_management/state_storage/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
export {
default as createStorageHash,
} from './create_storage_hash';

export {
default as HashingStore,
} from './hashing_store';
Expand Down