korbit-ai[bot] commented on code in PR #35264: URL: https://github.com/apache/superset/pull/35264#discussion_r2374749099
########## superset-frontend/src/utils/datasetCache.ts: ########## @@ -0,0 +1,70 @@ +/** + * 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 { supersetGetCache } from './cachedSupersetGet'; + +/** + * Clear all cached API responses for a specific dataset. + * This ensures that any subsequent API calls will fetch fresh data from the server. + * + * @param datasetId - The ID of the dataset to clear from cache + */ +export function clearDatasetCache(datasetId: number | string): void { + if (!datasetId) return; + + const keysToDelete: string[] = []; + + // Find all cache keys related to this dataset + supersetGetCache.forEach((_value, key) => { + // Match any endpoint that references this dataset ID + // This includes: + // - /api/v1/dataset/{id} + // - /api/v1/dataset/{id}?q=... + // - /api/v1/dataset/{id}/related_objects + // - etc. + if ( + key.includes(`/api/v1/dataset/${datasetId}`) || + key.includes(`/api/v1/dataset/${datasetId}/`) || + key.includes(`/api/v1/dataset/${datasetId}?`) + ) { + keysToDelete.push(key); + } + }); Review Comment: ### Inefficient Two-Pass Cache Operation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The function performs two separate operations: collecting keys and then deleting them, which could be combined into a single operation. ###### Why this matters The current implementation requires an extra array allocation and iteration, reducing performance and making the code more complex than necessary. ###### Suggested change ∙ *Feature Preview* Combine the operations into a single pass: ```typescript export function clearDatasetCache(datasetId: number | string): void { if (!datasetId) return; supersetGetCache.forEach((_value, key) => { if (isDatasetUrl(key, datasetId)) { supersetGetCache.delete(key); } }); } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e2a77e4-900f-40d0-9c21-791dfe8dc1f6/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e2a77e4-900f-40d0-9c21-791dfe8dc1f6?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e2a77e4-900f-40d0-9c21-791dfe8dc1f6?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e2a77e4-900f-40d0-9c21-791dfe8dc1f6?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5e2a77e4-900f-40d0-9c21-791dfe8dc1f6) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:28dfeba4-a726-4c21-8d6b-a36ee6d336fb --> [](28dfeba4-a726-4c21-8d6b-a36ee6d336fb) ########## superset-frontend/src/utils/datasetCache.ts: ########## @@ -0,0 +1,70 @@ +/** + * 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 { supersetGetCache } from './cachedSupersetGet'; + +/** + * Clear all cached API responses for a specific dataset. + * This ensures that any subsequent API calls will fetch fresh data from the server. + * + * @param datasetId - The ID of the dataset to clear from cache + */ +export function clearDatasetCache(datasetId: number | string): void { + if (!datasetId) return; + + const keysToDelete: string[] = []; + + // Find all cache keys related to this dataset + supersetGetCache.forEach((_value, key) => { + // Match any endpoint that references this dataset ID + // This includes: + // - /api/v1/dataset/{id} + // - /api/v1/dataset/{id}?q=... + // - /api/v1/dataset/{id}/related_objects + // - etc. + if ( + key.includes(`/api/v1/dataset/${datasetId}`) || + key.includes(`/api/v1/dataset/${datasetId}/`) || + key.includes(`/api/v1/dataset/${datasetId}?`) + ) { Review Comment: ### Redundant and imprecise cache key matching <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The cache key matching logic has overlapping conditions that create redundant checks and potential false positives. ###### Why this matters The first condition `/api/v1/dataset/${datasetId}` will match all URLs that the second and third conditions match, making those additional checks unnecessary. Additionally, this could match unintended URLs like `/api/v1/dataset/123abc` when looking for dataset ID `123`. ###### Suggested change ∙ *Feature Preview* Replace the overlapping conditions with a single precise regex pattern: ```typescript const datasetPattern = new RegExp(`/api/v1/dataset/${datasetId}(?:/|\?|$)`); if (datasetPattern.test(key)) { keysToDelete.push(key); } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e98d95d3-5961-44f6-871c-e846c3b1c684/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e98d95d3-5961-44f6-871c-e846c3b1c684?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e98d95d3-5961-44f6-871c-e846c3b1c684?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e98d95d3-5961-44f6-871c-e846c3b1c684?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e98d95d3-5961-44f6-871c-e846c3b1c684) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:486ea625-8e2a-48fd-b975-c58346b1fb05 --> [](486ea625-8e2a-48fd-b975-c58346b1fb05) ########## superset-frontend/src/components/Datasource/DatasourceModal/index.tsx: ########## @@ -203,6 +204,11 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({ const { json } = await SupersetClient.get({ endpoint: `/api/v1/dataset/${currentDatasource?.id}`, }); + + // Clear the dataset cache to ensure fresh data when fetching columns for filters + // This ensures that changes to the dataset are immediately reflected + clearDatasetCache(currentDatasource.id); Review Comment: ### Unawaited dataset cache clear <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Unawaited cache invalidation may lead to stale dataset data being visible in subsequent operations. ###### Why this matters If clearDatasetCache is asynchronous, not awaiting it can allow stale cache to persist while subsequent requests (e.g., fetching columns for filters) execute, resulting in UI showing outdated data after dataset updates. ###### Suggested change ∙ *Feature Preview* Await cache clearing to ensure cache invalidation completes before proceeding. Example: ``` await clearDatasetCache(currentDatasource.id); ``` If clearDatasetCache is not async, adjust its implementation to return a Promise that resolves when the cache is cleared, or wrap in Promise.resolve(clearDatasetCache(...)). ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cdd56b1e-4f95-42f6-bc48-ee2d7463aae8/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cdd56b1e-4f95-42f6-bc48-ee2d7463aae8?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cdd56b1e-4f95-42f6-bc48-ee2d7463aae8?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cdd56b1e-4f95-42f6-bc48-ee2d7463aae8?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cdd56b1e-4f95-42f6-bc48-ee2d7463aae8) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:a3b7f04f-3ba2-447e-b58b-52588c12f18a --> [](a3b7f04f-3ba2-447e-b58b-52588c12f18a) ########## superset-frontend/src/utils/datasetCache.ts: ########## @@ -0,0 +1,70 @@ +/** + * 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 { supersetGetCache } from './cachedSupersetGet'; + +/** + * Clear all cached API responses for a specific dataset. + * This ensures that any subsequent API calls will fetch fresh data from the server. + * + * @param datasetId - The ID of the dataset to clear from cache + */ +export function clearDatasetCache(datasetId: number | string): void { + if (!datasetId) return; Review Comment: ### Invalid dataset ID 0 rejected by falsy check <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The falsy check for datasetId will incorrectly reject valid dataset ID of 0. ###### Why this matters Dataset IDs can legitimately be 0, but the current falsy check will cause the function to exit early and not clear the cache for dataset 0, leading to stale data. ###### Suggested change ∙ *Feature Preview* Use a more precise check that allows 0 but rejects null, undefined, and empty strings: ```typescript if (datasetId === null || datasetId === undefined || datasetId === '') return; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7bbfdbea-e85f-497c-81d7-95ad0ed07a8e/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7bbfdbea-e85f-497c-81d7-95ad0ed07a8e?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7bbfdbea-e85f-497c-81d7-95ad0ed07a8e?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7bbfdbea-e85f-497c-81d7-95ad0ed07a8e?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7bbfdbea-e85f-497c-81d7-95ad0ed07a8e) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:be991dba-d98b-4617-adae-47b217ebf435 --> [](be991dba-d98b-4617-adae-47b217ebf435) -- 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]
