Merge branch 'master' into fix/dashboard-create-chart-button-spacing

Co-authored-by: Amin Ghadersohi <amin.ghadersohi@gmail.com>

.github/workflows/check-python-deps.yml

@@ -8,6 +8,10 @@ on:
   pull_request:
     types: [synchronize, opened, reopened, ready_for_review]
 
+permissions:
+  contents: read
+  pull-requests: read
+
 # cancel previous workflow jobs for PRs
 concurrency:
   group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }}

.github/workflows/docker.yml

@@ -9,6 +9,10 @@ on:
     branches:
       - "master"
 
+permissions:
+  contents: read
+  pull-requests: read
+
 concurrency:
   group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }}
   cancel-in-progress: true

.github/workflows/embedded-sdk-release.yml

@@ -6,6 +6,9 @@ on:
       - "master"
       - "[0-9].[0-9]*"
 
+permissions:
+  contents: read
+
 jobs:
   config:
     runs-on: ubuntu-24.04

.github/workflows/embedded-sdk-test.yml

@@ -6,6 +6,9 @@ on:
       - "superset-embedded-sdk/**"
     types: [synchronize, opened, reopened, ready_for_review]
 
+permissions:
+  contents: read
+
 # cancel previous workflow jobs for PRs
 concurrency:
   group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }}

.github/workflows/generate-FOSSA-report.yml

@@ -6,6 +6,9 @@ on:
       - "master"
       - "[0-9].[0-9]*"
 
+permissions:
+  contents: read
+
 jobs:
   config:
     runs-on: ubuntu-24.04

.github/workflows/github-action-validator.yml

@@ -8,6 +8,9 @@ on:
   pull_request:
     types: [synchronize, opened, reopened, ready_for_review]
 
+permissions:
+  contents: read
+
 jobs:
 
   validate-all-ghas:

.github/workflows/license-check.yml

@@ -4,6 +4,9 @@ on:
   pull_request:
     types: [synchronize, opened, reopened, ready_for_review]
 
+permissions:
+  contents: read
+
 # cancel previous workflow jobs for PRs
 concurrency:
   group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }}

.github/workflows/superset-app-cli.yml

@@ -8,6 +8,10 @@ on:
   pull_request:
     types: [synchronize, opened, reopened, ready_for_review]
 
+permissions:
+  contents: read
+  pull-requests: read
+
 # cancel previous workflow jobs for PRs
 concurrency:
   group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }}

.github/workflows/superset-extensions-cli.yml

@@ -8,6 +8,10 @@ on:
   pull_request:
     types: [synchronize, opened, reopened, ready_for_review]
 
+permissions:
+  contents: read
+  pull-requests: read
+
 # cancel previous workflow jobs for PRs
 concurrency:
   group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }}

.github/workflows/superset-helm-lint.yml

@@ -6,6 +6,9 @@ on:
     paths:
       - "helm/**"
 
+permissions:
+  contents: read
+
 # cancel previous workflow jobs for PRs
 concurrency:
   group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }}

.github/workflows/superset-translations.yml

@@ -8,6 +8,10 @@ on:
   pull_request:
     types: [synchronize, opened, reopened, ready_for_review]
 
+permissions:
+  contents: read
+  pull-requests: read
+
 # cancel previous workflow jobs for PRs
 concurrency:
   group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }}

.github/workflows/superset-websocket.yml

@@ -11,6 +11,9 @@ on:
       - "superset-websocket/**"
     types: [synchronize, opened, reopened, ready_for_review]
 
+permissions:
+  contents: read
+
 # cancel previous workflow jobs for PRs
 concurrency:
   group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }}

superset-frontend/src/dashboard/components/SliceAdder.tsx

@@ -118,7 +118,7 @@ const NewChartButtonContainer = styled.div`
   ${({ theme }) => css`
     display: flex;
     justify-content: flex-end;
-    padding-right: ${theme.sizeUnit * 2}px;
+    padding: ${theme.sizeUnit * 3}px ${theme.sizeUnit * 2}px 0;
   `}
 `;
 

superset/connectors/sqla/models.py

@@ -2072,6 +2072,7 @@ class SqlaTable(
                 self.database,
                 self.catalog,
                 self.schema or default_schema or "",
+                exclude_dataset_id=self.id,
             )
             # Add each predicate as a separate cache key component
             extra_cache_keys.extend(rls_predicates)

superset/mcp_service/chart/chart_utils.py

@@ -37,6 +37,7 @@ from superset.mcp_service.chart.schemas import (
     MixedTimeseriesChartConfig,
     PieChartConfig,
     PivotTableChartConfig,
+    SortByConfig,
     TableChartConfig,
     XYChartConfig,
 )
@@ -466,7 +467,14 @@ def map_table_config(config: TableChartConfig) -> Dict[str, Any]:
     _add_adhoc_filters(form_data, config.filters)
 
     if config.sort_by:
-        form_data["order_by_cols"] = config.sort_by
+        form_data["order_by_cols"] = [
+            json.dumps(
+                [entry.column, entry.ascending]
+                if isinstance(entry, SortByConfig)
+                else [entry, False]
+            )
+            for entry in config.sort_by
+        ]
 
     form_data["row_limit"] = config.row_limit
 

superset/mcp_service/chart/schemas.py

@@ -22,7 +22,7 @@ Pydantic schemas for chart-related responses
 from __future__ import annotations
 
 import difflib
-from datetime import datetime, timezone
+from datetime import datetime
 from typing import Annotated, Any, Dict, List, Literal, Protocol
 
 import humanize
@@ -50,7 +50,7 @@ from superset.mcp_service.common.cache_schemas import (
     OwnedByMeMixin,
     QueryCacheControl,
 )
-from superset.mcp_service.common.error_schemas import ChartGenerationError
+from superset.mcp_service.common.error_schemas import ChartGenerationError, MCPBaseError
 from superset.mcp_service.constants import DEFAULT_PAGE_SIZE, MAX_PAGE_SIZE
 from superset.mcp_service.privacy import filter_user_directory_fields
 from superset.mcp_service.system.schemas import (
@@ -183,16 +183,8 @@ class ChartInfo(BaseModel):
         return data
 
 
-class ChartError(BaseModel):
-    error: str = Field(..., description="Error message")
-    error_type: str = Field(..., description="Type of error")
-    timestamp: datetime = Field(
-        default_factory=lambda: datetime.now(timezone.utc),
-        description="Error timestamp",
-    )
-    model_config = ConfigDict(ser_json_timedelta="iso8601")
-
-    @field_validator("error")
+class ChartError(MCPBaseError):
+    @field_validator("message")
     @classmethod
     def sanitize_error_for_llm_context(cls, value: str) -> str:
         """Wrap error text before it is exposed to LLM context."""
@@ -806,6 +798,30 @@ class FilterConfig(BaseModel):
         return self
 
 
+class SortByConfig(BaseModel):
+    """Sort specification with explicit direction.
+
+    Accepts either this object or a bare column-name string in `sort_by`
+    lists. Bare strings default to descending, which matches the
+    sort-by-metric "top N" pattern most commonly used for tables.
+    """
+
+    model_config = ConfigDict(populate_by_name=True)
+
+    column: str = Field(
+        ...,
+        min_length=1,
+        max_length=255,
+        validation_alias=AliasChoices("column", "col"),
+        description="Column to sort by",
+    )
+    ascending: bool = Field(
+        False,
+        description="Sort ascending. Defaults to False (descending) to match "
+        "the typical sort-by-metric top-N use case.",
+    )
+
+
 # Actual chart types
 class PieChartConfig(UnknownFieldCheckMixin):
     model_config = ConfigDict(extra="ignore", populate_by_name=True)
@@ -1192,8 +1208,12 @@ class TableChartConfig(UnknownFieldCheckMixin):
         description="Structured filters (column/op/value). "
         "Do NOT use adhoc_filters or raw SQL expressions.",
     )
-    sort_by: List[str] | None = Field(
+    sort_by: List[str | SortByConfig] | None = Field(
         None,
+        description="Columns to sort by. Each entry is either a column-name "
+        "string (defaults to descending) or a SortByConfig object with "
+        "explicit `ascending`. Descending matches the sort-by-metric "
+        "top-N pattern most common for tables.",
         validation_alias=AliasChoices("sort_by", "order_by_cols", "order_by"),
     )
     row_limit: int = Field(1000, description="Max rows returned", ge=1, le=50000)

superset/mcp_service/common/error_schemas.py

@@ -19,9 +19,53 @@
 Enhanced error schemas for MCP chart generation with contextual information
 """
 
+from __future__ import annotations
+
+from datetime import datetime, timezone
 from typing import Any, Dict, List
 
-from pydantic import BaseModel, Field
+from pydantic import BaseModel, computed_field, ConfigDict, Field, model_validator
+
+
+class MCPBaseError(BaseModel):
+    """Base error shape for all MCP tool responses.
+
+    Provides a consistent set of fields that every error response includes,
+    allowing LLM clients to handle errors uniformly regardless of which tool
+    produced them.
+    """
+
+    error_type: str = Field(
+        ..., description="Type of error (validation, execution, etc.)"
+    )
+    message: str = Field(..., description="Human-readable error message")
+    timestamp: datetime = Field(
+        default_factory=lambda: datetime.now(timezone.utc),
+        description="Error timestamp",
+    )
+    details: str | None = Field(None, description="Detailed error explanation")
+    suggestions: list[str] = Field(
+        default_factory=list, description="Actionable suggestions to fix the error"
+    )
+    error_code: str | None = Field(
+        None, description="Unique error code for support reference"
+    )
+
+    model_config = ConfigDict(ser_json_timedelta="iso8601")
+
+    @model_validator(mode="before")
+    @classmethod
+    def _compat_error_to_message(cls, data: Any) -> Any:
+        """Allow construction with error= kwarg for backward compatibility."""
+        if isinstance(data, dict) and "error" in data and "message" not in data:
+            data["message"] = data.pop("error")
+        return data
+
+    @computed_field  # type: ignore[prop-decorator]
+    @property
+    def error(self) -> str:
+        """Backward-compatible field: mirrors 'message' in serialized output."""
+        return self.message
 
 
 class ColumnSuggestion(BaseModel):
@@ -67,13 +111,9 @@ class DatasetContext(BaseModel):
     )
 
 
-class ChartGenerationError(BaseModel):
+class ChartGenerationError(MCPBaseError):
     """Enhanced error response for chart generation failures"""
 
-    error_type: str = Field(
-        ..., description="Type of error (validation, execution, etc.)"
-    )
-    message: str = Field(..., description="High-level error message")
     details: str = Field(..., description="Detailed error explanation")
     validation_errors: List[ValidationError] = Field(
         default_factory=list, description="Specific field validation errors"
@@ -84,15 +124,9 @@ class ChartGenerationError(BaseModel):
     query_info: Dict[str, Any] | None = Field(
         None, description="Query execution details"
     )
-    suggestions: List[str] = Field(
-        default_factory=list, description="Actionable suggestions to fix the error"
-    )
     help_url: str | None = Field(
         None, description="URL to documentation for this error type"
     )
-    error_code: str | None = Field(
-        None, description="Unique error code for support reference"
-    )
 
 
 class ChartGenerationResponse(BaseModel):

superset/models/helpers.py

@@ -2060,12 +2060,17 @@ class ExploreMixin:  # pylint: disable=too-many-public-methods
             default_schema = self.database.get_default_schema(self.catalog)
             try:
                 rls_applied = False
+                # ``id`` lives on concrete subclasses (e.g. SqlaTable), not on
+                # ExploreMixin itself. getattr keeps this safe for non-dataset
+                # subclasses (e.g. SQL Lab Query), which have no RLS to dedupe.
+                self_id = getattr(self, "id", None)
                 for statement in parsed_script.statements:
                     if apply_rls(
                         self.database,
                         self.catalog,
                         self.schema or default_schema or "",
                         statement,
+                        exclude_dataset_id=self_id,
                     ):
                         rls_applied = True
 

superset/utils/rls.py

@@ -34,10 +34,16 @@ def apply_rls(
     catalog: str | None,
     schema: str,
     parsed_statement: BaseSQLStatement[Any],
+    exclude_dataset_id: int | None = None,
 ) -> bool:
     """
     Modify statement inplace to ensure RLS rules are applied.
 
+    :param exclude_dataset_id: When applying RLS to a virtual dataset's inner SQL,
+        pass the virtual dataset's id here so its own RLS isn't injected again
+        on top of the outer-WHERE application (avoids double-apply when the
+        virtual dataset's table_name collides with a table in its own SQL — for
+        example, after converting a physical dataset with RLS to virtual).
     :returns: True if any RLS predicates were actually applied, False otherwise.
     """
     # There are two ways to insert RLS: either replacing the table with a subquery
@@ -55,6 +61,7 @@ def apply_rls(
                 table,
                 database,
                 database.get_default_catalog(),
+                exclude_dataset_id=exclude_dataset_id,
             )
             if predicate
         ]
@@ -68,6 +75,7 @@ def get_predicates_for_table(
     table: Table,
     database: Database,
     default_catalog: str | None,
+    exclude_dataset_id: int | None = None,
 ) -> list[str]:
     """
     Get the RLS predicates for a table.
@@ -87,18 +95,21 @@ def get_predicates_for_table(
             SqlaTable.catalog.is_(None),
         )
 
-    dataset = (
-        db.session.query(SqlaTable)
-        .filter(
-            and_(
-                SqlaTable.database_id == database.id,
-                catalog_predicate,
-                SqlaTable.schema == table.schema,
-                SqlaTable.table_name == table.table,
-            )
-        )
-        .one_or_none()
-    )
+    filters = [
+        SqlaTable.database_id == database.id,
+        catalog_predicate,
+        SqlaTable.schema == table.schema,
+        SqlaTable.table_name == table.table,
+    ]
+    # When applying RLS to a virtual dataset's inner SQL, skip a match against
+    # the dataset itself — its RLS is already applied on the outer WHERE via
+    # get_sqla_row_level_filters(). Without this, a virtual dataset whose
+    # table_name happens to equal a table in its own SQL (e.g. after a
+    # physical→virtual conversion) double-applies its own predicates.
+    if exclude_dataset_id is not None:
+        filters.append(SqlaTable.id != exclude_dataset_id)
+
+    dataset = db.session.query(SqlaTable).filter(and_(*filters)).one_or_none()
     if not dataset:
         return []
 
@@ -130,6 +141,7 @@ def collect_rls_predicates_for_sql(
     database: Database,
     catalog: str | None,
     schema: str,
+    exclude_dataset_id: int | None = None,
 ) -> list[str]:
     """
     Collect all RLS predicates that would be applied to tables in the given SQL.
@@ -141,6 +153,9 @@ def collect_rls_predicates_for_sql(
     :param database: The database the query runs against
     :param catalog: The default catalog for the query
     :param schema: The default schema for the query
+    :param exclude_dataset_id: Mirror of the same parameter on apply_rls — pass
+        the virtual dataset's id so its self-match is excluded from the cache key
+        (kept consistent with what's actually applied at query time).
     :return: List of RLS predicate strings that would be applied
     """
     from superset.sql.parse import SQLScript
@@ -161,6 +176,7 @@ def collect_rls_predicates_for_sql(
                     table,
                     database,
                     default_catalog,
+                    exclude_dataset_id=exclude_dataset_id,
                 )
             }
         )

tests/unit_tests/mcp_service/chart/test_chart_utils.py

@@ -44,6 +44,7 @@ from superset.mcp_service.chart.schemas import (
     ColumnRef,
     FilterConfig,
     LegendConfig,
+    SortByConfig,
     TableChartConfig,
     XYChartConfig,
 )
@@ -295,7 +296,7 @@ class TestMapTableConfig:
         assert result["adhoc_filters"][2]["comparator"] == ["Sports", "Racing"]
 
     def test_map_table_config_with_sort(self) -> None:
-        """Test table config mapping with sort"""
+        """Bare strings default to descending."""
         config = TableChartConfig(
             chart_type="table",
             columns=[ColumnRef(name="product")],
@@ -303,7 +304,26 @@ class TestMapTableConfig:
         )
 
         result = map_table_config(config)
-        assert result["order_by_cols"] == ["product", "revenue"]
+        assert result["order_by_cols"] == ['["product", false]', '["revenue", false]']
+
+    def test_map_table_config_with_sort_direction(self) -> None:
+        """SortByConfig entries honor the explicit ascending flag."""
+        config = TableChartConfig(
+            chart_type="table",
+            columns=[ColumnRef(name="product")],
+            sort_by=[
+                SortByConfig(column="product", ascending=True),
+                SortByConfig(column="revenue", ascending=False),
+                "category",
+            ],
+        )
+
+        result = map_table_config(config)
+        assert result["order_by_cols"] == [
+            '["product", true]',
+            '["revenue", false]',
+            '["category", false]',
+        ]
 
     def test_map_table_config_ag_grid_table(self) -> None:
         """Test table config mapping with AG Grid Interactive Table viz_type"""

tests/unit_tests/sql_lab_test.py

@@ -285,8 +285,18 @@ def test_apply_rls(mocker: MockerFixture) -> None:
 
     get_predicates_for_table.assert_has_calls(
         [
-            mocker.call(Table("t1", "public", "examples"), database, "examples"),
-            mocker.call(Table("t2", "public", "examples"), database, "examples"),
+            mocker.call(
+                Table("t1", "public", "examples"),
+                database,
+                "examples",
+                exclude_dataset_id=None,
+            ),
+            mocker.call(
+                Table("t2", "public", "examples"),
+                database,
+                "examples",
+                exclude_dataset_id=None,
+            ),
         ]
     )
 
@@ -329,3 +339,27 @@ def test_get_predicates_for_table(mocker: MockerFixture) -> None:
     dataset.get_sqla_row_level_filters.assert_called_once_with(
         include_global_guest_rls=False
     )
+
+
+def test_get_predicates_for_table_excludes_self(mocker: MockerFixture) -> None:
+    """
+    When ``exclude_dataset_id`` is supplied, the lookup query must add an
+    ``id != exclude_dataset_id`` filter so a virtual dataset whose
+    ``table_name`` matches a table referenced inside its own SQL doesn't get
+    its own RLS injected into the inner SQL (would double-apply on top of the
+    outer WHERE). Regression test for the physical→virtual conversion bug.
+    """
+    database = mocker.MagicMock()
+    db = mocker.patch("superset.utils.rls.db")
+    db.session.query().filter().one_or_none.return_value = None
+
+    table = Table("orders", "public", "examples")
+    assert (
+        get_predicates_for_table(table, database, "examples", exclude_dataset_id=42)
+        == []
+    )
+    # The filter call should have received four base filters plus the exclusion
+    # filter, i.e. five total positional args inside and_().
+    filter_call = db.session.query().filter.call_args
+    and_clause = filter_call.args[0]
+    assert len(and_clause.clauses) == 5