sadpandajoe commented on code in PR #45795:
URL: https://github.com/apache/superset/pull/45795#discussion_r4418548728
##########
superset/daos/query.py:
##########
@@ -90,3 +90,33 @@ def stop_query(client_id: str) -> 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.
+
+ The check is best-effort: there is no database constraint behind it, so
+ truly concurrent requests can still race and create a duplicate.
+
+ :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,
Review Comment:
Thanks. You're right that the equality follows the metadata database's
collation, so on a case-insensitive collation `Report` and `report` count as
the same name. That's acceptable here (names that differ only by case are just
as ambiguous in the list), so we're keeping the plain equality rather than
adding database-specific lowercasing. I've corrected the PR description, which
had called the comparison case-sensitive.
##########
superset/daos/query.py:
##########
@@ -90,3 +90,33 @@ def stop_query(client_id: str) -> 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.
+
+ The check is best-effort: there is no database constraint behind it, so
+ truly concurrent requests can still race and create a duplicate.
+
+ :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()
Review Comment:
Agreed that two truly concurrent requests can both pass this check. Closing
that gap needs a database unique constraint, which means a migration plus a
cleanup of existing duplicate rows, and that is out of scope for this change.
The check is a best-effort guard for the realistic case (saving the same name
twice); the docstring and the PR description say so, and the modal now disables
Save/Update while a request is in flight so a double click can't submit twice.
##########
superset/queries/saved_queries/api.py:
##########
@@ -203,6 +206,31 @@ def pre_add(self, item: SavedQuery) -> None:
item.user = g.user
if item.label:
item.label = item.label.strip()
+ self._validate_label_uniqueness(item)
Review Comment:
Right, the MCP `save_sql_query` tool calls `SavedQueryDAO.create` directly,
so it isn't covered. Moving the check into the DAO would also change every
other caller (for example bundle import, which has to be able to restore
existing data), so this change stays scoped to the REST API and the SQL Lab
modal, and the MCP path is listed under the PR's known limits. How the MCP tool
should report a duplicate is a reasonable follow-up.
##########
superset-frontend/src/SqlLab/actions/sqlLab.ts:
##########
@@ -1214,15 +1237,16 @@ export function updateSavedQuery(
.then(() => {
dispatch(addSuccessToast(t('Your query was updated')));
dispatch(queryEditorSetTitle(query, query.name ?? '', clientId));
+ dispatch(updateQueryEditor(query));
Review Comment:
This reducer behavior pre-dates the change: `UPDATE_QUERY_EDITOR` matches
the first editor with the given `remoteId`, and
`dispatch(updateQueryEditor(query))` was already in `updateSavedQuery` on
master. This PR only moves that dispatch into the success path, so a failed
update no longer touches the editor. Handling the same saved query open in
several tabs is a separate issue, so it isn't changed here.
--
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]
