Copilot commented on code in PR #45854:
URL: https://github.com/apache/superset/pull/45854#discussion_r4417723146
##########
superset-frontend/src/pages/ChartCreation/index.tsx:
##########
@@ -303,25 +301,20 @@ export const ChartCreation = ({
);
useEffect(() => {
- const params = new URLSearchParams(window.location.search).get('dataset');
- if (params) {
- // The URL names a dataset that was just saved, so resolve it against
- // datasets only: a semantic view with the same name must not win.
- fetchDatasourceList(params, 0, 1, {
- exactMatch: true,
- datasetsOnly: true,
- })
- .then(({ result }) => {
- const [dataset] = result;
- setDatasource(dataset && toDatasourceOption(dataset));
- setLoading(false);
- })
- .catch(() => {
- setLoading(false);
- });
- addSuccessToast(t('The dataset has been saved'));
+ const datasetId = new URLSearchParams(window.location.search).get(
+ 'dataset_id',
+ );
+ if (!datasetId) {
Review Comment:
If the URL contains an empty dataset id (e.g. `?dataset_id=`),
`has('dataset_id')` can still make the page start in a loading state, but this
early-return won’t call `setLoading(false)`, leaving the spinner stuck
indefinitely. Treat an empty string as ‘no dataset’ and clear loading before
returning (or compute the initial loading state from `get('dataset_id')` rather
than `has(...)`).
##########
superset-frontend/src/pages/ChartCreation/index.tsx:
##########
@@ -241,7 +239,7 @@ export const ChartCreation = ({
);
const hasDatasetParam = useMemo(
- () => new URLSearchParams(window.location.search).has('dataset'),
+ () => new URLSearchParams(window.location.search).has('dataset_id'),
[],
);
Review Comment:
The variable name `hasDatasetParam` is now misleading since it specifically
checks `dataset_id`. Renaming it to something like `hasDatasetIdParam` (or
`hasDatasetId`) would better reflect its purpose and reduce confusion during
future maintenance.
##########
superset-frontend/src/features/datasets/fetchDatasourceList.ts:
##########
@@ -111,3 +76,21 @@ export const fetchDatasourceList = (
count: response.json.count ?? 0,
}));
};
+
+/**
+ * Fetches a single dataset by its ID.
+ *
+ * Dataset names are only unique per database, catalog and schema, so callers
+ * that know the ID (for example right after creating a dataset) must resolve
+ * it by ID rather than by name. Rejects when the dataset does not exist or is
+ * not accessible (404).
+ */
+export const fetchDatasetById = (
+ id: number | string,
+ { get = SupersetClient.get }: FetchDatasourceListOptions = {},
+): Promise<Dataset> => {
+ const query = rison.encode({ columns: DATASET_LIST_COLUMNS });
+ return get({ endpoint: `/api/v1/dataset/${id}?q=${query}` }).then(
+ (response: JsonResponse) => response.json.result as Dataset,
+ );
+};
Review Comment:
This builds a URL path segment directly from a user-controlled value (via
`dataset_id` in the caller). To prevent path-segment injection (e.g. slashes or
`..`), encode the ID as a path segment (e.g. `encodeURIComponent(String(id))`)
and/or constrain the function signature to `id: number` and require callers to
parse/validate before calling.
--
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]
