prevent passing cache contents by reference (#4184)

Signed-off-by: Nick Lincoln <[email protected]>

Author: nklincoln

packages/composer-runtime/lib/registry.js

@@ -48,7 +48,7 @@ class Registry extends EventEmitter {
         this.id = id;
         this.name = name;
         this.system = system;
-        this.resourceMap = new Map();
+        this.objectMap = new Map();
     }
 
     /**
@@ -80,7 +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);
+                this.objectMap.set(resource.getIdentifier(), object);
             } catch (error) {
                 // Ignore the error; we don't have access.
             }
@@ -95,15 +95,16 @@ class Registry extends EventEmitter {
      * object when complete, or rejected with an error.
      */
     async get(id) {
-        if (this.resourceMap.has(id)) {
-            return this.resourceMap.get(id);
+        if (this.objectMap.has(id)) {
+            let object = this.objectMap.get(id);
+            return this.serializer.fromJSON(object);
         } 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);
+                this.objectMap.set(id, object);
                 return resource;
             } catch (error) {
                 throw new Error(`Object with ID '${id}' in collection with ID '${this.type}:${this.id}' does not exist`);
@@ -123,15 +124,15 @@ class Registry extends EventEmitter {
             return false;
         }
 
-        if (this.resourceMap.has(id)) {
+        if (this.objectMap.has(id)) {
             return true;
         } 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);
+                this.objectMap.set(id, object);
                 return true;
             } catch (error) {
                 return false;
@@ -255,8 +256,9 @@ class Registry extends EventEmitter {
         object = this.addInternalProperties(object);
 
         let oldResource;
-        if (this.resourceMap.has(id)) {
-            oldResource = this.resourceMap.get(id);
+        if (this.objectMap.has(id)) {
+            let oldObject = this.objectMap.get(id);
+            oldResource = this.serializer.fromJSON(oldObject);
         } else {
             let oldObject = await this.dataCollection.get(id);
             oldResource = this.serializer.fromJSON(oldObject);
@@ -266,9 +268,9 @@ class Registry extends EventEmitter {
         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);
+        // If is within objectMap cache, update may have changed the ability to READ item, so remove it
+        if (this.objectMap.has(id)) {
+            this.objectMap.delete(id);
         }
 
         this.emit('resourceupdated', {
@@ -307,8 +309,9 @@ 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.
-            if (this.resourceMap.has(resource)) {
-                resource = this.resourceMap.get(resource);
+            if (this.objectMap.has(resource)) {
+                let object = this.objectMap.get(resource);
+                resource = this.serializer.fromJSON(object);
             } else {
                 let object = await this.dataCollection.get(resource);
                 object = Registry.removeInternalProperties(object);
@@ -320,8 +323,8 @@ class Registry extends EventEmitter {
         await this.dataCollection.remove(id);
 
         // Remove from cache if present
-        if (this.resourceMap.has(id)) {
-            this.resourceMap.delete(id);
+        if (this.objectMap.has(id)) {
+            this.objectMap.delete(id);
         }
 
         this.emit('resourceremoved', {

packages/composer-runtime/test/registry.js

@@ -107,7 +107,7 @@ describe('Registry', () => {
             }).returns(mockResource2);
         });
 
-        it('should get and parse all of the resources from the registry if not in the cache', () => {
+        it('should get and parse all of the resources from the registry if not in the object cache', () => {
             return registry.getAll()
                 .then((resources) => {
                     sinon.assert.calledTwice(mockAccessController.check);
@@ -118,12 +118,14 @@ describe('Registry', () => {
                 });
         });
 
-        it('should add resources to the cache during the retrieval', async () => {
+        it('should add objects to the object cache during the retrieval', async () => {
+            let map = registry.objectMap;
+            map.size.should.be.equal(0);
             await registry.getAll();
-            let map = registry.resourceMap;
-            let keys = map.keys();
-            for (let key of keys){
-                map.get(key).should.be.an.instanceOf(Resource);
+            map.size.should.be.equal(2);
+
+            for (let key of map.keys()){
+                map.get(key).should.be.an.instanceOf(Object);
             }
         });
 
@@ -167,17 +169,24 @@ describe('Registry', () => {
     describe('#get', () => {
 
         let mockResource;
+        let mockResource2;
 
         beforeEach(() => {
             mockDataCollection.get.withArgs('doge1').resolves({
                 $class: 'org.doge.Doge',
                 assetId: 'doge1'
             });
             mockResource = sinon.createStubInstance(Resource);
+            mockResource2 = sinon.createStubInstance(Resource);
+            mockResource2.getIdentifier.returns('cached');
             mockSerializer.fromJSON.withArgs({
                 $class: 'org.doge.Doge',
                 assetId: 'doge1'
             }).returns(mockResource);
+            mockSerializer.fromJSON.withArgs({
+                $class: 'org.doge.Doge',
+                assetId: 'doge2'
+            }).returns(mockResource2);
         });
 
         it('should get the specific resource from the registry if not in the cache', () => {
@@ -190,21 +199,23 @@ describe('Registry', () => {
                 });
         });
 
-        it('should add to the cache after registry retrieval', async () => {
+        it('should add to the object cache after registry retrieval', async () => {
             await registry.get('doge1');
-            let resource = registry.resourceMap.get('doge1');
-            resource.should.be.an.instanceOf(Resource);
+            let obj = registry.objectMap.get('doge1');
+            obj.should.be.an.instanceOf(Object);
         });
 
         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);
+            registry.objectMap.set('doge2', {
+                $class: 'org.doge.Doge',
+                assetId: 'doge2'
+            });
 
-            let resource = await registry.get('doge1');
+            let resource = await registry.get('doge2');
             resource.should.be.an.instanceOf(Resource);
-            resource.getIdentifier().should.equal('doge1');
+            resource.getIdentifier().should.equal('cached');
+
+            // Should not go the datacollection route
             sinon.assert.notCalled(mockDataCollection.get);
             sinon.assert.notCalled(mockAccessController.check);
         });
@@ -253,15 +264,19 @@ describe('Registry', () => {
             return registry.exists('doge1')
                 .should.eventually.be.true
                 .then((exists) => {
-                    registry.resourceMap.get('doge1').should.be.an.instanceOf(Resource);
+                    registry.objectMap.size.should.be.equal(1);
+                    registry.objectMap.get('doge1').should.deep.equal({
+                        $class: 'org.doge.Doge',
+                        assetId: 'doge1'
+                    });
                 });
         });
 
         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);
+            registry.objectMap.set('doge1', mockResource);
 
             return registry.exists('doge1')
                 .should.eventually.be.true
@@ -590,6 +605,7 @@ describe('Registry', () => {
 
         let mockResource, mockOldResource;
         let mockClassDeclaration, mockOldClassDeclaration;
+        let mockObject, mockOldObject;
 
         beforeEach(() => {
             // New resources.
@@ -600,11 +616,13 @@ describe('Registry', () => {
             mockClassDeclaration.getSystemType.returns('Asset');
             mockResource.getIdentifier.returns('doge1');
             mockResource.theValue = 'newValue';
-            mockSerializer.toJSON.withArgs(mockResource).onFirstCall().returns({
+            mockObject = {
                 $class: 'org.doge.Doge',
                 assetId: 'doge1',
                 newValue: 'newValue'
-            });
+            };
+            mockSerializer.toJSON.withArgs(mockResource).onFirstCall().returns(mockObject);
+            mockSerializer.fromJSON.withArgs(mockObject).returns(mockResource);
             // Old resources.
             mockOldResource = sinon.createStubInstance(Resource);
             mockOldClassDeclaration = sinon.createStubInstance(ClassDeclaration);
@@ -613,16 +631,13 @@ describe('Registry', () => {
             mockOldClassDeclaration.getSystemType.returns('Asset');
             mockOldResource.getIdentifier.returns('doge1');
             mockOldResource.theValue = 'oldValue';
-            mockDataCollection.get.withArgs('doge1').resolves({
-                $class: 'org.doge.Doge',
-                assetId: 'doge1',
-                newValue: 'oldValue'
-            });
-            mockSerializer.fromJSON.withArgs({
+            mockOldObject = {
                 $class: 'org.doge.Doge',
                 assetId: 'doge1',
                 newValue: 'oldValue'
-            }).returns(mockOldResource);
+            };
+            mockDataCollection.get.withArgs('doge1').resolves(mockOldObject);
+            mockSerializer.fromJSON.withArgs(mockOldObject).returns(mockOldResource);
         });
 
         it('should update the resource in the registry via datacollection retrieval if not in the cache', () => {
@@ -652,7 +667,7 @@ describe('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.objectMap.set('doge1', mockObject);
             registry.on('resourceupdated', mockEventHandler);
             return registry.update(mockResource)
                 .then(() => {
@@ -673,10 +688,10 @@ describe('Registry', () => {
         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.objectMap.set('doge1', mockResource);
             registry.on('resourceupdated', mockEventHandler);
             await registry.update(mockResource);
-            registry.resourceMap.has('doge1').should.be.false;
+            registry.objectMap.has('doge1').should.be.false;
         });
 
         it('should throw an error updating a resource to the wrong registry', () => {
@@ -776,21 +791,20 @@ describe('Registry', () => {
     describe('#remove', () => {
 
         let mockResource;
+        let mockObject;
 
         beforeEach(() => {
             // Old resources.
             // Deleting a resource by ID currently requires that we read it from the registry.
-            mockDataCollection.get.withArgs('doge1').resolves({
+            mockObject = {
                 $class: 'org.doge.Doge',
                 assetId: 'doge1'
-            });
+            };
+            mockDataCollection.get.withArgs('doge1').resolves(mockObject);
             mockResource = sinon.createStubInstance(Resource);
             mockResource.theValue = 'the value 1';
             mockResource.getIdentifier.returns('doge1');
-            mockSerializer.fromJSON.withArgs({
-                $class: 'org.doge.Doge',
-                assetId: 'doge1'
-            }).returns(mockResource);
+            mockSerializer.fromJSON.withArgs(mockObject).returns(mockResource);
             // End of resources.
         });
 
@@ -828,7 +842,7 @@ describe('Registry', () => {
 
         it('should remove the resource by ID from the registry via cache retrieval if present', () => {
             mockDataCollection.remove.resolves();
-            registry.resourceMap.set('doge1', mockResource);
+            registry.objectMap.set('doge1', mockObject);
             let mockEventHandler = sinon.stub();
             registry.on('resourceremoved', mockEventHandler);
             return registry.remove('doge1')
@@ -845,11 +859,11 @@ describe('Registry', () => {
 
         it('should remove the resource from the cache if present', async () => {
             mockDataCollection.remove.resolves();
-            registry.resourceMap.set('doge1', mockResource);
+            registry.objectMap.set('doge1', mockObject);
             let mockEventHandler = sinon.stub();
             registry.on('resourceremoved', mockEventHandler);
             await registry.remove('doge1');
-            registry.resourceMap.has('doge1').should.be.false;
+            registry.objectMap.has('doge1').should.be.false;
         });
 
         it('should throw if the access controller throws an exception', () => {