Add delete method do settings manager and implement it metadata manager (#241)

* Add delete method do settings manager and implement it metadata manager

* Implement EncrypedMetadataManager.delete + changesets + docs

* Update src/settings-manager/metadata-manager.test.ts

Co-authored-by: Krzysztof Wolski <krzysztof.k.wolski@gmail.com>

* Fix test

---------

Co-authored-by: Krzysztof Wolski <krzysztof.k.wolski@gmail.com>

.changeset/unlucky-ghosts-yell.md

@@ -0,0 +1,5 @@
+---
+"@saleor/app-sdk": minor
+---
+
+Added new method to SettingsManager - "delete". It can delete metadata by key in Saleor. Implemented in MetadataManager and EncryptedMetadataManager

docs/settings-manager.md

@@ -12,12 +12,28 @@ Entries in the manager are stored using structure:
   domain?: string;
 ```
 
+## `DeleteMetadataArg` interface
+
+Argument that can be used to remove metadata via `manager.delete()` method
+
+It contains key and domain:
+
+```typescript
+type DeleteMetadataArg = {
+  key: string;
+  domain: string;
+};
+```
+
 For values which should not be migrated during environment cloning (as private keys to payment provider), developer should use domain field to bind it to particular store instance.
 
 ## Available methods
 
 - `get: (key: string, domain?: string) => Promise<string | undefined>`
 - `set: (settings: SettingsValue[] | SettingsValue) => Promise<void>`
+- `delete: (args: string | string[] | DeleteMetadataArg | DeleteMetadataArg[]) => Promise<void>`
+
+Warning: delete method can throw, if instance of SettingsManager was not configured with proper mutation in constructor.
 
 # MetadataManager
 
@@ -31,6 +47,7 @@ import {
   FetchAppDetailsDocument,
   FetchAppDetailsQuery,
   UpdateAppMetadataDocument,
+  DeleteMetadataDocument,
 } from "../generated/graphql";
 
 export async function fetchAllMetadata(client: Client): Promise<MetadataEntry[]> {
@@ -56,6 +73,10 @@ export async function mutateMetadata(client: Client, metadata: MetadataEntry[])
     })) || []
   );
 }
+
+export async function deleteMetadata(client: Client, keys: string[]) {
+  return client.mutation(DeleteMetadataDocument, { keys }).toPromise();
+}
 ```
 
 And create MetadataManager instance:
@@ -64,6 +85,7 @@ And create MetadataManager instance:
 const settings = new MetadataManager({
   fetchMetadata: () => fetchAllMetadata(client),
   mutateMetadata: (md) => mutateMetadata(client, md),
+  deleteMetadata: (keys) => deleteMetadata(client, keys),
 });
 ```
 
@@ -77,6 +99,7 @@ new EncryptedMetadataManager({
   encryptionKey: process.env.SECRET_KEY, // secrets should be saved in the environment variables, never in the source code
   fetchMetadata: () => fetchAllMetadata(client),
   mutateMetadata: (metadata) => mutateMetadata(client, metadata),
+  deleteMetadata: (keys) => deleteMetadata(client, keys),
 });
 ```
 

src/settings-manager/encrypted-metadata-manager.test.ts

@@ -62,6 +62,8 @@ describe("settings-manager", () => {
     });
 
     describe("Manager operations", () => {
+      // eslint-disable-next-line @typescript-eslint/no-empty-function
+      const deleteMetadataMock = vi.fn(async () => {});
       const fetchMock = vi.fn(async () => metadata);
       const mutateMock = vi.fn(async (md: MetadataEntry[]) => [...metadata, ...md]);
       const manager = new EncryptedMetadataManager({
@@ -122,6 +124,48 @@ describe("settings-manager", () => {
         // Set method should populate cache with updated values, so fetch is never called
         expect(fetchMock).toBeCalledTimes(0);
       });
+
+      /**
+       * Smoke test operations for "delete" method.
+       * Details tests are in MetadataManager that is under the hood of EncryptedMetadataManager
+       */
+      it("Calls delete metadata mutation when key deleted - all formats", async () => {
+        const managerWithDelete = new EncryptedMetadataManager({
+          fetchMetadata: fetchMock,
+          mutateMetadata: mutateMock,
+          deleteMetadata: deleteMetadataMock,
+          encryptionKey: "key",
+        });
+
+        await managerWithDelete.delete("single-arg-1");
+        await managerWithDelete.delete(["multiple-arg-1", "multiple-arg-2"]);
+        await managerWithDelete.delete({
+          key: "single-arg-1-domain",
+          domain: "https://example.com/graphql/",
+        });
+        await managerWithDelete.delete([
+          {
+            key: "multiple-arg-1-domain",
+            domain: "https://example.com/graphql/",
+          },
+          {
+            key: "multiple-arg-2-domain",
+            domain: "https://example.com/graphql/",
+          },
+        ]);
+
+        expect(deleteMetadataMock).toBeCalledTimes(4);
+
+        expect(deleteMetadataMock).toHaveBeenNthCalledWith(1, ["single-arg-1"]);
+        expect(deleteMetadataMock).toHaveBeenNthCalledWith(2, ["multiple-arg-1", "multiple-arg-2"]);
+        expect(deleteMetadataMock).toHaveBeenNthCalledWith(3, [
+          "single-arg-1-domain__https://example.com/graphql/",
+        ]);
+        expect(deleteMetadataMock).toHaveBeenNthCalledWith(4, [
+          "multiple-arg-1-domain__https://example.com/graphql/",
+          "multiple-arg-2-domain__https://example.com/graphql/",
+        ]);
+      });
     });
   });
 });

src/settings-manager/encrypted-metadata-manager.ts

@@ -1,7 +1,7 @@
 import crypto from "crypto";
 
 import { MetadataManager, MetadataManagerConfig } from "./metadata-manager";
-import { SettingsManager, SettingsValue } from "./settings-manager";
+import { DeleteSettingsValue, SettingsManager, SettingsValue } from "./settings-manager";
 
 export type EncryptCallback = (value: string, secret: string) => string;
 
@@ -66,10 +66,12 @@ export class EncryptedMetadataManager implements SettingsManager {
     encryptionKey,
     encryptionMethod,
     decryptionMethod,
+    deleteMetadata,
   }: EncryptedMetadataManagerConfig) {
     this.metadataManager = new MetadataManager({
       fetchMetadata,
       mutateMetadata,
+      deleteMetadata,
     });
     if (encryptionKey) {
       this.encryptionKey = encryptionKey;
@@ -110,4 +112,8 @@ export class EncryptedMetadataManager implements SettingsManager {
     }));
     return this.metadataManager.set(encryptedSettings);
   }
+
+  async delete(args: DeleteSettingsValue | DeleteSettingsValue[] | string | string[]) {
+    await this.metadataManager.delete(args);
+  }
 }

src/settings-manager/metadata-manager.test.ts

@@ -13,6 +13,8 @@ describe("settings-manager", () => {
   describe("metadata-manager", () => {
     const fetchMock = vi.fn(async () => metadata);
     const mutateMock = vi.fn(async (md: MetadataEntry[]) => [...metadata, ...md]);
+    // eslint-disable-next-line @typescript-eslint/no-empty-function
+    const deleteMetadataMock = vi.fn(async () => {});
 
     beforeEach(() => {
       vi.restoreAllMocks();
@@ -53,5 +55,71 @@ describe("settings-manager", () => {
       // Set method should populate cache with updated values, so fetch is never called
       expect(fetchMock).toBeCalledTimes(0);
     });
+
+    describe("Delete metadata method", () => {
+      /**
+       * Ensure no breaking changes introduced
+       */
+      it("Constructs if deleteMetadata param is not passed", () => {
+        const manager = new MetadataManager({
+          fetchMetadata: fetchMock,
+          mutateMetadata: mutateMock,
+        });
+
+        expect(manager).toBeDefined();
+      });
+
+      it("Throws if \"delete\" method is called, but deleteMetadata was not passed to constructor", async () => {
+        const manager = new MetadataManager({
+          fetchMetadata: fetchMock,
+          mutateMetadata: mutateMock,
+        });
+
+        await expect(manager.delete("test")).rejects.toThrowErrorMatchingInlineSnapshot(
+          "\"Delete not implemented. Ensure MetadataManager is configured with deleteMetadata param in constructor\""
+        );
+      });
+
+      it("Calls deleteMetadata constructor param when \"delete\" method called", async () => {
+        const manager = new MetadataManager({
+          fetchMetadata: fetchMock,
+          mutateMetadata: mutateMock,
+          deleteMetadata: deleteMetadataMock,
+        });
+
+        await manager.delete("single-key");
+        await manager.delete(["multiple-key-1", "multiple-key-2"]);
+
+        expect(deleteMetadataMock).toBeCalledTimes(2);
+        /**
+         * Ensure callback is called with array type, even if single string was passed
+         */
+        expect(deleteMetadataMock).toHaveBeenNthCalledWith(1, ["single-key"]);
+        expect(deleteMetadataMock).toHaveBeenNthCalledWith(2, ["multiple-key-1", "multiple-key-2"]);
+      });
+
+      it("Accepts records with key and domain and constructs scoped suffixes", async () => {
+        const manager = new MetadataManager({
+          fetchMetadata: fetchMock,
+          mutateMetadata: mutateMock,
+          deleteMetadata: deleteMetadataMock,
+        });
+
+        await manager.delete({ key: "test", domain: "https://example.com/graphql/" });
+        await manager.delete([
+          { key: "test1", domain: "https://example.com/graphql/" },
+          { key: "test2", domain: "https://example.com/graphql/" },
+        ]);
+
+        expect(deleteMetadataMock).toBeCalledTimes(2);
+        expect(deleteMetadataMock).toHaveBeenNthCalledWith(1, [
+          "test__https://example.com/graphql/",
+        ]);
+        expect(deleteMetadataMock).toHaveBeenNthCalledWith(2, [
+          "test1__https://example.com/graphql/",
+          "test2__https://example.com/graphql/",
+        ]);
+      });
+    });
   });
 });

src/settings-manager/metadata-manager.ts

@@ -1,13 +1,16 @@
-import { SettingsManager, SettingsValue } from "./settings-manager";
+import { DeleteSettingsValue, SettingsManager, SettingsValue } from "./settings-manager";
 
 export type MetadataEntry = {
   key: string;
   value: string;
 };
 
+/**
+ * TODO Rename "Callback" suffixes, these are not callbacks
+ */
 export type FetchMetadataCallback = () => Promise<MetadataEntry[]>;
-
 export type MutateMetadataCallback = (metadata: MetadataEntry[]) => Promise<MetadataEntry[]>;
+export type DeleteMetadataCallback = (keys: string[]) => Promise<void>;
 
 const deserializeMetadata = ({ key, value }: MetadataEntry): SettingsValue => {
   // domain specific metadata use convention key__domain, e.g. `secret_key__example.com`
@@ -20,6 +23,8 @@ const deserializeMetadata = ({ key, value }: MetadataEntry): SettingsValue => {
   };
 };
 
+const constructDomainSpecificKey = (key: string, domain: string) => [key, domain].join("__");
+
 const serializeSettingsToMetadata = ({ key, value, domain }: SettingsValue): MetadataEntry => {
   // domain specific metadata use convention key__domain, e.g. `secret_key__example.com`
   if (!domain) {
@@ -27,7 +32,7 @@ const serializeSettingsToMetadata = ({ key, value, domain }: SettingsValue): Met
   }
 
   return {
-    key: [key, domain].join("__"),
+    key: constructDomainSpecificKey(key, domain),
     value,
   };
 };
@@ -35,6 +40,11 @@ const serializeSettingsToMetadata = ({ key, value, domain }: SettingsValue): Met
 export interface MetadataManagerConfig {
   fetchMetadata: FetchMetadataCallback;
   mutateMetadata: MutateMetadataCallback;
+  /**
+   * Keep it optional, to avoid breaking changes. If not provided, delete will throw.
+   * TODO: Make it required in next major version
+   */
+  deleteMetadata?: DeleteMetadataCallback;
 }
 
 /**
@@ -51,9 +61,12 @@ export class MetadataManager implements SettingsManager {
 
   private mutateMetadata: MutateMetadataCallback;
 
-  constructor({ fetchMetadata, mutateMetadata }: MetadataManagerConfig) {
+  private deleteMetadata?: DeleteMetadataCallback;
+
+  constructor({ fetchMetadata, mutateMetadata, deleteMetadata }: MetadataManagerConfig) {
     this.fetchMetadata = fetchMetadata;
     this.mutateMetadata = mutateMetadata;
+    this.deleteMetadata = deleteMetadata;
   }
 
   async get(key: string, domain?: string) {
@@ -77,4 +90,28 @@ export class MetadataManager implements SettingsManager {
     const metadata = await this.mutateMetadata(serializedMetadata);
     this.settings = metadata.map(deserializeMetadata);
   }
+
+  /**
+   * Typescript doesn't properly infer arguments, so they have to be rewritten explicitly
+   */
+  async delete(args: DeleteSettingsValue | DeleteSettingsValue[] | string | string[]) {
+    if (!this.deleteMetadata) {
+      throw new Error(
+        "Delete not implemented. Ensure MetadataManager is configured with deleteMetadata param in constructor"
+      );
+    }
+
+    const argsArray = Array.isArray(args) ? args : [args];
+    const keysToDelete = argsArray.map((keyOrDomainPair) => {
+      if (typeof keyOrDomainPair === "string") {
+        return keyOrDomainPair;
+      }
+      const { key, domain } = keyOrDomainPair;
+      return constructDomainSpecificKey(key, domain);
+    });
+
+    await this.deleteMetadata(keysToDelete);
+
+    this.settings = this.settings.filter((setting) => !argsArray.includes(setting.key));
+  }
 }

src/settings-manager/settings-manager.ts

@@ -4,7 +4,18 @@ export type SettingsValue = {
   domain?: string;
 };
 
+export type DeleteSettingsValue = {
+  key: string;
+  domain: string;
+};
+
+type DeleteFnSimple = (keys: string | string[]) => Promise<void>;
+type DeleteFnWithDomain = (
+  keysWithDomain: DeleteSettingsValue | DeleteSettingsValue[]
+) => Promise<void>;
+
 export interface SettingsManager {
   get: (key: string, domain?: string) => Promise<string | undefined>;
   set: (settings: SettingsValue[] | SettingsValue) => Promise<void>;
+  delete: DeleteFnSimple | DeleteFnWithDomain;
 }