Updates based on PR feedback

Shopify · Jul 18, 2022 · 3f81920 · 3f81920
1 parent 194d05b
commit 3f81920
Show file tree

Hide file tree

Showing 11 changed files with 166 additions and 63 deletions.
diff --git a/src/auth/session/session_storage.ts b/src/auth/session/session_storage.ts
@@ -33,13 +33,13 @@ interface SessionStorage {
   deleteSessions?(ids: string[]): Promise<boolean>;
 
   /**
-   * Return an array of sessions for a given shop (or undefined if none found).
+   * Return an array of sessions for a given shop (or [] if none found).
    *
    * @param shop shop of the session(s) to return
    */
   findSessionsByShop?(
     shop: string,
-  ): Promise<SessionInterface[] | {[key: string]: unknown}[] | undefined>;
+  ): Promise<SessionInterface[] | {[key: string]: unknown}[]>;
 }
 
 export {SessionStorage};
diff --git a/src/auth/session/storage/__tests__/battery-of-tests.ts b/src/auth/session/storage/__tests__/battery-of-tests.ts
@@ -4,6 +4,8 @@ import {sessionEqual} from '../../session-utils';
 import {SessionStorage} from '../../session_storage';
 import {SessionInterface} from '../../types';
 
+import {sessionArraysEqual} from './session-test-utils';
+
 export function batteryOfTests(storageFactory: () => Promise<SessionStorage>) {
   it('can store and delete all kinds of sessions', async () => {
     const sessionFactories = [
@@ -121,12 +123,12 @@ export function batteryOfTests(storageFactory: () => Promise<SessionStorage>) {
       expect(shop1Sessions).toBeDefined();
       if (shop1Sessions) {
         expect(shop1Sessions.length).toBe(2);
-        for (const session of shop1Sessions) {
-          expect(
-            sessionEqual(session as SessionInterface, sessions[0]) ||
-              sessionEqual(session as SessionInterface, sessions[2]),
-          ).toBe(true);
-        }
+        expect(
+          sessionArraysEqual(shop1Sessions as SessionInterface[], [
+            sessions[0] as SessionInterface,
+            sessions[2] as SessionInterface,
+          ]),
+        ).toBe(true);
       }
     }
   });
@@ -159,7 +161,7 @@ export function batteryOfTests(storageFactory: () => Promise<SessionStorage>) {
         shop1Sessions = await storage.findSessionsByShop(
           'delete-shop1-sessions',
         );
-        expect(shop1Sessions).toBeUndefined();
+        expect(shop1Sessions).toEqual([]);
       }
     }
   });

diff --git a/src/auth/session/storage/__tests__/session-test-utils.test.ts b/src/auth/session/storage/__tests__/session-test-utils.test.ts
@@ -0,0 +1,88 @@
+import {Session} from '../../session';
+
+import {sessionArraysEqual} from './session-test-utils';
+
+describe('test sessionArraysEqual', () => {
+  it('returns true for two identically ordered arrays', () => {
+    const sessionsExpected = [
+      new Session('test_sessions_1', 'shop1-sessions', 'state', true),
+      new Session('test_sessions_2', 'shop2-sessions', 'state', true),
+      new Session('test_sessions_3', 'shop1-sessions', 'state', true),
+      new Session('test_sessions_4', 'shop3-sessions', 'state', true),
+    ];
+    const sessionsToCompare = [
+      new Session('test_sessions_1', 'shop1-sessions', 'state', true),
+      new Session('test_sessions_2', 'shop2-sessions', 'state', true),
+      new Session('test_sessions_3', 'shop1-sessions', 'state', true),
+      new Session('test_sessions_4', 'shop3-sessions', 'state', true),
+    ];
+
+    expect(sessionArraysEqual(sessionsToCompare, sessionsExpected)).toBe(true);
+  });
+
+  it('returns true for two arrays with same content but out of order', () => {
+    const sessionsExpected = [
+      new Session('test_sessions_1', 'shop1-sessions', 'state', true),
+      new Session('test_sessions_2', 'shop2-sessions', 'state', true),
+      new Session('test_sessions_3', 'shop1-sessions', 'state', true),
+      new Session('test_sessions_4', 'shop3-sessions', 'state', true),
+    ];
+    const sessionsToCompare = [
+      new Session('test_sessions_1', 'shop1-sessions', 'state', true),
+      new Session('test_sessions_3', 'shop1-sessions', 'state', true),
+      new Session('test_sessions_2', 'shop2-sessions', 'state', true),
+      new Session('test_sessions_4', 'shop3-sessions', 'state', true),
+    ];
+
+    expect(sessionArraysEqual(sessionsToCompare, sessionsExpected)).toBe(true);
+  });
+
+  it('returns false for two arrays not the same size', () => {
+    const sessionsExpected = [
+      new Session('test_sessions_1', 'shop1-sessions', 'state', true),
+    ];
+    const sessionsToCompare = [
+      new Session('test_sessions_1', 'shop1-sessions', 'state', true),
+      new Session('test_sessions_3', 'shop1-sessions', 'state', true),
+      new Session('test_sessions_2', 'shop2-sessions', 'state', true),
+      new Session('test_sessions_4', 'shop3-sessions', 'state', true),
+    ];
+
+    expect(sessionArraysEqual(sessionsToCompare, sessionsExpected)).toBe(false);
+  });
+
+  it('returns false for two arrays of the same size but different content', () => {
+    const sessionsExpected = [
+      new Session('test_sessions_1', 'shop1-sessions', 'state', true),
+      new Session('test_sessions_3', 'shop1-sessions', 'state', true),
+      new Session('test_sessions_2', 'shop2-sessions', 'state', true),
+      new Session('test_sessions_4', 'shop3-sessions', 'state', true),
+    ];
+    let sessionsToCompare = [
+      new Session('test_sessions_1', 'shop1-sessions', 'state', true),
+      new Session('test_sessions_3', 'shop1-sessions', 'state', true),
+      new Session('test_sessions_2', 'shop2-sessions', 'state', true),
+      new Session('test_sessions_5', 'shop3-sessions', 'state', true),
+    ];
+
+    expect(sessionArraysEqual(sessionsToCompare, sessionsExpected)).toBe(false);
+
+    sessionsToCompare = [
+      new Session('test_sessions_1', 'shop1-sessions', 'state', true),
+      new Session('test_sessions_3', 'shop1-sessions', 'state', true),
+      new Session('test_sessions_2', 'shop2-sessions', 'state', true),
+      new Session('test_sessions_4', 'shop4-sessions', 'state', true),
+    ];
+
+    expect(sessionArraysEqual(sessionsToCompare, sessionsExpected)).toBe(false);
+
+    sessionsToCompare = [
+      new Session('test_sessions_1', 'shop1-sessions', 'state', true),
+      new Session('test_sessions_3', 'shop1-sessions', 'state', true),
+      new Session('test_sessions_2', 'shop2-sessions', 'state', true),
+      new Session('test_sessions_4', 'shop3-sessions', 'state', false),
+    ];
+
+    expect(sessionArraysEqual(sessionsToCompare, sessionsExpected)).toBe(false);
+  });
+});
diff --git a/src/auth/session/storage/__tests__/session-test-utils.ts b/src/auth/session/storage/__tests__/session-test-utils.ts
@@ -0,0 +1,25 @@
+import {sessionEqual} from '../../session-utils';
+import {SessionInterface} from '../../types';
+
+// compare two arrays of sessions that should contain
+// the same sessions but may be in a different order
+export function sessionArraysEqual(
+  sessionArray1: SessionInterface[],
+  sessionArray2: SessionInterface[],
+): boolean {
+  if (sessionArray1.length !== sessionArray2.length) {
+    return false;
+  }
+
+  for (const session1 of sessionArray1) {
+    let found = false;
+    for (const session2 of sessionArray2) {
+      if (sessionEqual(session1, session2)) {
+        found = true;
+        continue;
+      }
+    }
+    if (!found) return false;
+  }
+  return true;
+}
diff --git a/src/auth/session/storage/custom.ts b/src/auth/session/storage/custom.ts
@@ -12,14 +12,10 @@ export class CustomSessionStorage implements SessionStorage {
       id: string,
     ) => Promise<SessionInterface | {[key: string]: unknown} | undefined>,
     readonly deleteSessionCallback: (id: string) => Promise<boolean>,
-    readonly deleteSessionsCallback?: (
-      id: string[],
-    ) => Promise<boolean> | undefined,
+    readonly deleteSessionsCallback?: (ids: string[]) => Promise<boolean>,
     readonly findSessionsByShopCallback?: (
       shop: string,
-    ) =>
-      | Promise<SessionInterface[] | {[key: string]: unknown}[] | undefined>
-      | undefined,
+    ) => Promise<SessionInterface[] | {[key: string]: unknown}[]>,
   ) {
     this.storeSessionCallback = storeSessionCallback;
     this.loadSessionCallback = loadSessionCallback;
@@ -93,24 +89,27 @@ export class CustomSessionStorage implements SessionStorage {
   public async deleteSessions(ids: string[]): Promise<boolean> {
     if (this.deleteSessionsCallback) {
       try {
-        return (await this.deleteSessionsCallback(ids)) as boolean;
+        return await this.deleteSessionsCallback(ids);
       } catch (error) {
         throw new ShopifyErrors.SessionStorageError(
           `CustomSessionStorage failed to delete array of sessions. Error Details: ${error}`,
         );
       }
     } else {
-      throw new ShopifyErrors.SessionStorageError(
+      console.warn(
         `CustomSessionStorage failed to delete array of sessions. Error Details: deleteSessionsCallback not defined.`,
       );
     }
+    return false;
   }
 
   public async findSessionsByShop(
     shop: string,
-  ): Promise<SessionInterface[] | {[key: string]: unknown}[] | undefined> {
+  ): Promise<SessionInterface[] | {[key: string]: unknown}[]> {
+    const sessions: SessionInterface[] = [];
+
     if (this.findSessionsByShopCallback) {
-      let results: SessionInterface[] | {[key: string]: unknown}[] | undefined;
+      let results: SessionInterface[] | {[key: string]: unknown}[] = [];
 
       try {
         results = await this.findSessionsByShopCallback(shop);
@@ -122,18 +121,15 @@ export class CustomSessionStorage implements SessionStorage {
 
       if (results && results instanceof Array) {
         // loop through array and convert to SessionInterface
-        const sessions: SessionInterface[] = [];
-
         results.forEach((element) => {
           sessions.push(element as SessionInterface);
         });
-        return sessions;
       }
-      return undefined;
     } else {
-      throw new ShopifyErrors.SessionStorageError(
+      console.warn(
         `CustomSessionStorage failed to find sessions by shop. Error Details: findSessionsByShopCallback not defined.`,
       );
     }
+    return sessions;
   }
 }
diff --git a/src/auth/session/storage/memory.ts b/src/auth/session/storage/memory.ts
@@ -27,10 +27,10 @@ export class MemorySessionStorage implements SessionStorage {
 
   public async findSessionsByShop(
     shop: string,
-  ): Promise<SessionInterface[] | {[key: string]: unknown}[] | undefined> {
+  ): Promise<SessionInterface[] | {[key: string]: unknown}[]> {
     const results = Object.values(this.sessions).filter(
       (session) => session.shop === shop,
     );
-    return results.length === 0 ? undefined : results;
+    return results;
   }
 }
diff --git a/src/auth/session/storage/mongodb.ts b/src/auth/session/storage/mongodb.ts
@@ -52,7 +52,7 @@ export class MongoDBSessionStorage implements SessionStorage {
 
     await this.collection.findOneAndReplace(
       {id: session.id},
-      {id: session.id, entries: sessionEntries(session)},
+      Object.fromEntries(sessionEntries(session)),
       {
         upsert: true,
       },
@@ -63,10 +63,9 @@ export class MongoDBSessionStorage implements SessionStorage {
   public async loadSession(id: string): Promise<SessionInterface | undefined> {
     await this.ready;
 
-    const rawResult = await this.collection.findOne({id});
+    const result = await this.collection.findOne({id});
 
-    if (!rawResult) return undefined;
-    return sessionFromEntries(rawResult.entries);
+    return result ? sessionFromEntries(Object.entries(result)) : undefined;
   }
 
   public async deleteSession(id: string): Promise<boolean> {
@@ -77,25 +76,23 @@ export class MongoDBSessionStorage implements SessionStorage {
 
   public async deleteSessions(ids: string[]): Promise<boolean> {
     await this.ready;
-    for (const id of ids) {
-      await this.collection.deleteOne({id});
-    }
+    await this.collection.deleteMany({id: {$in: ids}});
     return true;
   }
 
   public async findSessionsByShop(
     shop: string,
-  ): Promise<SessionInterface[] | {[key: string]: unknown}[] | undefined> {
+  ): Promise<SessionInterface[] | {[key: string]: unknown}[]> {
     await this.ready;
 
-    const rawResults = await this.collection.find().toArray();
-    if (!rawResults || rawResults?.length === 0) return undefined;
+    const rawResults = await this.collection.find({shop}).toArray();
+    if (!rawResults || rawResults?.length === 0) return [];
 
-    const results = rawResults
-      .map((rawResult: any) => sessionFromEntries(rawResult.entries))
-      .filter((session: SessionInterface) => session.shop === shop);
+    const results = rawResults.map((rawResult: any) =>
+      sessionFromEntries(Object.entries(rawResult)),
+    );
 
-    return results.length === 0 ? undefined : results;
+    return results ? results : [];
   }
 
   public async disconnect(): Promise<void> {

diff --git a/src/auth/session/storage/mysql.ts b/src/auth/session/storage/mysql.ts
@@ -94,19 +94,18 @@ export class MySQLSessionStorage implements SessionStorage {
 
   public async findSessionsByShop(
     shop: string,
-  ): Promise<SessionInterface[] | {[key: string]: unknown}[] | undefined> {
+  ): Promise<SessionInterface[] | {[key: string]: unknown}[]> {
     await this.ready;
     const query = `
       SELECT * FROM ${this.options.sessionTableName}
       WHERE shop = ?;
     `;
     const [rows] = await this.query(query, [shop]);
-    if (!Array.isArray(rows) || rows?.length === 0) return undefined;
+    if (!Array.isArray(rows) || rows?.length === 0) return [];
 
-    const results: SessionInterface[] = [];
-    for (const row of rows) {
-      results.push(sessionFromEntries(Object.entries(row as any)));
-    }
+    const results: SessionInterface[] = rows.map((row) => {
+      return sessionFromEntries(Object.entries(row as any));
+    });
     return results;
   }
 

diff --git a/src/auth/session/storage/postgresql.ts b/src/auth/session/storage/postgresql.ts
@@ -100,19 +100,18 @@ export class PostgreSQLSessionStorage implements SessionStorage {
 
   public async findSessionsByShop(
     shop: string,
-  ): Promise<SessionInterface[] | {[key: string]: unknown}[] | undefined> {
+  ): Promise<SessionInterface[] | {[key: string]: unknown}[]> {
     await this.ready;
     const query = `
       SELECT * FROM ${this.options.sessionTableName}
       WHERE shop = $1;
     `;
     const rows = await this.query(query, [shop]);
-    if (!Array.isArray(rows) || rows?.length === 0) return undefined;
+    if (!Array.isArray(rows) || rows?.length === 0) return [];
 
-    const results: SessionInterface[] = [];
-    for (const row of rows) {
-      results.push(sessionFromEntries(Object.entries(row as any)));
-    }
+    const results: SessionInterface[] = rows.map((row) => {
+      return sessionFromEntries(Object.entries(row as any));
+    });
     return results;
   }
 

diff --git a/src/auth/session/storage/redis.ts b/src/auth/session/storage/redis.ts
@@ -73,15 +73,13 @@ export class RedisSessionStorage implements SessionStorage {
 
   public async deleteSessions(ids: string[]): Promise<boolean> {
     await this.ready;
-    for (const id of ids) {
-      await this.deleteSession(id);
-    }
+    await this.client.del(ids.map((id) => this.fullKey(id)));
     return true;
   }
 
   public async findSessionsByShop(
     shop: string,
-  ): Promise<SessionInterface[] | {[key: string]: unknown}[] | undefined> {
+  ): Promise<SessionInterface[] | {[key: string]: unknown}[]> {
     await this.ready;
 
     const keys = await this.client.keys('*');
@@ -94,7 +92,7 @@ export class RedisSessionStorage implements SessionStorage {
       if (session.shop === shop) results.push(session);
     }
 
-    return results.length === 0 ? undefined : results;
+    return results;
   }
 
   public async disconnect(): Promise<void> {