From 22924f18fc8ff2c1e75f9022da3118627a6e37db Mon Sep 17 00:00:00 2001 From: Andrew Henry Date: Fri, 29 Jul 2022 14:29:34 -0700 Subject: [PATCH] Better handling of persistence errors (#5576) * conflict errors in particular --- src/api/objects/ObjectAPI.js | 9 +++- src/api/objects/ObjectAPISpec.js | 50 +++++++++++++++++-- .../persistence/couch/CouchObjectProvider.js | 7 +-- 3 files changed, 57 insertions(+), 9 deletions(-) diff --git a/src/api/objects/ObjectAPI.js b/src/api/objects/ObjectAPI.js index 9f5a681510..ed5b53e6cd 100644 --- a/src/api/objects/ObjectAPI.js +++ b/src/api/objects/ObjectAPI.js @@ -230,6 +230,7 @@ export default class ObjectAPI { return result; }).catch((result) => { console.warn(`Failed to retrieve ${keystring}:`, result); + this.openmct.notifications.error(`Failed to retrieve object ${keystring}`); delete this.cache[keystring]; @@ -387,7 +388,13 @@ export default class ObjectAPI { } } - return result; + return result.catch((error) => { + if (error instanceof this.errors.Conflict) { + this.openmct.notifications.error(`Conflict detected while saving ${this.makeKeyString(domainObject.identifier)}`); + } + + throw error; + }); } /** diff --git a/src/api/objects/ObjectAPISpec.js b/src/api/objects/ObjectAPISpec.js index 8887a04c74..4f6574598b 100644 --- a/src/api/objects/ObjectAPISpec.js +++ b/src/api/objects/ObjectAPISpec.js @@ -7,6 +7,7 @@ describe("The Object API", () => { let openmct = {}; let mockDomainObject; const TEST_NAMESPACE = "test-namespace"; + const TEST_KEY = "test-key"; const FIFTEEN_MINUTES = 15 * 60 * 1000; beforeEach((done) => { @@ -22,7 +23,7 @@ describe("The Object API", () => { mockDomainObject = { identifier: { namespace: TEST_NAMESPACE, - key: "test-key" + key: TEST_KEY }, name: "test object", type: "test-type" @@ -84,6 +85,31 @@ describe("The Object API", () => { expect(mockProvider.create).not.toHaveBeenCalled(); expect(mockProvider.update).not.toHaveBeenCalled(); }); + + describe("Shows a notification on persistence conflict", () => { + beforeEach(() => { + openmct.notifications.error = jasmine.createSpy('error'); + }); + + it("on create", () => { + mockProvider.create.and.returnValue(Promise.reject(new openmct.objects.errors.Conflict("Test Conflict error"))); + + return objectAPI.save(mockDomainObject).catch(() => { + expect(openmct.notifications.error).toHaveBeenCalledWith(`Conflict detected while saving ${TEST_NAMESPACE}:${TEST_KEY}`); + }); + + }); + + it("on update", () => { + mockProvider.update.and.returnValue(Promise.reject(new openmct.objects.errors.Conflict("Test Conflict error"))); + mockDomainObject.persisted = Date.now() - FIFTEEN_MINUTES; + mockDomainObject.modified = Date.now(); + + return objectAPI.save(mockDomainObject).catch(() => { + expect(openmct.notifications.error).toHaveBeenCalledWith(`Conflict detected while saving ${TEST_NAMESPACE}:${TEST_KEY}`); + }); + }); + }); }); }); @@ -138,21 +164,33 @@ describe("The Object API", () => { }); it("Caches multiple requests for the same object", () => { + const promises = []; expect(mockProvider.get.calls.count()).toBe(0); - objectAPI.get(mockDomainObject.identifier); + promises.push(objectAPI.get(mockDomainObject.identifier)); expect(mockProvider.get.calls.count()).toBe(1); - objectAPI.get(mockDomainObject.identifier); + promises.push(objectAPI.get(mockDomainObject.identifier)); expect(mockProvider.get.calls.count()).toBe(1); + + return Promise.all(promises); }); it("applies any applicable interceptors", () => { expect(mockDomainObject.changed).toBeUndefined(); - objectAPI.get(mockDomainObject.identifier).then((object) => { + + return objectAPI.get(mockDomainObject.identifier).then((object) => { expect(object.changed).toBeTrue(); expect(object.alsoChanged).toBeTrue(); expect(object.shouldNotBeChanged).toBeUndefined(); }); }); + + it("displays a notification in the event of an error", () => { + mockProvider.get.and.returnValue(Promise.reject()); + + return objectAPI.get(mockDomainObject.identifier).catch(() => { + expect(openmct.notifications.error).toHaveBeenCalledWith(`Failed to retrieve object ${TEST_NAMESPACE}:${TEST_KEY}`); + }); + }); }); }); @@ -168,7 +206,7 @@ describe("The Object API", () => { testObject = { identifier: { namespace: TEST_NAMESPACE, - key: 'test-key' + key: TEST_KEY }, name: 'test object', type: 'notebook', @@ -195,6 +233,8 @@ describe("The Object API", () => { "observeObjectChanges" ]); mockProvider.get.and.returnValue(Promise.resolve(testObject)); + mockProvider.create.and.returnValue(Promise.resolve(true)); + mockProvider.update.and.returnValue(Promise.resolve(true)); mockProvider.observeObjectChanges.and.callFake(() => { callbacks[0](updatedTestObject); callbacks.splice(0, 1); diff --git a/src/plugins/persistence/couch/CouchObjectProvider.js b/src/plugins/persistence/couch/CouchObjectProvider.js index 1c5e76129a..f4eda7a62c 100644 --- a/src/plugins/persistence/couch/CouchObjectProvider.js +++ b/src/plugins/persistence/couch/CouchObjectProvider.js @@ -217,9 +217,11 @@ class CouchObjectProvider { this.indicator.setIndicatorToState(DISCONNECTED); console.error(error.message); throw new Error(`CouchDB Error - No response"`); - } + } else { + console.error(error.message); - console.error(error.message); + throw error; + } } } @@ -653,7 +655,6 @@ class CouchObjectProvider { let document = new CouchDocument(key, queued.model); document.metadata.created = Date.now(); this.request(key, "PUT", document).then((response) => { - console.log('create check response', key); this.#checkResponse(response, queued.intermediateResponse, key); }).catch(error => { queued.intermediateResponse.reject(error);