fix(tags): also catch SupersetParseError when validating tagged query access

Widen the access-validation carve-out to SupersetParseError, a sibling of
SupersetSecurityException under SupersetErrorException that process_jinja_sql
raises for unresolvable partition macros. It previously slipped past the
(SupersetSecurityException, TemplateError) tuple and surfaced as exactly the
unhandled 500 this PR set out to eliminate.

Split the handler into two arms: security denials stay silent (routine), while
TemplateError/SupersetParseError are logged for server-side visibility and their
error text is preserved in the surfaced message rather than discarded.

Add a regression test for the SupersetParseError path (verified to fail before
this change).

superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx

@@ -779,35 +779,6 @@ function EditorsSelector({
 const ResultTable =
   extensionsRegistry.get('sqleditor.extension.resultTable') ?? FilterableTable;
 
-// D3's '%' type is a valid spec that multiplies by 100, so it never trips
-// the "Invalid format" fallback even when applied to a raw count.
-const isPercentD3Format = (d3format?: string): boolean =>
-  !!d3format && d3format.trim().endsWith('%');
-
-const isCountExpression = (expression?: string): boolean =>
-  !!expression && /^\s*count\s*\(/i.test(expression);
-
-function renderMetricFormatWarning(item: Record<string, any>): ReactNode {
-  if (
-    !isCountExpression(item.expression) ||
-    !isPercentD3Format(item.d3format)
-  ) {
-    return null;
-  }
-  return (
-    <Alert
-      css={themeParam => ({ marginBottom: themeParam.sizeUnit * 4 })}
-      type="warning"
-      showIcon
-      message={t(
-        'This metric is a count, but its D3 format is a percentage. ' +
-          'Percent formats multiply the value by 100, which will make a ' +
-          'raw count render as a misleadingly large number.',
-      )}
-    />
-  );
-}
-
 // Redux connector types
 interface QueryPayload {
   client_id?: string;
@@ -2169,7 +2140,7 @@ function DatasourceEditor({
           }}
           expandFieldset={
             <FormContainer>
-              <Fieldset compact renderWarning={renderMetricFormatWarning}>
+              <Fieldset compact>
                 <Field
                   fieldKey="expression"
                   label={t('SQL expression')}

superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditorMetricFormatWarning.test.tsx

@@ -1,91 +0,0 @@
-/**
- * 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 fetchMock from 'fetch-mock';
-import { screen, userEvent, waitFor } from 'spec/helpers/testing-library';
-import {
-  createProps,
-  DATASOURCE_ENDPOINT,
-  setupDatasourceEditorMocks,
-  cleanupAsyncOperations,
-  fastRender,
-  dismissDatasourceWarning,
-} from './DatasourceEditor.test.utils';
-
-beforeEach(() => {
-  fetchMock.get(DATASOURCE_ENDPOINT, [], { name: DATASOURCE_ENDPOINT });
-  setupDatasourceEditorMocks();
-});
-
-afterEach(async () => {
-  await cleanupAsyncOperations();
-  fetchMock.clearHistory().removeRoutes();
-});
-
-const WARNING_TEXT = /D3 format is a percentage/i;
-
-// A '%' format is valid syntax, so it never hits the "Invalid format" fallback.
-test('warns when a percent D3 format is set on a COUNT metric', async () => {
-  const testProps = createProps();
-  fastRender(testProps);
-  await dismissDatasourceWarning();
-
-  await userEvent.click(await screen.findByTestId('collection-tab-Metrics'));
-  const expandToggles = await screen.findAllByLabelText(/expand row/i);
-  // Rows sort by metric id descending, so `COUNT(*)` (id 7) is first.
-  await userEvent.click(expandToggles[0]);
-
-  expect(screen.queryByText(WARNING_TEXT)).not.toBeInTheDocument();
-
-  await userEvent.type(await screen.findByPlaceholderText('%y/%m/%d'), '.0%');
-
-  expect(await screen.findByText(WARNING_TEXT)).toBeInTheDocument();
-});
-
-test('does not warn for a non-percent format on a COUNT metric', async () => {
-  const testProps = createProps();
-  fastRender(testProps);
-  await dismissDatasourceWarning();
-
-  await userEvent.click(await screen.findByTestId('collection-tab-Metrics'));
-  const expandToggles = await screen.findAllByLabelText(/expand row/i);
-  await userEvent.click(expandToggles[0]);
-
-  await userEvent.type(await screen.findByPlaceholderText('%y/%m/%d'), ',.0f');
-
-  await waitFor(() =>
-    expect(screen.queryByText(WARNING_TEXT)).not.toBeInTheDocument(),
-  );
-});
-
-test('does not warn for a percent format on a non-COUNT metric', async () => {
-  const testProps = createProps();
-  fastRender(testProps);
-  await dismissDatasourceWarning();
-
-  await userEvent.click(await screen.findByTestId('collection-tab-Metrics'));
-  const expandToggles = await screen.findAllByLabelText(/expand row/i);
-  // Rows sort by metric id descending, so id 1 (`SUM(...)`) sorts last.
-  await userEvent.click(expandToggles[6]);
-
-  await userEvent.type(await screen.findByPlaceholderText('%y/%m/%d'), '.0%');
-
-  await waitFor(() =>
-    expect(screen.queryByText(WARNING_TEXT)).not.toBeInTheDocument(),
-  );
-});

superset-frontend/src/components/Datasource/components/Fieldset/index.tsx

@@ -28,7 +28,6 @@ export interface FieldsetProps {
   item?: Record<string, any>;
   title?: ReactNode;
   compact?: boolean;
-  renderWarning?: (item: Record<string, any>) => ReactNode;
 }
 
 type fieldKeyType = string | number;
@@ -39,7 +38,6 @@ export default function Fieldset({
   item = {},
   title = null,
   compact = false,
-  renderWarning,
 }: FieldsetProps) {
   // Controls report their edits asynchronously - TextControl debounces by
   // FAST_DEBOUNCE - so the callback that eventually fires was built during an
@@ -80,7 +78,6 @@ export default function Fieldset({
         </Typography.Title>
       )}
 
-      {renderWarning?.(item)}
       {recurseReactClone(children, Field, propExtender)}
     </Form>
   );

superset-frontend/src/dashboard/util/permissionUtils.test.ts

@@ -259,21 +259,6 @@ describe('isUserEditorOrAdmin', () => {
   test('returns false when editors is omitted', () => {
     expect(isUserEditorOrAdmin(outsiderUser)).toEqual(false);
   });
-
-  test('returns true when the user is granted editorship only through extra_editors', () => {
-    expect(isUserEditorOrAdmin(editorUser, [], [10])).toEqual(true);
-  });
-
-  test('unions editors and extra_editors rather than preferring one', () => {
-    const nonMatchingSubject: Subject = { id: 999, label: 'Other', type: 1 };
-    expect(isUserEditorOrAdmin(editorUser, [nonMatchingSubject], [10])).toEqual(
-      true,
-    );
-  });
-
-  test('returns false when extra_editors names other subjects', () => {
-    expect(isUserEditorOrAdmin(editorUser, [], [999])).toEqual(false);
-  });
 });
 
 // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks

superset-frontend/src/dashboard/util/permissionUtils.ts

@@ -55,6 +55,9 @@ export const isUserInSubjects = (
   );
 };
 
+const isUserInEditors = (editors: Subject[] = []): boolean =>
+  isUserInSubjects(editors);
+
 export const isUserAdmin = (
   user?: UserWithPermissionsAndRoles | UndefinedUser,
 ) =>
@@ -63,12 +66,10 @@ export const isUserAdmin = (
     role => role.toLowerCase() === ADMIN_ROLE_NAME.toLowerCase(),
   );
 
-/** `extraEditors` is editorship granted via a deployment's EXTRA_EDITORS_RESOLVER. */
 export const isUserEditorOrAdmin = (
   user?: UserWithPermissionsAndRoles | UndefinedUser,
   editors: Subject[] = [],
-  extraEditors?: SubjectRef[] | null,
-): boolean => isUserInSubjects(editors, extraEditors) || isUserAdmin(user);
+): boolean => isUserInEditors(editors) || isUserAdmin(user);
 
 /**
  * Editorship of *dashboard*, matching the server's `is_editor`: the explicit

superset-frontend/src/explore/components/DataTablesPane/components/ResultsPaneOnDashboard.tsx

@@ -88,12 +88,7 @@ export const ResultsPaneOnDashboard = ({
 
   return (
     <Wrapper>
-      <Tabs
-        fullHeight
-        activeKey={activeTabKey}
-        onChange={setActiveTabKey}
-        items={items}
-      />
+      <Tabs activeKey={activeTabKey} onChange={setActiveTabKey} items={items} />
     </Wrapper>
   );
 };

superset-frontend/src/features/charts/ChartCard.tsx

@@ -96,11 +96,7 @@ export default function ChartCard({
   const canEdit = hasPerm('can_write');
   const canDelete = hasPerm('can_write');
   const canExport = hasPerm('can_export');
-  const allowEdit = isUserEditorOrAdmin(
-    user,
-    chart.editors,
-    chart.extra_editors,
-  );
+  const allowEdit = isUserEditorOrAdmin(user, chart.editors);
   const menuItems: MenuItem[] = [];
 
   if (canEdit) {

superset-frontend/src/features/dashboards/DashboardCard.tsx

@@ -83,11 +83,7 @@ function DashboardCard({
   const canEdit = hasPerm('can_write');
   const canDelete = hasPerm('can_write');
   const canExport = hasPerm('can_export');
-  const allowEdit = isUserEditorOrAdmin(
-    user,
-    dashboard.editors,
-    dashboard.extra_editors,
-  );
+  const allowEdit = isUserEditorOrAdmin(user, dashboard.editors);
   const digest = dashboard.changed_on_utc || dashboard.changed_on;
   const thumbnailUrl =
     isFeatureEnabled(FeatureFlag.Thumbnails) && dashboard.id && digest

superset-frontend/src/pages/ChartList/index.tsx

@@ -650,11 +650,7 @@ function ChartList(props: ChartListProps) {
       },
       {
         Cell: ({ row: { original } }: CellProps<Chart>) => {
-          const allowEdit = isUserEditorOrAdmin(
-            user,
-            original.editors,
-            original.extra_editors,
-          );
+          const allowEdit = isUserEditorOrAdmin(user, original.editors);
           const openEditModal = () => openChartEditModal(original);
           const handleExport = () => handleBulkChartExport([original]);
           if (!canEdit && !canDelete && !canExport) {

superset-frontend/src/pages/DashboardList/index.tsx

@@ -122,8 +122,6 @@ export interface Dashboard {
   description?: string;
   thumbnail_url?: string | null;
   editors?: Subject[];
-  // Bare subject ids from a deployment's EXTRA_EDITORS_RESOLVER.
-  extra_editors?: number[];
   viewers?: Subject[];
   tags: TagType[];
   created_by: object;
@@ -507,11 +505,7 @@ function DashboardList(props: DashboardListProps) {
       },
       {
         Cell: ({ row: { original } }: CellProps<Dashboard>) => {
-          const allowEdit = isUserEditorOrAdmin(
-            user,
-            original.editors,
-            original.extra_editors,
-          );
+          const allowEdit = isUserEditorOrAdmin(user, original.editors);
           const handleDelete = () =>
             handleDashboardDelete(
               original,

superset-frontend/src/types/Chart.ts

@@ -45,8 +45,6 @@ export interface Chart {
   cache_timeout: number | null;
   thumbnail_url?: string;
   editors?: Subject[];
-  // Bare subject ids from a deployment's EXTRA_EDITORS_RESOLVER.
-  extra_editors?: number[];
   viewers?: Subject[];
   tags?: TagType[];
   last_saved_at?: string;

superset-frontend/src/views/CRUD/types.ts

@@ -67,8 +67,6 @@ export interface Dashboard {
   url: string;
   thumbnail_url?: string | null;
   editors?: Subject[];
-  // Bare subject ids from a deployment's EXTRA_EDITORS_RESOLVER.
-  extra_editors?: number[];
   viewers?: Subject[];
   loading?: boolean;
 }

superset/charts/api.py

@@ -92,10 +92,7 @@ from superset.exceptions import (
 )
 from superset.extensions import event_logger, security_manager
 from superset.models.slice import Slice
-from superset.security.manager import (
-    get_extra_editor_subject_ids,
-    get_extra_editors_by_pk,
-)
+from superset.security.manager import get_extra_editor_subject_ids
 from superset.subjects.filters import (
     FilterRelatedSubjects,
     subject_type_filter,
@@ -413,15 +410,6 @@ class ChartRestApi(SoftDeleteApiMixin, BaseSupersetModelRestApi):
         except ChartNotFoundError:
             return self.response_404()
 
-    def pre_get_list(self, data: dict[str, Any]) -> None:
-        """Attach ``extra_editors`` to each row, matching the single-object GET."""
-        super().pre_get_list(data)
-        ids = data.get("ids", [])
-        extra_editors_by_id = get_extra_editors_by_pk(Slice, ids)
-        for row, row_id in zip(data.get("result", []), ids, strict=False):
-            if row_id in extra_editors_by_id:
-                row["extra_editors"] = extra_editors_by_id[row_id]
-
     @expose("/<pk>/deck_layers/", methods=("GET",))
     @protect()
     @safe

superset/commands/tag/create.py

@@ -18,6 +18,8 @@ import logging
 from functools import partial
 from typing import Any
 
+from jinja2.exceptions import TemplateError
+
 from superset import security_manager
 from superset.commands.base import BaseCommand, CreateMixin
 from superset.commands.tag.exceptions import TagCreateFailedError, TagInvalidError
@@ -27,7 +29,7 @@ from superset.commands.tag.utils import (
     to_object_type,
 )
 from superset.daos.tag import TagDAO
-from superset.exceptions import SupersetSecurityException
+from superset.exceptions import SupersetParseError, SupersetSecurityException
 from superset.tags.models import ObjectType, TagType
 from superset.utils.decorators import on_error, transaction
 
@@ -98,8 +100,30 @@ class CreateCustomTagCommand(CreateMixin, BaseCommand):
                     )
                 )
         except SupersetSecurityException:
+            # A routine, expected authorization denial; swallowed silently by
+            # design (no logging) and surfaced to the caller as a validation
+            # failure rather than an unhandled 500.
             exceptions.append(
-                TagCreateFailedError(f"Access denied for {object_type} {object_id}")
+                TagCreateFailedError(
+                    f"Could not validate access for {object_type} {object_id}"
+                )
+            )
+        except (TemplateError, SupersetParseError) as ex:
+            # Authorizing a saved query parses its Jinja-templated SQL to resolve
+            # table references. Malformed Jinja (TemplateError) or an
+            # unresolvable partition macro (SupersetParseError) is a validation
+            # failure, not an unhandled 500 -- but unlike an access denial it is
+            # genuinely unexpected, so log it for server-side visibility and
+            # preserve the underlying error text instead of discarding it.
+            logger.warning(
+                "Could not parse query %s while validating tag access: %s",
+                object_id,
+                str(ex),
+            )
+            exceptions.append(
+                TagCreateFailedError(
+                    f"Could not validate access for {object_type} {object_id}: {ex}"
+                )
             )
 
 

superset/common/form_data_query_context.py

@@ -213,9 +213,7 @@ def orderby_from_form_data(
     # The drag-and-drop "sort by" control persists a list; the frontend unwraps it
     # with ``ensureIsArray(...)[0]`` (``plugin-chart-table/src/buildQuery.ts:67``).
     # Read raw, a list would nest inside ``orderby`` and fail the query.
-    raw_sort_metric = form_data.get("series_limit_metric") or form_data.get(
-        "timeseries_limit_metric"
-    )
+    raw_sort_metric = form_data.get("timeseries_limit_metric")
     sort_metric = (
         next(iter(as_list(raw_sort_metric)), None) if raw_sort_metric else None
     ) or (metrics[0] if form_data.get("sort_by_metric") else None)

superset/dashboards/api.py

@@ -142,10 +142,7 @@ from superset.extensions import event_logger, security_manager
 from superset.models.dashboard import Dashboard
 from superset.models.embedded_dashboard import EmbeddedDashboard
 from superset.security.guest_token import GuestUser
-from superset.security.manager import (
-    get_extra_editor_subject_ids,
-    get_extra_editors_by_pk,
-)
+from superset.security.manager import get_extra_editor_subject_ids
 from superset.subjects.filters import (
     FilterRelatedSubjects,
     subject_type_filter,
@@ -436,15 +433,6 @@ class DashboardRestApi(
         """
         return super().get_list(**kwargs)
 
-    def pre_get_list(self, data: dict[str, Any]) -> None:
-        """Attach ``extra_editors`` to each row, matching the single-object GET."""
-        super().pre_get_list(data)
-        ids = data.get("ids", [])
-        extra_editors_by_id = get_extra_editors_by_pk(Dashboard, ids)
-        for row, row_id in zip(data.get("result", []), ids, strict=False):
-            if row_id in extra_editors_by_id:
-                row["extra_editors"] = extra_editors_by_id[row_id]
-
     list_select_columns = list_columns + ["changed_on", "created_on", "changed_by_fk"]
     order_columns = [
         "changed_by.first_name",

superset/examples/birth_names.py

@@ -290,7 +290,7 @@ def create_slices(tbl: SqlaTable) -> tuple[list[Slice], list[Slice]]:
                 groupby=["name"],
                 adhoc_filters=[gen_filter("gender", "girl")],
                 row_limit=50,
-                series_limit_metric=metric,
+                timeseries_limit_metric=metric,
                 metrics=[metric],
             ),
             editors=[],
@@ -321,7 +321,7 @@ def create_slices(tbl: SqlaTable) -> tuple[list[Slice], list[Slice]]:
                 groupby=["name"],
                 adhoc_filters=[gen_filter("gender", "boy")],
                 row_limit=50,
-                series_limit_metric=metric,
+                timeseries_limit_metric=metric,
                 metrics=[metric],
             ),
             editors=[],
@@ -498,7 +498,7 @@ def create_slices(tbl: SqlaTable) -> tuple[list[Slice], list[Slice]]:
                 viz_type="echarts_timeseries_line",
                 granularity_sqla="ds",
                 groupby=["name"],
-                series_limit_metric={
+                timeseries_limit_metric={
                     "expressionType": "SIMPLE",
                     "column": {
                         "column_name": "num_california",
@@ -522,7 +522,7 @@ def create_slices(tbl: SqlaTable) -> tuple[list[Slice], list[Slice]]:
                 metrics=metrics,
                 groupby=["name"],
                 row_limit=50,
-                series_limit_metric={
+                timeseries_limit_metric={
                     "expressionType": "SIMPLE",
                     "column": {
                         "column_name": "num_california",

superset/examples/usa_births_names/charts/Boys.yaml

@@ -36,8 +36,8 @@ params:
   metrics:
   - sum__num
   row_limit: 50
-  series_limit_metric: sum__num
   time_range: '100 years ago : now'
+  timeseries_limit_metric: sum__num
   viz_type: table
 query_context: null
 slice_name: Boys

superset/examples/usa_births_names/charts/Girls.yaml

@@ -36,8 +36,8 @@ params:
   metrics:
   - sum__num
   row_limit: 50
-  series_limit_metric: sum__num
   time_range: '100 years ago : now'
+  timeseries_limit_metric: sum__num
   viz_type: table
 query_context: null
 slice_name: Girls

superset/security/manager.py

@@ -170,36 +170,6 @@ def get_extra_editor_subject_ids(resource: Model) -> list[int]:
     return subject_ids
 
 
-def get_extra_editors_by_pk(
-    model_cls: type[Model], primary_keys: list[Any]
-) -> dict[Any, list[int]]:
-    """
-    Resolve extra editor subject IDs for a batch of resources, keyed by
-    primary key. List responses only have serialized rows, not model
-    instances, so this re-queries the page's rows in one batched query.
-    """
-    if not primary_keys or not (
-        has_app_context() and current_app.config.get("EXTRA_EDITORS_RESOLVER")
-    ):
-        return {}
-
-    # pylint: disable=import-outside-toplevel
-    from superset import db
-    from superset.models.helpers import SKIP_VISIBILITY_FILTER_CLASSES
-
-    pk_col = inspect(model_cls).primary_key[0]
-    resources = (
-        db.session.query(model_cls)
-        .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {model_cls}})
-        .filter(pk_col.in_(primary_keys))
-        .all()
-    )
-    return {
-        getattr(resource, pk_col.name): get_extra_editor_subject_ids(resource)
-        for resource in resources
-    }
-
-
 def _render_permission_instructions_link(
     *,
     datasource_id: str = "",

tests/integration_tests/dashboards/api_tests.py

@@ -922,55 +922,6 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa
             db.session.delete(dashboard)
             db.session.commit()
 
-    def test_get_dashboards_list_omits_extra_editors_by_default(self):
-        """No EXTRA_EDITORS_RESOLVER configured: list rows omit extra_editors."""
-        admin = self.get_user("admin")
-        dashboard = self.insert_dashboard(
-            "no_extra_editors_list_dashboard",
-            "no-extra-editors-list-dashboard",
-            [admin.id],
-        )
-        try:
-            self.login(ADMIN_USERNAME)
-            rv = self.client.get("api/v1/dashboard/")
-            assert rv.status_code == 200
-            data = json.loads(rv.data.decode("utf-8"))
-            row = next(
-                d
-                for d in data["result"]
-                if d["dashboard_title"] == dashboard.dashboard_title
-            )
-            assert "extra_editors" not in row
-        finally:
-            db.session.delete(dashboard)
-            db.session.commit()
-
-    @with_config({"EXTRA_EDITORS_RESOLVER": lambda resource: [123]})
-    def test_get_dashboards_list_includes_extra_editors_when_resolver_configured(
-        self,
-    ):
-        """List rows get extra_editors too, mirroring the single-object GET."""
-        admin = self.get_user("admin")
-        dashboard = self.insert_dashboard(
-            "extra_editors_list_dashboard",
-            "extra-editors-list-dashboard",
-            [admin.id],
-        )
-        try:
-            self.login(ADMIN_USERNAME)
-            rv = self.client.get("api/v1/dashboard/")
-            assert rv.status_code == 200
-            data = json.loads(rv.data.decode("utf-8"))
-            row = next(
-                d
-                for d in data["result"]
-                if d["dashboard_title"] == dashboard.dashboard_title
-            )
-            assert row["extra_editors"] == [123]
-        finally:
-            db.session.delete(dashboard)
-            db.session.commit()
-
     def test_get_charts_admin_sees_existing_charts(self):
         """Regression for #25890: GET /api/v1/chart/ as an Admin user should
         return existing charts, not an empty list."""
@@ -993,41 +944,6 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa
             db.session.delete(chart)
             db.session.commit()
 
-    def test_get_charts_list_omits_extra_editors_by_default(self):
-        """No EXTRA_EDITORS_RESOLVER configured: list rows omit extra_editors."""
-        admin = self.get_user("admin")
-        chart = self.insert_chart(
-            "no_extra_editors_list_chart", [admin.id], 1, params="{}"
-        )
-        try:
-            self.login(ADMIN_USERNAME)
-            rv = self.client.get("api/v1/chart/")
-            assert rv.status_code == 200
-            data = json.loads(rv.data.decode("utf-8"))
-            row = next(c for c in data["result"] if c["slice_name"] == chart.slice_name)
-            assert "extra_editors" not in row
-        finally:
-            db.session.delete(chart)
-            db.session.commit()
-
-    @with_config({"EXTRA_EDITORS_RESOLVER": lambda resource: [123]})
-    def test_get_charts_list_includes_extra_editors_when_resolver_configured(self):
-        """List rows get extra_editors too, mirroring the single-object GET."""
-        admin = self.get_user("admin")
-        chart = self.insert_chart(
-            "extra_editors_list_chart", [admin.id], 1, params="{}"
-        )
-        try:
-            self.login(ADMIN_USERNAME)
-            rv = self.client.get("api/v1/chart/")
-            assert rv.status_code == 200
-            data = json.loads(rv.data.decode("utf-8"))
-            row = next(c for c in data["result"] if c["slice_name"] == chart.slice_name)
-            assert row["extra_editors"] == [123]
-        finally:
-            db.session.delete(chart)
-            db.session.commit()
-
     def test_get_dashboards_filter(self):
         """
         Dashboard API: Test get dashboards filter

tests/unit_tests/common/test_form_data_query_context.py

@@ -218,31 +218,6 @@ def test_orderby_uses_timeseries_limit_metric_and_order_desc() -> None:
     assert query["orderby"] == [["revenue", True]]
 
 
-def test_orderby_uses_series_limit_metric_and_order_desc() -> None:
-    # series_limit_metric is the current field name; timeseries_limit_metric is
-    # the deprecated alias kept above for back-compat with old saved charts.
-    form_data = {
-        "metrics": ["count"],
-        "groupby": ["c"],
-        "series_limit_metric": "revenue",
-        "order_desc": False,
-    }
-    query = build_query_context_from_form_data(form_data, DATASOURCE)["queries"][0]
-    assert query["orderby"] == [["revenue", True]]
-
-
-def test_orderby_prefers_series_limit_metric_over_deprecated_alias() -> None:
-    form_data = {
-        "metrics": ["count"],
-        "groupby": ["c"],
-        "series_limit_metric": "revenue",
-        "timeseries_limit_metric": "profit",
-        "order_desc": False,
-    }
-    query = build_query_context_from_form_data(form_data, DATASOURCE)["queries"][0]
-    assert query["orderby"] == [["revenue", True]]
-
-
 def test_orderby_pie_sort_by_metric() -> None:
     form_data = {"metric": "count", "groupby": ["c"], "sort_by_metric": True}
     query = build_query_context_from_form_data(form_data, DATASOURCE, viz_type="pie")[

tests/unit_tests/tags/commands/create_test.py

@@ -108,6 +108,75 @@ def test_create_command_success(session_with_data: Session, mocker: MockerFixtur
         )
 
 
+def test_validate_object_access_query_malformed_jinja(
+    session_with_data: Session, mocker: MockerFixture
+):
+    """A saved query whose Jinja-templated SQL fails to parse during access
+    checks must surface as a validation error, not an unhandled
+    ``jinja2.TemplateError`` escaping as a 500.
+
+    When ``raise_for_access(query=...)`` authorizes a saved query via
+    per-table permissions it parses the query's Jinja SQL (e.g. an unclosed
+    ``{% if %}`` block raises ``TemplateSyntaxError``). Mock that call to raise
+    the ``TemplateError`` directly so the test stays hermetic and does not open
+    a live DB connection to introspect table-level perms.
+    """
+    from jinja2.exceptions import TemplateError
+
+    from superset.commands.tag.create import CreateCustomTagCommand
+    from superset.commands.tag.exceptions import TagInvalidError
+    from superset.models.sql_lab import SavedQuery
+    from superset.tags.models import ObjectType
+
+    query = db.session.query(SavedQuery).first()
+
+    mocker.patch("superset.commands.tag.create.to_object_model", return_value=query)
+    mocker.patch(
+        "superset.commands.tag.create.security_manager.raise_for_access",
+        side_effect=TemplateError("unclosed {% if %}"),
+    )
+
+    command = CreateCustomTagCommand(ObjectType.query, query.id, ["tag"])
+
+    with pytest.raises(TagInvalidError):
+        command.validate()
+
+
+def test_validate_object_access_query_unresolvable_partition_macro(
+    session_with_data: Session, mocker: MockerFixture
+):
+    """A saved query whose partition macro cannot be resolved statically raises
+    ``SupersetParseError`` during access checks. Like ``TemplateError``, it is a
+    sibling of ``SupersetSecurityException`` under ``SupersetErrorException`` and
+    would otherwise escape as an unhandled 500, so it must also surface as a
+    validation error.
+
+    Mock ``raise_for_access`` to raise the error directly so the test stays
+    hermetic and does not open a live DB connection to introspect table perms.
+    """
+    from superset.commands.tag.create import CreateCustomTagCommand
+    from superset.commands.tag.exceptions import TagInvalidError
+    from superset.exceptions import SupersetParseError
+    from superset.models.sql_lab import SavedQuery
+    from superset.tags.models import ObjectType
+
+    query = db.session.query(SavedQuery).first()
+
+    mocker.patch("superset.commands.tag.create.to_object_model", return_value=query)
+    mocker.patch(
+        "superset.commands.tag.create.security_manager.raise_for_access",
+        side_effect=SupersetParseError(
+            sql="select * from {{ latest_partition('foo') }}",
+            message="Unresolvable partition macro",
+        ),
+    )
+
+    command = CreateCustomTagCommand(ObjectType.query, query.id, ["tag"])
+
+    with pytest.raises(TagInvalidError):
+        command.validate()
+
+
 def test_create_command_success_clear(
     session_with_data: Session, mocker: MockerFixture
 ):