Updates from PR feedback (round 2)

Shopify · Jul 19, 2022 · 665c113 · 665c113
1 parent 3f81920
commit 665c113
Show file tree

Hide file tree

Showing 10 changed files with 107 additions and 55 deletions.
diff --git a/src/auth/session/session_storage.ts b/src/auth/session/session_storage.ts
@@ -37,9 +37,7 @@ interface SessionStorage {
    *
    * @param shop shop of the session(s) to return
    */
-  findSessionsByShop?(
-    shop: string,
-  ): Promise<SessionInterface[] | {[key: string]: unknown}[]>;
+  findSessionsByShop?(shop: string): Promise<SessionInterface[]>;
 }
 
 export {SessionStorage};
diff --git a/src/auth/session/storage/__tests__/battery-of-tests.ts b/src/auth/session/storage/__tests__/battery-of-tests.ts
@@ -124,7 +124,7 @@ export function batteryOfTests(storageFactory: () => Promise<SessionStorage>) {
       if (shop1Sessions) {
         expect(shop1Sessions.length).toBe(2);
         expect(
-          sessionArraysEqual(shop1Sessions as SessionInterface[], [
+          sessionArraysEqual(shop1Sessions, [
             sessions[0] as SessionInterface,
             sessions[2] as SessionInterface,
           ]),
@@ -155,9 +155,7 @@ export function batteryOfTests(storageFactory: () => Promise<SessionStorage>) {
       if (shop1Sessions) {
         expect(shop1Sessions.length).toBe(2);
         const idsToDelete = shop1Sessions.map((session) => session.id);
-        await expect(
-          storage.deleteSessions(idsToDelete as string[]),
-        ).resolves.toBe(true);
+        await expect(storage.deleteSessions(idsToDelete)).resolves.toBe(true);
         shop1Sessions = await storage.findSessionsByShop(
           'delete-shop1-sessions',
         );

diff --git a/src/auth/session/storage/__tests__/custom.test.ts b/src/auth/session/storage/__tests__/custom.test.ts
@@ -3,7 +3,7 @@ import {CustomSessionStorage} from '../custom';
 import {SessionStorageError} from '../../../../error';
 
 describe('custom session storage', () => {
-  test('can perform actions', async () => {
+  test('can perform core actions', async () => {
     const sessionId = 'test_session';
     let session: Session | undefined = new Session(
       sessionId,
@@ -56,6 +56,89 @@ describe('custom session storage', () => {
     expect(deleteCalled).toBe(true);
   });
 
+  test('can perform optional actions', async () => {
+    const prefix = 'custom_sessions';
+    let sessions = [
+      new Session(`${prefix}_1`, 'shop1-sessions', 'state', true),
+      new Session(`${prefix}_2`, 'shop2-sessions', 'state', true),
+      new Session(`${prefix}_3`, 'shop1-sessions', 'state', true),
+      new Session(`${prefix}_4`, 'shop3-sessions', 'state', true),
+    ];
+
+    let deleteSessionsCalled = false;
+    let findSessionsByShopCalled = false;
+    const storage = new CustomSessionStorage(
+      () => {
+        return Promise.resolve(true);
+      },
+      () => {
+        return Promise.resolve(sessions[0]);
+      },
+      () => {
+        return Promise.resolve(true);
+      },
+      () => {
+        deleteSessionsCalled = true;
+        sessions = [sessions[1], sessions[3]];
+        return Promise.resolve(true);
+      },
+      () => {
+        findSessionsByShopCalled = true;
+        if (deleteSessionsCalled) {
+          return Promise.resolve([]);
+        } else {
+          return Promise.resolve([sessions[0], sessions[2]]);
+        }
+      },
+    );
+
+    await expect(
+      storage.findSessionsByShop('shop1_sessinons'),
+    ).resolves.toEqual([sessions[0], sessions[2]]);
+    expect(findSessionsByShopCalled).toBe(true);
+
+    await expect(
+      storage.deleteSessions([`${prefix}_1`, `${prefix}_3`]),
+    ).resolves.toBe(true);
+    expect(deleteSessionsCalled).toBe(true);
+    expect(sessions.length).toBe(2);
+    await expect(
+      storage.findSessionsByShop('shop1_sessinons'),
+    ).resolves.toEqual([]);
+  });
+
+  test('missing optional actions generate warnings', async () => {
+    const warnSpy = jest.spyOn(console, 'warn').mockImplementation();
+    const prefix = 'custom_sessions';
+    const session = new Session(`${prefix}_1`, 'shop1-sessions', 'state', true);
+
+    const storage = new CustomSessionStorage(
+      () => {
+        return Promise.resolve(true);
+      },
+      () => {
+        return Promise.resolve(session);
+      },
+      () => {
+        return Promise.resolve(true);
+      },
+    );
+
+    await expect(
+      storage.findSessionsByShop('shop1_sessinons'),
+    ).resolves.toEqual([]);
+    expect(warnSpy).toHaveBeenCalledWith(
+      expect.stringContaining('findSessionsByShopCallback not defined.'),
+    );
+
+    await expect(
+      storage.deleteSessions([`${prefix}_1`, `${prefix}_3`]),
+    ).resolves.toBe(false);
+    expect(warnSpy).toHaveBeenCalledWith(
+      expect.stringContaining('deleteSessionsCallback not defined.'),
+    );
+  });
+
   test('failures and exceptions are raised', () => {
     const sessionId = 'test_session';
     const session = new Session(sessionId, 'shop-url', 'state', true);

diff --git a/src/auth/session/storage/custom.ts b/src/auth/session/storage/custom.ts
@@ -5,28 +5,26 @@ import * as ShopifyErrors from '../../../error';
 
 export class CustomSessionStorage implements SessionStorage {
   constructor(
-    readonly storeSessionCallback: (
-      session: SessionInterface,
-    ) => Promise<boolean>,
-    readonly loadSessionCallback: (
+    readonly storeCallback: (session: SessionInterface) => Promise<boolean>,
+    readonly loadCallback: (
       id: string,
     ) => Promise<SessionInterface | {[key: string]: unknown} | undefined>,
-    readonly deleteSessionCallback: (id: string) => Promise<boolean>,
+    readonly deleteCallback: (id: string) => Promise<boolean>,
     readonly deleteSessionsCallback?: (ids: string[]) => Promise<boolean>,
     readonly findSessionsByShopCallback?: (
       shop: string,
-    ) => Promise<SessionInterface[] | {[key: string]: unknown}[]>,
+    ) => Promise<SessionInterface[]>,
   ) {
-    this.storeSessionCallback = storeSessionCallback;
-    this.loadSessionCallback = loadSessionCallback;
-    this.deleteSessionCallback = deleteSessionCallback;
+    this.storeCallback = storeCallback;
+    this.loadCallback = loadCallback;
+    this.deleteCallback = deleteCallback;
     this.deleteSessionsCallback = deleteSessionsCallback;
     this.findSessionsByShopCallback = findSessionsByShopCallback;
   }
 
   public async storeSession(session: SessionInterface): Promise<boolean> {
     try {
-      return await this.storeSessionCallback(session);
+      return await this.storeCallback(session);
     } catch (error) {
       throw new ShopifyErrors.SessionStorageError(
         `CustomSessionStorage failed to store a session. Error Details: ${error}`,
@@ -37,7 +35,7 @@ export class CustomSessionStorage implements SessionStorage {
   public async loadSession(id: string): Promise<SessionInterface | undefined> {
     let result: SessionInterface | {[key: string]: unknown} | undefined;
     try {
-      result = await this.loadSessionCallback(id);
+      result = await this.loadCallback(id);
     } catch (error) {
       throw new ShopifyErrors.SessionStorageError(
         `CustomSessionStorage failed to load a session. Error Details: ${error}`,
@@ -78,7 +76,7 @@ export class CustomSessionStorage implements SessionStorage {
 
   public async deleteSession(id: string): Promise<boolean> {
     try {
-      return await this.deleteSessionCallback(id);
+      return await this.deleteCallback(id);
     } catch (error) {
       throw new ShopifyErrors.SessionStorageError(
         `CustomSessionStorage failed to delete a session. Error Details: ${error}`,
@@ -103,28 +101,17 @@ export class CustomSessionStorage implements SessionStorage {
     return false;
   }
 
-  public async findSessionsByShop(
-    shop: string,
-  ): Promise<SessionInterface[] | {[key: string]: unknown}[]> {
-    const sessions: SessionInterface[] = [];
+  public async findSessionsByShop(shop: string): Promise<SessionInterface[]> {
+    let sessions: SessionInterface[] = [];
 
     if (this.findSessionsByShopCallback) {
-      let results: SessionInterface[] | {[key: string]: unknown}[] = [];
-
       try {
-        results = await this.findSessionsByShopCallback(shop);
+        sessions = await this.findSessionsByShopCallback(shop);
       } catch (error) {
         throw new ShopifyErrors.SessionStorageError(
           `CustomSessionStorage failed to find sessions by shop. Error Details: ${error}`,
         );
       }
-
-      if (results && results instanceof Array) {
-        // loop through array and convert to SessionInterface
-        results.forEach((element) => {
-          sessions.push(element as SessionInterface);
-        });
-      }
     } else {
       console.warn(
         `CustomSessionStorage failed to find sessions by shop. Error Details: findSessionsByShopCallback not defined.`,

diff --git a/src/auth/session/storage/memory.ts b/src/auth/session/storage/memory.ts
@@ -25,9 +25,7 @@ export class MemorySessionStorage implements SessionStorage {
     return true;
   }
 
-  public async findSessionsByShop(
-    shop: string,
-  ): Promise<SessionInterface[] | {[key: string]: unknown}[]> {
+  public async findSessionsByShop(shop: string): Promise<SessionInterface[]> {
     const results = Object.values(this.sessions).filter(
       (session) => session.shop === shop,
     );

diff --git a/src/auth/session/storage/mongodb.ts b/src/auth/session/storage/mongodb.ts
@@ -80,19 +80,15 @@ export class MongoDBSessionStorage implements SessionStorage {
     return true;
   }
 
-  public async findSessionsByShop(
-    shop: string,
-  ): Promise<SessionInterface[] | {[key: string]: unknown}[]> {
+  public async findSessionsByShop(shop: string): Promise<SessionInterface[]> {
     await this.ready;
 
     const rawResults = await this.collection.find({shop}).toArray();
     if (!rawResults || rawResults?.length === 0) return [];
 
-    const results = rawResults.map((rawResult: any) =>
+    return rawResults.map((rawResult: any) =>
       sessionFromEntries(Object.entries(rawResult)),
     );
-
-    return results ? results : [];
   }
 
   public async disconnect(): Promise<void> {

diff --git a/src/auth/session/storage/mysql.ts b/src/auth/session/storage/mysql.ts
@@ -92,9 +92,7 @@ export class MySQLSessionStorage implements SessionStorage {
     return true;
   }
 
-  public async findSessionsByShop(
-    shop: string,
-  ): Promise<SessionInterface[] | {[key: string]: unknown}[]> {
+  public async findSessionsByShop(shop: string): Promise<SessionInterface[]> {
     await this.ready;
     const query = `
       SELECT * FROM ${this.options.sessionTableName}

diff --git a/src/auth/session/storage/postgresql.ts b/src/auth/session/storage/postgresql.ts
@@ -98,9 +98,7 @@ export class PostgreSQLSessionStorage implements SessionStorage {
     return true;
   }
 
-  public async findSessionsByShop(
-    shop: string,
-  ): Promise<SessionInterface[] | {[key: string]: unknown}[]> {
+  public async findSessionsByShop(shop: string): Promise<SessionInterface[]> {
     await this.ready;
     const query = `
       SELECT * FROM ${this.options.sessionTableName}

diff --git a/src/auth/session/storage/redis.ts b/src/auth/session/storage/redis.ts
@@ -77,9 +77,7 @@ export class RedisSessionStorage implements SessionStorage {
     return true;
   }
 
-  public async findSessionsByShop(
-    shop: string,
-  ): Promise<SessionInterface[] | {[key: string]: unknown}[]> {
+  public async findSessionsByShop(shop: string): Promise<SessionInterface[]> {
     await this.ready;
 
     const keys = await this.client.keys('*');

diff --git a/src/auth/session/storage/sqlite.ts b/src/auth/session/storage/sqlite.ts
@@ -76,9 +76,7 @@ export class SQLiteSessionStorage implements SessionStorage {
     return true;
   }
 
-  public async findSessionsByShop(
-    shop: string,
-  ): Promise<SessionInterface[] | {[key: string]: unknown}[]> {
+  public async findSessionsByShop(shop: string): Promise<SessionInterface[]> {
     await this.ready;
     const query = `
       SELECT * FROM ${this.options.sessionTableName}