alex-poor commented on code in PR #39906:
URL: https://github.com/apache/superset/pull/39906#discussion_r3199827588
##########
superset-frontend/plugins/preset-chart-deckgl/src/layers/Geojson/transformProps.ts:
##########
@@ -32,14 +32,25 @@ export default function transformProps(chartProps:
ChartProps) {
}
const records = getRecordsFromQuery(chartProps.queriesData);
+ const crossFilterCol = formData.cross_filter_column || undefined;
// Parse each record's geojson column value (replicates backend
DeckGeoJson.get_properties)
const features = records
.map((record: DataRecord) => {
const geojsonStr = record[geojsonCol];
if (geojsonStr == null) return null;
try {
- return JSON.parse(String(geojsonStr));
+ const feature = JSON.parse(String(geojsonStr));
+ // Surface cross_filter_column from the row onto feature.properties so
+ // that picking can emit a dimension filter even when the GeoJSON blob
+ // doesn't carry the column itself.
+ if (crossFilterCol && record[crossFilterCol] !== undefined) {
+ feature.properties = {
+ ...feature.properties,
+ [crossFilterCol]: record[crossFilterCol],
+ };
Review Comment:
Computed property names in object literals (`{[k]: v}`) do not trigger
prototype mutation, even when `k === '__proto__'`. The ECMAScript spec only
carves out the *literal* `__proto__: value` form to mutate the prototype;
computed keys go through CreateDataPropertyOrThrow and always create own
properties. The spread (`...feature.properties`) is also safe — spread only
copies own enumerable properties.
Quick sanity check:
```js
const k = '__proto__';
const y = {a: 1, [k]: 'EVIL'};
Object.getPrototypeOf(y) === Object.prototype // true, unchanged
y.hasOwnProperty('__proto__') // true, just an own property
```
Additionally, `crossFilterCol` is set by an authenticated chart owner via
the column-picker control which only offers real dataset column names —
exploiting this would require the dataset to have a column literally named
`__proto__`. Not actionable.
##########
superset-frontend/plugins/preset-chart-deckgl/src/layers/Geojson/buildQuery.ts:
##########
@@ -61,6 +63,10 @@ export default function buildQuery(formData:
DeckGeoJsonFormData) {
const withJsColumns = addJsColumnsToColumns(columnStrings, js_columns);
columns = withJsColumns as QueryFormColumn[];
+ if (cross_filter_column && !columns.includes(cross_filter_column)) {
+ columns.push(cross_filter_column);
+ }
Review Comment:
The `as QueryFormColumn[]` cast on the line above is type-system fiction.
`addJsColumnsToColumns` returns `string[]`, so at runtime `columns` is a plain
string array at the point of the `.includes` check — the comparison works
correctly. Tooltip columns are added *after* this check (line 71), so the
`QueryFormColumn` object form doesn't appear in `columns` until after my push.
This also matches the convention in `Polygon/buildQuery.ts`. Not changing.
--
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]
