You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Vitor-Avila (via GitHub)" <gi...@apache.org> on 2025/08/13 04:05:01 UTC
[PR] fix: Avoid dataset drill request if no perm [superset]
Vitor-Avila opened a new pull request, #34665:
URL: https://github.com/apache/superset/pull/34665
### SUMMARY
This is a follow up to https://github.com/apache/superset/pull/34620. Users might not have drilling access, and in this case the API request would return a `403`. While this is expected, having failed requests is suboptimal for embedded use-cases, so I've added a `skip` flag to the method (since we can't conditionally call it).
Unsure if it's the best way to solve this -- but open to feedback.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI changes.
### TESTING INSTRUCTIONS
Test coverage added.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Avoid dataset drill request if no perm [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #34665:
URL: https://github.com/apache/superset/pull/34665#discussion_r2272016686
##########
superset-frontend/src/hooks/apiResources/datasets.ts:
##########
@@ -71,6 +72,15 @@ export const useDatasetDrillInfo = (
});
useEffect(() => {
+ if (skip) {
+ // short circuit if `skip` is `true`
+ setResource({
+ status: ResourceStatus.Complete,
+ result: {} as Dataset,
Review Comment:
### Unsafe Type Casting of Empty Object <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Type casting an empty object to Dataset type can lead to runtime errors as the required properties of Dataset type are missing.
###### Why this matters
Components consuming this hook expect a valid Dataset object with specific properties. When accessing these properties on an empty object, it may cause runtime errors even though TypeScript compilation succeeds.
###### Suggested change ∙ *Feature Preview*
Return null as the result when skipping, or provide a minimal valid Dataset object with default values:
```typescript
result: skip ? null : {
columns: [],
metrics: [],
verbose_map: {},
} as Dataset
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6fe43efb-2efa-4c4e-822c-537302483ef6/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6fe43efb-2efa-4c4e-822c-537302483ef6?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6fe43efb-2efa-4c4e-822c-537302483ef6?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6fe43efb-2efa-4c4e-822c-537302483ef6?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6fe43efb-2efa-4c4e-822c-537302483ef6)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:8bdfeed0-1cbe-44eb-a097-47bdddc32767 -->
[](8bdfeed0-1cbe-44eb-a097-47bdddc32767)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Avoid dataset drill request if no perm [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #34665:
URL: https://github.com/apache/superset/pull/34665#discussion_r2288574231
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -184,6 +184,7 @@ const SliceHeaderControls = (
props.slice.datasource,
props.dashboardId,
props.formData,
+ !canDrillToDetail,
Review Comment:
@michael-s-molina I agree your suggestion looks a lot better, and also that the caller should decide if the request is needed or not. However, that would be a conditional hook, which then raises on `pre-commit`.
I believe the ideal/long-term solution would be to refactor this whole flow, but for now hopefully this `skip` param is fine.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Avoid dataset drill request if no perm [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila merged PR #34665:
URL: https://github.com/apache/superset/pull/34665
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Avoid dataset drill request if no perm [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #34665:
URL: https://github.com/apache/superset/pull/34665#discussion_r2276357009
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -184,6 +184,7 @@ const SliceHeaderControls = (
props.slice.datasource,
props.dashboardId,
props.formData,
+ !canDrillToDetail,
Review Comment:
I think this should be handled by callers of this hook. The hook's functionality is to make a fetch request and return the drill info. You shouldn't call the hook if the required permissions are not there.
```
const datasetResource = canDrillToDetail ? useDatasetDrillInfo(...) : undefined;
```
Then you can handle the `undefined` `datasetResource`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Avoid dataset drill request if no perm [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #34665:
URL: https://github.com/apache/superset/pull/34665#discussion_r2274612327
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -184,6 +184,7 @@ const SliceHeaderControls = (
props.slice.datasource,
props.dashboardId,
props.formData,
+ !canDrillToDetail,
Review Comment:
Only improvement i could think of at first is to move `useDatasetDrillInfo` inside the actual components but not sure if it's an improvement at all. I think having a skip argument is fine.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org