From 17faf000b0786590bb0f92742e18715dc456a0dd Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Mon, 21 Mar 2016 10:18:06 -0700 Subject: [PATCH 01/11] [Add] Cache models on instantiation ...to remove need for Edit to persist these immediately, which in turn causes #770 and #678 --- platform/core/bundle.js | 12 +++- .../src/capabilities/PersistenceCapability.js | 6 ++ .../core/src/models/CachingModelDecorator.js | 18 +++--- platform/core/src/models/ModelCacheService.js | 55 +++++++++++++++++++ platform/core/src/services/Instantiate.js | 7 ++- 5 files changed, 86 insertions(+), 12 deletions(-) create mode 100644 platform/core/src/models/ModelCacheService.js diff --git a/platform/core/bundle.js b/platform/core/bundle.js index 3c56e7e51b..abb0ce9fab 100644 --- a/platform/core/bundle.js +++ b/platform/core/bundle.js @@ -27,6 +27,7 @@ define([ "./src/models/StaticModelProvider", "./src/models/RootModelProvider", "./src/models/ModelAggregator", + "./src/models/ModelCacheService", "./src/models/PersistedModelProvider", "./src/models/CachingModelDecorator", "./src/models/MissingModelDecorator", @@ -58,6 +59,7 @@ define([ StaticModelProvider, RootModelProvider, ModelAggregator, + ModelCacheService, PersistedModelProvider, CachingModelDecorator, MissingModelDecorator, @@ -182,7 +184,10 @@ define([ { "provides": "modelService", "type": "decorator", - "implementation": CachingModelDecorator + "implementation": CachingModelDecorator, + "depends": [ + "cacheService" + ] }, { "provides": "modelService", @@ -319,6 +324,7 @@ define([ "key": "persistence", "implementation": PersistenceCapability, "depends": [ + "cacheService", "persistenceService", "identifierService", "notificationService", @@ -354,6 +360,10 @@ define([ } ], "services": [ + { + "key": "cacheService", + "implementation": ModelCacheService + }, { "key": "now", "implementation": Now diff --git a/platform/core/src/capabilities/PersistenceCapability.js b/platform/core/src/capabilities/PersistenceCapability.js index 9c7d769d22..91c5147bfe 100644 --- a/platform/core/src/capabilities/PersistenceCapability.js +++ b/platform/core/src/capabilities/PersistenceCapability.js @@ -46,6 +46,7 @@ define( * @implements {Capability} */ function PersistenceCapability( + cacheService, persistenceService, identifierService, notificationService, @@ -56,6 +57,7 @@ define( this.modified = domainObject.getModel().modified; this.domainObject = domainObject; + this.cacheService = cacheService; this.identifierService = identifierService; this.persistenceService = persistenceService; this.notificationService = notificationService; @@ -130,6 +132,7 @@ define( domainObject = this.domainObject, model = domainObject.getModel(), modified = model.modified, + cacheService = this.cacheService, persistenceService = this.persistenceService, persistenceFn = model.persisted !== undefined ? this.persistenceService.updateObject : @@ -146,6 +149,9 @@ define( getKey(domainObject.getId()), domainObject.getModel() ]).then(function(result){ + if (result) { + cacheService.remove(domainObject.getId()); + } return rejectIfFalsey(result, self.$q); }).catch(function(error){ return notifyOnError(error, domainObject, self.notificationService, self.$q); diff --git a/platform/core/src/models/CachingModelDecorator.js b/platform/core/src/models/CachingModelDecorator.js index a338d6770f..ffa377cf0a 100644 --- a/platform/core/src/models/CachingModelDecorator.js +++ b/platform/core/src/models/CachingModelDecorator.js @@ -35,9 +35,8 @@ define( * @param {ModelService} modelService this service to decorate * @implements {ModelService} */ - function CachingModelDecorator(modelService) { - this.cache = {}; - this.cached = {}; + function CachingModelDecorator(cacheService, modelService) { + this.cacheService = cacheService; this.modelService = modelService; } @@ -51,10 +50,9 @@ define( } CachingModelDecorator.prototype.getModels = function (ids) { - var cache = this.cache, - cached = this.cached, + var cacheService = this.cacheService, neededIds = ids.filter(function notCached(id) { - return !cached[id]; + return !cacheService.has(id); }); // Update the cached instance of a model to a new value. @@ -71,7 +69,7 @@ define( // If we'd previously cached an undefined value, or are now // seeing undefined, replace the item in the cache entirely. if (oldModel === undefined || model === undefined) { - cache[id] = model; + cacheService.put(id, model); return model; } @@ -91,15 +89,15 @@ define( // Store the provided models in our cache function cacheAll(models) { Object.keys(models).forEach(function (id) { - cache[id] = cached[id] ? + var model = cacheService.has(id) ? updateModel(id, models[id]) : models[id]; - cached[id] = true; + cacheService.put(id, model); }); } // Expose the cache (for promise chaining) function giveCache() { - return cache; + return cacheService.all(); } // Look up if we have unknown IDs diff --git a/platform/core/src/models/ModelCacheService.js b/platform/core/src/models/ModelCacheService.js new file mode 100644 index 0000000000..ee6edd522c --- /dev/null +++ b/platform/core/src/models/ModelCacheService.js @@ -0,0 +1,55 @@ +/***************************************************************************** + * Open MCT Web, Copyright (c) 2014-2015, United States Government + * as represented by the Administrator of the National Aeronautics and Space + * Administration. All rights reserved. + * + * Open MCT Web is licensed under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0. + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + * Open MCT Web includes source code licensed under additional open source + * licenses. See the Open Source Licenses file (LICENSES.md) included with + * this source code distribution or the Licensing information page available + * at runtime from the About dialog for additional information. + *****************************************************************************/ +/*global define*/ + +define([], function () { + 'use strict'; + + function ModelCacheService() { + this.cache = {}; + this.cached = {}; + } + + ModelCacheService.prototype.put = function (id, model) { + this.cached[id] = true; + this.cache[id] = model; + }; + + ModelCacheService.prototype.get = function (id) { + return this.cache[id]; + }; + + ModelCacheService.prototype.has = function (id) { + return this.cached[id]; + }; + + ModelCacheService.prototype.remove = function (id) { + delete this.cached[id]; + delete this.cache[id]; + }; + + ModelCacheService.prototype.all = function () { + return this.cache; + }; + + return ModelCacheService; +}); diff --git a/platform/core/src/services/Instantiate.js b/platform/core/src/services/Instantiate.js index c184e08f84..2b6032e80e 100644 --- a/platform/core/src/services/Instantiate.js +++ b/platform/core/src/services/Instantiate.js @@ -44,10 +44,15 @@ define( * @param {IdentifierService} identifierService service to generate * new identifiers */ - function Instantiate(capabilityService, identifierService) { + function Instantiate( + capabilityService, + identifierService, + cacheService + ) { return function (model, id) { var capabilities = capabilityService.getCapabilities(model); id = id || identifierService.generate(); + cacheService.put(id, model); return new DomainObjectImpl(id, model, capabilities); }; } From 8fa030437e38d81b9f4fb3397f39d00cabb1114e Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Mon, 21 Mar 2016 10:22:25 -0700 Subject: [PATCH 02/11] [Add] Remove edit awareness Remove step where Added objects are persisted via the editor capability; instead, persist via the usual persistence capability, such that Edit mode may intervene (or not) as necessary. As instantiated models are cached at least until persisted, this workaround to allow newly-created models to be available during editing is no longer necessary (and undesired consequences such as #770 no longer occur) --- platform/commonUI/browse/src/creation/AddAction.js | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/platform/commonUI/browse/src/creation/AddAction.js b/platform/commonUI/browse/src/creation/AddAction.js index 3832280130..357ad1f3d7 100644 --- a/platform/commonUI/browse/src/creation/AddAction.js +++ b/platform/commonUI/browse/src/creation/AddAction.js @@ -100,20 +100,14 @@ define( }); } - function save(object) { - /* - It's necessary to persist the new sub-object in order - that it can be retrieved for composition in the parent. - Future refactoring that allows temporary objects to be - retrieved from object services will make this unnecessary. - */ - return object.getCapability('editor').save(true); + function persistNewObject(object) { + return object.getCapability('persistence').persist(); } return this.dialogService .getUserInput(wizard.getFormStructure(false), wizard.getInitialFormValue()) .then(populateObjectFromInput) - .then(save) + .then(persistNewObject) .then(addToParent); }; From 3fe41575bd03c94e3600cb1fef6d88f5794c8809 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Mon, 21 Mar 2016 10:31:09 -0700 Subject: [PATCH 03/11] [Add] Add missing dependency ...to support caching of domain objects created during edit mode. --- platform/core/bundle.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/platform/core/bundle.js b/platform/core/bundle.js index abb0ce9fab..9ad64d47f9 100644 --- a/platform/core/bundle.js +++ b/platform/core/bundle.js @@ -394,7 +394,8 @@ define([ "implementation": Instantiate, "depends": [ "capabilityService", - "identifierService" + "identifierService", + "cacheService" ] } ], From 9c9db3c24f816af126e8d8bdcfd88fe278384d0d Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Mon, 21 Mar 2016 10:31:38 -0700 Subject: [PATCH 04/11] [Add] Remove obsolete variable reference The cache has been externalized to allow writing to it upon domain object instantiation. --- platform/core/src/models/CachingModelDecorator.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/platform/core/src/models/CachingModelDecorator.js b/platform/core/src/models/CachingModelDecorator.js index ffa377cf0a..1ceae1bf88 100644 --- a/platform/core/src/models/CachingModelDecorator.js +++ b/platform/core/src/models/CachingModelDecorator.js @@ -59,7 +59,7 @@ define( // We update in-place to ensure there is only ever one instance // of any given model exposed by the modelService as a whole. function updateModel(id, model) { - var oldModel = cache[id]; + var oldModel = cacheService.get(id); // Same object instance is a possibility, so don't copy if (oldModel === model) { @@ -108,7 +108,7 @@ define( } // Otherwise, just expose the cache directly - return fastPromise(cache); + return fastPromise(cacheService.all()); }; return CachingModelDecorator; From d12111d9b8b6c651f32fe4f8917a094ed650c166 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Mon, 21 Mar 2016 10:32:02 -0700 Subject: [PATCH 05/11] [Add] Fix promise chaining in AddAction --- .../commonUI/browse/src/creation/AddAction.js | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/platform/commonUI/browse/src/creation/AddAction.js b/platform/commonUI/browse/src/creation/AddAction.js index 357ad1f3d7..4685e80b75 100644 --- a/platform/commonUI/browse/src/creation/AddAction.js +++ b/platform/commonUI/browse/src/creation/AddAction.js @@ -93,21 +93,23 @@ define( return wizard.populateObjectFromInput(formValue, newObject); } - function addToParent (populatedObject) { - parentObject.getCapability('composition').add(populatedObject); - return parentObject.getCapability('persistence').persist().then(function(){ - return parentObject; - }); + function persistAndReturn(domainObject) { + return domainObject.getCapability('persistence') + .persist() + .then(function () { + return domainObject; + }); } - function persistNewObject(object) { - return object.getCapability('persistence').persist(); + function addToParent (populatedObject) { + parentObject.getCapability('composition').add(populatedObject); + return persistAndReturn(parentObject); } return this.dialogService .getUserInput(wizard.getFormStructure(false), wizard.getInitialFormValue()) .then(populateObjectFromInput) - .then(persistNewObject) + .then(persistAndReturn) .then(addToParent); }; From 9f29382e188c39c074f993002bc6adcc45bbb962 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Mon, 21 Mar 2016 15:04:03 -0700 Subject: [PATCH 06/11] [Add] Update spec for Instantiate ...to reflect usage of a model cache for #770 --- platform/core/test/services/InstantiateSpec.js | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/platform/core/test/services/InstantiateSpec.js b/platform/core/test/services/InstantiateSpec.js index cb25feaac2..fcad45813a 100644 --- a/platform/core/test/services/InstantiateSpec.js +++ b/platform/core/test/services/InstantiateSpec.js @@ -32,8 +32,7 @@ define( mockIdentifierService, mockCapabilityConstructor, mockCapabilityInstance, - mockCapabilities, - mockIdentifier, + mockCacheService, idCounter, testModel, instantiate, @@ -62,11 +61,17 @@ define( "some-id-" + (idCounter += 1); }); + mockCacheService = jasmine.createSpyObj( + 'cacheService', + [ 'get', 'put', 'remove', 'all' ] + ); + testModel = { someKey: "some value" }; instantiate = new Instantiate( mockCapabilityService, - mockIdentifierService + mockIdentifierService, + mockCacheService ); domainObject = instantiate(testModel); }); @@ -92,6 +97,13 @@ define( expect(instantiate(testModel).getId()) .not.toEqual(domainObject.getId()); }); + + it("caches the instantiated model", function () { + expect(mockCacheService.put).toHaveBeenCalledWith( + domainObject.getId(), + testModel + ); + }); }); } From 1e4ff5a73f25952856a8772eeab54f4cb07b8b6d Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Mon, 21 Mar 2016 15:05:28 -0700 Subject: [PATCH 07/11] [Add] Use cacheService from decorator spec --- .../core/test/models/CachingModelDecoratorSpec.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/platform/core/test/models/CachingModelDecoratorSpec.js b/platform/core/test/models/CachingModelDecoratorSpec.js index acbdc2bd01..d6103dabd9 100644 --- a/platform/core/test/models/CachingModelDecoratorSpec.js +++ b/platform/core/test/models/CachingModelDecoratorSpec.js @@ -22,8 +22,11 @@ /*global define,Promise,describe,it,expect,beforeEach,waitsFor,jasmine*/ define( - ["../../src/models/CachingModelDecorator"], - function (CachingModelDecorator) { + [ + "../../src/models/CachingModelDecorator", + "../../src/models/ModelCacheService" + ], + function (CachingModelDecorator, ModelCacheService) { "use strict"; describe("The caching model decorator", function () { @@ -67,7 +70,10 @@ define( b: { someOtherKey: "some other value" } }; mockModelService.getModels.andReturn(asPromise(testModels)); - decorator = new CachingModelDecorator(mockModelService); + decorator = new CachingModelDecorator( + new ModelCacheService(), + mockModelService + ); }); it("loads models from its wrapped model service", function () { @@ -150,4 +156,4 @@ define( }); } -); \ No newline at end of file +); From baccd005dce30f8153cdcf2496d0afa1898a8461 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Mon, 21 Mar 2016 15:08:27 -0700 Subject: [PATCH 08/11] [Add] Update persistence capability spec ...to reflect removal of cached domain object models. --- .../test/capabilities/PersistenceCapabilitySpec.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/platform/core/test/capabilities/PersistenceCapabilitySpec.js b/platform/core/test/capabilities/PersistenceCapabilitySpec.js index 5b46ca3890..d9b93c9e12 100644 --- a/platform/core/test/capabilities/PersistenceCapabilitySpec.js +++ b/platform/core/test/capabilities/PersistenceCapabilitySpec.js @@ -36,6 +36,7 @@ define( mockDomainObject, mockIdentifier, mockNofificationService, + mockCacheService, mockQ, id = "object id", model, @@ -81,6 +82,10 @@ define( "notificationService", ["error"] ); + mockCacheService = jasmine.createSpyObj( + "cacheService", + [ "get", "put", "remove", "all" ] + ); mockDomainObject = { getId: function () { return id; }, @@ -96,6 +101,7 @@ define( mockIdentifierService.parse.andReturn(mockIdentifier); mockIdentifier.getSpace.andReturn(SPACE); persistence = new PersistenceCapability( + mockCacheService, mockPersistenceService, mockIdentifierService, mockNofificationService, @@ -170,6 +176,11 @@ define( expect(mockQ.reject).not.toHaveBeenCalled(); expect(mockNofificationService.error).not.toHaveBeenCalled(); }); + + it("removes the model from the cache", function () { + persistence.persist(); + expect(mockCacheService.remove).toHaveBeenCalledWith(id); + }); }); describe("unsuccessful persistence", function() { var sadPromise = { From 6c2a28aba296dc41bc168089e0f5a9d423b06620 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Mon, 21 Mar 2016 16:05:05 -0700 Subject: [PATCH 09/11] [Add] Add JSDoc to model cache --- platform/core/src/models/ModelCacheService.js | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/platform/core/src/models/ModelCacheService.js b/platform/core/src/models/ModelCacheService.js index ee6edd522c..67a143935f 100644 --- a/platform/core/src/models/ModelCacheService.js +++ b/platform/core/src/models/ModelCacheService.js @@ -24,29 +24,60 @@ define([], function () { 'use strict'; + /** + * Provides a cache for domain object models which exist in memory, + * but may or may not exist in backing persistene stores. + * @constructor + * @memberof platform/core + */ function ModelCacheService() { this.cache = {}; this.cached = {}; } + /** + * Put a domain object model in the cache. + * @param {string} id the domain object's identifier + * @param {object} model the domain object's model + */ ModelCacheService.prototype.put = function (id, model) { this.cached[id] = true; this.cache[id] = model; }; + /** + * Retrieve a domain object model from the cache. + * @param {string} id the domain object's identifier + * @returns {object} the domain object's model + */ ModelCacheService.prototype.get = function (id) { return this.cache[id]; }; + /** + * Check if a domain object model is in the cache. + * @param {string} id the domain object's identifier + * @returns {boolean} true if present; false if not + */ ModelCacheService.prototype.has = function (id) { return this.cached[id]; }; + /** + * Remove a domain object model from the cache. + * @param {string} id the domain object's identifier + */ ModelCacheService.prototype.remove = function (id) { delete this.cached[id]; delete this.cache[id]; }; + /** + * Retrieve all cached domain object models. These are given + * as an object containing key-value pairs, where keys are + * domain object identifiers and values are domain object models. + * @returns {object} all domain object models + */ ModelCacheService.prototype.all = function () { return this.cache; }; From ec0cc572f64a6928f1947d1577c72ca6d58a3d96 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Mon, 21 Mar 2016 16:11:40 -0700 Subject: [PATCH 10/11] [Add] Test model cache --- platform/core/src/models/ModelCacheService.js | 2 +- .../core/test/models/ModelCacheServiceSpec.js | 69 +++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 platform/core/test/models/ModelCacheServiceSpec.js diff --git a/platform/core/src/models/ModelCacheService.js b/platform/core/src/models/ModelCacheService.js index 67a143935f..d6eb3b51b7 100644 --- a/platform/core/src/models/ModelCacheService.js +++ b/platform/core/src/models/ModelCacheService.js @@ -60,7 +60,7 @@ define([], function () { * @returns {boolean} true if present; false if not */ ModelCacheService.prototype.has = function (id) { - return this.cached[id]; + return !!this.cached[id]; }; /** diff --git a/platform/core/test/models/ModelCacheServiceSpec.js b/platform/core/test/models/ModelCacheServiceSpec.js new file mode 100644 index 0000000000..f8254779ab --- /dev/null +++ b/platform/core/test/models/ModelCacheServiceSpec.js @@ -0,0 +1,69 @@ +/***************************************************************************** + * Open MCT Web, Copyright (c) 2014-2015, United States Government + * as represented by the Administrator of the National Aeronautics and Space + * Administration. All rights reserved. + * + * Open MCT Web is licensed under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0. + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + * Open MCT Web includes source code licensed under additional open source + * licenses. See the Open Source Licenses file (LICENSES.md) included with + * this source code distribution or the Licensing information page available + * at runtime from the About dialog for additional information. + *****************************************************************************/ +/*global define,Promise,describe,it,expect,beforeEach,waitsFor,jasmine*/ + +define(['../../src/models/ModelCacheService'], function (ModelCacheService) { + 'use strict'; + describe("ModelCacheService", function () { + var testIds, + testModels, + cacheService; + + beforeEach(function () { + testIds = [ 'a', 'b', 'c', 'd' ]; + testModels = testIds.reduce(function (models, id) { + models[id] = { someKey: "some value for " + id }; + return models; + }, {}); + cacheService = new ModelCacheService(); + }); + + describe("when populated with models", function () { + beforeEach(function () { + testIds.forEach(function (id) { + cacheService.put(id, testModels[id]); + }); + }); + + it("indicates that it has these models", function () { + testIds.forEach(function (id) { + expect(cacheService.has(id)).toBe(true); + }); + }); + + it("provides all of these models", function () { + expect(cacheService.all()).toEqual(testModels); + }); + + it("allows models to be retrieved", function () { + testIds.forEach(function (id) { + expect(cacheService.get(id)).toEqual(testModels[id]); + }); + }); + + it("allows models to be removed", function () { + cacheService.remove('a'); + expect(cacheService.has('a')).toBe(false); + }); + }); + }); +}); From ddbb72b88a1de1d236b29c3a87dbf993ad588990 Mon Sep 17 00:00:00 2001 From: Pete Richards Date: Thu, 24 Mar 2016 12:57:53 -0700 Subject: [PATCH 11/11] [CacheService] Don't track ids twice Track whether an object is in the cache based on whether it is in the cache instead of utilizing a separate object for tracking contents of cache. See comment on https://github.com/nasa/openmct/pull/773/files --- platform/core/src/models/ModelCacheService.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/platform/core/src/models/ModelCacheService.js b/platform/core/src/models/ModelCacheService.js index d6eb3b51b7..4bcee0b93d 100644 --- a/platform/core/src/models/ModelCacheService.js +++ b/platform/core/src/models/ModelCacheService.js @@ -32,7 +32,6 @@ define([], function () { */ function ModelCacheService() { this.cache = {}; - this.cached = {}; } /** @@ -41,7 +40,6 @@ define([], function () { * @param {object} model the domain object's model */ ModelCacheService.prototype.put = function (id, model) { - this.cached[id] = true; this.cache[id] = model; }; @@ -60,7 +58,7 @@ define([], function () { * @returns {boolean} true if present; false if not */ ModelCacheService.prototype.has = function (id) { - return !!this.cached[id]; + return this.cache.hasOwnProperty(id); }; /** @@ -68,7 +66,6 @@ define([], function () { * @param {string} id the domain object's identifier */ ModelCacheService.prototype.remove = function (id) { - delete this.cached[id]; delete this.cache[id]; };