Skip to content

Commit

Permalink
introduce registry cache to prevent unnecessary lookups (hyperledger-…
Browse files Browse the repository at this point in the history 
…archives#4156)

Signed-off-by: Nick Lincoln <[email protected]>
  • Loading branch information
@nklincoln
nklincoln authored Jun 14, 2018
1 parent cc5d754 commit 2bbfa75
Show file tree
Hide file tree
Showing 2 changed files with 186 additions and 42 deletions.
108 changes: 72 additions & 36 deletions packages/composer-runtime/lib/registry.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,21 +29,6 @@ const baseDefaultOptions = {
*/
class Registry extends EventEmitter {

/**
* Remove any internal properties to the specified JSON object before
* reinflating it back into a resource.
* @param {Object} json The JSON object.
* @return {Object} The JSON object.
*/
static removeInternalProperties(json) {
if (!json || typeof json !== 'object' || Array.isArray(json)) {
throw new Error('Can only add properties to JSON objects');
}
delete json.$registryType;
delete json.$registryId;
return json;
}

/**
* Constructor.
* @param {string} dataCollection The data collection to use.
Expand All @@ -63,6 +48,22 @@ class Registry extends EventEmitter {
this.id = id;
this.name = name;
this.system = system;
this.resourceMap = new Map();
}

/**
* Remove any internal properties to the specified JSON object before
* reinflating it back into a resource.
* @param {Object} json The JSON object.
* @return {Object} The JSON object.
*/
static removeInternalProperties(json) {
if (!json || typeof json !== 'object' || Array.isArray(json)) {
throw new Error('Can only add properties to JSON objects');
}
delete json.$registryType;
delete json.$registryId;
return json;
}

/**
Expand All @@ -79,6 +80,7 @@ class Registry extends EventEmitter {
const resource = this.serializer.fromJSON(object);
await this.accessController.check(resource, 'READ');
resources.push(resource);
this.resourceMap.set(resource.getIdentifier(), resource);
} catch (error) {
// Ignore the error; we don't have access.
}
Expand All @@ -93,14 +95,19 @@ class Registry extends EventEmitter {
* object when complete, or rejected with an error.
*/
async get(id) {
let object = await this.dataCollection.get(id);
object = Registry.removeInternalProperties(object);
try {
const resource = this.serializer.fromJSON(object);
await this.accessController.check(resource, 'READ');
return resource;
} catch (error) {
throw new Error(`Object with ID '${id}' in collection with ID '${this.type}:${this.id}' does not exist`);
if (this.resourceMap.has(id)) {
return this.resourceMap.get(id);
} else {
let object = await this.dataCollection.get(id);
object = Registry.removeInternalProperties(object);
try {
const resource = this.serializer.fromJSON(object);
await this.accessController.check(resource, 'READ');
this.resourceMap.set(id, resource);
return resource;
} catch (error) {
throw new Error(`Object with ID '${id}' in collection with ID '${this.type}:${this.id}' does not exist`);
}
}
}

Expand All @@ -115,14 +122,20 @@ class Registry extends EventEmitter {
if (!exists) {
return false;
}
let object = await this.dataCollection.get(id);
object = Registry.removeInternalProperties(object);
try {
const resource = this.serializer.fromJSON(object);
await this.accessController.check(resource, 'READ');

if (this.resourceMap.has(id)) {
return true;
} catch (error) {
return false;
} else {
let object = await this.dataCollection.get(id);
object = Registry.removeInternalProperties(object);
try {
const resource = this.serializer.fromJSON(object);
await this.accessController.check(resource, 'READ');
this.resourceMap.set(id, resource);
return true;
} catch (error) {
return false;
}
}
}

Expand Down Expand Up @@ -200,7 +213,7 @@ class Registry extends EventEmitter {
}
}

/**
/**
* An event signalling that a resource has been updated in this registry.
* @event Registry#resourceupdated
* @protected
Expand Down Expand Up @@ -240,11 +253,24 @@ class Registry extends EventEmitter {
options = Object.assign({}, baseDefaultOptions, options);
let object = this.serializer.toJSON(resource, options);
object = this.addInternalProperties(object);
const oldObject = await this.dataCollection.get(id);
const oldResource = this.serializer.fromJSON(oldObject);

let oldResource;
if (this.resourceMap.has(id)) {
oldResource = this.resourceMap.get(id);
} else {
let oldObject = await this.dataCollection.get(id);
oldResource = this.serializer.fromJSON(oldObject);
}

// We must perform access control checks on the old version of the resource!
await this.accessController.check(oldResource, 'UPDATE');
await this.dataCollection.update(id, object);

// If is within resourceMap cache, update may have changed the ability to READ item, so remove it
if (this.resourceMap.has(id)) {
this.resourceMap.delete(id);
}

this.emit('resourceupdated', {
registry: this,
oldResource: oldResource,
Expand Down Expand Up @@ -281,13 +307,23 @@ class Registry extends EventEmitter {
// the resource using its ID from the registry. We need to
// do this to figure out the type of the resource for
// access control.
let object = await this.dataCollection.get(resource);
object = Registry.removeInternalProperties(object);
resource = this.serializer.fromJSON(object);
if (this.resourceMap.has(resource)) {
resource = this.resourceMap.get(resource);
} else {
let object = await this.dataCollection.get(resource);
object = Registry.removeInternalProperties(object);
resource = this.serializer.fromJSON(object);
}
}
const id = resource.getIdentifier();
await this.accessController.check(resource, 'DELETE');
await this.dataCollection.remove(id);

// Remove from cache if present
if (this.resourceMap.has(id)) {
this.resourceMap.delete(id);
}

this.emit('resourceremoved', {
registry: this,
resourceID: id
Expand Down
120 changes: 114 additions & 6 deletions packages/composer-runtime/test/registry.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,10 @@ describe('Registry', () => {
}]);
mockResource1 = sinon.createStubInstance(Resource);
mockResource1.theValue = 'the value 1';
mockResource1.getIdentifier.returns('1');
mockResource2 = sinon.createStubInstance(Resource);
mockResource2.theValue = 'the value 2';
mockResource2.getIdentifier.returns('2');
mockSerializer.fromJSON.withArgs({
$class: 'org.doge.Doge',
assetId: 'doge1'
Expand All @@ -105,7 +107,7 @@ describe('Registry', () => {
}).returns(mockResource2);
});

it('should get and parse all of the resources in the registry', () => {
it('should get and parse all of the resources from the registry if not in the cache', () => {
return registry.getAll()
.then((resources) => {
sinon.assert.calledTwice(mockAccessController.check);
Expand All @@ -116,6 +118,15 @@ describe('Registry', () => {
});
});

it('should add resources to the cache during the retrieval', async () => {
await registry.getAll();
let map = registry.resourceMap;
let keys = map.keys();
for (let key of keys){
map.get(key).should.be.an.instanceOf(Resource);
}
});

it('should not throw or leak information about resources that cannot be accessed due to ACL rules', () => {
mockAccessController.check.withArgs(mockResource2, 'READ').rejects(new AccessException(mockResource2, 'READ', mockParticipant));
return registry.getAll()
Expand Down Expand Up @@ -169,7 +180,7 @@ describe('Registry', () => {
}).returns(mockResource);
});

it('should get the specific resource in the registry', () => {
it('should get the specific resource from the registry if not in the cache', () => {
return registry.get('doge1')
.then((resource) => {
sinon.assert.calledOnce(mockAccessController.check);
Expand All @@ -179,6 +190,25 @@ describe('Registry', () => {
});
});

it('should add to the cache after registry retrieval', async () => {
await registry.get('doge1');
let resource = registry.resourceMap.get('doge1');
resource.should.be.an.instanceOf(Resource);
});

it('should get the specific resource from the cache if present', async () => {
let mockResource = sinon.createStubInstance(Resource);
mockResource.theValue = 'the value 1';
mockResource.getIdentifier.returns('doge1');
registry.resourceMap.set('doge1', mockResource);

let resource = await registry.get('doge1');
resource.should.be.an.instanceOf(Resource);
resource.getIdentifier().should.equal('doge1');
sinon.assert.notCalled(mockDataCollection.get);
sinon.assert.notCalled(mockAccessController.check);
});

it('should not throw or leak information about resources that cannot be accessed', () => {
mockAccessController.check.withArgs(mockResource, 'READ').rejects(new AccessException(mockResource, 'READ', mockParticipant));
return registry.get('doge1')
Expand Down Expand Up @@ -210,7 +240,7 @@ describe('Registry', () => {
}).returns(mockResource);
});

it('should determine whether a specific resource exists in the registry', () => {
it('should determine whether a specific resource exists in the registry via direct lookup', () => {
return registry.exists('doge1')
.should.eventually.be.true
.then((exists) => {
Expand All @@ -219,6 +249,28 @@ describe('Registry', () => {
});
});

it('should add to the cache after a direct lookup', () => {
return registry.exists('doge1')
.should.eventually.be.true
.then((exists) => {
registry.resourceMap.get('doge1').should.be.an.instanceOf(Resource);
});
});

it('should determine whether a specific resource exists via cache existence', async () => {
let mockResource = sinon.createStubInstance(Resource);
mockResource.theValue = 'the value 1';
mockResource.getIdentifier.returns('doge1');
registry.resourceMap.set('doge1', mockResource);

return registry.exists('doge1')
.should.eventually.be.true
.then((exists) => {
sinon.assert.notCalled(mockAccessController.check);
sinon.assert.notCalled(mockDataCollection.get);
});
});

it('should determine whether a specific resource does not exist in the registry', () => {
return registry.exists('doge2')
.should.eventually.be.false;
Expand Down Expand Up @@ -573,7 +625,7 @@ describe('Registry', () => {
}).returns(mockOldResource);
});

it('should update the resource in the registry', () => {
it('should update the resource in the registry via datacollection retrieval if not in the cache', () => {
mockDataCollection.update.resolves();
let mockEventHandler = sinon.stub();
registry.on('resourceupdated', mockEventHandler);
Expand All @@ -596,8 +648,38 @@ describe('Registry', () => {
});
});
});
it('should throw an error updating a resource to the wrong registry', () => {

it('should update the resource in the registry via cache retrieval if present', () => {
mockDataCollection.update.resolves();
let mockEventHandler = sinon.stub();
registry.resourceMap.set('doge1', mockResource);
registry.on('resourceupdated', mockEventHandler);
return registry.update(mockResource)
.then(() => {
sinon.assert.notCalled(mockDataCollection.get);
sinon.assert.calledOnce(mockAccessController.check);
sinon.assert.calledWith(mockAccessController.check, mockResource, 'UPDATE');
sinon.assert.calledWith(mockDataCollection.update, 'doge1', {
$registryType: 'Asset',
$registryId: 'doges',
$class: 'org.doge.Doge',
assetId: 'doge1',
newValue: 'newValue'
});
sinon.assert.calledOnce(mockEventHandler);
});
});

it('should remove the item from teh cache if updated via cache retrieval', async () => {
mockDataCollection.update.resolves();
let mockEventHandler = sinon.stub();
registry.resourceMap.set('doge1', mockResource);
registry.on('resourceupdated', mockEventHandler);
await registry.update(mockResource);
registry.resourceMap.has('doge1').should.be.false;
});

it('should throw an error updating a resource to the wrong registry', () => {
mockDataCollection.update.resolves();
mockClassDeclaration.getSystemType.returns('Transaction');
mockOldClassDeclaration.getSystemType.returns('Transaction');
Expand Down Expand Up @@ -729,12 +811,29 @@ describe('Registry', () => {
});
});

it('should remove the resource by ID from the registry', () => {
it('should remove the resource by ID from the registry via registry lookup if not in the cache', () => {
mockDataCollection.remove.resolves();
let mockEventHandler = sinon.stub();
registry.on('resourceremoved', mockEventHandler);
return registry.remove('doge1')
.then(() => {
sinon.assert.calledWith(mockDataCollection.remove, 'doge1');
sinon.assert.calledOnce(mockEventHandler);
sinon.assert.calledWith(mockEventHandler, {
registry: registry,
resourceID: 'doge1'
});
});
});

it('should remove the resource by ID from the registry via cache retrieval if present', () => {
mockDataCollection.remove.resolves();
registry.resourceMap.set('doge1', mockResource);
let mockEventHandler = sinon.stub();
registry.on('resourceremoved', mockEventHandler);
return registry.remove('doge1')
.then(() => {
sinon.assert.notCalled(mockDataCollection.get);
sinon.assert.calledWith(mockDataCollection.remove, 'doge1');
sinon.assert.calledOnce(mockEventHandler);
sinon.assert.calledWith(mockEventHandler, {
Expand All @@ -744,6 +843,15 @@ describe('Registry', () => {
});
});

it('should remove the resource from the cache if present', async () => {
mockDataCollection.remove.resolves();
registry.resourceMap.set('doge1', mockResource);
let mockEventHandler = sinon.stub();
registry.on('resourceremoved', mockEventHandler);
await registry.remove('doge1');
registry.resourceMap.has('doge1').should.be.false;
});

it('should throw if the access controller throws an exception', () => {
mockAccessController.check.withArgs(mockResource, 'DELETE').rejects(new AccessException(mockResource, 'DELETE', mockParticipant));
mockDataCollection.remove.resolves();
Expand Down

0 comments on commit 2bbfa75

Please sign in to comment.