Fix pie charts crashing on the string 'null' (#83432)

* give the string 'null' a color

* add color fallback for safety

frontend/src/metabase/viz-core/shared/settings/pie.ts

@@ -1,5 +1,6 @@
 import _ from "underscore";
 
+import { color } from "metabase/ui/colors";
 import { getColorsForValues } from "metabase/ui/colors/charts";
 import {
   NULL_DIMENSION_KEY,
@@ -196,27 +197,37 @@ export function getColors(
   }
 
   // historically we used "null" rather than NULL_DIMENSION_KEY in `getColorsForValues`
-  // to avoid changing existing charts, we'll convert NULL_DIMENSION_KEY to "null"
+  // to avoid changing existing charts, we'll convert NULL_DIMENSION_KEY to "null".
+  // When the string "null" is itself a dimension value the conversion can't be
+  // reversed, so we skip it (metabase#81868)
+  const useLegacyNullKey = !dimensionValues.includes("null");
+  const toColorKey = (key: string) =>
+    useLegacyNullKey && key === NULL_DIMENSION_KEY ? "null" : key;
+  const fromColorKey = (key: string) =>
+    useLegacyNullKey && key === "null" ? NULL_DIMENSION_KEY : key;
+
   const colors = getColorsForValues(
-    dimensionValues.map((value) =>
-      value === NULL_DIMENSION_KEY ? "null" : value,
-    ),
+    dimensionValues.map(toColorKey),
     Object.fromEntries(
       Object.entries(existingColorMapping).map(([key, value]) => [
-        key === NULL_DIMENSION_KEY ? "null" : key,
+        toColorKey(key),
         value,
       ]),
     ),
   );
-  // then flip it back
   return Object.fromEntries(
     Object.entries(colors).map(([key, value]) => [
-      key === "null" ? NULL_DIMENSION_KEY : key,
+      fromColorKey(key),
       getHexColor(value),
     ]),
   );
 }
 
+function getColor(colors: Record<string, string>, key: string) {
+  // fallback in case a key isn't present so the viz doesn't crash (metabase#81868)
+  return colors[key] ?? getHexColor(color("text-secondary"));
+}
+
 export function getPieRows(
   rawSeries: RawSeries,
   settings: ComputedVisualizationSettings,
@@ -311,7 +322,7 @@ export function getPieRows(
     newPieRows = sortedCurrentDataRows.map((dataRow) => {
       const dimensionValue = dataRow[dimensionDesc.index];
       const key = getKeyFromDimensionValue(dimensionValue);
-      const color = colors[key];
+      const color = getColor(colors, key);
 
       const savedRow = keyToSavedPieRow.get(key);
       if (savedRow != null) {
@@ -348,7 +359,7 @@ export function getPieRows(
       }
       const newRow = { ...savedPieRow, hidden: false };
       if (savedPieRow.defaultColor) {
-        newRow.color = colors[keptKey];
+        newRow.color = getColor(colors, keptKey);
       }
       return newRow;
     });
@@ -368,7 +379,7 @@ export function getPieRows(
         const dimensionValue = addedDataRow[dimensionDesc.index];
 
         const key = getKeyFromDimensionValue(dimensionValue);
-        const color = colors[key];
+        const color = getColor(colors, key);
         const displayValue = getDisplayValue(dimensionValue);
         const name = formatDimensionValue(displayValue);
 

frontend/src/metabase/viz-core/shared/settings/pie.unit.spec.ts

@@ -1,13 +1,16 @@
 import { addLocale, useLocale } from "ttag";
 
 import { color } from "metabase/ui/colors";
+import { getColorsForValues } from "metabase/ui/colors/charts";
 import { NULL_DIMENSION_KEY } from "metabase/utils/constants";
-import type { PieRow, RawSeries } from "metabase-types/api";
+import type { PieRow, RawSeries, RowValues } from "metabase-types/api";
 import {
   createMockColumn,
   createMockDatasetData,
 } from "metabase-types/api/mocks";
 
+import { getHexColor } from "../../lib/color";
+
 import { getAggregatedRows, getColors, getKeyFromDimensionValue } from "./pie";
 
 describe("getKeyFromDimensionValue", () => {
@@ -208,6 +211,69 @@ describe("getColors", () => {
 
     expect(getColors(rawSeries, settings).Doohickey).toBe(color("accent3"));
   });
+
+  describe("null dimension values (metabase#81868)", () => {
+    const getRawSeries = (rows: RowValues[]) =>
+      // getColors only reads rows and cols off the series
+      [
+        {
+          data: createMockDatasetData({
+            rows,
+            cols: [
+              createMockColumn({ name: "Category" }),
+              createMockColumn({ name: "Count" }),
+            ],
+          }),
+        },
+      ] as RawSeries;
+    const settings = {
+      "pie.metric": "Count",
+      "pie.dimension": "Category",
+    };
+
+    it("should keep the default color a null slice has always had", () => {
+      const rawSeries = getRawSeries([
+        ["Doohickey", 2],
+        [null, 1],
+      ]);
+
+      expect(getColors(rawSeries, settings)[NULL_DIMENSION_KEY]).toBe(
+        getHexColor(getColorsForValues(["Doohickey", "null"]).null),
+      );
+    });
+
+    it("should give a color to the string 'null'", () => {
+      const rawSeries = getRawSeries([
+        ["Doohickey", 2],
+        ["null", 1],
+      ]);
+
+      expect(getColors(rawSeries, settings)).toEqual({
+        Doohickey: expect.stringMatching(/^#[0-9a-f]{6}$/i),
+        null: expect.stringMatching(/^#[0-9a-f]{6}$/i),
+      });
+    });
+
+    it("should give separate colors to null and the string 'null'", () => {
+      const rawSeries = getRawSeries([
+        ["null", 2],
+        [null, 1],
+      ]);
+      const result = getColors(rawSeries, {
+        ...settings,
+        // getColors only reads the key, color and defaultColor of a row
+        "pie.rows": [
+          { key: "null", defaultColor: false, color: "#FF0000" },
+          { key: NULL_DIMENSION_KEY, defaultColor: false, color: "#0000FF" },
+        ] as PieRow[],
+      });
+
+      expect(result).toEqual({
+        null: "#FF0000",
+        [NULL_DIMENSION_KEY]: "#0000FF",
+      });
+    });
+  });
 });
 
 describe("with a non-English locale", () => {