sadpandajoe commented on code in PR #40981: URL: https://github.com/apache/superset/pull/40981#discussion_r3685932549
########## superset/dataset_relationship/api.py: ########## @@ -0,0 +1,755 @@ +# 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. +"""REST API for Dataset Relationships. + +Exposes CRUD endpoints for managing relationships between datasets. +Follows the standard Superset API patterns using ``BaseSupersetModelRestApi`` +and delegates business logic to the Command layer. +""" +from __future__ import annotations + +import logging +from typing import Any + +from flask import request, Response +from flask_appbuilder.api import expose, protect, rison as parse_rison, safe +from flask_appbuilder.models.sqla.interface import SQLAInterface +from marshmallow import ValidationError + +from superset import event_logger, is_feature_enabled +from superset.commands.dataset_relationship.create import ( + CreateDatasetRelationshipCommand, +) +from superset.commands.dataset_relationship.delete import ( + DeleteDatasetRelationshipCommand, +) +from superset.commands.dataset_relationship.exceptions import ( + DatasetRelationshipCreateFailedError, + DatasetRelationshipDeleteFailedError, + DatasetRelationshipForbiddenError, + DatasetRelationshipInvalidError, + DatasetRelationshipNotFoundError, + DatasetRelationshipUpdateFailedError, +) +from superset.commands.dataset_relationship.update import ( + UpdateDatasetRelationshipCommand, +) +from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod +from superset.daos.dataset_relationship import DatasetRelationshipDAO +from superset.dataset_relationship.schemas import ( + DatasetRelationshipGetSchema, + DatasetRelationshipPostSchema, + DatasetRelationshipPutSchema, + get_delete_ids_schema, + openapi_spec_methods_override, +) +from superset.models.dataset_relationships import DatasetRelationship +from superset.views.base_api import ( + BaseSupersetModelRestApi, + requires_json, + statsd_metrics, +) + +DATASET_RELATIONSHIPS_FLAG = "DATASET_RELATIONSHIPS" + +logger = logging.getLogger(__name__) + + +class DatasetRelationshipRestApi(BaseSupersetModelRestApi): + """REST API for managing dataset relationships. + + Provides full CRUD plus a custom endpoint to list relationships + for a specific dataset. Uses ``class_permission_name = "Dataset"`` + so that existing Dataset read/write permissions control access. + + Example requests:: + + # List all relationships + GET /api/v1/dataset_relationship/ + + # Get a specific relationship + GET /api/v1/dataset_relationship/1 + + # Create a new relationship + POST /api/v1/dataset_relationship/ + { + "source_dataset_id": 1, + "target_dataset_id": 2, + "relationship_type": "many_to_one", + "join_type": "LEFT", + "columns": [ + { + "source_column_name": "customer_id", + "target_column_name": "id" + } + ] + } + + # Update a relationship + PUT /api/v1/dataset_relationship/1 + {"is_active": false} + + # Delete a relationship + DELETE /api/v1/dataset_relationship/1 + + # List relationships for a specific dataset + GET /api/v1/dataset_relationship/dataset/42 + """ + + datamodel = SQLAInterface(DatasetRelationship) + resource_name = "dataset_relationship" + allow_browser_login = True + + # Re-use Dataset permissions so no new permission rows are needed. + class_permission_name = "Dataset" + method_permission_name = { + **MODEL_API_RW_METHOD_PERMISSION_MAP, + "get_by_dataset": "read", + } + + include_route_methods = RouteMethod.REST_MODEL_VIEW_CRUD_SET | { + "bulk_delete", + "get_by_dataset", + } Review Comment: `include_route_methods` omits `resolve_values`, so FAB will not register the endpoint that the filter-translation client calls and those requests will receive 404s. Could we include the route and add an API-level registration test? ########## superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx: ########## @@ -439,10 +440,39 @@ const Chart = (props: ChartProps) => { ), ); + const crossDatasetFilters = useCrossDatasetFilters( + props.id, + chart?.form_data?.datasource, + chartConfiguration, + dataMask, + nativeFilters, + allSliceIds, + ); + + const translatedFilters = useMemo( + () => + crossDatasetFilters.length > 0 + ? crossDatasetFilters.map(f => ({ + col: f.targetColumn, + op: 'IN', + val: f.translatedValues, + })) + : undefined, + [crossDatasetFilters], + ); + const formData = useMemo( () => getFormDataWithExtraFilters({ - chart: { id: chart?.id ?? props.id, form_data: chart?.form_data }, // avoid passing the whole chart object + chart: { + id: chart?.id ?? props.id, + form_data: { + ...chart?.form_data, + ...(translatedFilters && { + relationship_translated_filters: translatedFilters, Review Comment: The translation completes asynchronously, but `translatedFilters` is missing from this memo's dependency list. When it updates, `formData` stays cached without `relationship_translated_filters`, so the target chart is not requeried until an unrelated input changes; should the translated result invalidate this memo? ########## superset-frontend/src/dashboard/util/charts/useCrossDatasetFilters.ts: ########## @@ -0,0 +1,155 @@ +/** + * 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 { useEffect, useState, useRef } from 'react'; +import { + FeatureFlag, + isFeatureEnabled, +} from '@superset-ui/core'; +import { + filterTranslationEngine, + FilterValue, + TranslatedFilter, +} from 'src/features/datasets/relationships/filterTranslation'; +import { getAllActiveFilters } from 'src/dashboard/util/activeAllDashboardFilters'; +import { getChartIdAndColumnFromFilterKey } from 'src/dashboard/util/getDashboardFilterKey'; +import type { + ActiveFilters, + ChartConfiguration, + DataMaskStateWithId, + PartialFilters, +} from 'src/dashboard/types'; + +/** + * Determines which dataset a chart belongs to from its datasource key. + * The datasource is formatted as "{datasetId}__{type}". + */ +function getDatasetIdFromDatasource(datasource: unknown): number | null { + if (typeof datasource !== 'string') return null; + const parts = datasource.split('__'); + const id = parseInt(parts[0], 10); + return Number.isNaN(id) ? null : id; +} + +/** + * Hook that resolves cross-database filter translation for dashboard charts. + * + * When cross-dataset filters are enabled, this hook examines the active filters + * on a chart, checks if the filter columns correspond to dataset relationships, + * and for cross-database relationships, translates the filter values. + * + * Returns the translated filters that should be added as extra filters. + */ +export function useCrossDatasetFilters( + chartId: number, + datasource: unknown, + chartConfiguration: ChartConfiguration | null, + dataMask: DataMaskStateWithId, + nativeFilters: PartialFilters, + allSliceIds: number[], +): TranslatedFilter[] { + const [translated, setTranslated] = useState<TranslatedFilter[]>([]); + const lastKeyRef = useRef<string>(''); + const datasetId = getDatasetIdFromDatasource(datasource); + const activeRelationships = + chartConfiguration?.[chartId]?.activeRelationships; + + useEffect(() => { + if ( + !isFeatureEnabled(FeatureFlag.DatasetRelationships) || + !datasetId || + !activeRelationships || + activeRelationships.length === 0 + ) { + setTranslated([]); + return; + } + + const activeFilters: ActiveFilters = getAllActiveFilters({ + chartConfiguration: chartConfiguration ?? {}, + nativeFilters, + dataMask, + allSliceIds, + }); + + // Build the key for dedup — only re-resolve when active filters change + const filterKey = Object.entries(activeFilters) + .filter(([, f]) => f.scope.includes(chartId)) + .map(([id, f]) => `${id}:${JSON.stringify(f.values)}`) + .join('|'); + + if (filterKey === lastKeyRef.current) return; + lastKeyRef.current = filterKey; + + let cancelled = false; + + (async () => { + // Resolve active filters applied to this chart + // ActiveFilters key format: "{sourceChartId}_{column}" + const chartFilters = Object.entries(activeFilters) + .filter(([key]) => { + try { + const { chartId: sourceChartId } = getChartIdAndColumnFromFilterKey(key); + return sourceChartId === chartId; Review Comment: For a filter emitted by chart A and scoped to related chart B, this hook runs for B but discards the filter because its key contains A's source chart id. Could the selection preserve filters scoped to the target chart and pass the actual source dataset into translation? ########## superset-frontend/package.json: ########## @@ -164,52 +161,54 @@ "@visx/scale": "^3.5.0", "@visx/tooltip": "^3.0.0", "@visx/xychart": "^3.5.1", - "ag-grid-community": "35.3.1", - "ag-grid-react": "35.3.1", + "@xyflow/react": "^12.10.2", + "ag-grid-community": "35.2.1", Review Comment: The manifest and lockfile also roll back a broad set of unrelated current dependencies and scripts (including React, Storybook, ESLint, ag-grid, and the translation commands), producing a 37k-line lockfile rewrite unrelated to dataset relationships. Could this be rebased onto the current manifest and retain only the dependency this feature adds? ########## tests/integration_tests/dataset_relationship_api_tests/api_tests.py: ########## @@ -0,0 +1,482 @@ +# 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. +"""Integration tests for the Dataset Relationship REST API. + +Covers all CRUD endpoints, validation rules, authentication and permission +checks, and edge cases (404, 403, 422). +""" +from __future__ import annotations + +import pytest + +from superset.connectors.sqla.models import SqlaTable +from superset.extensions import db, security_manager +from superset.models.dataset_relationships import ( + DatasetRelationship, + DatasetRelationshipColumn, +) +from superset.utils import json +from superset.utils.database import get_example_database, get_main_database +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.constants import ( + ADMIN_USERNAME, + ALPHA_USERNAME, + GAMMA_USERNAME, +) + +API_URL = "api/v1/dataset_relationship/" + + +class TestDatasetRelationshipApi(SupersetTestCase): + """Integration tests for ``DatasetRelationshipRestApi``.""" + + items_to_delete: list[SqlaTable | DatasetRelationship] = [] + + def setUp(self) -> None: + self.items_to_delete = [] + + def tearDown(self) -> None: + for item in reversed(self.items_to_delete): + try: + db.session.delete(item) + db.session.commit() + except Exception: + db.session.rollback() + super().tearDown() + + # ------------------------------------------------------------------ + # Helpers + # ------------------------------------------------------------------ + def _insert_dataset( + self, + table_name: str, + schema: str | None = None, + database=None, + ) -> SqlaTable: + """Create a minimal ``SqlaTable`` for testing.""" + database = database or get_main_database() + admin = db.session.query(security_manager.user_model).filter_by( + username=ADMIN_USERNAME + ).first() + table = SqlaTable( Review Comment: This fixture creates datasets with no `TableColumn` rows, while the happy-path payload maps `customer_id` to `id`; production validation rejects both missing columns, so the create test cannot reach its expected 201. Could the fixture attach those columns so the API happy path exercises the real validation contract? ########## superset/commands/dataset_relationship/create.py: ########## @@ -0,0 +1,237 @@ +# 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. +"""Command for creating a dataset relationship.""" +from __future__ import annotations + +import logging +from functools import partial +from typing import Any + +from flask_appbuilder.models.sqla import Model +from marshmallow import ValidationError + +from superset.commands.base import BaseCommand +from superset.commands.dataset_relationship.exceptions import ( + DatasetRelationshipColumnsValidationError, + DatasetRelationshipCreateFailedError, + DatasetRelationshipExistsValidationError, + DatasetRelationshipInvalidError, + DatasetRelationshipSelfReferenceValidationError, + DatasetRelationshipSourceNotFoundValidationError, + DatasetRelationshipTargetNotFoundValidationError, +) +from superset.connectors.sqla.models import SqlaTable, TableColumn +from superset.daos.dataset import DatasetDAO +from superset.daos.dataset_relationship import DatasetRelationshipDAO +from superset.extensions import db +from superset.models.dataset_relationships import RELATIONSHIP_TYPES, JOIN_TYPES +from superset.utils.decorators import on_error, transaction + +logger = logging.getLogger(__name__) + + +class CreateDatasetRelationshipCommand(BaseCommand): + """Create a new dataset relationship. + + Expected ``data`` keys:: + + { + "source_dataset_id": int, + "target_dataset_id": int, + "relationship_type": str, # optional, default "many_to_one" + "join_type": str, # optional, default "LEFT" + "is_active": bool, # optional, default True + "name": str | None, # optional + "description": str | None, # optional + "columns": [ # at least one required + { + "source_column_name": str, + "target_column_name": str, + "operator": str, # optional, default "=" + "ordinal": int, # optional, default 0 + }, + ], + } + """ + + def __init__(self, data: dict[str, Any]) -> None: + self._properties = data.copy() + + @transaction( + on_error=partial(on_error, reraise=DatasetRelationshipCreateFailedError) + ) + def run(self) -> Model: + """Validate and persist the new relationship. + + Returns: + The created :class:`DatasetRelationship` instance. + + Raises: + DatasetRelationshipInvalidError: On validation failure. + DatasetRelationshipCreateFailedError: On persistence failure. + """ + self.validate() + relationship = DatasetRelationshipDAO.create( + attributes=self._properties, + ) + logger.info( + "Created dataset relationship id=%s (%s -> %s)", + relationship.id, + relationship.source_dataset_id, + relationship.target_dataset_id, + ) + return relationship + + def validate(self) -> None: + """Run business-rule validations before creation. + + Raises: + DatasetRelationshipInvalidError: When one or more validations fail. + """ + exceptions: list[ValidationError] = [] + + source_dataset_id: int | None = self._properties.get("source_dataset_id") + target_dataset_id: int | None = self._properties.get("target_dataset_id") + columns_data: list[dict[str, Any]] | None = self._properties.get("columns") + relationship_type: str = self._properties.get( + "relationship_type", "many_to_one" + ) + join_type: str = self._properties.get("join_type", "LEFT") + + # 1. Self-reference check + if ( + source_dataset_id is not None + and target_dataset_id is not None + and source_dataset_id == target_dataset_id + ): + exceptions.append(DatasetRelationshipSelfReferenceValidationError()) + + # 2. Source dataset must exist + source_dataset = None + if source_dataset_id is not None: + source_dataset = DatasetDAO.find_by_id(source_dataset_id) + if not source_dataset: + exceptions.append( + DatasetRelationshipSourceNotFoundValidationError() + ) + + # 3. Target dataset must exist + target_dataset = None + if target_dataset_id is not None: + target_dataset = DatasetDAO.find_by_id(target_dataset_id) + if not target_dataset: + exceptions.append( + DatasetRelationshipTargetNotFoundValidationError() + ) + + # 4. Uniqueness check + if source_dataset_id is not None and target_dataset_id is not None: + if not DatasetRelationshipDAO.validate_uniqueness( + source_dataset_id, target_dataset_id + ): + exceptions.append(DatasetRelationshipExistsValidationError()) + + # 5. Validate relationship_type + if relationship_type not in RELATIONSHIP_TYPES: + exceptions.append( + ValidationError( + f"Invalid relationship_type '{relationship_type}'. " + f"Must be one of {RELATIONSHIP_TYPES}.", + field_name="relationship_type", + ) + ) + + # 6. Validate join_type + if join_type not in JOIN_TYPES: + exceptions.append( + ValidationError( + f"Invalid join_type '{join_type}'. " + f"Must be one of {JOIN_TYPES}.", + field_name="join_type", + ) + ) + + # 7. Column mappings required + if not columns_data: + exceptions.append( + DatasetRelationshipColumnsValidationError( + "At least one column mapping is required." + ) + ) + else: + # Validate column names exist on respective datasets + for idx, col in enumerate(columns_data): + src_col = col.get("source_column_name", "") + tgt_col = col.get("target_column_name", "") + if not src_col or not src_col.strip(): + exceptions.append( + DatasetRelationshipColumnsValidationError( + f"Column mapping #{idx}: " + "source_column_name is required." + ) + ) + if not tgt_col or not tgt_col.strip(): + exceptions.append( + DatasetRelationshipColumnsValidationError( + f"Column mapping #{idx}: " + "target_column_name is required." + ) + ) + + # Validate columns exist on datasets (if datasets loaded) + if source_dataset and src_col: + source_col_names = { + c.column_name for c in source_dataset.columns + } + if src_col not in source_col_names: + exceptions.append( + DatasetRelationshipColumnsValidationError( + f"Column '{src_col}' not found in source " + f"dataset '{source_dataset.table_name}'." + ) + ) + if target_dataset and tgt_col: + target_col_names = { + c.column_name for c in target_dataset.columns + } + if tgt_col not in target_col_names: + exceptions.append( + DatasetRelationshipColumnsValidationError( + f"Column '{tgt_col}' not found in target " + f"dataset '{target_dataset.table_name}'." + ) + ) + + # 8. Auto-detect cross-database flag + if source_dataset and target_dataset: + self._properties["is_cross_database"] = ( + source_dataset.database_id != target_dataset.database_id + ) + + # 9. Cycle check (D2) + if source_dataset_id is not None and target_dataset_id is not None: + if DatasetRelationshipDAO.would_create_cycle( + source_dataset_id, target_dataset_id + ): + exceptions.append( + DatasetRelationshipCycleValidationError( Review Comment: When this edge closes a cycle, this branch raises `NameError` because `DatasetRelationshipCycleValidationError` is not imported; the update command has the same undefined name. Could we import it in both commands and cover create/update cycle rejection? ########## superset-frontend/src/pages/DatasetList/index.tsx: ########## @@ -1158,6 +1152,14 @@ const DatasetList: FunctionComponent<DatasetListProps> = ({ }); } + if (canCreate && isFeatureEnabled(FeatureFlag.DatasetRelationships)) { + buttonArr.push({ + name: t('Relationships'), + buttonStyle: 'link', + onClick: () => history.push('/dataset/relationships/'), Review Comment: This URL is caught by the existing `/dataset/:datasetId` route because the canvas is registered at `/superset/dataset/relationships/`; clicking Relationships therefore opens the dataset page with `datasetId="relationships"`. Should this push the registered route and get a click/route regression test? ########## superset/commands/explore/get.py: ########## @@ -157,18 +159,32 @@ def run(self) -> Optional[dict[str, Any]]: # noqa: C901 except SQLAlchemyError: message = "SQLAlchemy error" + # Inject dataset relationships into the explore context + if is_feature_enabled("DATASET_RELATIONSHIPS"): + from superset.dataset_relationship.schemas import ( + DatasetRelationshipGetSchema, + ) + try: + relationship_dao = DatasetRelationshipDAO + relationships = relationship_dao.find_by_dataset_id( + self._datasource_id, active_only=True + ) + schema = DatasetRelationshipGetSchema(many=True) + datasource_data["relationships"] = schema.dump(relationships) + except Exception: # pylint: disable=broad-exception-caught + logger.exception( + "Failed to inject relationships for dataset %s", + self._datasource_id, + ) + datasource_data["relationships"] = [] + metadata = None if slc: - extra_owners = [] - if resolver := current_app.config.get("EXTRA_OWNERS_RESOLVER"): - extra_owners = resolver(slc) - metadata = { Review Comment: This change also removes the configured `EXTRA_OWNERS_RESOLVER` result from Explore metadata, so installations using that extension lose their extra ownership information even when this feature flag is off. Could the existing `extra_owners` behavior be preserved? ########## superset/dataset_relationship/api.py: ########## @@ -0,0 +1,755 @@ +# 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. +"""REST API for Dataset Relationships. + +Exposes CRUD endpoints for managing relationships between datasets. +Follows the standard Superset API patterns using ``BaseSupersetModelRestApi`` +and delegates business logic to the Command layer. +""" +from __future__ import annotations + +import logging +from typing import Any + +from flask import request, Response +from flask_appbuilder.api import expose, protect, rison as parse_rison, safe +from flask_appbuilder.models.sqla.interface import SQLAInterface +from marshmallow import ValidationError + +from superset import event_logger, is_feature_enabled +from superset.commands.dataset_relationship.create import ( + CreateDatasetRelationshipCommand, +) +from superset.commands.dataset_relationship.delete import ( + DeleteDatasetRelationshipCommand, +) +from superset.commands.dataset_relationship.exceptions import ( + DatasetRelationshipCreateFailedError, + DatasetRelationshipDeleteFailedError, + DatasetRelationshipForbiddenError, + DatasetRelationshipInvalidError, + DatasetRelationshipNotFoundError, + DatasetRelationshipUpdateFailedError, +) +from superset.commands.dataset_relationship.update import ( + UpdateDatasetRelationshipCommand, +) +from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod +from superset.daos.dataset_relationship import DatasetRelationshipDAO +from superset.dataset_relationship.schemas import ( + DatasetRelationshipGetSchema, + DatasetRelationshipPostSchema, + DatasetRelationshipPutSchema, + get_delete_ids_schema, + openapi_spec_methods_override, +) +from superset.models.dataset_relationships import DatasetRelationship +from superset.views.base_api import ( + BaseSupersetModelRestApi, + requires_json, + statsd_metrics, +) + +DATASET_RELATIONSHIPS_FLAG = "DATASET_RELATIONSHIPS" + +logger = logging.getLogger(__name__) + + +class DatasetRelationshipRestApi(BaseSupersetModelRestApi): + """REST API for managing dataset relationships. + + Provides full CRUD plus a custom endpoint to list relationships + for a specific dataset. Uses ``class_permission_name = "Dataset"`` + so that existing Dataset read/write permissions control access. + + Example requests:: + + # List all relationships + GET /api/v1/dataset_relationship/ + + # Get a specific relationship + GET /api/v1/dataset_relationship/1 + + # Create a new relationship + POST /api/v1/dataset_relationship/ + { + "source_dataset_id": 1, + "target_dataset_id": 2, + "relationship_type": "many_to_one", + "join_type": "LEFT", + "columns": [ + { + "source_column_name": "customer_id", + "target_column_name": "id" + } + ] + } + + # Update a relationship + PUT /api/v1/dataset_relationship/1 + {"is_active": false} + + # Delete a relationship + DELETE /api/v1/dataset_relationship/1 + + # List relationships for a specific dataset + GET /api/v1/dataset_relationship/dataset/42 + """ + + datamodel = SQLAInterface(DatasetRelationship) + resource_name = "dataset_relationship" + allow_browser_login = True + + # Re-use Dataset permissions so no new permission rows are needed. + class_permission_name = "Dataset" + method_permission_name = { + **MODEL_API_RW_METHOD_PERMISSION_MAP, + "get_by_dataset": "read", + } + + include_route_methods = RouteMethod.REST_MODEL_VIEW_CRUD_SET | { + "bulk_delete", + "get_by_dataset", + } + + @staticmethod + def _check_feature_flag() -> Response | None: + """Return a 403 response if the feature flag is disabled.""" + flag_val = is_feature_enabled(DATASET_RELATIONSHIPS_FLAG) + logger.warning( + "_check_feature_flag: flag=%s, constant=%s, result=%s", + flag_val, DATASET_RELATIONSHIPS_FLAG, not flag_val, + ) + if not flag_val: + return Response( + status=403, + response="Dataset Relationships feature is not enabled.", + content_type="text/plain", + ) + return None + + list_columns = [ + "id", + "uuid", + "source_dataset_id", + "target_dataset_id", + "relationship_type", + "join_type", + "is_cross_database", + "is_active", + "name", + "description", + "created_on", + "changed_on", + "created_by_fk", + "changed_by_fk", + ] + show_columns = list_columns + [ + "columns.id", + "columns.source_column_name", + "columns.target_column_name", + "columns.operator", + "columns.ordinal", + ] + add_columns = [ + "source_dataset_id", + "target_dataset_id", + "relationship_type", + "join_type", + "is_active", + "name", + "description", + ] + edit_columns = [ + "source_dataset_id", + "target_dataset_id", + "relationship_type", + "join_type", + "is_active", + "name", + "description", + ] + order_columns = [ + "id", + "source_dataset_id", + "target_dataset_id", + "relationship_type", + "join_type", + "is_active", + "created_on", + "changed_on", + ] + search_columns = [ + "id", + "source_dataset_id", + "target_dataset_id", + "relationship_type", + "join_type", + "is_active", + "is_cross_database", + "name", + ] + + add_model_schema = DatasetRelationshipPostSchema() + edit_model_schema = DatasetRelationshipPutSchema() + + openapi_spec_tag = "Dataset Relationships" + openapi_spec_methods = openapi_spec_methods_override + openapi_spec_component_schemas = ( + DatasetRelationshipGetSchema, + DatasetRelationshipPostSchema, + DatasetRelationshipPutSchema, + ) + + # ------------------------------------------------------------------ # + # GET /api/v1/dataset_relationship/ (list) + # ------------------------------------------------------------------ # + @expose("/", methods=("GET",)) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.get_list", + log_to_statsd=False, + ) + def get_list(self, **kwargs: Any) -> Response: + """Get a list of dataset relationships. + --- + get: + summary: Get a list of dataset relationships + description: >- + Gets a list of dataset relationships. Use Rison or JSON + query parameters for filtering, sorting, pagination and + for selecting specific columns and metadata. + responses: + 200: + description: List of relationships + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 500: + $ref: '#/components/responses/500' + """ + flag_response = self._check_feature_flag() + if flag_response: + return flag_response + return super().get_list(**kwargs) + + # ------------------------------------------------------------------ # + # GET /api/v1/dataset_relationship/<pk> (info) + # ------------------------------------------------------------------ # + @expose("/info", methods=("GET",)) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.info", + log_to_statsd=False, + ) + def info(self) -> Response: + """Get metadata info. + --- + get: + summary: Get API metadata + responses: + 200: + description: API metadata + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 500: + $ref: '#/components/responses/500' + """ + flag_response = self._check_feature_flag() + if flag_response: + return flag_response + return super().info() + + # ------------------------------------------------------------------ # + # POST /api/v1/dataset_relationship/ + # ------------------------------------------------------------------ # + @expose("/", methods=("POST",)) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.post", + log_to_statsd=False, + ) + @requires_json + def post(self) -> Response: + """Create a new dataset relationship. + --- + post: + summary: Create a new dataset relationship + description: >- + Creates a directed relationship between two datasets, including + column-pair mappings that define the join condition. + requestBody: + description: Dataset relationship schema + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/DatasetRelationshipPostSchema' + responses: + 201: + description: Relationship created + content: + application/json: + schema: + type: object + properties: + id: + type: integer + description: ID of the newly created relationship + result: + $ref: '#/components/schemas/DatasetRelationshipPostSchema' + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + flag_response = self._check_feature_flag() + if flag_response: + return flag_response + try: + item = self.add_model_schema.load(request.json) + except ValidationError as error: + return self.response_400(message=error.messages) + + try: + new_model = CreateDatasetRelationshipCommand(item).run() + return self.response(201, id=new_model.id, result=item) + except DatasetRelationshipInvalidError as ex: + return self.response_422(message=ex.normalized_messages()) + except DatasetRelationshipCreateFailedError as ex: + logger.error( + "Error creating model %s: %s", + self.__class__.__name__, + str(ex), + exc_info=True, + ) + return self.response_422(message=str(ex)) + + # ------------------------------------------------------------------ # + # PUT /api/v1/dataset_relationship/<pk> + # ------------------------------------------------------------------ # + @expose("/<pk>", methods=("PUT",)) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.put", + log_to_statsd=False, + ) + @requires_json + def put(self, pk: int) -> Response: + """Update a dataset relationship. + --- + put: + summary: Update a dataset relationship + description: >- + Partially updates an existing dataset relationship. Only provided + fields are modified; omitted fields retain their current values. + If ``columns`` is provided the full set of column mappings is + replaced. + parameters: + - in: path + schema: + type: integer + name: pk + required: true + description: Relationship ID + requestBody: + description: Dataset relationship schema + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/DatasetRelationshipPutSchema' + responses: + 200: + description: Relationship updated + content: + application/json: + schema: + type: object + properties: + id: + type: integer + result: + $ref: '#/components/schemas/DatasetRelationshipPutSchema' + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + flag_response = self._check_feature_flag() + if flag_response: + return flag_response + try: + item = self.edit_model_schema.load(request.json) + except ValidationError as error: + return self.response_400(message=error.messages) + + try: + updated_model = UpdateDatasetRelationshipCommand(pk, item).run() + return self.response(200, id=updated_model.id, result=item) + except DatasetRelationshipNotFoundError: + return self.response_404() + except DatasetRelationshipForbiddenError: + return self.response_403() + except DatasetRelationshipInvalidError as ex: + return self.response_422(message=ex.normalized_messages()) + except DatasetRelationshipUpdateFailedError as ex: + logger.error( + "Error updating model %s: %s", + self.__class__.__name__, + str(ex), + exc_info=True, + ) + return self.response_422(message=str(ex)) + + # ------------------------------------------------------------------ # + # DELETE /api/v1/dataset_relationship/<pk> + # ------------------------------------------------------------------ # + @expose("/<pk>", methods=("DELETE",)) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.delete", + log_to_statsd=False, + ) + def delete(self, pk: int) -> Response: + """Delete a dataset relationship. + --- + delete: + summary: Delete a dataset relationship + parameters: + - in: path + schema: + type: integer + name: pk + required: true + description: Relationship ID + responses: + 200: + description: Relationship deleted + content: + application/json: + schema: + type: object + properties: + message: + type: string + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + flag_response = self._check_feature_flag() + if flag_response: + return flag_response + try: + DeleteDatasetRelationshipCommand([pk]).run() + return self.response(200, message="OK") + except DatasetRelationshipNotFoundError: + return self.response_404() + except DatasetRelationshipForbiddenError: + return self.response_403() + except DatasetRelationshipDeleteFailedError as ex: + logger.error( + "Error deleting model %s: %s", + self.__class__.__name__, + str(ex), + exc_info=True, + ) + return self.response_422(message=str(ex)) + + # ------------------------------------------------------------------ # + # DELETE /api/v1/dataset_relationship/ (bulk) + # ------------------------------------------------------------------ # + @expose("/", methods=("DELETE",)) + @protect() + @safe + @statsd_metrics + @parse_rison(get_delete_ids_schema) + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.bulk_delete", + log_to_statsd=False, + ) + def bulk_delete(self, **kwargs: Any) -> Response: + """Bulk delete dataset relationships. + --- + delete: + summary: Bulk delete dataset relationships + parameters: + - in: query + name: q + content: + application/json: + schema: + $ref: '#/components/schemas/get_delete_ids_schema' + responses: + 200: + description: Relationships deleted + content: + application/json: + schema: + type: object + properties: + message: + type: string + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + item_ids = kwargs["rison"] + flag_response = self._check_feature_flag() + if flag_response: + return flag_response + try: + DeleteDatasetRelationshipCommand(item_ids).run() + return self.response( + 200, + message=f"Deleted {len(item_ids)} dataset relationship(s)", + ) + except DatasetRelationshipNotFoundError: + return self.response_404() + except DatasetRelationshipForbiddenError: + return self.response_403() + except DatasetRelationshipDeleteFailedError as ex: + logger.error( + "Error bulk-deleting models %s: %s", + self.__class__.__name__, + str(ex), + exc_info=True, + ) + return self.response_422(message=str(ex)) + + # ------------------------------------------------------------------ # + # GET /api/v1/dataset_relationship/dataset/<dataset_id> + # ------------------------------------------------------------------ # + @expose("/dataset/<int:dataset_id>", methods=("GET",)) + @protect(allow_browser_login=True) + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: ( + f"{self.__class__.__name__}.get_by_dataset" + ), + log_to_statsd=False, + ) + def get_by_dataset(self, dataset_id: int) -> Response: + """Get all relationships for a specific dataset. + --- + get: + summary: Get relationships for a dataset + description: >- + Returns all relationships where the given dataset appears as + either source or target. Accepts an optional ``active_only`` + query parameter (defaults to ``true``). + parameters: + - in: path + schema: + type: integer + name: dataset_id + required: true + description: Dataset ID to look up relationships for + - in: query + schema: + type: boolean + name: active_only + description: >- + If true (default), only active relationships are returned. + responses: + 200: + description: List of relationships for the dataset + content: + application/json: + schema: + type: object + properties: + count: + type: integer + description: Total number of matching relationships + result: + type: array + items: + $ref: '#/components/schemas/DatasetRelationshipGetSchema' + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 500: + $ref: '#/components/responses/500' + """ + active_only_str = request.args.get("active_only", "true") + flag_response = self._check_feature_flag() + if flag_response: + return flag_response + active_only = active_only_str.lower() not in ("false", "0", "no") + + relationships = DatasetRelationshipDAO.find_by_dataset_id( + dataset_id, active_only=active_only + ) + result_schema = DatasetRelationshipGetSchema(many=True) + result = result_schema.dump(relationships) + return self.response(200, count=len(result), result=result) + + # ------------------------------------------------------------------ # + # POST /api/v1/dataset_relationship/resolve_values/ (cross-DB filter) + # ------------------------------------------------------------------ # + @expose("/resolve_values/", methods=("POST",)) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: ( + f"{self.__class__.__name__}.resolve_values" + ), + log_to_statsd=False, + ) + @requires_json + def resolve_values(self) -> Response: + """Resolve cross-DB filter value mapping. + --- + post: + summary: Resolve cross-database filter values + description: >- + Given source values from one dataset, returns the corresponding + target values from a related dataset via the relationship's + column mapping. Used for cross-database filter translation. + requestBody: + required: true + content: + application/json: + schema: + type: object + properties: + source_dataset_id: + type: integer + target_dataset_id: + type: integer + source_column: + type: string + target_column: + type: string + source_values: + type: array + responses: + 200: + description: Mapped values + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + flag_response = self._check_feature_flag() + if flag_response: + return flag_response + + data = request.json or {} + source_dataset_id = data.get("source_dataset_id") + target_dataset_id = data.get("target_dataset_id") + source_column = data.get("source_column") + target_column = data.get("target_column") + source_values = data.get("source_values", []) + + if not all([source_dataset_id, target_dataset_id, source_column, target_column]): + return self.response_400( + message="source_dataset_id, target_dataset_id, source_column, and target_column are required." + ) + + try: + from superset import db + from superset.connectors.sqla.models import SqlaTable + + # Query the target dataset for distinct values that match + target_table = ( + db.session.query(SqlaTable) + .filter(SqlaTable.id == target_dataset_id) + .one_or_none() + ) + + if not target_table: + return self.response_404() + + # Get the target table's select expression + target_sqla_table = target_table.get_sqla_table() + + # Validate column exists in target dataset's schema + valid_columns = {c.name for c in target_sqla_table.c} + if target_column not in valid_columns: + return self.response_400( + message=f"Column '{target_column}' not found in target dataset." + ) + target_col = target_sqla_table.c[target_column] + + # Query distinct target values + query = ( + db.session.query(target_col.distinct()) + .select_from(target_sqla_table) + ) + + # If we have source values and the datasets are linked, + # we need to join with the source table to filter + # For now, return all distinct values from the target column + # A more sophisticated implementation would use the relationship + # to join and filter + results = [row[0] for row in query.limit(10000).all()] Review Comment: This returns distinct values from any requested target dataset/column without checking datasource access or a declared relationship, and `source_values` is ignored entirely. Could this enforce access to both datasets and resolve only the validated mapping for the supplied source values? ########## superset/views/core.py: ########## @@ -864,11 +864,12 @@ def dashboard_permalink( ) if url_params := state.get("urlParams"): for param_key, param_val in url_params: - # URL-encode every param value (including native_filters) so a - # value containing '&'/'#'/'=' cannot inject extra parameters - # into the redirect target. Flask decodes the value back on read. - params = parse.urlencode([(param_key, param_val)]) - url = f"{url}&{params}" + if param_key == "native_filters": + # native_filters doesnt need to be encoded here + url = f"{url}&native_filters={param_val}" Review Comment: Concatenating `native_filters` verbatim reintroduces query-string injection: a value containing `&`, `#`, or `=` can add or truncate redirect parameters, which the replaced `urlencode` path prevented. Can this keep encoding the value like the other URL parameters? -- 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]
