codeant-ai-for-open-source[bot] commented on code in PR #39760:
URL: https://github.com/apache/superset/pull/39760#discussion_r3613252952
##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/mapUtil.tsx:
##########
@@ -50,3 +57,157 @@ export const fitMapToCharts = (olMap: Map, chartConfigs:
ChartConfig) => {
size: [250, 250],
});
};
+
+const fitToData = (
+ olMap: Map,
+ features: Feature[],
+ padding?: FitOptions['padding'] | undefined,
+) => {
+ const view = olMap.getView();
+ const extent = getExtentFromFeatures(features) || defaultExtent;
+
+ if (padding) {
+ view.fit(extent, { padding });
+ } else {
+ view.fit(extent);
+ }
+};
+
+const extractSridFromWkt = (wkt: string) => {
+ const extract: { geom: string; srid: string | null } = {
+ geom: wkt,
+ srid: null,
+ };
+ if (wkt.startsWith('SRID=')) {
+ // WKT with SRID, strip it
+ const srid = wkt.match(/SRID=(\d+);/);
+ if (srid) {
+ extract.srid = `EPSG:${srid[1]}`;
+ extract.geom = wkt.replace(/SRID=\d+;/, '');
+ }
+ }
+ return extract;
+};
+
+export const wkbToGeoJSON = (wkb: string) => {
+ const format = new WKB();
+ const feature = format.readFeature(wkb, {
+ featureProjection: 'EPSG:3857',
+ });
+ return new GeoJSON().writeFeatureObject(feature, {
+ featureProjection: 'EPSG:3857',
+ });
+};
+
+export const wktToGeoJSON = (wkt: string) => {
+ const format = new WKT();
+ const wktOpts: ReadOptions = {
+ featureProjection: 'EPSG:3857',
+ dataProjection: 'EPSG:4326', // default to WGS84
+ };
+ const extract = extractSridFromWkt(wkt);
+ const cleanedWkt = extract.geom;
+ if (extract.srid) {
+ wktOpts.dataProjection = extract.srid;
+ }
+ const feature = format.readFeature(cleanedWkt, wktOpts);
+ return new GeoJSON().writeFeatureObject(feature, {
+ featureProjection: 'EPSG:3857',
+ });
+};
+
+/**
+ * Create OL Features from data records.
+ *
+ * @param dataRecords The data records to transform.
+ * @param geomColumn The name of the column holding the geodata.
+ * @returns List of OL Features.
+ */
+export const dataRecordsToOlFeatures = (
+ dataRecords: DataRecord[],
+ geomColumn: string,
+ geomFormat: GeometryFormat.WKB | GeometryFormat.WKT,
+) => {
+ let format: WKB | WKT = new WKB();
+
+ if (geomFormat === GeometryFormat.WKT) {
+ format = new WKT();
+ }
+ const features = dataRecords
+ .map(item => {
+ const geom = item[geomColumn];
+ if (typeof geom !== 'string') {
+ return undefined;
+ }
+
+ let cleanedGeom = geom;
+ const opts: ReadOptions = {
+ featureProjection: 'EPSG:3857',
+ };
+ if (geomFormat === GeometryFormat.WKT) {
+ const extract = extractSridFromWkt(geom);
+ cleanedGeom = extract.geom;
+ if (extract.srid) {
+ opts.dataProjection = extract.srid;
+ }
+ }
+ const feature = format.readFeature(cleanedGeom, opts);
+ feature.setProperties({ ...item });
Review Comment:
**Suggestion:** A single malformed WKT/WKB value will throw from
`readFeature` and abort processing of the entire dataset. Wrap parsing per
record and skip invalid geometries so one bad row does not crash map
fitting/rendering. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Cartodiagram map crashes on malformed WKT/WKB record.
- ⚠️ Map extent fitting fails for affected datasets.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In
`superset-frontend/plugins/plugin-chart-cartodiagram/src/util/mapUtil.tsx:189`,
call
the exported function `fitMapToDataRecords(olMap, dataRecords, geomColumn,
geomFormat,
padding)` with `geomFormat` set to `GeometryFormat.WKT` or
`GeometryFormat.WKB`.
2. Ensure `dataRecords` includes at least one record where
`dataRecord[geomColumn]` is a
string that is not valid WKT/WKB (for example, a truncated or otherwise
malformed geometry
string).
3. Inside `fitMapToDataRecords`, at `mapUtil.tsx:196-201`, the function calls
`dataRecordsToOlFeatures(dataRecords, geomColumn, geomFormat)` to convert
each record into
OpenLayers `Feature` instances.
4. In `dataRecordsToOlFeatures` (`mapUtil.tsx:126-161`), when processing the
malformed
record, `format.readFeature(cleanedGeom, opts)` at line 154 throws from the
OpenLayers
WKT/WKB parser; because this exception is not caught, it aborts
`dataRecordsToOlFeatures`,
`fitToData` (`mapUtil.tsx:61-74`), and thus `fitMapToDataRecords`, causing
any caller
using this helper to fail rendering/fitting the cartodiagram map.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1a47b2a8e4f840159f460a321fd9d019&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1a47b2a8e4f840159f460a321fd9d019&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/mapUtil.tsx
**Line:** 154:155
**Comment:**
*Possible Bug: A single malformed WKT/WKB value will throw from
`readFeature` and abort processing of the entire dataset. Wrap parsing per
record and skip invalid geometries so one bad row does not crash map
fitting/rendering.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=6062995249c9b4e64e382e196067055f791f27a2502255170719a8f1d172c25a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=6062995249c9b4e64e382e196067055f791f27a2502255170719a8f1d172c25a&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts:
##########
@@ -230,39 +298,167 @@ export const stripGeomColumnFromLabelMap = (
* @returns query data without geom column.
*/
export const stripGeomColumnFromQueryData = (
- queryData: any,
+ queryData: TimeseriesChartDataResponseResult,
geomColumn: string,
+ geomFormat: GeometryFormat,
) => {
const queryDataClone = {
...structuredClone(queryData),
- ...stripGeomFromColnamesAndTypes(queryData, geomColumn),
+ ...stripGeomFromColnamesAndTypes(queryData, geomColumn, geomFormat),
};
if (queryDataClone.label_map) {
queryDataClone.label_map = stripGeomColumnFromLabelMap(
queryData.label_map,
geomColumn,
+ geomFormat,
);
}
return queryDataClone;
};
+// copy of
+// superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
+export const formatSeriesName = (
+ name: DataRecordValue | undefined,
+ {
+ numberFormatter,
+ timeFormatter,
+ coltype,
+ }: {
+ numberFormatter?: ValueFormatter;
+ timeFormatter?: TimeFormatter;
+ coltype?: GenericDataType;
+ } = {},
+) => {
+ if (name === undefined || name === null) {
+ return NULL_STRING;
+ }
+ if (typeof name === 'boolean' || typeof name === 'bigint') {
+ return name.toString();
+ }
+ if (name instanceof Date || coltype === GenericDataType.Temporal) {
+ const normalizedName =
+ typeof name === 'string' ? normalizeTimestamp(name) : name;
+ const d =
+ normalizedName instanceof Date
+ ? normalizedName
+ : new Date(normalizedName);
+
+ return timeFormatter ? timeFormatter(d) : d.toISOString();
+ }
+ if (typeof name === 'number') {
+ return numberFormatter ? numberFormatter(name) : name.toString();
+ }
+ return name;
+};
+
+// copy of
+// superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
+export const extractGroupbyLabel = ({
+ datum = {},
+ groupby,
+ numberFormatter,
+ timeFormatter,
+ coltypeMapping = {},
+}: {
+ datum?: DataRecord;
+ groupby?: string[] | null;
+ numberFormatter?: NumberFormatter;
+ timeFormatter?: TimeFormatter;
+ coltypeMapping?: Record<string, GenericDataType>;
+}) =>
+ ensureIsArray(groupby)
+ .map(val =>
+ formatSeriesName(datum[val], {
+ numberFormatter,
+ timeFormatter,
+ ...(coltypeMapping[val] && { coltype: coltypeMapping[val] }),
+ }),
+ )
+ .join(', ');
+
+// copy of
+// superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
+export const getColtypesMapping = ({
+ coltypes = [],
+ colnames = [],
+}: Pick<ChartDataResponseResult, 'coltypes' | 'colnames'>): Record<
+ string,
+ GenericDataType
+> =>
+ colnames.reduce(
+ (accumulator, item, index) => ({ ...accumulator, [item]: coltypes[index]
}),
+ {},
+ );
+
+/**
+ * Reserve label colors for the chart.
+ *
+ * We call the CategoricalColorNamespace singleton to reserve
+ * label colors for the chart. This is necessary to ensure that
+ * we do not run into color collisions when rendering multiple
+ * charts in the same cartodiagram.
+ *
+ * TODO This only works in the context of a dashboard,
+ * since only there, the CategoricalColorNamespace singleton is being
used.
+ * In the explore view, label colors cannot be reserved without changing
+ * the overall color handling in superset.
+ *
+ * @param formData The formdata of the underlying chart
+ * @param dataByLocation The data grouped by location
+ * @param strippedQueryData The query data without the geom column
+ * @param sliceId The id of the chart slice
+ */
+export const reserveLabelColors = (
+ formData: Record<string, any>,
+ dataByLocation: LocationConfigMapping,
+ strippedQueryData: any,
+ sliceId: number,
+) => {
+ // Call color singleton to reserve label colors
+ // get needed control values from underlying chart config
+ const { colorScheme = '', groupby = [], dateFormat } = formData;
+ const colorFn = CategoricalColorNamespace.getScale(colorScheme as string);
+ const groupbyLabels = groupby.map(getColumnLabel);
+ Object.keys(dataByLocation).forEach(location => {
Review Comment:
**Suggestion:** `groupby` can be `null` in Superset form data, and this code
calls `.map` on it directly, which will throw at runtime. Normalize `groupby`
with an array guard (for example via `ensureIsArray`) before mapping so color
reservation does not crash chart rendering. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Cartodiagram fails when underlying chart has null groupby.
- ⚠️ Dashboard render may show blank or error state.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render a cartodiagram visualization that references another Superset
chart whose saved
form-data has `groupby` explicitly set to `null` (not `undefined`), which is
passed
through `selectedChart.params` into `getChartConfigs` in
`superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts:451-458`.
2. In `getChartConfigs`, `chartFormData` is created from
`selectedChart.params` and stored
in `baseConfig.formData` (`transformPropsUtil.ts:459-463`), then
`reserveLabelColors(baseConfig.formData, dataByLocation, strippedQueryData,
sliceId)` is
called at `transformPropsUtil.ts:507-512`.
3. Inside `reserveLabelColors` (`transformPropsUtil.ts:412-438`),
destructuring at line
421 assigns `groupby = null` because the default `[]` only applies when the
property is
`undefined`, not `null`.
4. The subsequent line `groupby.map(getColumnLabel)` at
`transformPropsUtil.ts:423`
executes `map` on `null`, throwing `TypeError: groupby.map is not a
function`, which
aborts `reserveLabelColors` and `getChartConfigs`, preventing the
cartodiagram plugin’s
transformProps from completing and resulting in the map failing to render.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cf2dfc7a2c3545599ee0afda7a96ef63&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=cf2dfc7a2c3545599ee0afda7a96ef63&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts
**Line:** 421:423
**Comment:**
*Possible Bug: `groupby` can be `null` in Superset form data, and this
code calls `.map` on it directly, which will throw at runtime. Normalize
`groupby` with an array guard (for example via `ensureIsArray`) before mapping
so color reservation does not crash chart rendering.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=342d37d35c0765747ce98ab71b34c6b180d39546e199b592d6cb70b5a407553e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=342d37d35c0765747ce98ab71b34c6b180d39546e199b592d6cb70b5a407553e&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts:
##########
@@ -95,16 +160,16 @@ export const groupByLocationGenericX = (
return;
}
- const geojsonCols = getGeojsonColumns(labelMap);
+ const geomColumns = getGeomColumns(geomFormat, labelMap);
- if (geojsonCols.length > 1) {
+ if (geomColumns.length > 1) {
// TODO what should we do, if there is more than one geom column?
console.log(
'More than one geometry column detected. Using first found.',
);
}
- const location = labelMap[geojsonCols[0]];
- const filter = geojsonCols.length ? [geojsonCols[0]] : [];
+ const location = labelMap[geomColumns[0]];
+ const filter = geomColumns.length ? [geomColumns[0]] : [];
const leftOverKey = createColumnName(labelMap, filter);
Review Comment:
**Suggestion:** When no geometry column is detected, this indexes `labelMap`
with `undefined`, which creates an invalid location key and can later cascade
into geometry parsing failures. Add an explicit `geomColumns.length === 0`
branch to skip or safely handle records with no detected geometry. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Data grouped under invalid 'undefined' geometry location.
- ⚠️ Downstream geometry parsing fails for such locations.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use a referenced chart whose `label_map` entries for a non-`x_axis` key
contain no
values detectable as geometry by `getGeomColumns(geomFormat, labelMap)` (for
example, a
metric column only), and render a cartodiagram with generic x-axis
configuration so
`groupByLocationGenericX` is used (`transformPropsUtil.ts:140-149`).
2. In `groupByLocationGenericX` (`transformPropsUtil.ts:140-225`), for such
a key `k`,
`labelMap` is taken from `queryData.label_map[k]` at lines 154-155, and
`geomColumns =
getGeomColumns(geomFormat, labelMap)` at line 163 returns an empty array.
3. With `geomColumns.length === 0`, `location = labelMap[geomColumns[0]]` at
line 171
yields `location === undefined`, while `filter` becomes `[]` at line 172 and
`leftOverKey`
is computed from the full `labelMap` at line 173.
4. The code then uses `locations[location]` at lines 175-176, creating a
location key
`'undefined'`; later, in `getChartConfigs` (`transformPropsUtil.ts:480-549`),
`Object.keys(dataByLocation)` is iterated, and each `location` string is
treated as
geometry (parsed via JSON/WKB/WKT in `getChartConfigs` at lines 525-537), so
the
`'undefined'` key leads to invalid geometry parsing and can cause runtime
errors or
missing features in the resulting FeatureCollection.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=40da8fcb931249389b98961e11cdbc09&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=40da8fcb931249389b98961e11cdbc09&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts
**Line:** 171:173
**Comment:**
*Logic Error: When no geometry column is detected, this indexes
`labelMap` with `undefined`, which creates an invalid location key and can
later cascade into geometry parsing failures. Add an explicit
`geomColumns.length === 0` branch to skip or safely handle records with no
detected geometry.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=c0a4ce073249cf0a1aff7e325c92c3a6c32f2dc6cd368c429f400060236b53dc&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=c0a4ce073249cf0a1aff7e325c92c3a6c32f2dc6cd368c429f400060236b53dc&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
