fix funnel chart settings crash on null (#45262)

e2e/test/scenarios/visualizations-charts/visualizations-charts-reproductions.cy.spec.ts

+import { SAMPLE_DB_ID } from "e2e/support/cypress_data";
 import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
 import {
   createQuestion,
   modal,
+  getDraggableElements,
+  moveDnDKitElement,
   popover,
   restore,
+  sidebar,
+  visitQuestionAdhoc,
   type StructuredQuestionDetails,
 } from "e2e/support/helpers";
 
@@ -61,3 +66,38 @@ describe("issue 41133", () => {
     });
   });
 });
+
+describe("issue 45255", () => {
+  beforeEach(() => {
+    restore();
+    cy.signInAsAdmin();
+
+    visitQuestionAdhoc({
+      dataset_query: {
+        type: "native",
+        native: {
+          query:
+            "select 'foo' step, 10 v union all select 'baz', 8 union all select null, 6 union all select 'bar', 4",
+          "template-tags": {},
+        },
+        database: SAMPLE_DB_ID,
+      },
+      display: "funnel",
+    });
+  });
+
+  it("should work on native queries with null dimension values (metabase#45255)", () => {
+    cy.findByTestId("viz-settings-button").click();
+
+    // Has (empty) in the settings sidebar
+    sidebar().findByText("(empty)");
+
+    // Can reorder (empty)
+    getDraggableElements().eq(2).should("have.text", "(empty)");
+    moveDnDKitElement(getDraggableElements().first(), { vertical: 100 });
+    getDraggableElements().eq(1).should("have.text", "(empty)");
+
+    // Has (empty) in the chart
+    cy.findByTestId("funnel-chart").findByText("(empty)");
+  });
+});

frontend/src/metabase/lib/constants.js

@@ -4,8 +4,4 @@ export const SEARCH_DEBOUNCE_DURATION = 300;
 
 export const DEFAULT_SEARCH_LIMIT = 50;
 
-// A part of hack required to work with both null and 0
-// values in numeric dimensions
-export const NULL_NUMERIC_VALUE = -Infinity;
-
 export const NULL_DISPLAY_VALUE = t`(empty)`;

frontend/src/metabase/lib/formatting/types.ts

@@ -33,6 +33,7 @@ export interface OptionsType extends TimeOnlyOptions {
   removeYear?: boolean;
   rich?: boolean;
   scale?: number;
+  stringifyNull?: boolean;
   show_mini_bar?: boolean;
   suffix?: string;
   type?: string;

frontend/src/metabase/lib/formatting/value.tsx

@@ -7,7 +7,7 @@ import ReactMarkdown from "react-markdown";
 
 import ExternalLink from "metabase/core/components/ExternalLink";
 import CS from "metabase/css/core/index.css";
-import { NULL_DISPLAY_VALUE, NULL_NUMERIC_VALUE } from "metabase/lib/constants";
+import { NULL_DISPLAY_VALUE } from "metabase/lib/constants";
 import { renderLinkTextForClick } from "metabase/lib/formatting/link";
 import {
   clickBehaviorIsValid,
@@ -144,10 +144,8 @@ export function formatValueRaw(
     return remapped;
   }
 
-  if (value === NULL_NUMERIC_VALUE) {
-    return NULL_DISPLAY_VALUE;
-  } else if (value == null) {
-    return null;
+  if (value == null) {
+    return options.stringifyNull ? NULL_DISPLAY_VALUE : null;
   } else if (
     options.view_as !== "image" &&
     options.click_behavior &&

frontend/src/metabase/visualizations/components/FunnelNormal.jsx

@@ -11,6 +11,7 @@ import {
   formatNumber,
   formatValue,
 } from "metabase/lib/formatting";
+import { formatNullable } from "metabase/lib/formatting/nullable";
 import {
   FunnelNormalRoot,
   FunnelStart,
@@ -53,7 +54,9 @@ export default class FunnelNormal extends Component {
     const sortedRows = settings["funnel.rows"]
       ? settings["funnel.rows"]
           .filter(fr => fr.enabled)
-          .map(fr => rows.find(row => row[dimensionIndex] === fr.key))
+          .map(fr =>
+            rows.find(row => formatNullable(row[dimensionIndex]) === fr.key),
+          )
       : rows;
 
     const isNarrow = gridSize && gridSize.width < 7;
@@ -64,6 +67,7 @@ export default class FunnelNormal extends Component {
       formatValue(dimension, {
         ...settings.column(cols[dimensionIndex]),
         jsx,
+        stringifyNull: true,
         majorWidth: 0,
       });
     const formatMetric = (metric, jsx = true) =>

frontend/src/metabase/visualizations/lib/renderer_utils.js

 import { getIn } from "icepick";
 import _ from "underscore";
 
-import { NULL_NUMERIC_VALUE } from "metabase/lib/constants";
 import { formatNullable } from "metabase/lib/formatting/nullable";
 import { parseTimestamp } from "metabase/lib/time";
 import { datasetContainsNoResults } from "metabase-lib/v1/queries/utils/dataset";
@@ -158,9 +157,3 @@ export const isRemappedToString = series =>
 export const hasClickBehavior = series =>
   getIn(series, [0, "card", "visualization_settings", "click_behavior"]) !=
   null;
-
-// Hack: for numeric dimensions we have to replace null values
-// with anything else since crossfilter groups merge 0 and null
-export function replaceNullValuesForOrdinal(value) {
-  return value === null ? NULL_NUMERIC_VALUE : value;
-}

frontend/src/metabase/visualizations/visualizations/Funnel/Funnel.tsx

@@ -3,6 +3,7 @@ import { t } from "ttag";
 import _ from "underscore";
 
 import CS from "metabase/css/core/index.css";
+import { formatNullable } from "metabase/lib/formatting/nullable";
 import ChartCaption from "metabase/visualizations/components/ChartCaption";
 import { TransformedVisualization } from "metabase/visualizations/components/TransformedVisualization";
 import { ChartSettingOrderedSimple } from "metabase/visualizations/components/settings/ChartSettingOrderedSimple";
@@ -131,7 +132,7 @@ Object.assign(Funnel, {
         const dimension = settings["funnel.dimension"];
 
         const rowsOrder = settings["funnel.rows"];
-        const rowsKeys = rows.map(row => row[dimensionIndex]);
+        const rowsKeys = rows.map(row => formatNullable(row[dimensionIndex]));
 
         const getDefault = (keys: RowValue[]) =>
           keys.map(key => ({

frontend/test/metabase/lib/formatting.unit.spec.js

@@ -226,6 +226,16 @@ describe("formatting", () => {
   });
 
   describe("formatValue", () => {
+    it("should return null on nullish values by default", () => {
+      expect(formatValue(null)).toEqual(null);
+      expect(formatValue(undefined)).toEqual(null);
+    });
+    it("should format null as (empty) when stringifyNull option is true", () => {
+      expect(formatValue(null, { stringifyNull: true })).toEqual("(empty)");
+      expect(formatValue(undefined, { stringifyNull: true })).toEqual(
+        "(empty)",
+      );
+    });
     it("should format numbers with null column", () => {
       expect(formatValue(12345)).toEqual("12345");
     });