rename to UserSettingsStorage and adjust error handling

Signed-off-by: Fredrik Adelöw <freben@gmail.com>

.changeset/healthy-oranges-fly.md

@@ -3,6 +3,7 @@
 '@backstage/plugin-user-settings-backend': minor
 ---
 
-Add new plugin @backstage/user-settings-backend to store user related settings
-in the database. Additionally adding a PersistentStorage implementation to use
-easily use the new plugin as drop-in replacement for the WebStorage.
+Add new plugin `@backstage/user-settings-backend` to store user related settings
+in the database. Additionally adding a `UserSettingsStorage` implementation of
+the `StorageApi` to easily use the new plugin as drop-in replacement for the
+`WebStorage`.

packages/core-app-api/api-report.md

@@ -506,35 +506,6 @@ export type OneLoginAuthCreateOptions = {
   provider?: AuthProviderInfo;
 };
 
-// @public
-export class PersistentStorage implements StorageApi {
-  constructor(
-    namespace: string,
-    fetchApi: FetchApi,
-    discoveryApi: DiscoveryApi,
-    errorApi: ErrorApi,
-  );
-  // (undocumented)
-  static create(options: {
-    fetchApi: FetchApi;
-    discoveryApi: DiscoveryApi;
-    errorApi: ErrorApi;
-    namespace?: string;
-  }): PersistentStorage;
-  // (undocumented)
-  forBucket(name: string): StorageApi;
-  // (undocumented)
-  observe$<T extends JsonValue>(
-    key: string,
-  ): Observable<StorageValueSnapshot<T>>;
-  // (undocumented)
-  remove(key: string): Promise<void>;
-  // (undocumented)
-  set<T extends JsonValue>(key: string, data: T): Promise<void>;
-  // (undocumented)
-  snapshot<T extends JsonValue>(key: string): StorageValueSnapshot<T>;
-}
-
 // @public
 export class SamlAuth
   implements ProfileInfoApi, BackstageIdentityApi, SessionApi
@@ -572,6 +543,29 @@ export class UrlPatternDiscovery implements DiscoveryApi {
   getBaseUrl(pluginId: string): Promise<string>;
 }
 
+// @public
+export class UserSettingsStorage implements StorageApi {
+  // (undocumented)
+  static create(options: {
+    fetchApi: FetchApi;
+    discoveryApi: DiscoveryApi;
+    errorApi: ErrorApi;
+    namespace?: string;
+  }): UserSettingsStorage;
+  // (undocumented)
+  forBucket(name: string): StorageApi;
+  // (undocumented)
+  observe$<T extends JsonValue>(
+    key: string,
+  ): Observable<StorageValueSnapshot<T>>;
+  // (undocumented)
+  remove(key: string): Promise<void>;
+  // (undocumented)
+  set<T extends JsonValue>(key: string, data: T): Promise<void>;
+  // (undocumented)
+  snapshot<T extends JsonValue>(key: string): StorageValueSnapshot<T>;
+}
+
 // @public
 export class WebStorage implements StorageApi {
   constructor(namespace: string, errorApi: ErrorApi);

packages/core-app-api/src/apis/implementations/StorageApi/UserSettingsStorage.test.ts

@@ -14,22 +14,21 @@
  * limitations under the License.
  */
 
-import { PersistentStorage } from './PersistentStorage';
 import {
+  DiscoveryApi,
   ErrorApi,
   FetchApi,
   StorageApi,
-  DiscoveryApi,
 } from '@backstage/core-plugin-api';
 import { MockFetchApi, setupRequestMockHandlers } from '@backstage/test-utils';
-
 import { rest } from 'msw';
 import { setupServer } from 'msw/node';
-
-const server = setupServer();
+import { UserSettingsStorage } from './UserSettingsStorage';
 
 describe('Persistent Storage API', () => {
+  const server = setupServer();
   setupRequestMockHandlers(server);
+
   const mockBaseUrl = 'http://backstage:9191/api';
   const mockErrorApi = { post: jest.fn(), error$: jest.fn() };
   const mockDiscoveryApi = { getBaseUrl: async () => mockBaseUrl };
@@ -42,7 +41,7 @@ describe('Persistent Storage API', () => {
       namespace?: string;
     }>,
   ): StorageApi => {
-    return PersistentStorage.create({
+    return UserSettingsStorage.create({
       errorApi: mockErrorApi,
       fetchApi: new MockFetchApi(),
       discoveryApi: mockDiscoveryApi,
@@ -58,7 +57,9 @@ describe('Persistent Storage API', () => {
     );
   });
 
-  afterEach(() => jest.resetAllMocks());
+  afterEach(() => {
+    jest.resetAllMocks();
+  });
 
   it('should return undefined for values which are unset', async () => {
     const storage = createPersistentStorage();
@@ -267,7 +268,7 @@ describe('Persistent Storage API', () => {
     expect(mockErrorApi.post).not.toHaveBeenCalled();
   });
 
-  it('should call the error api when the json can not be parsed in local storage', async () => {
+  it('should silently treat the value as absent when the json can not be parsed', async () => {
     const selectedKeyNextHandler = jest.fn();
     const rootStorage = createPersistentStorage({
       namespace: 'Test.Mock.Thing',
@@ -278,10 +279,8 @@ describe('Persistent Storage API', () => {
         `${mockBaseUrl}/buckets/:bucket/keys/:key`,
         async (req, res, ctx) => {
           const { bucket, key } = req.params;
-
           expect(bucket).toEqual('Test.Mock.Thing');
           expect(key).toEqual('key');
-
           return res(ctx.text('{ invalid: json string }'));
         },
       ),
@@ -305,7 +304,6 @@ describe('Persistent Storage API', () => {
       presence: 'absent',
       value: undefined,
     });
-    expect(mockErrorApi.post).toHaveBeenCalledWith(expect.any(Error));
   });
 
   it('should freeze the snapshot value', async () => {

packages/core-app-api/src/apis/implementations/StorageApi/UserSettingsStorage.ts

@@ -21,11 +21,11 @@ import {
   StorageApi,
   StorageValueSnapshot,
 } from '@backstage/core-plugin-api';
-import { NotFoundError } from '@backstage/errors';
+import { ResponseError } from '@backstage/errors';
 import { JsonValue, Observable } from '@backstage/types';
 import ObservableImpl from 'zen-observable';
 
-const buckets = new Map<string, PersistentStorage>();
+const buckets = new Map<string, UserSettingsStorage>();
 
 /**
  * An implementation of the storage API, that uses the user-settings backend to
@@ -33,7 +33,7 @@ const buckets = new Map<string, PersistentStorage>();
  *
  * @public
  */
-export class PersistentStorage implements StorageApi {
+export class UserSettingsStorage implements StorageApi {
   private subscribers = new Set<
     ZenObservable.SubscriptionObserver<StorageValueSnapshot<JsonValue>>
   >();
@@ -47,7 +47,7 @@ export class PersistentStorage implements StorageApi {
     };
   });
 
-  constructor(
+  private constructor(
     private readonly namespace: string,
     private readonly fetchApi: FetchApi,
     private readonly discoveryApi: DiscoveryApi,
@@ -59,8 +59,8 @@ export class PersistentStorage implements StorageApi {
     discoveryApi: DiscoveryApi;
     errorApi: ErrorApi;
     namespace?: string;
-  }): PersistentStorage {
-    return new PersistentStorage(
+  }): UserSettingsStorage {
+    return new UserSettingsStorage(
       options.namespace ?? 'default',
       options.fetchApi,
       options.discoveryApi,
@@ -75,7 +75,7 @@ export class PersistentStorage implements StorageApi {
     if (!buckets.has(bucketPath)) {
       buckets.set(
         bucketPath,
-        new PersistentStorage(
+        new UserSettingsStorage(
           bucketPath,
           this.fetchApi,
           this.discoveryApi,
@@ -90,10 +90,14 @@ export class PersistentStorage implements StorageApi {
   async remove(key: string): Promise<void> {
     const fetchUrl = await this.getFetchUrl(key);
 
-    await this.fetchApi.fetch(fetchUrl, {
+    const response = await this.fetchApi.fetch(fetchUrl, {
       method: 'DELETE',
     });
 
+    if (!response.ok && response.status !== 404) {
+      throw ResponseError.fromResponse(response);
+    }
+
     this.notifyChanges({
       key,
       presence: 'absent',
@@ -109,6 +113,10 @@ export class PersistentStorage implements StorageApi {
       body,
     });
 
+    if (!response.ok) {
+      throw ResponseError.fromResponse(response);
+    }
+
     const { value } = await response.json();
 
     this.notifyChanges({
@@ -121,12 +129,16 @@ export class PersistentStorage implements StorageApi {
   observe$<T extends JsonValue>(
     key: string,
   ): Observable<StorageValueSnapshot<T>> {
+    // TODO(freben): Introduce server polling or similar, to ensure that different devices update when values change
     return this.observable.filter(({ key: messageKey }) => messageKey === key);
   }
 
   snapshot<T extends JsonValue>(key: string): StorageValueSnapshot<T> {
-    // trigger a reload
-    this.get(key).then(snapshot => this.notifyChanges(snapshot));
+    // trigger a reload, ensure it happens on the next tick (after returning)
+    Promise.resolve()
+      .then(() => this.get(key))
+      .then(snapshot => this.notifyChanges(snapshot))
+      .catch(error => this.errorApi.post(error));
 
     return {
       key,
@@ -137,20 +149,21 @@ export class PersistentStorage implements StorageApi {
   private async get<T extends JsonValue>(
     key: string,
   ): Promise<StorageValueSnapshot<T>> {
+    const fetchUrl = await this.getFetchUrl(key);
+    const response = await this.fetchApi.fetch(fetchUrl);
+
+    if (response.status === 404) {
+      return {
+        key,
+        presence: 'absent',
+      };
+    }
+
+    if (!response.ok) {
+      throw ResponseError.fromResponse(response);
+    }
+
     try {
-      const fetchUrl = await this.getFetchUrl(key);
-      const response = await this.fetchApi.fetch(fetchUrl);
-
-      if (response.status === 404) {
-        throw new NotFoundError(
-          `Setting '${key}' is not set in bucket '${this.namespace}'`,
-        );
-      } else if (!response.ok) {
-        throw new Error(
-          `Unable to fetch '${key}' from bucket '${this.namespace}'`,
-        );
-      }
-
       const { value: rawValue } = await response.json();
       const value = JSON.parse(rawValue, (_key, val) => {
         if (typeof val === 'object' && val !== null) {
@@ -164,16 +177,11 @@ export class PersistentStorage implements StorageApi {
         presence: 'present',
         value,
       };
-    } catch (error) {
-      // NotFoundError shouldn't be recorded
-      if (error && !(error instanceof NotFoundError)) {
-        this.errorApi.post(error);
-      }
-
+    } catch {
+      // If the value is not valid JSON, we return an unknown presence. This should never happen
       return {
         key,
         presence: 'absent',
-        value: undefined,
       };
     }
   }
@@ -185,9 +193,15 @@ export class PersistentStorage implements StorageApi {
     return `${baseUrl}/buckets/${encodedNamespace}/keys/${encodedKey}`;
   }
 
-  private async notifyChanges<T>(snapshot: StorageValueSnapshot<T>) {
+  private async notifyChanges<T extends JsonValue>(
+    snapshot: StorageValueSnapshot<T>,
+  ) {
     for (const subscription of this.subscribers) {
-      subscription.next(snapshot);
+      try {
+        subscription.next(snapshot);
+      } catch {
+        // ignore
+      }
     }
   }
 }

packages/core-app-api/src/apis/implementations/StorageApi/index.ts

@@ -15,4 +15,4 @@
  */
 
 export { WebStorage } from './WebStorage';
-export { PersistentStorage } from './PersistentStorage';
+export { UserSettingsStorage } from './UserSettingsStorage';

plugins/user-settings-backend/src/database/DatabaseUserSettingsStore.test.ts

@@ -13,9 +13,9 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 import { TestDatabaseId, TestDatabases } from '@backstage/backend-test-utils';
 import { Knex } from 'knex';
-
 import {
   DatabaseUserSettingsStore,
   RawDbUserSettingsRow,

plugins/user-settings-backend/src/database/DatabaseUserSettingsStore.ts

@@ -13,13 +13,13 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 import {
   PluginDatabaseManager,
   resolvePackagePath,
 } from '@backstage/backend-common';
 import { NotFoundError } from '@backstage/errors';
 import { Knex } from 'knex';
-
 import { type UserSetting, UserSettingsStore } from './UserSettingsStore';
 
 const migrationsDir = resolvePackagePath(

plugins/user-settings-backend/src/database/index.ts

@@ -13,6 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 export {
   DatabaseUserSettingsStore,
   type RawDbUserSettingsRow,

plugins/user-settings-backend/src/index.ts

@@ -13,5 +13,6 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 export * from './service';
 export * from './database';

plugins/user-settings-backend/src/run.ts

@@ -13,6 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 import { getRootLogger } from '@backstage/backend-common';
 import yn from 'yn';
 

plugins/user-settings-backend/src/service/index.ts

@@ -13,4 +13,5 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 export { createRouter, type RouterOptions } from './router';

plugins/user-settings-backend/src/service/router.test.ts

@@ -13,11 +13,11 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 import { AuthenticationError } from '@backstage/errors';
 import { IdentityClient } from '@backstage/plugin-auth-node';
 import express from 'express';
 import request from 'supertest';
-
 import { UserSettingsStore } from '../database';
 import { createRouter } from './router';
 

plugins/user-settings-backend/src/service/router.ts

@@ -13,6 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 import { errorHandler } from '@backstage/backend-common';
 import { AuthenticationError, InputError } from '@backstage/errors';
 import {
@@ -21,7 +22,6 @@ import {
 } from '@backstage/plugin-auth-node';
 import express, { Request } from 'express';
 import Router from 'express-promise-router';
-
 import { UserSettingsStore } from '../database';
 
 /**

plugins/user-settings-backend/src/service/standaloneServer.ts

@@ -13,16 +13,15 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-import { Server } from 'http';
 
 import { createServiceBuilder, useHotMemoize } from '@backstage/backend-common';
-import Knex from 'knex';
-import { Logger } from 'winston';
-
-import { DatabaseUserSettingsStore } from '../database';
-import { createRouter } from './router';
 import { IdentityClient } from '@backstage/plugin-auth-node';
 import { Router } from 'express';
+import { Server } from 'http';
+import Knex from 'knex';
+import { Logger } from 'winston';
+import { DatabaseUserSettingsStore } from '../database';
+import { createRouter } from './router';
 
 export interface ServerOptions {
   port: number;

plugins/user-settings-backend/src/setupTests.ts

@@ -13,4 +13,5 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 export {};