bfs: fix comparing chart configs

Author: jansule

superset-frontend/plugins/plugin-chart-cartodiagram/src/components/ChartLayer.tsx

@@ -179,10 +179,7 @@ export class ChartLayer extends Layer {
 
       const chartComponent = createChartComponent(
         this.chartVizType,
-        // Seems like the registered chart components change some
-        // props in-place, which leads to unwanted side effects. Therefore,
-        // we only pass a copy.
-        JSON.parse(JSON.stringify(feature)),
+        feature,
         chartWidth,
         chartHeight,
         this.theme,
@@ -217,10 +214,7 @@ export class ChartLayer extends Layer {
 
       const chartComponent = createChartComponent(
         this.chartVizType,
-        // Seems like the registered chart components change some
-        // props in-place, which leads to unwanted side effects. Therefore,
-        // we only pass a copy.
-        JSON.parse(JSON.stringify(chart.feature)),
+        chart.feature,
         chartWidth,
         chartHeight,
         this.theme,

superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx

@@ -33,6 +33,7 @@ import {
   MapViewConfigs,
   OlChartMapProps,
 } from '../types';
+import { isChartConfigEqual } from '../util/chartUtil';
 
 /** The name to reference the chart layer */
 const CHART_LAYER_NAME = 'openlayers-chart-layer';
@@ -83,9 +84,7 @@ export const OlChartMap = (props: OlChartMapProps) => {
    */
   useEffect(() => {
     setCurrentChartConfigs(oldCurrentChartConfigs => {
-      if (
-        JSON.stringify(chartConfigs) === JSON.stringify(oldCurrentChartConfigs)
-      ) {
+      if (isChartConfigEqual(chartConfigs, oldCurrentChartConfigs)) {
         return oldCurrentChartConfigs;
       }
       return chartConfigs;
@@ -370,10 +369,7 @@ export const OlChartMap = (props: OlChartMapProps) => {
         chartLayer.setChartVizType(chartVizType, true);
         recreateCharts = true;
       }
-      if (
-        JSON.stringify(currentChartConfigs) !==
-        JSON.stringify(chartLayer.chartConfigs)
-      ) {
+      if (!isChartConfigEqual(currentChartConfigs, chartLayer.chartConfigs)) {
         chartLayer.setChartConfig(currentChartConfigs, true);
         recreateCharts = true;
       }

superset-frontend/plugins/plugin-chart-cartodiagram/src/util/chartUtil.tsx

@@ -19,7 +19,7 @@
 import React from 'react';
 
 import { SupersetTheme } from '@superset-ui/core';
-import { ChartConfigFeature } from '../types';
+import { ChartConfig, ChartConfigFeature } from '../types';
 import ChartWrapper from '../components/ChartWrapper';
 
 /**
@@ -47,3 +47,42 @@ export const createChartComponent = (
     theme={chartTheme}
   />
 );
+
+/**
+ * Simplifies a chart configuration by removing
+ * non-serializable properties.
+ *
+ * @param config The chart configuration to simplify.
+ * @returns The simplified chart configuration.
+ */
+export const simplifyConfig = (config: ChartConfig) => {
+  const simplifiedConfig: ChartConfig = {
+    type: config.type,
+    features: config.features.map(f => ({
+      type: f.type,
+      geometry: f.geometry,
+      properties: Object.keys(f.properties)
+        .filter(k => k !== 'refs')
+        .reduce((prev, cur) => ({ ...prev, [cur]: f.properties[cur] }), {}),
+    })),
+  };
+  return simplifiedConfig;
+};
+
+/**
+ * Check if two chart configurations are equal (deep equality).
+ *
+ * @param configA The first chart config for comparison.
+ * @param configB The second chart config for comparison.
+ * @returns True, if configurations are equal. False otherwise.
+ */
+export const isChartConfigEqual = (
+  configA: ChartConfig,
+  configB: ChartConfig,
+) => {
+  const simplifiedConfigA = simplifyConfig(configA);
+  const simplifiedConfigB = simplifyConfig(configB);
+  return (
+    JSON.stringify(simplifiedConfigA) === JSON.stringify(simplifiedConfigB)
+  );
+};

superset-frontend/plugins/plugin-chart-cartodiagram/test/util/chartUtil.test.ts

@@ -0,0 +1,77 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { ChartConfig } from '../../src/types';
+import { isChartConfigEqual, simplifyConfig } from '../../src/util/chartUtil';
+
+describe('chartUtil', () => {
+  const configA: ChartConfig = {
+    type: 'FeatureCollection',
+    features: [
+      {
+        type: 'Feature',
+        geometry: {
+          type: 'Point',
+          coordinates: [],
+        },
+        properties: {
+          refs: 'foo',
+        },
+      },
+    ],
+  };
+
+  const configB: ChartConfig = {
+    type: 'FeatureCollection',
+    features: [
+      {
+        type: 'Feature',
+        geometry: {
+          type: 'Point',
+          coordinates: [],
+        },
+        properties: {
+          refs: 'foo',
+          foo: 'bar',
+        },
+      },
+    ],
+  };
+
+  describe('simplifyConfig', () => {
+    it('removes the refs property from a feature', () => {
+      const simplifiedConfig = simplifyConfig(configA);
+      const propKeys = Object.keys(simplifiedConfig.features[0].properties);
+
+      expect(propKeys).toHaveLength(0);
+    });
+  });
+
+  describe('isChartConfigEqual', () => {
+    it('returns true, if configurations are equal', () => {
+      const isEqual = isChartConfigEqual(configA, structuredClone(configA));
+      expect(isEqual).toBe(true);
+    });
+
+    it('returns false if configurations are not equal', () => {
+      const isEqual = isChartConfigEqual(configA, configB);
+      expect(isEqual).toBe(false);
+    });
+  });
+});