From 432fc534c64f3a97d8b8703a8c2adc14434f8d13 Mon Sep 17 00:00:00 2001 From: Sebastian Malton Date: Tue, 3 May 2022 08:13:43 -0700 Subject: [PATCH] Fix last item not being removed on initial loadAll (#5309) --- .../__tests__/kube-object.store.test.ts | 146 ++++++++++++++++++ src/common/k8s-api/kube-object.store.ts | 28 ++-- 2 files changed, 165 insertions(+), 9 deletions(-) create mode 100644 src/common/k8s-api/__tests__/kube-object.store.test.ts diff --git a/src/common/k8s-api/__tests__/kube-object.store.test.ts b/src/common/k8s-api/__tests__/kube-object.store.test.ts new file mode 100644 index 0000000000..f0f26ec0d8 --- /dev/null +++ b/src/common/k8s-api/__tests__/kube-object.store.test.ts @@ -0,0 +1,146 @@ +/** + * Copyright (c) OpenLens Authors. All rights reserved. + * Licensed under MIT License. See LICENSE in root directory for more information. + */ + +import type { ClusterContext } from "../cluster-context"; +import type { KubeApi } from "../kube-api"; +import { KubeObject } from "../kube-object"; +import type { KubeObjectStoreLoadingParams } from "../kube-object.store"; +import { KubeObjectStore } from "../kube-object.store"; + +class FakeKubeObjectStore extends KubeObjectStore { + _context = { + allNamespaces: [], + contextNamespaces: [], + hasSelectedAll: false, + } as ClusterContext; + + get context() { + return this._context; + } + + constructor(private readonly _loadItems: (params: KubeObjectStoreLoadingParams) => KubeObject[], api: Partial>) { + super(api as KubeApi); + } + + async loadItems(params: KubeObjectStoreLoadingParams) { + return this._loadItems(params); + } +} + +describe("KubeObjectStore", () => { + it("should remove an object from the list of items after it is not returned from listing the same namespace again", async () => { + const loadItems = jest.fn(); + const obj = new KubeObject({ + apiVersion: "v1", + kind: "Foo", + metadata: { + name: "some-obj-name", + resourceVersion: "1", + uid: "some-uid", + namespace: "default", + }, + }); + const store = new FakeKubeObjectStore(loadItems, { + isNamespaced: true, + }); + + loadItems.mockImplementationOnce(() => [obj]); + + await store.loadAll({ + namespaces: ["default"], + }); + + expect(store.items).toContain(obj); + + loadItems.mockImplementationOnce(() => []); + + await store.loadAll({ + namespaces: ["default"], + }); + + expect(store.items).not.toContain(obj); + }); + + it("should not remove an object that is not returned, if it is in a different namespace", async () => { + const loadItems = jest.fn(); + const objInDefaultNamespace = new KubeObject({ + apiVersion: "v1", + kind: "Foo", + metadata: { + name: "some-obj-name", + resourceVersion: "1", + uid: "some-uid", + namespace: "default", + }, + }); + const objNotInDefaultNamespace = new KubeObject({ + apiVersion: "v1", + kind: "Foo", + metadata: { + name: "some-obj-name", + resourceVersion: "1", + uid: "some-uid", + namespace: "not-default", + }, + }); + const store = new FakeKubeObjectStore(loadItems, { + isNamespaced: true, + }); + + loadItems.mockImplementationOnce(() => [objInDefaultNamespace]); + + await store.loadAll({ + namespaces: ["default"], + }); + + expect(store.items).toContain(objInDefaultNamespace); + + loadItems.mockImplementationOnce(() => [objNotInDefaultNamespace]); + + await store.loadAll({ + namespaces: ["not-default"], + }); + + expect(store.items).toContain(objInDefaultNamespace); + }); + + it("should remove all objects not returned if the api is cluster-scoped", async () => { + const loadItems = jest.fn(); + const clusterScopedObject1 = new KubeObject({ + apiVersion: "v1", + kind: "Foo", + metadata: { + name: "some-obj-name", + resourceVersion: "1", + uid: "some-uid", + }, + }); + const clusterScopedObject2 = new KubeObject({ + apiVersion: "v1", + kind: "Foo", + metadata: { + name: "some-obj-name", + resourceVersion: "1", + uid: "some-uid", + namespace: "not-default", + }, + }); + const store = new FakeKubeObjectStore(loadItems, { + isNamespaced: false, + }); + + loadItems.mockImplementationOnce(() => [clusterScopedObject1]); + + await store.loadAll({}); + + expect(store.items).toContain(clusterScopedObject1); + + loadItems.mockImplementationOnce(() => [clusterScopedObject2]); + + await store.loadAll({}); + + expect(store.items).not.toContain(clusterScopedObject1); + }); +}); diff --git a/src/common/k8s-api/kube-object.store.ts b/src/common/k8s-api/kube-object.store.ts index 9e6d21bc2d..c7780585e8 100644 --- a/src/common/k8s-api/kube-object.store.ts +++ b/src/common/k8s-api/kube-object.store.ts @@ -60,10 +60,18 @@ export interface KubeObjectStoreSubscribeParams { abortController?: AbortController; } +export interface MergeItemsOptions { + merge?: boolean; + updateStore?: boolean; + sort?: boolean; + filter?: boolean; + namespaces: string[]; +} + export abstract class KubeObjectStore extends ItemStore { static defaultContext = observable.box(); // TODO: support multiple cluster contexts - public api: KubeApi; + public readonly api: KubeApi; public readonly limit?: number; public readonly bufferSize: number = 50000; @observable private loadedNamespaces?: string[]; @@ -227,7 +235,7 @@ export abstract class KubeObjectStore extends ItemStore } @action - async loadAll({ namespaces, merge = true, reqInit, onLoadFailure }: KubeObjectStoreLoadAllParams = {}): Promise { + async loadAll({ namespaces, merge = true, reqInit, onLoadFailure }: KubeObjectStoreLoadAllParams = {}): Promise { await this.contextReady; namespaces ??= this.context.contextNamespaces; this.isLoading = true; @@ -235,7 +243,7 @@ export abstract class KubeObjectStore extends ItemStore try { const items = await this.loadItems({ namespaces, reqInit, onLoadFailure }); - this.mergeItems(items, { merge }); + this.mergeItems(items, { merge, namespaces }); this.isLoaded = true; this.failedLoading = false; @@ -248,29 +256,31 @@ export abstract class KubeObjectStore extends ItemStore } finally { this.isLoading = false; } + + return undefined; } @action - async reloadAll(opts: { force?: boolean; namespaces?: string[]; merge?: boolean } = {}) { + async reloadAll(opts: { force?: boolean; namespaces?: string[]; merge?: boolean } = {}): Promise { const { force = false, ...loadingOptions } = opts; if (this.isLoading || (this.isLoaded && !force)) { - return; + return undefined; } return this.loadAll(loadingOptions); } @action - protected mergeItems(partialItems: T[], { merge = true, updateStore = true, sort = true, filter = true } = {}): T[] { + protected mergeItems(partialItems: T[], { merge = true, updateStore = true, sort = true, filter = true, namespaces }: MergeItemsOptions): T[] { let items = partialItems; // update existing items - if (merge) { - const namespaces = partialItems.map(item => item.getNs()); + if (merge && this.api.isNamespaced) { + const ns = new Set(namespaces); items = [ - ...this.items.filter(existingItem => !namespaces.includes(existingItem.getNs())), + ...this.items.filter(existingItem => !ns.has(existingItem.getNs())), ...partialItems, ]; }