msyavuz commented on code in PR #43334:
URL: https://github.com/apache/superset/pull/43334#discussion_r3861229096
##########
superset/commands/dataset/create.py:
##########
@@ -50,7 +55,28 @@ def run(self) -> Model:
self.validate()
dataset = DatasetDAO.create(attributes=self._properties)
- dataset.fetch_metadata()
+ try:
+ dataset.fetch_metadata()
+ except OAuth2RedirectError:
Review Comment:
This re-raise doesn't survive the API layer: `DatasetRestApi.post` is
wrapped in FAB's `@safe`, which catches it and returns a 500 with
`url`/`tab_id` stripped, so the client still can't start the dance —
`databases/api.py` gets away with the same pattern only because its `post` has
no `@safe`. The command-level ordering and rollback here are correct; it needs
the same `except OAuth2RedirectError: raise` in the dataset `post` handler.
##########
superset/mcp_service/dataset/tool/create_virtual_dataset.py:
##########
@@ -218,6 +218,8 @@ async def create_virtual_dataset( # noqa: C901
error=f"Failed to update dataset metadata (creation rolled back):
{exc}",
)
except SupersetGenericDBErrorException as exc:
+ # Defensive backstop for direct raises (see
Review Comment:
`test_create_virtual_dataset_sql_error_is_actionable` patches
`CreateDatasetCommand` wholesale, so it doesn't exercise a real caller reaching
this branch — is there an unmocked path left, or is this keeping the test
green? With `run()` wrapping every `SupersetException` into
`DatasetInvalidError` and `_update_virtual_dataset` raising
`DatasetUpdateFailedError`, nothing in the try block produces a bare
`SupersetGenericDBErrorException` in production.
##########
superset-frontend/src/SqlLab/components/SaveQuery/index.tsx:
##########
@@ -113,6 +113,17 @@ const SaveQuery = ({
const [label, setLabel] = useState<string>(defaultLabel);
const [showSave, setShowSave] = useState<boolean>(false);
const [showSaveDatasetModal, setShowSaveDatasetModal] = useState(false);
+ // Saving a dataset runs the SQL to introspect columns, so it needs a
+ // successful run of the SQL being saved that produced at least one column
+ // -- editing after a run invalidates it, and running a selection only
+ // validates that selection.
+ const latestQuery = useSelector<SqlLabRootState, Query | undefined>(
+ ({ sqlLab }) => sqlLab.queries[queryEditor.latestQueryId || ''],
+ );
+ const canSaveDataset =
+ latestQuery?.state === QueryState.Success &&
+ latestQuery.sql === queryEditor.sql &&
Review Comment:
Unrelated to the original findings, but `runQuery` stores `sql:
qe.selectedText || qe.sql`, so highlighting one statement in a multi-statement
tab and running it leaves `latestQuery.sql` as the selection and Save dataset
stays disabled until the whole tab is re-run — intended?
--
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]
