You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by jl...@apache.org on 2026/10/27 18:02:49 UTC
(superset) 02/04: fix(sqllab): reject duplicate saved query names
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch fix-saved-query-duplicate-name
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 07a1292b129c06f2f6830d8186d6ec2cb3c94140
Author: Joe Li <jo...@preset.io>
AuthorDate: Tue Oct 27 17:38:59 2026 +0000
fix(sqllab): reject duplicate saved query names
Saving a query with the name of one of the user's existing saved queries on
the same database was silently accepted, leaving indistinguishable rows in
the Saved Queries list.
The saved query API now rejects a create or rename that collides with another
saved query of the same owner and database with a 422 ("A saved query with
this name already exists"). The label is compared exactly, after surrounding
whitespace is stripped. An update that changes neither the label nor the
database is not checked, so existing duplicates remain editable. No schema
change is made.
In SQL Lab, the saveQuery and updateSavedQuery actions now reject when the
request fails instead of swallowing the error, and the Save query modal stays
open, keeps the user's input and shows the server message next to the Name
field. Other failures keep the existing toast. This also stops a failed save
from being followed by the "Your query was saved" toast.
Co-Authored-By: Claude Sonnet 5.5 <no...@anthropic.com>
---
.../src/SqlLab/actions/sqlLab.test.ts | 89 +++++++++++++++
superset-frontend/src/SqlLab/actions/sqlLab.ts | 42 +++++--
.../SqlLab/components/SaveQuery/SaveQuery.test.tsx | 30 +++++
.../src/SqlLab/components/SaveQuery/index.tsx | 35 ++++--
superset/commands/query/exceptions.py | 5 +
superset/daos/query.py | 27 +++++
superset/queries/saved_queries/api.py | 30 ++++-
.../queries/saved_queries/api_tests.py | 122 +++++++++++++++++++++
8 files changed, 362 insertions(+), 18 deletions(-)
diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.test.ts b/superset-frontend/src/SqlLab/actions/sqlLab.test.ts
index 43687f67b0b..18b7afafb26 100644
--- a/superset-frontend/src/SqlLab/actions/sqlLab.test.ts
+++ b/superset-frontend/src/SqlLab/actions/sqlLab.test.ts
@@ -195,6 +195,95 @@ describe('async actions', () => {
});
});
+ // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
+ describe('saved query rejections', () => {
+ const duplicateError = 'A saved query with this name already exists';
+ const rejectedResponse = (status: number, body: Record<string, unknown>) =>
+ new Response(JSON.stringify(body), { status });
+
+ const spies: jest.SpyInstance[] = [];
+ const track = <T extends jest.SpyInstance>(spy: T): T => {
+ spies.push(spy);
+ return spy;
+ };
+
+ afterEach(() => {
+ spies.splice(0).forEach(spy => spy.mockRestore());
+ });
+
+ test('saveQuery rejects with the server message on a 422 without a toast', async () => {
+ track(jest.spyOn(SupersetClient, 'post')).mockRejectedValueOnce(
+ rejectedResponse(422, { error: duplicateError }),
+ );
+ const store = mockStore(initialState);
+
+ await expect(
+ store.dispatch(actions.saveQuery(query, queryId)),
+ ).rejects.toThrow(duplicateError);
+ expect(store.getActions()).toEqual([]);
+ });
+
+ test('saveQuery toasts and rejects on other failures', async () => {
+ track(jest.spyOn(SupersetClient, 'post')).mockRejectedValueOnce(
+ rejectedResponse(500, { message: 'boom' }),
+ );
+ const store = mockStore(initialState);
+
+ await expect(
+ store.dispatch(actions.saveQuery(query, queryId)),
+ ).rejects.toBeInstanceOf(Response);
+ expect(store.getActions().map(a => a.type)).toEqual([ADD_TOAST]);
+ expect(store.getActions()[0].payload.toastType).toBe(ToastType.Danger);
+ });
+
+ test('updateSavedQuery rejects with the server message on a 422 without a toast or editor update', async () => {
+ track(jest.spyOn(SupersetClient, 'put')).mockRejectedValueOnce(
+ rejectedResponse(422, { error: duplicateError }),
+ );
+ track(jest.spyOn(console, 'error')).mockImplementation(() => {});
+ const store = mockStore(initialState);
+
+ await expect(
+ store.dispatch(
+ actions.updateSavedQuery({ ...query, remoteId: 1 }, queryId),
+ ),
+ ).rejects.toThrow(duplicateError);
+ expect(store.getActions()).toEqual([]);
+ });
+
+ test('updateSavedQuery toasts without updating the editor on other failures', async () => {
+ track(jest.spyOn(SupersetClient, 'put')).mockRejectedValueOnce(
+ rejectedResponse(500, { message: 'boom' }),
+ );
+ track(jest.spyOn(console, 'error')).mockImplementation(() => {});
+ const store = mockStore(initialState);
+
+ await expect(
+ store.dispatch(
+ actions.updateSavedQuery({ ...query, remoteId: 1 }, queryId),
+ ),
+ ).rejects.toBeInstanceOf(Response);
+ expect(store.getActions().map(a => a.type)).toEqual([ADD_TOAST]);
+ });
+
+ test('updateSavedQuery updates the editor and toasts success on success', async () => {
+ track(jest.spyOn(SupersetClient, 'put')).mockResolvedValueOnce({
+ json: {},
+ } as never);
+ const store = mockStore(initialState);
+
+ await store.dispatch(
+ actions.updateSavedQuery({ ...query, remoteId: 1 }, queryId),
+ );
+
+ expect(store.getActions().map(a => a.type)).toEqual([
+ ADD_TOAST,
+ actions.QUERY_EDITOR_SET_TITLE,
+ actions.UPDATE_QUERY_EDITOR,
+ ]);
+ });
+ });
+
// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
describe('formatQuery', () => {
const formatQueryEndpoint = 'glob:*/api/v1/sqllab/format_sql/';
diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.ts b/superset-frontend/src/SqlLab/actions/sqlLab.ts
index 632fafb1eff..3cd2e99cffc 100644
--- a/superset-frontend/src/SqlLab/actions/sqlLab.ts
+++ b/superset-frontend/src/SqlLab/actions/sqlLab.ts
@@ -1143,6 +1143,25 @@ export function queryEditorSetTitle(
};
}
+// Rejects so callers (the Save query modal) know the save failed. A rejected
+// request that the server flagged as invalid (422) rejects with an `Error`
+// carrying the server's message, to be shown next to the form; any other
+// failure raises the generic toast and rejects with the original error.
+const rejectFailedSavedQueryRequest = async (
+ dispatch: AppDispatch,
+ error: unknown,
+ toastMessage: string,
+): Promise<never> => {
+ const clientError = await getClientErrorObject(
+ error as Parameters<typeof getClientErrorObject>[0],
+ );
+ if (clientError.status === 422 && clientError.error) {
+ throw new Error(clientError.error);
+ }
+ dispatch(addDangerToast(toastMessage));
+ throw error;
+};
+
export function saveQuery(
query: Partial<QueryEditor>,
clientId: string,
@@ -1170,8 +1189,12 @@ export function saveQuery(
dispatch(queryEditorSetTitle(query, query.name ?? '', clientId));
return savedQuery;
})
- .catch(() =>
- dispatch(addDangerToast(t('Your query could not be saved'))),
+ .catch(error =>
+ rejectFailedSavedQueryRequest(
+ dispatch,
+ error,
+ t('Your query could not be saved'),
+ ),
);
}
@@ -1214,15 +1237,16 @@ export function updateSavedQuery(
.then(() => {
dispatch(addSuccessToast(t('Your query was updated')));
dispatch(queryEditorSetTitle(query, query.name ?? '', clientId));
+ dispatch(updateQueryEditor(query));
})
- .catch(e => {
- const message = t('Your query could not be updated');
+ .catch(error => {
// eslint-disable-next-line no-console
- console.error(message, e);
- dispatch(addDangerToast(message));
- })
- .then(() => {
- dispatch(updateQueryEditor(query));
+ console.error(t('Your query could not be updated'), error);
+ return rejectFailedSavedQueryRequest(
+ dispatch,
+ error,
+ t('Your query could not be updated'),
+ );
});
}
diff --git a/superset-frontend/src/SqlLab/components/SaveQuery/SaveQuery.test.tsx b/superset-frontend/src/SqlLab/components/SaveQuery/SaveQuery.test.tsx
index 5c37fa02ec7..2cc5fc7af79 100644
--- a/superset-frontend/src/SqlLab/components/SaveQuery/SaveQuery.test.tsx
+++ b/superset-frontend/src/SqlLab/components/SaveQuery/SaveQuery.test.tsx
@@ -564,4 +564,34 @@ describe('SavedQuery', () => {
).toBeInTheDocument();
expect(screen.getAllByRole('textbox')[0]).toHaveValue('My saved query');
});
+
+ test('clears the inline error when the name changes and closes after a successful retry', async () => {
+ const duplicateError = 'A saved query with this name already exists';
+ const mockOnSave = jest
+ .fn()
+ .mockRejectedValueOnce(new Error(duplicateError))
+ .mockResolvedValueOnce(undefined);
+
+ render(<SaveQuery {...mockedProps} onSave={mockOnSave} />, {
+ useRedux: true,
+ store: mockStore(mockState),
+ });
+
+ await userEvent.click(screen.getByRole('button', { name: /save/i }));
+ const modalSaveBtn = () =>
+ screen.getAllByRole('button', { name: /save/i })[1];
+ await userEvent.click(modalSaveBtn());
+ expect(await screen.findByText(duplicateError)).toBeInTheDocument();
+
+ await userEvent.type(screen.getAllByRole('textbox')[0], ' 2');
+ expect(screen.queryByText(duplicateError)).not.toBeInTheDocument();
+
+ await userEvent.click(modalSaveBtn());
+ await waitFor(() =>
+ expect(
+ screen.queryByRole('heading', { name: /save query/i }),
+ ).not.toBeInTheDocument(),
+ );
+ expect(mockOnSave).toHaveBeenCalledTimes(2);
+ });
});
diff --git a/superset-frontend/src/SqlLab/components/SaveQuery/index.tsx b/superset-frontend/src/SqlLab/components/SaveQuery/index.tsx
index dccdb3b497a..900d9ad6fe7 100644
--- a/superset-frontend/src/SqlLab/components/SaveQuery/index.tsx
+++ b/superset-frontend/src/SqlLab/components/SaveQuery/index.tsx
@@ -111,6 +111,7 @@ const SaveQuery = ({
query.description || '',
);
const [label, setLabel] = useState<string>(defaultLabel);
+ const [labelError, setLabelError] = useState<string>('');
const [showSave, setShowSave] = useState<boolean>(false);
const [showSaveDatasetModal, setShowSaveDatasetModal] = useState(false);
// Saving a dataset runs the SQL to introspect columns, so it needs a
@@ -150,21 +151,34 @@ const SaveQuery = ({
if (!isSaved) setLabel(defaultLabel);
}, [defaultLabel]);
- const close = () => setShowSave(false);
+ const close = () => {
+ setShowSave(false);
+ setLabelError('');
+ };
- const onSaveWrapper = async () => {
- logAction(LOG_ACTIONS_SQLLAB_SAVE_QUERY, {});
- await onSave(queryPayload(), query.id);
+ // A rejected save keeps the modal open with the user's input so the name
+ // can be changed and the save retried.
+ const submit = async (action: SaveQueryProps['onSave']) => {
+ setLabelError('');
+ try {
+ await action(queryPayload(), query.id);
+ } catch (error) {
+ setLabelError(error instanceof Error ? error.message : '');
+ return;
+ }
close();
};
- const onUpdateWrapper = async () => {
- await onUpdate(queryPayload(), query.id);
- close();
+ const onSaveWrapper = () => {
+ logAction(LOG_ACTIONS_SQLLAB_SAVE_QUERY, {});
+ return submit(onSave);
};
+ const onUpdateWrapper = () => submit(onUpdate);
+
const onLabelChange = (e: ChangeEvent<HTMLInputElement>) => {
setLabel(e.target.value);
+ setLabelError('');
};
const onDescriptionChange = (e: ChangeEvent<HTMLTextAreaElement>) => {
@@ -175,7 +189,12 @@ const SaveQuery = ({
<Form layout="vertical">
<Row>
<Col xs={24}>
- <FormItem label={t('Name')} htmlFor="save-query-name">
+ <FormItem
+ label={t('Name')}
+ htmlFor="save-query-name"
+ validateStatus={labelError ? 'error' : undefined}
+ help={labelError || undefined}
+ >
<Input
id="save-query-name"
type="text"
diff --git a/superset/commands/query/exceptions.py b/superset/commands/query/exceptions.py
index 20797955e3d..35607df67c3 100644
--- a/superset/commands/query/exceptions.py
+++ b/superset/commands/query/exceptions.py
@@ -38,3 +38,8 @@ class SavedQueryImportError(ImportFailedError):
class SavedQueryInvalidError(CommandInvalidError):
message = _("Saved query parameters are invalid.")
+
+
+class SavedQueryLabelDuplicateError(CommandException):
+ status = 422
+ message = _("A saved query with this name already exists")
diff --git a/superset/daos/query.py b/superset/daos/query.py
index 1ebce6df53a..198d100349a 100644
--- a/superset/daos/query.py
+++ b/superset/daos/query.py
@@ -90,3 +90,30 @@ class QueryDAO(BaseDAO[Query]):
class SavedQueryDAO(BaseDAO[SavedQuery]):
base_filter = SavedQueryFilter
+
+ @staticmethod
+ def validate_label_uniqueness(
+ label: str,
+ owner_id: int | None,
+ db_id: int | None,
+ saved_query_id: int | None = None,
+ ) -> bool:
+ """
+ Validate that no other saved query of the same owner and database uses
+ this label. The comparison is exact on the label as stored.
+
+ :param label: The (already stripped) saved query label
+ :param owner_id: The id of the user owning the saved query
+ :param db_id: The id of the database the saved query targets
+ :param saved_query_id: The saved query's own id, excluded from the
+ comparison (only for validating on updates)
+ :return: True when the label is not taken
+ """
+ query = db.session.query(SavedQuery).filter(
+ SavedQuery.label == label,
+ SavedQuery.created_by_fk == owner_id,
+ SavedQuery.db_id.is_(None) if db_id is None else SavedQuery.db_id == db_id,
+ )
+ if saved_query_id is not None:
+ query = query.filter(SavedQuery.id != saved_query_id)
+ return not db.session.query(query.exists()).scalar()
diff --git a/superset/queries/saved_queries/api.py b/superset/queries/saved_queries/api.py
index 37d794d6009..a14267133a2 100644
--- a/superset/queries/saved_queries/api.py
+++ b/superset/queries/saved_queries/api.py
@@ -24,6 +24,7 @@ from flask import g, request, Response
from flask_appbuilder.api import expose, protect, rison as parse_rison, safe
from flask_appbuilder.models.sqla.interface import SQLAInterface
from flask_babel import ngettext
+from sqlalchemy import inspect
from superset.commands.importers.exceptions import (
IncorrectFormatError,
@@ -36,13 +37,15 @@ from superset.commands.importers.v1.utils import (
from superset.commands.query.delete import DeleteSavedQueryCommand
from superset.commands.query.exceptions import (
SavedQueryDeleteFailedError,
+ SavedQueryLabelDuplicateError,
SavedQueryNotFoundError,
)
from superset.commands.query.export import ExportSavedQueriesCommand
from superset.commands.query.importers.dispatcher import ImportSavedQueriesCommand
from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod
+from superset.daos.query import SavedQueryDAO
from superset.databases.filters import DatabaseFilter
-from superset.extensions import event_logger
+from superset.extensions import db, event_logger
from superset.models.sql_lab import SavedQuery
from superset.queries.saved_queries.filters import (
SavedQueryAllTextFilter,
@@ -203,6 +206,31 @@ class SavedQueryRestApi(BaseSupersetModelRestApi):
item.user = g.user
if item.label:
item.label = item.label.strip()
+ self._validate_label_uniqueness(item)
+
+ def _validate_label_uniqueness(self, item: SavedQuery) -> None:
+ """
+ Reject a label already used by another saved query of the same owner on
+ the same database. Updates that leave the label and database untouched
+ are not checked, so pre-existing duplicates remain editable.
+ """
+ if not item.label:
+ return
+ attrs = inspect(item).attrs
+ if item.id is not None and not (
+ attrs.label.history.has_changes() or attrs.db_id.history.has_changes()
+ ):
+ return
+ with db.session.no_autoflush:
+ is_unique = SavedQueryDAO.validate_label_uniqueness(
+ item.label, g.user.id, item.db_id, item.id
+ )
+ if not is_unique:
+ # The loaded item already carries the rejected values; discard them so
+ # a later commit on this session (e.g. event logging) cannot persist
+ # the rename.
+ db.session.rollback() # pylint: disable=consider-using-transaction
+ raise SavedQueryLabelDuplicateError()
def pre_update(self, item: SavedQuery) -> None:
self.pre_add(item)
diff --git a/tests/integration_tests/queries/saved_queries/api_tests.py b/tests/integration_tests/queries/saved_queries/api_tests.py
index 55e53219f03..321227b0e5f 100644
--- a/tests/integration_tests/queries/saved_queries/api_tests.py
+++ b/tests/integration_tests/queries/saved_queries/api_tests.py
@@ -779,6 +779,128 @@ class TestSavedQueryApi(SupersetTestCase):
model = db.session.query(SavedQuery).get(saved_query.id)
assert model.label == "label1"
+ def _post_saved_query(self, label: str, db_id: int):
+ return self.client.post(
+ "api/v1/saved_query/",
+ json={
+ "schema": "schema1",
+ "label": label,
+ "sql": "SELECT col1, col2 from table1",
+ "db_id": db_id,
+ },
+ )
+
+ def _delete_saved_queries_by_label(self, label: str) -> None:
+ db.session.query(SavedQuery).filter(SavedQuery.label == label).delete(
+ synchronize_session=False
+ )
+ db.session.commit()
+
+ def test_create_saved_query_duplicate_label_ignores_surrounding_whitespace(
+ self,
+ ) -> None:
+ """
+ Saved Query API: Test create rejects a padded copy of an existing label
+ """
+ example_db = get_example_database()
+ self.login(ADMIN_USERNAME)
+ try:
+ assert self._post_saved_query(
+ "padded label", example_db.id
+ ).status_code == (201)
+ rv = self._post_saved_query(" padded label ", example_db.id)
+ assert rv.status_code == 422
+ finally:
+ self._delete_saved_queries_by_label("padded label")
+
+ def test_create_saved_query_same_label_different_database(self) -> None:
+ """
+ Saved Query API: Test the same label is allowed on a different database
+ """
+ example_db = get_example_database()
+ other_db = Database(database_name="other_db", sqlalchemy_uri="sqlite://")
+ db.session.add(other_db)
+ db.session.commit()
+ self.login(ADMIN_USERNAME)
+ try:
+ assert self._post_saved_query(
+ "shared label", example_db.id
+ ).status_code == (201)
+ assert self._post_saved_query("shared label", other_db.id).status_code == (
+ 201
+ )
+ finally:
+ self._delete_saved_queries_by_label("shared label")
+ db.session.delete(other_db)
+ db.session.commit()
+
+ def test_create_saved_query_same_label_different_user(self) -> None:
+ """
+ Saved Query API: Test the same label is allowed for a different user
+ """
+ example_db = get_example_database()
+ self.insert_default_saved_query(
+ label="other users label", username="gamma_sqllab"
+ )
+ self.login(ADMIN_USERNAME)
+ try:
+ rv = self._post_saved_query("other users label", example_db.id)
+ assert rv.status_code == 201
+ finally:
+ self._delete_saved_queries_by_label("other users label")
+
+ def test_update_saved_query_keeps_own_label(self) -> None:
+ """
+ Saved Query API: Test update may resubmit the saved query's own label
+ """
+ saved_query = self.insert_default_saved_query(label="own label")
+ self.login(ADMIN_USERNAME)
+ try:
+ rv = self.client.put(
+ f"api/v1/saved_query/{saved_query.id}",
+ json={"label": "own label", "description": "changed"},
+ )
+ assert rv.status_code == 200
+ finally:
+ self._delete_saved_queries_by_label("own label")
+
+ def test_update_saved_query_rejects_padded_duplicate_label(self) -> None:
+ """
+ Saved Query API: Test update rejects a padded copy of another label
+ """
+ self.insert_default_saved_query(label="taken label")
+ saved_query = self.insert_default_saved_query(label="free label")
+ self.login(ADMIN_USERNAME)
+ try:
+ rv = self.client.put(
+ f"api/v1/saved_query/{saved_query.id}", json={"label": " taken label "}
+ )
+ assert rv.status_code == 422
+ finally:
+ self._delete_saved_queries_by_label("taken label")
+ self._delete_saved_queries_by_label("free label")
+
+ def test_update_saved_query_legacy_duplicate_stays_editable(self) -> None:
+ """
+ Saved Query API: Test a pre-existing duplicate can be edited as long as
+ its label is not changed
+ """
+ first = self.insert_default_saved_query(label="legacy label")
+ self.insert_default_saved_query(label="legacy label")
+ self.login(ADMIN_USERNAME)
+ try:
+ rv = self.client.put(
+ f"api/v1/saved_query/{first.id}",
+ json={"description": "edited", "sql": "SELECT 1"},
+ )
+ assert rv.status_code == 200
+ rv = self.client.put(
+ f"api/v1/saved_query/{first.id}", json={"label": "legacy label"}
+ )
+ assert rv.status_code == 200
+ finally:
+ self._delete_saved_queries_by_label("legacy label")
+
@pytest.mark.usefixtures("create_saved_queries")
def test_update_saved_query_blank_label(self) -> None:
"""