Enable persistence operations from Object Providers (#3200)

* Implement 'save' method in Object API

* Refactor legacy persistence code to work with new save object API

* Added 'isPersistable' check to object API

* Fixed incompatibility between object API changes and composition policies

* Make save method private

Co-authored-by: Deep Tailor <deep.j.tailor@nasa.gov>

platform/commonUI/edit/src/policies/EditPersistableObjectsPolicy.js

@@ -36,8 +36,6 @@ define(
         }
 
         EditPersistableObjectsPolicy.prototype.allow = function (action, context) {
-            var identifier;
-            var provider;
             var domainObject = context.domainObject;
             var key = action.getMetadata().key;
             var category = (context || {}).category;
@@ -46,9 +44,8 @@ define(
             // is also invoked during the create process which should be allowed,
             // because it may be saved elsewhere
             if ((key === 'edit' && category === 'view-control') || key === 'properties') {
-                identifier = objectUtils.parseKeyString(domainObject.getId());
+                let newStyleObject = objectUtils.toNewFormat(domainObject, domainObject.getId());
-                provider = this.openmct.objects.getProvider(identifier);
+                return this.openmct.objects.isPersistable(newStyleObject);
-                return provider.save !== undefined;
             }
 
             return true;

platform/commonUI/edit/test/policies/EditPersistableObjectsSpec.js

@@ -43,7 +43,7 @@ define(
                 );
 
                 mockObjectAPI = jasmine.createSpyObj('objectAPI', [
-                    'getProvider'
+                    'isPersistable'
                 ]);
 
                 mockAPI = {
@@ -69,34 +69,31 @@ define(
             });
 
             it("Applies to edit action", function () {
-                mockObjectAPI.getProvider.and.returnValue({});
+                expect(mockObjectAPI.isPersistable).not.toHaveBeenCalled();
-                expect(mockObjectAPI.getProvider).not.toHaveBeenCalled();
 
                 policy.allow(mockEditAction, testContext);
-                expect(mockObjectAPI.getProvider).toHaveBeenCalled();
+                expect(mockObjectAPI.isPersistable).toHaveBeenCalled();
             });
 
             it("Applies to properties action", function () {
-                mockObjectAPI.getProvider.and.returnValue({});
+                expect(mockObjectAPI.isPersistable).not.toHaveBeenCalled();
-                expect(mockObjectAPI.getProvider).not.toHaveBeenCalled();
 
                 policy.allow(mockPropertiesAction, testContext);
-                expect(mockObjectAPI.getProvider).toHaveBeenCalled();
+                expect(mockObjectAPI.isPersistable).toHaveBeenCalled();
             });
 
             it("does not apply to other actions", function () {
-                mockObjectAPI.getProvider.and.returnValue({});
+                expect(mockObjectAPI.isPersistable).not.toHaveBeenCalled();
-                expect(mockObjectAPI.getProvider).not.toHaveBeenCalled();
 
                 policy.allow(mockOtherAction, testContext);
-                expect(mockObjectAPI.getProvider).not.toHaveBeenCalled();
+                expect(mockObjectAPI.isPersistable).not.toHaveBeenCalled();
             });
 
             it("Tests object provider for editability", function () {
-                mockObjectAPI.getProvider.and.returnValue({});
+                mockObjectAPI.isPersistable.and.returnValue(false);
                 expect(policy.allow(mockEditAction, testContext)).toBe(false);
-                expect(mockObjectAPI.getProvider).toHaveBeenCalled();
+                expect(mockObjectAPI.isPersistable).toHaveBeenCalled();
-                mockObjectAPI.getProvider.and.returnValue({save: function () {}});
+                mockObjectAPI.isPersistable.and.returnValue(true);
                 expect(policy.allow(mockEditAction, testContext)).toBe(true);
             });
         });

platform/containment/src/PersistableCompositionPolicy.js

@@ -48,9 +48,8 @@ define(
             // prevents editing of objects that cannot be persisted, so we can assume that this
             // is a new object.
             if (!(parent.hasCapability('editor') && parent.getCapability('editor').isEditContextRoot())) {
-                var identifier = objectUtils.parseKeyString(parent.getId());
+                let newStyleObject = objectUtils.toNewFormat(parent, parent.getId());
-                var provider = this.openmct.objects.getProvider(identifier);
+                return this.openmct.objects.isPersistable(newStyleObject);
-                return provider.save !== undefined;
             }
             return true;
         };

platform/containment/test/PersistableCompositionPolicySpec.js

@@ -33,7 +33,7 @@ define(
 
             beforeEach(function () {
                 objectAPI = jasmine.createSpyObj('objectsAPI', [
-                    'getProvider'
+                    'isPersistable'
                 ]);
 
                 mockOpenMCT = {
@@ -51,10 +51,6 @@ define(
                     'isEditContextRoot'
                 ]);
                 mockParent.getCapability.and.returnValue(mockEditorCapability);
-
-                objectAPI.getProvider.and.returnValue({
-                    save: function () {}
-                });
                 persistableCompositionPolicy = new PersistableCompositionPolicy(mockOpenMCT);
             });
 
@@ -65,19 +61,22 @@ define(
 
             it("Does not allow composition for objects that are not persistable", function () {
                 mockEditorCapability.isEditContextRoot.and.returnValue(false);
+                objectAPI.isPersistable.and.returnValue(true);
                 expect(persistableCompositionPolicy.allow(mockParent, mockChild)).toBe(true);
-                objectAPI.getProvider.and.returnValue({});
+                objectAPI.isPersistable.and.returnValue(false);
                 expect(persistableCompositionPolicy.allow(mockParent, mockChild)).toBe(false);
             });
 
             it("Always allows composition of objects in edit mode to support object creation", function () {
                 mockEditorCapability.isEditContextRoot.and.returnValue(true);
+                objectAPI.isPersistable.and.returnValue(true);
                 expect(persistableCompositionPolicy.allow(mockParent, mockChild)).toBe(true);
-                expect(objectAPI.getProvider).not.toHaveBeenCalled();
+                expect(objectAPI.isPersistable).not.toHaveBeenCalled();
 
                 mockEditorCapability.isEditContextRoot.and.returnValue(false);
+                objectAPI.isPersistable.and.returnValue(true);
                 expect(persistableCompositionPolicy.allow(mockParent, mockChild)).toBe(true);
-                expect(objectAPI.getProvider).toHaveBeenCalled();
+                expect(objectAPI.isPersistable).toHaveBeenCalled();
             });
 
         });

platform/core/bundle.js

@@ -297,7 +297,8 @@ define([
                             "persistenceService",
                             "identifierService",
                             "notificationService",
-                            "$q"
+                            "$q",
+                            "openmct"
                         ]
                     },
                     {

platform/core/src/capabilities/PersistenceCapability.js

@@ -20,8 +20,8 @@
  * at runtime from the About dialog for additional information.
  *****************************************************************************/
 
-define(
+define(["objectUtils"],
-    function () {
+    function (objectUtils) {
 
         /**
          * Defines the `persistence` capability, used to trigger the
@@ -47,6 +47,7 @@ define(
             identifierService,
             notificationService,
             $q,
+            openmct,
             domainObject
         ) {
             // Cache modified timestamp
@@ -58,6 +59,7 @@ define(
             this.persistenceService = persistenceService;
             this.notificationService = notificationService;
             this.$q = $q;
+            this.openmct = openmct;
         }
 
         /**
@@ -66,7 +68,7 @@ define(
          */
         function rejectIfFalsey(value, $q) {
             if (!value) {
-                return $q.reject("Error persisting object");
+                return Promise.reject("Error persisting object");
             } else {
                 return value;
             }
@@ -98,7 +100,7 @@ define(
                 dismissable: true
             });
 
-            return $q.reject(error);
+            return Promise.reject(error);
         }
 
         /**
@@ -110,30 +112,12 @@ define(
          */
         PersistenceCapability.prototype.persist = function () {
             var self = this,
-                domainObject = this.domainObject,
+                domainObject = this.domainObject;
-                model = domainObject.getModel(),
-                modified = model.modified,
-                persisted = model.persisted,
-                persistenceService = this.persistenceService,
-                persistenceFn = persisted !== undefined ?
-                    this.persistenceService.updateObject :
-                    this.persistenceService.createObject;
 
-            if (persisted !== undefined && persisted === modified) {
+            let newStyleObject = objectUtils.toNewFormat(domainObject.getModel(), domainObject.getId());
-                return this.$q.when(true);
+            return this.openmct.objects
-            }
+                .save(newStyleObject)
-
+                .then(function (result) {
-            // Update persistence timestamp...
-            domainObject.useCapability("mutation", function (m) {
-                m.persisted = modified;
-            }, modified);
-
-            // ...and persist
-            return persistenceFn.apply(persistenceService, [
-                this.getSpace(),
-                this.getKey(),
-                domainObject.getModel()
-            ]).then(function (result) {
                     return rejectIfFalsey(result, self.$q);
                 }).catch(function (error) {
                     return notifyOnError(error, domainObject, self.notificationService, self.$q);

platform/core/test/capabilities/PersistenceCapabilitySpec.js

@@ -19,7 +19,6 @@
  * this source code distribution or the Licensing information page available
  * at runtime from the About dialog for additional information.
  *****************************************************************************/
-
 /**
  * PersistenceCapabilitySpec. Created by vwoeltje on 11/6/14.
  */
@@ -40,7 +39,8 @@ define(
                 model,
                 SPACE = "some space",
                 persistence,
-                happyPromise;
+                mockOpenMCT,
+                mockNewStyleDomainObject;
 
             function asPromise(value, doCatch) {
                 return (value || {}).then ? value : {
@@ -56,7 +56,6 @@ define(
             }
 
             beforeEach(function () {
-                happyPromise = asPromise(true);
                 model = { someKey: "some value", name: "domain object"};
 
                 mockPersistenceService = jasmine.createSpyObj(
@@ -94,12 +93,23 @@ define(
                     },
                     useCapability: jasmine.createSpy()
                 };
+
+                mockNewStyleDomainObject = Object.assign({}, model);
+                mockNewStyleDomainObject.identifier = {
+                    namespace: "",
+                    key: id
+                }
+
                 // Simulate mutation capability
                 mockDomainObject.useCapability.and.callFake(function (capability, mutator) {
                     if (capability === 'mutation') {
                         model = mutator(model) || model;
                     }
                 });
+
+                mockOpenMCT = {};
+                mockOpenMCT.objects = jasmine.createSpyObj('Object API', ['save']);
+
                 mockIdentifierService.parse.and.returnValue(mockIdentifier);
                 mockIdentifier.getSpace.and.returnValue(SPACE);
                 mockIdentifier.getKey.and.returnValue(key);
@@ -110,51 +120,28 @@ define(
                     mockIdentifierService,
                     mockNofificationService,
                     mockQ,
+                    mockOpenMCT,
                     mockDomainObject
                 );
             });
 
             describe("successful persistence", function () {
                 beforeEach(function () {
-                    mockPersistenceService.updateObject.and.returnValue(happyPromise);
+                    mockOpenMCT.objects.save.and.returnValue(Promise.resolve(true));
-                    mockPersistenceService.createObject.and.returnValue(happyPromise);
                 });
                 it("creates unpersisted objects with the persistence service", function () {
                     // Verify precondition; no call made during constructor
-                    expect(mockPersistenceService.createObject).not.toHaveBeenCalled();
+                    expect(mockOpenMCT.objects.save).not.toHaveBeenCalled();
 
                     persistence.persist();
 
-                    expect(mockPersistenceService.createObject).toHaveBeenCalledWith(
+                    expect(mockOpenMCT.objects.save).toHaveBeenCalledWith(mockNewStyleDomainObject);
-                        SPACE,
-                        key,
-                        model
-                    );
-                });
-
-                it("updates previously persisted objects with the persistence service", function () {
-                    // Verify precondition; no call made during constructor
-                    expect(mockPersistenceService.updateObject).not.toHaveBeenCalled();
-
-                    model.persisted = 12321;
-                    persistence.persist();
-
-                    expect(mockPersistenceService.updateObject).toHaveBeenCalledWith(
-                        SPACE,
-                        key,
-                        model
-                    );
                 });
 
                 it("reports which persistence space an object belongs to", function () {
                     expect(persistence.getSpace()).toEqual(SPACE);
                 });
 
-                it("updates persisted timestamp on persistence", function () {
-                    model.modified = 12321;
-                    persistence.persist();
-                    expect(model.persisted).toEqual(12321);
-                });
                 it("refreshes the domain object model from persistence", function () {
                     var refreshModel = {someOtherKey: "some other value"};
                     model.persisted = 1;
@@ -165,32 +152,39 @@ define(
 
                 it("does not trigger error notification on successful" +
                     " persistence", function () {
-                    persistence.persist();
+                    let rejected = false;
-                    expect(mockQ.reject).not.toHaveBeenCalled();
+                    return persistence.persist()
+                        .catch(() => rejected = true)
+                        .then(() => {
+                            expect(rejected).toBe(false);
                             expect(mockNofificationService.error).not.toHaveBeenCalled();
                         });
                 });
+            });
 
             describe("unsuccessful persistence", function () {
-                var sadPromise = {
-                    then: function (callback) {
-                        return asPromise(callback(0), true);
-                    }
-                };
                 beforeEach(function () {
-                    mockPersistenceService.createObject.and.returnValue(sadPromise);
+                    mockOpenMCT.objects.save.and.returnValue(Promise.resolve(false));
                 });
                 it("rejects on falsey persistence result", function () {
-                    persistence.persist();
+                    let rejected = false;
-                    expect(mockQ.reject).toHaveBeenCalled();
+                    return persistence.persist()
+                        .catch(() => rejected = true)
+                        .then(() => {
+                            expect(rejected).toBe(true);
+                        });
                 });
 
                 it("notifies user on persistence failure", function () {
-                    persistence.persist();
+                    let rejected = false;
-                    expect(mockQ.reject).toHaveBeenCalled();
+                    return persistence.persist()
+                        .catch(() => rejected = true)
+                        .then(() => {
+                            expect(rejected).toBe(true);
                             expect(mockNofificationService.error).toHaveBeenCalled();
                         });
                 });
             });
+        });
     }
 );

src/adapter/services/LegacyObjectAPIInterceptor.js

@@ -25,10 +25,11 @@ define([
 ], function (
     utils
 ) {
-    function ObjectServiceProvider(eventEmitter, objectService, instantiate, topic) {
+    function ObjectServiceProvider(eventEmitter, objectService, instantiate, topic, $injector) {
         this.eventEmitter = eventEmitter;
         this.objectService = objectService;
         this.instantiate = instantiate;
+        this.$injector = $injector;
 
         this.generalTopic = topic('mutation');
         this.bridgeEventBuses();
@@ -68,16 +69,53 @@ define([
         removeGeneralTopicListener = this.generalTopic.listen(handleLegacyMutation);
     };
 
-    ObjectServiceProvider.prototype.save = function (object) {
+    ObjectServiceProvider.prototype.create = async function (object) {
-        var key = object.key;
+        let model = utils.toOldFormat(object);
 
-        return object.getCapability('persistence')
+        return this.getPersistenceService().createObject(
-            .persist()
+            this.getSpace(utils.makeKeyString(object.identifier)),
-            .then(function () {
+            object.identifier.key,
-                return utils.toNewFormat(object, key);
+            model
-            });
+        );
+    }
+    ObjectServiceProvider.prototype.update = async function (object) {
+        let model = utils.toOldFormat(object);
+
+        return this.getPersistenceService().updateObject(
+            this.getSpace(utils.makeKeyString(object.identifier)),
+            object.identifier.key,
+            model
+        );
+    }
+
+    /**
+     * Get the space in which this domain object is persisted;
+     * this is useful when, for example, decided which space a
+     * newly-created domain object should be persisted to (by
+     * default, this should be the space of its containing
+     * object.)
+     *
+     * @returns {string} the name of the space which should
+     *          be used to persist this object
+     */
+    ObjectServiceProvider.prototype.getSpace = function (keystring) {
+        return this.getIdentifierService().parse(keystring).getSpace();
     };
 
+    ObjectServiceProvider.prototype.getIdentifierService = function () {
+        if (this.identifierService === undefined) {
+            this.identifierService = this.$injector.get('identifierService');
+        }
+        return this.identifierService;
+    };
+
+    ObjectServiceProvider.prototype.getPersistenceService = function () {
+        if (this.persistenceService === undefined) {
+            this.persistenceService = this.$injector.get('persistenceService');
+        }
+        return this.persistenceService;
+    }
+
     ObjectServiceProvider.prototype.delete = function (object) {
         // TODO!
     };
@@ -118,7 +156,8 @@ define([
                 eventEmitter,
                 objectService,
                 instantiate,
-                topic
+                topic,
+                openmct.$injector
             )
         );
 

src/api/objects/ObjectAPI.js

@@ -101,14 +101,25 @@ define([
      */
 
     /**
-     * Save this domain object in its current state.
+     * Create the given domain object in the corresponding persistence store
      *
-     * @method save
+     * @method create
      * @memberof module:openmct.ObjectProvider#
      * @param {module:openmct.DomainObject} domainObject the domain object to
-     *        save
+     *        create
      * @returns {Promise} a promise which will resolve when the domain object
-     *          has been saved, or be rejected if it cannot be saved
+     *          has been created, or be rejected if it cannot be saved
+     */
+
+    /**
+     * Update this domain object in its persistence store
+     *
+     * @method update
+     * @memberof module:openmct.ObjectProvider#
+     * @param {module:openmct.DomainObject} domainObject the domain object to
+     *        update
+     * @returns {Promise} a promise which will resolve when the domain object
+     *          has been updated, or be rejected if it cannot be saved
      */
 
     /**
@@ -161,8 +172,41 @@ define([
         throw new Error('Delete not implemented');
     };
 
-    ObjectAPI.prototype.save = function () {
+    ObjectAPI.prototype.isPersistable = function (domainObject) {
-        throw new Error('Save not implemented');
+        let provider = this.getProvider(domainObject.identifier);
+        return provider !== undefined &&
+            provider.create !== undefined &&
+            provider.update !== undefined;
+    }
+
+    /**
+     * Save this domain object in its current state. EXPERIMENTAL
+     *
+     * @private
+     * @memberof module:openmct.ObjectAPI#
+     * @param {module:openmct.DomainObject} domainObject the domain object to
+     *        save
+     * @returns {Promise} a promise which will resolve when the domain object
+     *          has been saved, or be rejected if it cannot be saved
+     */
+    ObjectAPI.prototype.save = function (domainObject) {
+        let provider = this.getProvider(domainObject.identifier);
+        let result;
+
+        if (!this.isPersistable(domainObject)) {
+            result = Promise.reject('Object provider does not support saving');
+        } else if (hasAlreadyBeenPersisted(domainObject)) {
+            result = Promise.resolve(true);
+        } else {
+            if (domainObject.persisted === undefined) {
+                this.mutate(domainObject, 'persisted', domainObject.modified);
+                result = provider.create(domainObject);
+            } else {
+                this.mutate(domainObject, 'persisted', domainObject.modified);
+                result = provider.update(domainObject);
+            }
+        }
+        return result;
     };
 
     /**
@@ -276,5 +320,9 @@ define([
      * @memberof module:openmct
      */
 
+    function hasAlreadyBeenPersisted(domainObject) {
+        return domainObject.persisted !== undefined &&
+            domainObject.persisted === domainObject.modified;
+    }
     return ObjectAPI;
 });

src/api/objects/ObjectAPISpec.js

@@ -0,0 +1,60 @@
+import ObjectAPI from './ObjectAPI.js';
+
+describe("The Object API", () => {
+    let objectAPI;
+    let mockDomainObject;
+    const TEST_NAMESPACE = "test-namespace";
+    const FIFTEEN_MINUTES = 15 * 60 * 1000;
+
+    beforeEach(() => {
+        objectAPI = new ObjectAPI();
+        mockDomainObject = {
+            identifier: {
+                namespace: TEST_NAMESPACE,
+                key: "test-key"
+            },
+            name: "test object",
+            type: "test-type"
+        };
+    })
+    describe("The save function", () => {
+        it("Rejects if no provider available", () => {
+            let rejected = false;
+            return objectAPI.save(mockDomainObject)
+                .catch(() => rejected = true)
+                .then(() => expect(rejected).toBe(true));
+        });
+        describe("when a provider is available", () => {
+            let mockProvider;
+            beforeEach(() => {
+                mockProvider = jasmine.createSpyObj("mock provider", [
+                    "create",
+                    "update"
+                ]);
+                objectAPI.addProvider(TEST_NAMESPACE, mockProvider);
+            })
+            it("Calls 'create' on provider if object is new", () => {
+                objectAPI.save(mockDomainObject);
+                expect(mockProvider.create).toHaveBeenCalled();
+                expect(mockProvider.update).not.toHaveBeenCalled();
+            });
+            it("Calls 'update' on provider if object is not new", () => {
+                mockDomainObject.persisted = Date.now() - FIFTEEN_MINUTES;
+                mockDomainObject.modified = Date.now();
+
+                objectAPI.save(mockDomainObject);
+                expect(mockProvider.create).not.toHaveBeenCalled();
+                expect(mockProvider.update).toHaveBeenCalled();
+            });
+
+            it("Does not persist if the object is unchanged", () => {
+                mockDomainObject.persisted =
+                    mockDomainObject.modified = Date.now();
+
+                objectAPI.save(mockDomainObject);
+                expect(mockProvider.create).not.toHaveBeenCalled();
+                expect(mockProvider.update).not.toHaveBeenCalled();
+            });
+        });
+    })
+});