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

Rename HashingStore interface methods for consistency and clarity. #17

Merged
merged 1 commit into from
Aug 24, 2016
Merged

Rename HashingStore interface methods for consistency and clarity. #17

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
30 changes: 14 additions & 16 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 @@ -13,7 +13,7 @@ import StubBrowserStorage from 'test_utils/stub_browser_storage';
import EventsProvider from 'ui/events';

describe('State Management', function () {
const notify = new Notifier();
const notifier = new Notifier();
Copy link
Author

@cjcenizal cjcenizal Aug 24, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I think this works better as a noun as a verb, since it's an object. Also, I think it's a more common convention to see a class instance assume the name of the class but with a lowercase first letter.

Copy link
Owner

Choose a reason for hiding this comment

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

I agree, only used notify to be consistent with the rest of the code base, but I'm fine changing it.

let $rootScope;
let $location;
let State;
Expand All @@ -33,13 +33,13 @@ describe('State Management', function () {
sinon.stub(config, 'get').withArgs('state:storeInSessionStorage').returns(!!storeInHash);
const store = new StubBrowserStorage();
const hashingStore = new HashingStore({ store });
const state = new State(param, initial, { hashingStore, notify });
const state = new State(param, initial, { hashingStore, notifier });

const getUnhashedSearch = state => {
return unhashQueryString($location.search(), [ state ]);
};

return { notify, store, hashingStore, state, getUnhashedSearch };
return { notifier, store, hashingStore, state, getUnhashedSearch };
};
}));

Expand Down Expand Up @@ -185,7 +185,6 @@ describe('State Management', function () {
});
});


describe('Hashing', () => {
it('stores state values in a hashingStore, writing the hash to the url', () => {
const { state, hashingStore } = setup({ storeInHash: true });
Expand All @@ -194,7 +193,7 @@ describe('State Management', function () {
const urlVal = $location.search()[state.getQueryParamName()];

expect(hashingStore.isHash(urlVal)).to.be(true);
expect(hashingStore.lookup(urlVal)).to.eql({ foo: 'bar' });
expect(hashingStore.getItemAtHash(urlVal)).to.eql({ foo: 'bar' });
});

it('should replace rison in the URL with a hash', () => {
Expand All @@ -208,29 +207,28 @@ describe('State Management', function () {
const urlVal = $location.search()._s;
expect(urlVal).to.not.be(rison);
expect(hashingStore.isHash(urlVal)).to.be(true);
expect(hashingStore.lookup(urlVal)).to.eql(obj);
expect(hashingStore.getItemAtHash(urlVal)).to.eql(obj);
});

context('error handling', () => {
it('notifies the user when a hash value does not map to a stored value', () => {
const { state, hashingStore, notify } = setup({ storeInHash: true });
const { state, hashingStore, notifier } = setup({ storeInHash: true });
const search = $location.search();
const badHash = hashingStore.add({});
hashingStore.remove(badHash);
const badHash = hashingStore._getShortHash('{"a": "b"}');

search[state.getQueryParamName()] = badHash;
$location.search(search);

expect(notify._notifs).to.have.length(0);
expect(notifier._notifs).to.have.length(0);
state.fetch();
expect(notify._notifs).to.have.length(1);
expect(notify._notifs[0].content).to.match(/use the share functionality/i);
expect(notifier._notifs).to.have.length(1);
expect(notifier._notifs[0].content).to.match(/use the share functionality/i);
});

it('presents fatal error linking to github when hashingStore.add fails', () => {
const { state, hashingStore, notify } = setup({ storeInHash: true });
const fatalStub = sinon.stub(notify, 'fatal').throws();
sinon.stub(hashingStore, 'add').throws();
it('presents fatal error linking to github when hashingStore.hashAndSetItem fails', () => {
const { state, hashingStore, notifier } = setup({ storeInHash: true });
const fatalStub = sinon.stub(notifier, 'fatal').throws();
sinon.stub(hashingStore, 'hashAndSetItem').throws();

expect(() => {
state.toQueryParam();
Expand Down
23 changes: 12 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 @@ -18,14 +18,14 @@ export default function StateProvider(Private, $rootScope, $location, config) {
const Events = Private(EventsProvider);

_.class(State).inherits(Events);
function State(urlParam, defaults, { hashingStore, notify } = {}) {
function State(urlParam, defaults, { hashingStore, notifier } = {}) {
State.Super.call(this);

let self = this;
self.setDefaults(defaults);
self._urlParam = urlParam || '_s';
this._notify = notify || new Notifier();
self._hasher = hashingStore || new HashingStore({
this._notifier = notifier || new Notifier();
self._hashingStore = hashingStore || new HashingStore({
Copy link
Author

@cjcenizal cjcenizal Aug 24, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Same ideas here -- use a lower-cased version of the class name for the instance name. I think it helps make the relationship clearer.

store: new LazyLruStore({
id: `${this._urlParam}:state`,
store: window.sessionStorage,
Expand Down Expand Up @@ -67,7 +67,7 @@ export default function StateProvider(Private, $rootScope, $location, config) {
return null;
}

if (this._hasher.isHash(urlVal)) {
if (this._hashingStore.isHash(urlVal)) {
return this._parseQueryParamValue(urlVal);
}

Expand All @@ -80,7 +80,7 @@ export default function StateProvider(Private, $rootScope, $location, config) {
}

if (unableToParse) {
this._notify.error('Unable to parse URL');
this._notifier.error('Unable to parse URL');
search[this._urlParam] = this.toQueryParam(this._defaults);
$location.search(search).replace();
}
Expand Down Expand Up @@ -194,13 +194,13 @@ export default function StateProvider(Private, $rootScope, $location, config) {
* @return {any} - the stored value, or null if hash does not resolve
*/
State.prototype._parseQueryParamValue = function (queryParam) {
if (!this._hasher.isHash(queryParam)) {
if (!this._hashingStore.isHash(queryParam)) {
return rison.decode(queryParam);
}

const stored = this._hasher.lookup(queryParam);
const stored = this._hashingStore.getItemAtHash(queryParam);
if (stored === null) {
this._notify.error('Unable to completely restore the URL, be sure to use the share functionality.');
this._notifier.error('Unable to completely restore the URL, be sure to use the share functionality.');
}

return stored;
Expand Down Expand Up @@ -228,10 +228,11 @@ export default function StateProvider(Private, $rootScope, $location, config) {
}

try {
return this._hasher.add(state);
const hash = this._hashingStore.hashAndSetItem(state);
return hash;
} catch (err) {
this._notify.log('Unable to create hash of State due to error: ' + (state.stack || state.message));
this._notify.fatal(
this._notifier.log('Unable to create hash of State due to error: ' + (state.stack || state.message));
this._notifier.fatal(
new Error(
'Kibana is unable to store history items in your session ' +
'because it is full and there don\'t seem to be items any items safe ' +
Expand Down
45 changes: 17 additions & 28 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
Expand Up @@ -11,12 +11,12 @@ const setup = ({ createHash } = {}) => {
return { store, hashingStore };
};

describe('State Management Hashing Store', () => {
describe('#add', () => {
describe('Hashing Store', () => {
describe('#hashAndSetItem', () => {
it('adds a value to the store and returns its hash', () => {
const { hashingStore, store } = setup();
const val = { foo: 'bar' };
const hash = hashingStore.add(val);
const hash = hashingStore.hashAndSetItem(val);
expect(hash).to.be.a('string');
expect(hash).to.be.ok();
expect(store).to.have.length(1);
Expand All @@ -25,8 +25,8 @@ describe('State Management Hashing Store', () => {
it('json encodes the values it stores', () => {
const { hashingStore, store } = setup();
const val = { toJSON() { return 1; } };
const hash = hashingStore.add(val);
expect(hashingStore.lookup(hash)).to.eql(1);
const hash = hashingStore.hashAndSetItem(val);
expect(hashingStore.getItemAtHash(hash)).to.eql(1);
});

it('addresses values with a short hash', () => {
Expand All @@ -36,7 +36,7 @@ describe('State Management Hashing Store', () => {
createHash: () => longHash
});

const hash = hashingStore.add(val);
const hash = hashingStore.hashAndSetItem(val);
expect(hash.length < longHash.length).to.be.ok();
});

Expand Down Expand Up @@ -64,56 +64,45 @@ describe('State Management Hashing Store', () => {
}
});

const hash1 = hashingStore.add(fixtures[0].val);
const hash2 = hashingStore.add(fixtures[1].val);
const hash3 = hashingStore.add(fixtures[2].val);
const hash1 = hashingStore.hashAndSetItem(fixtures[0].val);
const hash2 = hashingStore.hashAndSetItem(fixtures[1].val);
const hash3 = hashingStore.hashAndSetItem(fixtures[2].val);

expect(hash3).to.have.length(hash2.length + 1);
expect(hash2).to.have.length(hash1.length + 1);
});

it('bubbles up the error if the store fails to setItem', () => {
it('bubbles up the error if the store fails to hashAndSetItem', () => {
const { store, hashingStore } = setup();
const err = new Error();
sinon.stub(store, 'setItem').throws(err);
expect(() => {
hashingStore.add({});
hashingStore.hashAndSetItem({});
}).to.throwError(e => expect(e).to.be(err));
});
});

describe('#lookup', () => {
describe('#getItemAtHash', () => {
it('reads a value from the store by its hash', () => {
const { hashingStore } = setup();
const val = { foo: 'bar' };
const hash = hashingStore.add(val);
expect(hashingStore.lookup(hash)).to.eql(val);
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 val = { foo: 'bar' };
const hash = hashingStore.add(val);
expect(hashingStore.lookup(`${hash} break`)).to.be(null);
});
});

describe('#remove', () => {
it('removes the value by its hash', () => {
const { hashingStore } = setup();
const val = { foo: 'bar' };
const hash = hashingStore.add(val);
expect(hashingStore.lookup(hash)).to.eql(val);
hashingStore.remove(hash);
expect(hashingStore.lookup(hash)).to.be(null);
const hash = hashingStore.hashAndSetItem(val);
expect(hashingStore.getItemAtHash(`${hash} break`)).to.be(null);
});
});

describe('#isHash', () => {
it('can identify values that look like hashes', () => {
const { hashingStore } = setup();
const val = { foo: 'bar' };
const hash = hashingStore.add(val);
const hash = hashingStore.hashAndSetItem(val);
expect(hashingStore.isHash(hash)).to.be(true);
});

Expand Down
2 changes: 1 addition & 1 deletion src/ui/public/state_management/state_storage/_tests__/lazy_lru_store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ const setup = (opts = {}) => {
return { lru, store };
};

describe('State Management LazyLruStore', () => {
describe('LazyLruStore', () => {
Copy link
Author

Choose a reason for hiding this comment

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

I kept getting confused when reading the test error output in the terminal, because the State Management term is used for the state.js tests, too. This helped me find the offending tests more quickly.

describe('#getItem()', () => {
it('returns null when item not found', () => {
const { lru } = setup();
Expand Down
30 changes: 11 additions & 19 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
Expand Up @@ -5,15 +5,13 @@ import { Sha256 } from 'ui/crypto';
import StubBrowserStorage from 'test_utils/stub_browser_storage';
import { LazyLruStore } from './lazy_lru_store';

const TAG = 'h@';

/**
Copy link
Author

Choose a reason for hiding this comment

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

I would be OK keeping this here instead of as a static property on the class, but I think a name like HASH_TAG makes its role clearer.

* The HashingStore is a wrapper around a browser store object
* that hashes the items added to it and stores them by their
* hash. This hash is then returned so that the item can be received
* at a later time.
*/
export default class HashingStore {
class HashingStore {
constructor({ store, createHash, maxItems } = {}) {
this._store = store || window.sessionStorage;
if (createHash) this._createHash = createHash;
Expand All @@ -22,11 +20,11 @@ export default class HashingStore {
/**
* Determine if the passed value looks like a hash
*
* @param {string} hash
* @param {string} str
* @return {boolean}
*/
isHash(hash) {
return String(hash).slice(0, TAG.length) === TAG;
isHash(str) {
return String(str).indexOf(HashingStore.HASH_TAG) === 0;
}

/**
Expand All @@ -35,7 +33,7 @@ export default class HashingStore {
* @param {string} hash
* @return {any}
*/
lookup(hash) {
getItemAtHash(hash) {
try {
return JSON.parse(this._store.getItem(hash));
} catch (err) {
Expand All @@ -50,23 +48,13 @@ export default class HashingStore {
* @param {any} the value to hash
* @return {string} the hash of the value
*/
add(object) {
hashAndSetItem(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 the actions of hashAndSetItem and getItemAtHash are a bit more complementary than add and lookup, especially since add doesn't imply you're getting a hash back, which seems like a really critical feature here.

const json = angular.toJson(object);
const hash = this._getShortHash(json);
this._store.setItem(hash, json);
return hash;
}

/**
* Remove a value identified by the hash from the store
*
* @param {string} hash
* @return {undefined}
*/
remove(hash) {
this._store.removeItem(hash);
}

// private api

/**
Expand All @@ -89,7 +77,7 @@ export default class HashingStore {
* @param {string} shortHash
*/
_getShortHash(json) {
const fullHash = `${TAG}${this._createHash(json)}`;
const fullHash = `${HashingStore.HASH_TAG}${this._createHash(json)}`;

let short;
for (let i = 7; i < fullHash.length; i++) {
Expand All @@ -101,3 +89,7 @@ export default class HashingStore {
return short;
}
}

HashingStore.HASH_TAG = 'h@';

export default HashingStore;
Loading