catalog-react: refactor and fixes for DefaultStarredEntitiesApi

Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>

.changeset/silver-boxes-flash.md

@@ -0,0 +1,7 @@
+---
+'@backstage/plugin-catalog-react': patch
+---
+
+Fixed a risky behavior where `DefaultStarredEntitiesApi` forwarded values to observers that were later mutated.
+
+Removed the `isStarred` method from `DefaultStarredEntitiesApi`, as it is not part of the `StarredEntitiesApi`.

plugins/catalog-react/api-report.md

@@ -142,8 +142,6 @@ export type DefaultEntityFilters = {
 export class DefaultStarredEntitiesApi implements StarredEntitiesApi {
   constructor(opts: { storageApi: StorageApi });
   // (undocumented)
-  isStarred(entityRef: string): boolean;
-  // (undocumented)
   starredEntitie$(): Observable<Set<string>>;
   // (undocumented)
   toggleStarred(entityRef: string): Promise<void>;

plugins/catalog-react/src/apis/StarredEntitiesApi/DefaultStarredEntitiesApi.test.ts

@@ -14,33 +14,27 @@
  * limitations under the License.
  */
 
-import { stringifyEntityRef } from '@backstage/catalog-model';
-import { StorageApi } from '@backstage/core-plugin-api';
 import { MockStorageApi } from '@backstage/test-utils';
 import { DefaultStarredEntitiesApi } from './DefaultStarredEntitiesApi';
 import { performMigrationToTheNewBucket } from './migration';
 
 jest.mock('./migration');
 
-describe('DefaultStarredEntitiesApi', () => {
-  let mockStorage: StorageApi;
-  let starredEntitiesApi: DefaultStarredEntitiesApi;
-
-  const mockEntityRef = stringifyEntityRef({
-    apiVersion: '1',
-    kind: 'Component',
-    metadata: {
-      name: 'mock',
-    },
+function getStarred(api: DefaultStarredEntitiesApi) {
+  return new Promise((resolve, reject) => {
+    const subscription = api.starredEntitie$().subscribe({
+      next(starred) {
+        resolve(starred);
+        subscription.unsubscribe();
+      },
+      error: reject,
+    });
   });
+}
 
+describe('DefaultStarredEntitiesApi', () => {
   beforeEach(() => {
     (performMigrationToTheNewBucket as jest.Mock).mockResolvedValue(undefined);
-
-    mockStorage = MockStorageApi.create();
-    starredEntitiesApi = new DefaultStarredEntitiesApi({
-      storageApi: mockStorage,
-    });
   });
 
   afterEach(() => {
@@ -49,55 +43,53 @@ describe('DefaultStarredEntitiesApi', () => {
 
   describe('constructor', () => {
     it('should call migration', () => {
-      expect(performMigrationToTheNewBucket).toBeCalledTimes(1);
-    });
-  });
-
-  describe('toggleStarred', () => {
-    it('should star unstarred entity', async () => {
-      expect(starredEntitiesApi.isStarred(mockEntityRef)).toBe(false);
-
-      await starredEntitiesApi.toggleStarred(mockEntityRef);
-
-      expect(starredEntitiesApi.isStarred(mockEntityRef)).toBe(true);
-    });
-
-    it('should unstar starred entity', async () => {
-      const bucket = mockStorage.forBucket('starredEntities');
-      await bucket.set('entityRefs', ['component:default/mock']);
-
-      expect(starredEntitiesApi.isStarred(mockEntityRef)).toBe(true);
-
-      await starredEntitiesApi.toggleStarred(mockEntityRef);
-
-      expect(starredEntitiesApi.isStarred(mockEntityRef)).toBe(false);
-    });
-  });
-
-  describe('starredEntities$', () => {
-    const handler = jest.fn();
-
-    beforeEach(async () => {
-      await new Promise<void>(resolve => {
-        starredEntitiesApi.starredEntitie$().subscribe({
-          next: (...args) => {
-            handler(...args);
-
-            if (handler.mock.calls.length >= 2) {
-              resolve();
-            }
-          },
-        });
-
-        const bucket = mockStorage.forBucket('starredEntities');
-        bucket.set('entityRefs', ['component:default/mock']).then();
+      const api = new DefaultStarredEntitiesApi({
+        storageApi: MockStorageApi.create(),
       });
+      expect(performMigrationToTheNewBucket).toBeCalledTimes(1);
+      expect(api).toBeDefined();
+    });
+  });
+
+  it('should notify and toggle starred entities', async () => {
+    const entityRef = 'component:default/mock';
+
+    const storageApi = MockStorageApi.create();
+    const storageBucket = storageApi.forBucket('starredEntities');
+    const api = new DefaultStarredEntitiesApi({ storageApi });
+
+    const values = new Array<Set<string>>();
+    api.starredEntitie$().subscribe({
+      next: value => {
+        values.push(value);
+      },
     });
 
-    it('should receive updates', async () => {
-      expect(handler).toBeCalledTimes(2);
-      expect(handler).toBeCalledWith(new Set());
-      expect(handler).toBeCalledWith(new Set(['component:default/mock']));
-    });
+    await expect(getStarred(api)).resolves.toEqual(new Set());
+
+    await api.toggleStarred(entityRef);
+    await expect(getStarred(api)).resolves.toEqual(new Set([entityRef]));
+    expect(storageBucket.snapshot('entityRefs')).toEqual(
+      expect.objectContaining({ presence: 'present', value: [entityRef] }),
+    );
+
+    await api.toggleStarred(entityRef);
+    await expect(getStarred(api)).resolves.toEqual(new Set());
+    expect(storageBucket.snapshot('entityRefs')).toEqual(
+      expect.objectContaining({ presence: 'present', value: [] }),
+    );
+
+    expect(values).toEqual([new Set(), new Set([entityRef]), new Set()]);
+  });
+
+  it('should read starred entities from storage', async () => {
+    const entityRef = 'component:default/mock';
+
+    const storageApi = MockStorageApi.create();
+    const storageBucket = storageApi.forBucket('starredEntities');
+    storageBucket.set('entityRefs', [entityRef]);
+    const api = new DefaultStarredEntitiesApi({ storageApi });
+
+    await expect(getStarred(api)).resolves.toEqual(new Set([entityRef]));
   });
 });

plugins/catalog-react/src/apis/StarredEntitiesApi/DefaultStarredEntitiesApi.ts

@@ -64,17 +64,13 @@ export class DefaultStarredEntitiesApi implements StarredEntitiesApi {
     return this.observable;
   }
 
-  isStarred(entityRef: string): boolean {
-    return this.starredEntities.has(entityRef);
-  }
-
   private readonly subscribers = new Set<
     ZenObservable.SubscriptionObserver<Set<string>>
   >();
 
   private readonly observable = new ObservableImpl<Set<string>>(subscriber => {
     // forward the the latest value
-    subscriber.next(this.starredEntities);
+    subscriber.next(new Set(this.starredEntities));
 
     this.subscribers.add(subscriber);
     return () => {
@@ -84,7 +80,7 @@ export class DefaultStarredEntitiesApi implements StarredEntitiesApi {
 
   private notifyChanges() {
     for (const subscription of this.subscribers) {
-      subscription.next(this.starredEntities);
+      subscription.next(new Set(this.starredEntities));
     }
   }
 }