From f77f5862fba1d6878d405ac4cc9e4aff236511fe Mon Sep 17 00:00:00 2001
From: Ryan Laurie <30528226+iethree@users.noreply.github.com>
Date: Mon, 1 May 2023 16:35:07 -0600
Subject: [PATCH] Don't crash with null collection search result (#30378)

* don't crash with null collection search result

* add test coverage
---
 frontend/src/metabase/collections/utils.ts    |  8 +-
 .../metabase/collections/utils.unit.spec.js   | 15 ----
 .../metabase/collections/utils.unit.spec.ts   | 73 +++++++++++++++++++
 3 files changed, 79 insertions(+), 17 deletions(-)
 delete mode 100644 frontend/src/metabase/collections/utils.unit.spec.js
 create mode 100644 frontend/src/metabase/collections/utils.unit.spec.ts

diff --git a/frontend/src/metabase/collections/utils.ts b/frontend/src/metabase/collections/utils.ts
index 41d6ffa6ca6..b11b2323d13 100644
--- a/frontend/src/metabase/collections/utils.ts
+++ b/frontend/src/metabase/collections/utils.ts
@@ -56,7 +56,7 @@ export function isPersonalCollectionChild(
 }
 
 export function isRootCollection(collection: Pick<Collection, "id">): boolean {
-  return canonicalCollectionId(collection.id) === null;
+  return canonicalCollectionId(collection?.id) === null;
 }
 
 export function isItemPinned(item: CollectionItem) {
@@ -121,7 +121,11 @@ export function coerceCollectionId(
 export function canonicalCollectionId(
   collectionId: string | number | null | undefined,
 ): number | null {
-  if (collectionId === "root" || collectionId == null) {
+  if (
+    collectionId === "root" ||
+    collectionId === null ||
+    collectionId === undefined
+  ) {
     return null;
   } else if (typeof collectionId === "number") {
     return collectionId;
diff --git a/frontend/src/metabase/collections/utils.unit.spec.js b/frontend/src/metabase/collections/utils.unit.spec.js
deleted file mode 100644
index f6fc72f8769..00000000000
--- a/frontend/src/metabase/collections/utils.unit.spec.js
+++ /dev/null
@@ -1,15 +0,0 @@
-import { isPersonalCollection } from "metabase/collections/utils";
-
-describe("isPersonalCollection", () => {
-  it("returns true if personal_owner_id is a number", () => {
-    const collection = { personal_owner_id: 1 };
-
-    expect(isPersonalCollection(collection)).toBe(true);
-  });
-
-  it("returns false if personal_owner_id is not a number", () => {
-    const collection = {};
-
-    expect(isPersonalCollection(collection)).toBe(false);
-  });
-});
diff --git a/frontend/src/metabase/collections/utils.unit.spec.ts b/frontend/src/metabase/collections/utils.unit.spec.ts
new file mode 100644
index 00000000000..6c1d216d87d
--- /dev/null
+++ b/frontend/src/metabase/collections/utils.unit.spec.ts
@@ -0,0 +1,73 @@
+import {
+  isPersonalCollection,
+  canonicalCollectionId,
+  isRootCollection,
+} from "metabase/collections/utils";
+
+import { createMockCollection } from "metabase-types/api/mocks";
+
+describe("Collections > utils", () => {
+  describe("isPersonalCollection", () => {
+    it("returns true if personal_owner_id is a number", () => {
+      const collection = { personal_owner_id: 1 };
+
+      expect(isPersonalCollection(collection)).toBe(true);
+    });
+
+    it("returns false if personal_owner_id is not a number", () => {
+      const collection = {};
+
+      expect(isPersonalCollection(collection)).toBe(false);
+    });
+  });
+
+  describe("canonicalCollectionId", () => {
+    it("returns the id of the collection if it is not a root collection", () => {
+      expect(canonicalCollectionId(1337)).toBe(1337);
+      expect(canonicalCollectionId(1)).toBe(1);
+    });
+    it("handles string id inputs", () => {
+      expect(canonicalCollectionId("1337")).toBe(1337);
+      expect(canonicalCollectionId("1")).toBe(1);
+    });
+    it('returns null if the collection id is "root"', () => {
+      expect(canonicalCollectionId("root")).toBe(null);
+    });
+
+    it("returns null if the collection id is null or undefined", () => {
+      expect(canonicalCollectionId(null)).toBe(null);
+      /* @ts-expect-error checking if a race condition not returning expected data behaves as expected */
+      expect(canonicalCollectionId()).toBe(null);
+    });
+  });
+
+  describe("isRootCollection", () => {
+    it("returns true if the collection is the root collection", () => {
+      /* @ts-expect-error the null id should have been coerced to 'root' but lets make sure this still doesn't blow up */
+      expect(isRootCollection(createMockCollection({ id: null }))).toBe(true);
+      expect(isRootCollection(createMockCollection({ id: "root" }))).toBe(true);
+      expect(isRootCollection(createMockCollection({ id: undefined }))).toBe(
+        true,
+      );
+    });
+
+    it("returns true if there is no collection", () => {
+      /* @ts-expect-error checking if a race condition not returning expected data behaves as expected */
+      expect(isRootCollection({})).toBe(true);
+      /* @ts-expect-error checking if a race condition not returning expected data behaves as expected */
+      expect(isRootCollection(null)).toBe(true);
+      /* @ts-expect-error checking if a race condition not returning expected data behaves as expected */
+      expect(isRootCollection(undefined)).toBe(true);
+    });
+
+    it("returns false if the collection is not the root collection", () => {
+      expect(isRootCollection(createMockCollection({ id: 1 }))).toBe(false);
+      /* @ts-expect-error unclear why ids are sometimes strings, but they are */
+      expect(isRootCollection(createMockCollection({ id: "1" }))).toBe(false);
+      /* @ts-expect-error unclear why ids are sometimes strings, but they are */
+      expect(isRootCollection(createMockCollection({ id: "foobar" }))).toBe(
+        false,
+      );
+    });
+  });
+});
-- 
GitLab