codeant-ai-for-open-source[bot] commented on code in PR #40804: URL: https://github.com/apache/superset/pull/40804#discussion_r3364231814
########## superset-frontend/packages/superset-ui-core/src/utils/mapStyles.ts: ########## @@ -0,0 +1,180 @@ +/** + * 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. + */ + +export type MapProvider = 'maplibre' | 'mapbox'; + +export type MapStyleChoice = { + value: string; + label: string; + attribution?: string; +}; + +export type MapRendererOption = { + value: MapProvider; + label: string; + disabled?: boolean; +}; + +export type RasterTileMapStyle = { + version: 8; + sources: { + [sourceId: string]: { + type: 'raster'; + tiles: string[]; + tileSize: 256; + attribution: string; + }; + }; + layers: [ + { + id: string; + type: 'raster'; + source: string; + minzoom: 0; + maxzoom: 22; + }, + ]; +}; + +export type ResolvedMapStyle = string | RasterTileMapStyle; + +export const OSM_TILE_STYLE_URL = + 'https://tile.openstreetmap.org/{z}/{x}/{y}.png'; +export const OSM_TILE_ATTRIBUTION = '© OpenStreetMap contributors'; +export const OSM_TILE_STYLE_CHOICE: MapStyleChoice = { + value: OSM_TILE_STYLE_URL, + label: 'Streets (OSM)', + attribution: OSM_TILE_ATTRIBUTION, +}; + +export const MAPLIBRE_RENDERER_OPTION: MapRendererOption = { + value: 'maplibre', + label: 'MapLibre (open-source)', +}; +export const MAPBOX_RENDERER_OPTION: MapRendererOption = { + value: 'mapbox', + label: 'Mapbox (API key required)', +}; +export const DISABLED_MAPBOX_RENDERER_OPTION: MapRendererOption = { + ...MAPBOX_RENDERER_OPTION, + label: 'Mapbox (MAPBOX_API_KEY required)', + disabled: true, +}; + +const TILE_PROTOCOL = 'tile://'; +const RASTER_SOURCE_ID = 'osm-raster-tiles'; +const RASTER_LAYER_ID = 'osm-raster-layer'; + +type BootstrapData = { + common?: { + conf?: { + MAPBOX_API_KEY?: unknown; + }; + }; +}; + +export function getBootstrapDataFromDocument(): unknown { + if (typeof document === 'undefined') { + return undefined; + } + + try { + const appContainer = document.getElementById('app'); + const dataBootstrap = appContainer?.getAttribute('data-bootstrap'); + return dataBootstrap ? JSON.parse(dataBootstrap) : undefined; + } catch { + return undefined; + } +} + +export function getMapboxApiKeyFromBootstrap( + bootstrapData: unknown = getBootstrapDataFromDocument(), +): string { + const mapboxApiKey = (bootstrapData as BootstrapData | undefined)?.common + ?.conf?.MAPBOX_API_KEY; + return typeof mapboxApiKey === 'string' ? mapboxApiKey : ''; +} + +export function hasMapboxApiKey( + bootstrapData: unknown = getBootstrapDataFromDocument(), +): boolean { + return getMapboxApiKeyFromBootstrap(bootstrapData).trim().length > 0; +} + +export function getMapRendererOptions({ + hasMapboxKey, +}: { + hasMapboxKey: boolean; + currentValue?: MapProvider; +}): MapRendererOption[] { + return [ + MAPLIBRE_RENDERER_OPTION, + hasMapboxKey ? MAPBOX_RENDERER_OPTION : DISABLED_MAPBOX_RENDERER_OPTION, + ]; +} + +function unwrapTileProtocol(value: string): string { + return value.startsWith(TILE_PROTOCOL) + ? value.slice(TILE_PROTOCOL.length) + : value; +} + +export function isRasterTileTemplate(value: unknown): value is string { + if (typeof value !== 'string') { + return false; + } + const tileUrl = unwrapTileProtocol(value); + return ['{z}', '{x}', '{y}'].every(templateParam => + tileUrl.includes(templateParam), + ); +} + +export function buildRasterTileMapStyle(value: string): RasterTileMapStyle { + return { + version: 8, + sources: { + [RASTER_SOURCE_ID]: { + type: 'raster', + tiles: [unwrapTileProtocol(value)], + tileSize: 256, + attribution: OSM_TILE_ATTRIBUTION, Review Comment: **Suggestion:** `buildRasterTileMapStyle` hardcodes OpenStreetMap attribution for every raster tile template, including user-provided free-form URLs. This will display incorrect attribution text for non-OSM providers and can violate tile-provider attribution requirements. Pass attribution as an input (or only apply OSM attribution when the URL is the known OSM template) instead of forcing `OSM_TILE_ATTRIBUTION` for all raster styles. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Non-OSM raster tiles show incorrect OpenStreetMap attribution. - ⚠️ Violates custom tile provider attribution requirements. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Create or migrate a Deck GL-based map chart so that its form data uses the MapLibre renderer with a raster tile template, for example by saving chart params where `maplibre_style` is a `tile://` URL like `tile://https://tiles.example.com/{z}/{x}/{y}.png` (params structure and `maplibre_style` field are exercised in `tests/integration_tests/migrations/ce6bd21901ab_migrate_deckgl_and_mapbox__test.py:101-117`). 2. When the chart is rendered, the Deck GL factory at `superset-frontend/plugins/preset-chart-deckgl/src/factory.tsx:194-201` passes `formData.maplibre_style` into `DeckGLContainerStyledWrapper` as the `mapStyle` prop and sets `mapProvider` to `'maplibre'`. 3. Inside `DeckGLContainer` at `superset-frontend/plugins/preset-chart-deckgl/src/DeckGLContainer.tsx:11-15`, because `mapProvider` is `'maplibre'`, it computes `mapStyle` by calling `resolveMapStyle(props.mapStyle, DEFAULT_MAP_STYLE)`, where `props.mapStyle` is the user-supplied tile template URL. 4. `resolveMapStyle` in `superset-frontend/packages/superset-ui-core/src/utils/mapStyles.ts:52-60` calls `isRasterTileTemplate(value)`; `isRasterTileTemplate` only checks for `{z}`, `{x}`, `{y}` placeholders after unwrapping the `tile://` protocol (lines 19-27), so any `tile://http.../{z}/{x}/{y}.png` URL (including non-OSM providers) is treated as a raster template. 5. For such a raster template, `resolveMapStyle` calls `buildRasterTileMapStyle(value)` at `mapStyles.ts:29-49`, which constructs a MapLibre style object whose source attribution is hardcoded to `OSM_TILE_ATTRIBUTION` (`'© OpenStreetMap contributors'`) at `mapStyles.ts:37`, regardless of the actual tile host. 6. The resulting style object is passed into the MapLibre map component in `DeckGLContainer` (`DeckGLContainer.tsx:50-67`), so the rendered map displays an OpenStreetMap attribution string even when the tiles come from a non-OSM provider, leading to incorrect and potentially non-compliant attribution for user-configured tile servers. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=39bed20fe4a94aa7898184773e89fb46&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=39bed20fe4a94aa7898184773e89fb46&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/packages/superset-ui-core/src/utils/mapStyles.ts **Line:** 156:156 **Comment:** *Logic Error: `buildRasterTileMapStyle` hardcodes OpenStreetMap attribution for every raster tile template, including user-provided free-form URLs. This will display incorrect attribution text for non-OSM providers and can violate tile-provider attribution requirements. Pass attribution as an input (or only apply OSM attribution when the URL is the known OSM template) instead of forcing `OSM_TILE_ATTRIBUTION` for all raster styles. 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%2F40804&comment_hash=659d6a4f15deada96d7bf45a063654cdbd4a24911b526256e053c8a89d0e6341&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40804&comment_hash=659d6a4f15deada96d7bf45a063654cdbd4a24911b526256e053c8a89d0e6341&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]
