codeant-ai-for-open-source[bot] commented on code in PR #43303: URL: https://github.com/apache/superset/pull/43303#discussion_r3994501830
########## superset/commands/chart/query_context_builder.py: ########## @@ -0,0 +1,186 @@ +# 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. +""" +Derive a ``query_context`` payload from a chart's stored ``params``. + +Charts imported via a v1 ZIP bundle persist with ``Slice.query_context = NULL`` +(the importer never synthesizes one), so the first +``GET /api/v1/chart/{pk}/data/`` returns HTTP 400 "Chart has no query context +saved" (Apache Superset #33615). This module builds a valid ``query_context`` +payload from the chart's viz ``params`` + its importer-resolved datasource, so +the imported row becomes queryable on first read — or classifies the chart +non-derivable and returns ``None`` (honest-fail; never a fabricated context). + +The function is **pure** (no DB / network / analytical-DB access). Its output, +when passed to ``QueryContextFactory.create(**payload)``, constructs a valid +``QueryContext``. See ADR-013 (synthesize-at-import) and ADR-014 +(datasource authz/RLS preservation). +""" + +from __future__ import annotations + +from typing import Any + +# Datasource-less viz types render static/markdown content rather than a +# datasource-backed query; a NULL query_context is the *correct* state for them +# (FR-003), not a bug. Kept as a small, explicit, conservatively-expanded set. +_NON_DATASOURCE_VIZ: frozenset[str] = frozenset({"markup", "handlebars", "divider"}) + +# Default row limit mirrors the query-object default used across the read path. +_DEFAULT_ROW_LIMIT = 5000 + + +def _translate_adhoc_filters( + adhoc_filters: list[Any] | None, +) -> tuple[list[dict[str, Any]], list[str], list[str]]: + """ + Translate viz ``adhoc_filters`` into simple ``{col, op, val}`` filters. + + - ``expressionType == "SIMPLE"`` → a ``{col, op, val}`` filter. + - ``expressionType == "SQL"`` → routed to ``extras.where`` (or + ``extras.having`` for a HAVING clause) so a hand-written SQL predicate is + preserved rather than lost. + - Anything that cannot be mapped cleanly (missing subject/operator, malformed + entry) is dropped **without raising** (RISK-T05) — an imported chart must + never abort its bundle over a single unmappable filter. + + Returns ``(filters, where_expressions, having_expressions)``. + """ + filters: list[dict[str, Any]] = [] + where_expressions: list[str] = [] + having_expressions: list[str] = [] + for adhoc_filter in adhoc_filters or []: + if not isinstance(adhoc_filter, dict): + continue + expression_type = adhoc_filter.get("expressionType") or "SIMPLE" + if expression_type == "SIMPLE": + subject = adhoc_filter.get("subject") + operator = adhoc_filter.get("operator") + if subject and operator: + filters.append( + { + "col": subject, + "op": operator, + "val": adhoc_filter.get("comparator"), + } + ) + # else: unmappable SIMPLE filter — dropped, no crash (RISK-T05). + elif expression_type == "SQL": + sql_expression = adhoc_filter.get("sqlExpression") + if sql_expression: + clause = (adhoc_filter.get("clause") or "WHERE").upper() + if clause == "HAVING": + having_expressions.append(sql_expression) + else: + where_expressions.append(sql_expression) + return filters, where_expressions, having_expressions + + +def _derive_orderby(params: dict[str, Any]) -> list[list[Any]]: + """ + Best-effort ordering from ``params`` (FR-002). + + Handles an explicit ``orderby`` list (either ``[[col, asc_bool], ...]`` or a + flat list of expressions) and a single sort metric + (``timeseries_limit_metric`` / ``sort_by_metric``). Falls back to no + ordering when nothing is derivable — the query builder supplies defaults. + """ + order_asc = not params.get("order_desc", True) + + orderby = params.get("orderby") + if isinstance(orderby, list) and orderby: + normalized: list[list[Any]] = [] + for entry in orderby: + if isinstance(entry, (list, tuple)) and len(entry) == 2: + normalized.append([entry[0], bool(entry[1])]) + else: + normalized.append([entry, order_asc]) + return normalized + + sort_metric = params.get("timeseries_limit_metric") or params.get( + "sort_by_metric" + ) + if sort_metric: + return [[sort_metric, order_asc]] + return [] + + +def build_query_context_config( + params: dict[str, Any] | None, + viz_type: str, + datasource_id: int | None, + datasource_type: str = "table", +) -> dict[str, Any] | None: + """ + Map a chart's ``params`` + resolved datasource to a ``query_context`` payload. + + :param params: the chart's viz form-data (a dict at ``import_chart`` time). + :param viz_type: the chart's viz type (classification input). + :param datasource_id: the importer-resolved datasource id. **Datasource is + taken from this argument only, never from ``params`` (ADR-014 / RISK-T02)** + so the synthesized context names the same real datasource the authz layer + vets at read time. + :param datasource_type: the datasource type — ``"table"`` on import. + :returns: a ``query_context`` payload dict whose keys are the kwargs of + ``QueryContextFactory.create``, or ``None`` when the chart is + non-derivable (FR-003): no datasource, nothing to query, or a + datasource-less viz type. Never returns a fabricated/invalid context. + """ + params = params or {} + + # FR-003 non-derivable classification — return None, importer leaves NULL. + if not datasource_id or viz_type in _NON_DATASOURCE_VIZ: + return None + metrics = params.get("metrics") or [] + # `groupby` is the deprecated alias of `columns`. + columns = params.get("columns") or params.get("groupby") or [] + if not metrics and not columns: + return None + + filters, where_expressions, having_expressions = _translate_adhoc_filters( + params.get("adhoc_filters", []) + ) + + query_object = { + "time_range": params.get("time_range", " : "), + "granularity": params.get("granularity_sqla") or params.get("granularity"), + "filters": filters, + "extras": { + "time_grain_sqla": params.get("time_grain_sqla"), + "having": " AND ".join(having_expressions), + "where": " AND ".join(where_expressions), + }, + "applied_time_extras": {}, + "columns": columns, + "metrics": metrics, + "orderby": _derive_orderby(params), + "annotation_layers": [], + "row_limit": params.get("row_limit", _DEFAULT_ROW_LIMIT), Review Comment: Yes, this is a valid issue. The fallback should match `QueryObjectFactory` rather than introduce a separate 5,000-row limit. Use the runtime Flask configuration: ```python from flask import current_app row_limit = params.get("row_limit") if row_limit is None: row_limit = current_app.config["ROW_LIMIT"] ``` Then assign: ```python "row_limit": row_limit, ``` This preserves an explicitly saved chart limit while making imported charts without one use the deployment’s configured `ROW_LIMIT` (50,000 by default). Alternatively, omitting `row_limit` entirely would allow the downstream factory to apply its default, but using the runtime value keeps the synthesized payload explicit and consistent. ########## superset-frontend/scripts/gen-qc-registry.mjs: ########## @@ -0,0 +1,187 @@ +// Deterministic, re-runnable codegen: maps every plugin `buildQuery` module to the +// viz_type key(s) it is registered under, and emits registry.generated.ts consumed +// by entry.ts. Join: buildQuery module <- (index.ts that imports it) -> plugin class +// name -> MainPreset `.configure({ key: VizType.X })` -> VizType enum string. +// A single builder legitimately maps to several keys (e.g. echarts_timeseries + _line/_bar/...). +import { readFileSync, writeFileSync, readdirSync, statSync, existsSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const FE = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); +const PLUGINS = path.join(FE, 'plugins'); +const OUT = path.join(FE, 'src', 'backend-querycontext', 'registry.generated.ts'); + +// --- 1. VizType enum: EnumName -> 'string_value' --- +const vizTypeSrc = readFileSync( + path.join(FE, 'packages/superset-ui-core/src/chart/types/VizType.ts'), + 'utf8', +); +const VIZ_ENUM = {}; +for (const m of vizTypeSrc.matchAll(/(\w+)\s*=\s*['"]([\w-]+)['"]/g)) VIZ_ENUM[m[1]] = m[2]; + +// --- 2. MainPreset: ClassName -> viz string --- +const mainPreset = readFileSync( + path.join(FE, 'src/visualizations/presets/MainPreset.js'), + 'utf8', +); +// MainPreset renames on import (e.g. `import { PivotTableChartPlugin as +// PivotTableChartPluginV2 } from '...'`), then `new PivotTableChartPluginV2()`. Map +// each local `new X()` name back to the ORIGINAL package export name the codegen sees. +const importOrig = {}; // localName -> package-export name +for (const im of mainPreset.matchAll(/import\s*\{([^}]*)\}\s*from\s*['"][^'"]+['"]/g)) { + for (let spec of im[1].split(',')) { + spec = spec.trim(); + if (!spec) continue; + const as = spec.match(/^(\w+)\s+as\s+(\w+)$/); + if (as) importOrig[as[2]] = as[1]; + else if (/^\w+$/.test(spec)) importOrig[spec] = spec; + } +} +const CLASS_TO_VIZ = {}; +for (const m of mainPreset.matchAll( + /new\s+(\w+)\s*\(\s*\)\s*\.configure\(\s*\{\s*key:\s*VizType\.(\w+)/g, Review Comment: Yes, this is a valid gap. The current regex only matches `new Plugin().configure(...)`, so `CartodiagramPlugin` is excluded from `CLASS_TO_VIZ` and its `buildQuery` is never bundled. Please update the registration discovery to allow constructor arguments, preferably without trying to fully parse JavaScript with a fragile regex. For the current registrations, a minimal fix is: ```javascript for (const m of mainPreset.matchAll( /new\s+(\w+)\s*\([^)]*\)\s*\.configure\(\s*\{\s*key:\s*VizType\.(\w+)/g, )) { const [, local, enumName] = m; if (!VIZ_ENUM[enumName]) continue; CLASS_TO_VIZ[importOrig[local] ?? local] = VIZ_ENUM[enumName]; CLASS_TO_VIZ[local] = VIZ_ENUM[enumName]; } ``` However, a small scanner or TypeScript AST-based lookup would be safer if constructor options can contain nested parentheses. The generated-registry check should also assert that Cartodiagram’s build-query module is mapped, rather than merely reporting it under `unmapped`. Please add a Cartodiagram fixture containing the relevant nested `selected_chart` form data and generate its golden through the same frontend `buildQuery` path. The fixture should verify that: - `cartodiagram` appears in `VIZ_TYPES`; - generation does not return `__unsupported__` or `__error__`; - the resulting query contains the expected metric, columns, and datasource. This preserves frontend parity and avoids relying on the generic fallback for Cartodiagram’s nested query intent. No other comments need to be addressed for this specific thread unless you want to fix the remaining review items as well. ########## superset/commands/chart/query_context_builder.py: ########## @@ -0,0 +1,186 @@ +# 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. +""" +Derive a ``query_context`` payload from a chart's stored ``params``. + +Charts imported via a v1 ZIP bundle persist with ``Slice.query_context = NULL`` +(the importer never synthesizes one), so the first +``GET /api/v1/chart/{pk}/data/`` returns HTTP 400 "Chart has no query context +saved" (Apache Superset #33615). This module builds a valid ``query_context`` +payload from the chart's viz ``params`` + its importer-resolved datasource, so +the imported row becomes queryable on first read — or classifies the chart +non-derivable and returns ``None`` (honest-fail; never a fabricated context). + +The function is **pure** (no DB / network / analytical-DB access). Its output, +when passed to ``QueryContextFactory.create(**payload)``, constructs a valid +``QueryContext``. See ADR-013 (synthesize-at-import) and ADR-014 +(datasource authz/RLS preservation). +""" + +from __future__ import annotations + +from typing import Any + +# Datasource-less viz types render static/markdown content rather than a +# datasource-backed query; a NULL query_context is the *correct* state for them +# (FR-003), not a bug. Kept as a small, explicit, conservatively-expanded set. +_NON_DATASOURCE_VIZ: frozenset[str] = frozenset({"markup", "handlebars", "divider"}) + +# Default row limit mirrors the query-object default used across the read path. +_DEFAULT_ROW_LIMIT = 5000 + + +def _translate_adhoc_filters( + adhoc_filters: list[Any] | None, +) -> tuple[list[dict[str, Any]], list[str], list[str]]: + """ + Translate viz ``adhoc_filters`` into simple ``{col, op, val}`` filters. + + - ``expressionType == "SIMPLE"`` → a ``{col, op, val}`` filter. + - ``expressionType == "SQL"`` → routed to ``extras.where`` (or + ``extras.having`` for a HAVING clause) so a hand-written SQL predicate is + preserved rather than lost. + - Anything that cannot be mapped cleanly (missing subject/operator, malformed + entry) is dropped **without raising** (RISK-T05) — an imported chart must + never abort its bundle over a single unmappable filter. + + Returns ``(filters, where_expressions, having_expressions)``. + """ + filters: list[dict[str, Any]] = [] + where_expressions: list[str] = [] + having_expressions: list[str] = [] + for adhoc_filter in adhoc_filters or []: + if not isinstance(adhoc_filter, dict): + continue + expression_type = adhoc_filter.get("expressionType") or "SIMPLE" + if expression_type == "SIMPLE": + subject = adhoc_filter.get("subject") + operator = adhoc_filter.get("operator") + if subject and operator: + filters.append( + { + "col": subject, + "op": operator, + "val": adhoc_filter.get("comparator"), + } + ) + # else: unmappable SIMPLE filter — dropped, no crash (RISK-T05). + elif expression_type == "SQL": + sql_expression = adhoc_filter.get("sqlExpression") + if sql_expression: + clause = (adhoc_filter.get("clause") or "WHERE").upper() + if clause == "HAVING": + having_expressions.append(sql_expression) + else: + where_expressions.append(sql_expression) Review Comment: Agreed. The fallback should be conservative: if any `adhoc_filter` cannot be translated, it must return `None` so the chart remains non-derivable rather than persisting a broader query. A minimal fix is to validate the filters before calling the shared splitter: ```python def _translate_adhoc_filters( adhoc_filters: list[Any] | None, ) -> tuple[list[dict[str, Any]], str, str] | None: sanitized: list[dict[str, Any]] = [] for filter_ in adhoc_filters or []: if not isinstance(filter_, dict): return None expression_type = filter_.get("expressionType") if expression_type == "SIMPLE": if not filter_.get("subject") or not filter_.get("operator"): return None elif expression_type == "SQL": if not isinstance(filter_.get("sqlExpression"), str): return None else: return None sanitized.append(filter_) form_data: dict[str, Any] = {"adhoc_filters": sanitized} split_adhoc_filters_into_base_filters(form_data) return ( form_data.get("filters") or [], form_data.get("where") or "", form_data.get("having") or "", ) ``` Then propagate the result in `build_query_context_config`: ```python translated = _translate_adhoc_filters(params.get("adhoc_filters", [])) if translated is None: return None filters, where, having = translated ``` The existing tests expecting malformed filters to be silently dropped should be updated to assert `None`; add coverage for unknown expression types and malformed `SIMPLE`/`SQL` filters. The V8 path remains authoritative, while the Python fallback now fails honestly whenever it cannot preserve the chart’s filters. ########## superset/commands/chart/importers/v1/__init__.py: ########## @@ -103,7 +112,16 @@ def _import( "datasource_name": dataset.table_name, } config = update_chart_config_dataset(config, dataset_dict) + # Capture the pre-import context state before `import_chart` + # mutates `config` (it synthesizes into config["query_context"]). + had_query_context = bool(config.get("query_context")) chart = import_chart(config, overwrite=overwrite) + if had_query_context: + n_preserved += 1 + elif chart.query_context: + n_queryable += 1 + else: + n_non_derivable += 1 Review Comment: Correct. The summary should only count synthesis performed during this invocation, not merely observe that the returned `Slice` has a context. A minimal fix is to have `import_chart` return import metadata alongside the chart, for example: ```python @dataclass class ChartImportResult: chart: Slice query_context_synthesized: bool = False query_context_preserved: bool = False unchanged: bool = False ``` Then the importer can count the explicit outcome: ```python result = import_chart( config, overwrite=overwrite, default_viewers=default_viewers ) if result.query_context_synthesized: n_queryable += 1 elif result.query_context_preserved: n_preserved += 1 elif result.unchanged: # Do not count an existing chart returned by a non-overwrite import. pass else: n_non_derivable += 1 ``` If changing the public return type is undesirable, the same metadata can be returned through an internal helper or captured before the early return. The important part is that `n_queryable` is incremented only inside the actual synthesis path, not inferred from `chart.query_context` after `import_chart` returns. This keeps re-import summaries accurate. -- 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]
