You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "sha174n (via GitHub)" <gi...@apache.org> on 2026/08/11 09:54:04 UTC
[PR] fix(dataset): apply the import overwrite permission check to identity-matched datasets [superset]
sha174n opened a new pull request, #43058:
URL: https://github.com/apache/superset/pull/43058
`SqlaTable.import_from_dict` matches an existing row on any of the model's unique constraints, which include `(database_id, catalog, schema, table_name)` as well as `uuid`. A dataset import whose config carried a fresh UUID but a physical identity already held by an active dataset was therefore matched and updated on that identity without passing through the overwrite permission gate, which only ran on a UUID match.
This resolves an active identity collision to the existing dataset before the gate runs (aligning the UUID so the subsequent import updates that row deterministically), so the same editor/admin overwrite check applies. Adds a regression test.
This mirrors the treatment already applied to the dashboard importer for slug collisions.
--
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(dataset): apply the import overwrite permission check to identity-matched datasets [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #43058:
URL: https://github.com/apache/superset/pull/43058#issuecomment-5583461964
### <span aria-hidden="true">โ
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">๐จ</span> Latest commit | 11e3dcd21f1a97a95eebaf26fb28508735a82000 |
|<span aria-hidden="true">๐</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a9fdec5dd40ed00080a4f69 |
|<span aria-hidden="true">๐</span> Deploy Preview | [https://deploy-preview-43058--superset-docs-preview.netlify.app](https://deploy-preview-43058--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">๐ฑ</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">๐ค</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#Dco9DoAgDEDhu3TW2EGJeAOPUWmJJPIT6KAx3l2m94bvhVJzLAobwABHpeTO_j7cE5NSEx1DLLn2sCQN-ozknLTWNUu58rNz94asZ3EL84zCiLgizd5Y-H4) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-43058--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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(dataset): apply the import overwrite permission check to identity-matched datasets [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43058:
URL: https://github.com/apache/superset/pull/43058#issuecomment-5589833209
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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(dataset): apply the import overwrite permission check to identity-matched datasets [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43058:
URL: https://github.com/apache/superset/pull/43058#issuecomment-5251620297
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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(dataset): apply the import overwrite permission check to identity-matched datasets [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #43058:
URL: https://github.com/apache/superset/pull/43058#issuecomment-5251709095
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43058?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `7 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.27%. Comparing base ([`5ee2793`](https://app.codecov.io/gh/apache/superset/commit/5ee279363ca78372fa21eae1c73f4e72cb5718e5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f216cc5`](https://app.codecov.io/gh/apache/superset/commit/f216cc5f32c2ec17e07e479281b9179bba477943?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/43058?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/commands/dataset/importers/v1/utils.py](https://app.codecov.io/gh/apache/superset/pull/43058?src=pr&el=tree&filepath=superset%2Fcommands%2Fdataset%2Fimporters%2Fv1%2Futils.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvZGF0YXNldC9pbXBvcnRlcnMvdjEvdXRpbHMucHk=) | 0.00% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43058?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (5ee2793) and HEAD (f216cc5). Click for more details.
>
> <details><summary>HEAD has 30 uploads less than BASE</summary>
>
>| Flag | BASE (5ee2793) | HEAD (f216cc5) |
>|------|------|------|
>|python|26|2|
>|presto|4|1|
>|hive|4|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #43058 +/- ##
==========================================
- Coverage 66.55% 57.27% -9.28%
==========================================
Files 2864 2864
Lines 161894 161900 +6
Branches 37305 37307 +2
==========================================
- Hits 107742 92725 -15017
- Misses 52106 68314 +16208
+ Partials 2046 861 -1185
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/43058/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage ฮ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/43058/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.15% <0.00%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/43058/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/43058/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/43058/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.11% <0.00%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/43058/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.16% <0.00%> (-18.96%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/43058/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/43058/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/43058?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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(dataset): apply the import overwrite permission check to identity-matched datasets [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43058:
URL: https://github.com/apache/superset/pull/43058#issuecomment-5595402761
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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(dataset): apply the import overwrite permission check to identity-matched datasets [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #43058:
URL: https://github.com/apache/superset/pull/43058#discussion_r3756942519
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -248,7 +248,30 @@ def import_dataset( # noqa: C901
# implicit-restore re-import is a clean replacement, not a merge.
is_soft_deleted_match = False
- if existing := find_existing_for_import(SqlaTable, config["uuid"]):
+ existing = find_existing_for_import(SqlaTable, config["uuid"])
+ if not existing and can_write:
+ # A config carrying a fresh UUID but a (database, catalog, schema,
+ # table) identity that already belongs to an active dataset would be
+ # matched-and-updated on that unique key by ``import_from_dict``,
+ # without passing through the overwrite gate below (which only ran on a
+ # UUID match). Resolve that collision to the existing dataset here so
+ # the same gate applies; align the UUID so the subsequent import updates
+ # that row deterministically. (Soft-deleted twins are handled in the
+ # create branch further down.)
+ active_twin = (
+ db.session.query(SqlaTable)
+ .filter_by(
+ database_id=config["database_id"],
+ catalog=config.get("catalog"),
+ schema=config.get("schema"),
+ table_name=config["table_name"],
+ )
+ .first()
Review Comment:
**Suggestion:** The active-twin probe uses a raw `catalog=None` equality, while `import_from_dict` omits nullable fields from its uniqueness predicate. If the incoming config has no catalog and the existing active dataset stores the database default catalog, this query misses the twin, but the later import lookup still matches it by database, schema, and table name. The import then updates the active dataset without reaching the overwrite permission check. Use the same null-aware/default-catalog identity predicate as the DAO or resolve the identity through the shared uniqueness helper. [security]
<details>
<summary><b>Severity Level:</b> Critical ๐จ</summary>
```mdx
- โ Unauthorized imports can overwrite active datasets.
- โ Dataset SQL and metadata can be clobbered.
- โ ๏ธ Affects catalog-default database imports.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4389f10e5472417bacea788a9578232c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4389f10e5472417bacea788a9578232c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/dataset/importers/v1/utils.py
**Line:** 261:269
**Comment:**
*Security: The active-twin probe uses a raw `catalog=None` equality, while `import_from_dict` omits nullable fields from its uniqueness predicate. If the incoming config has no catalog and the existing active dataset stores the database default catalog, this query misses the twin, but the later import lookup still matches it by database, schema, and table name. The import then updates the active dataset without reaching the overwrite permission check. Use the same null-aware/default-catalog identity predicate as the DAO or resolve the identity through the shared uniqueness helper.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43058&comment_hash=a10e6d21c9725cec8cf59d0c50b8dae1715fc6e67649daa2ece1b96875e505db&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43058&comment_hash=a10e6d21c9725cec8cf59d0c50b8dae1715fc6e67649daa2ece1b96875e505db&reaction=dislike'>๐</a>
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -248,7 +248,30 @@ def import_dataset( # noqa: C901
# implicit-restore re-import is a clean replacement, not a merge.
is_soft_deleted_match = False
- if existing := find_existing_for_import(SqlaTable, config["uuid"]):
+ existing = find_existing_for_import(SqlaTable, config["uuid"])
+ if not existing and can_write:
+ # A config carrying a fresh UUID but a (database, catalog, schema,
+ # table) identity that already belongs to an active dataset would be
+ # matched-and-updated on that unique key by ``import_from_dict``,
+ # without passing through the overwrite gate below (which only ran on a
+ # UUID match). Resolve that collision to the existing dataset here so
+ # the same gate applies; align the UUID so the subsequent import updates
+ # that row deterministically. (Soft-deleted twins are handled in the
+ # create branch further down.)
+ active_twin = (
+ db.session.query(SqlaTable)
+ .filter_by(
+ database_id=config["database_id"],
+ catalog=config.get("catalog"),
+ schema=config.get("schema"),
+ table_name=config["table_name"],
+ )
+ .first()
+ )
+ if active_twin is not None:
+ config["uuid"] = str(active_twin.uuid)
+ existing = find_existing_for_import(SqlaTable, config["uuid"])
Review Comment:
**Suggestion:** The collision path rewrites the caller-owned `config` before the defensive copy later in the function. Consequently, callers retaining the uploaded configuration observe its fresh UUID replaced with the victim dataset's UUID; reusing or retrying that same dictionary can then address the victim by UUID and take a different overwrite path than the original import. Copy the input before resolving the collision, or keep the resolved UUID in a separate local import configuration. [stale reference]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Reused import configs retain the victim UUID.
- โ ๏ธ Retries can select different UUID-based branches.
- โ ๏ธ Affects callers retaining bundle configuration dictionaries.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=03a254331a094cd1aee42b796da896d6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=03a254331a094cd1aee42b796da896d6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/dataset/importers/v1/utils.py
**Line:** 272:273
**Comment:**
*Stale Reference: The collision path rewrites the caller-owned `config` before the defensive copy later in the function. Consequently, callers retaining the uploaded configuration observe its fresh UUID replaced with the victim dataset's UUID; reusing or retrying that same dictionary can then address the victim by UUID and take a different overwrite path than the original import. Copy the input before resolving the collision, or keep the resolved UUID in a separate local import configuration.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43058&comment_hash=5a20408120bc0e84e70e5db1c10358ab6c42bfb38398042392eb8d4f419e377c&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43058&comment_hash=5a20408120bc0e84e70e5db1c10358ab6c42bfb38398042392eb8d4f419e377c&reaction=dislike'>๐</a>
--
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(dataset): apply the import overwrite permission check to identity-matched datasets [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43058:
URL: https://github.com/apache/superset/pull/43058#issuecomment-5398587835
@sha174n CodeAnt's catalog thread above is accurate, not a false positive: `filter_by(catalog=None)` builds a real `IS NULL` predicate, but `import_from_dict`'s own matching drops a None field entirely, so a twin with a non-null default catalog still gets updated there while missing this probe. Should the probe use that same None-omitting logic instead?
--
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(dataset): apply the import overwrite permission check to identity-matched datasets [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on code in PR #43058:
URL: https://github.com/apache/superset/pull/43058#discussion_r3962679791
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -248,7 +248,30 @@ def import_dataset( # noqa: C901
# implicit-restore re-import is a clean replacement, not a merge.
is_soft_deleted_match = False
- if existing := find_existing_for_import(SqlaTable, config["uuid"]):
+ existing = find_existing_for_import(SqlaTable, config["uuid"])
+ if not existing and can_write:
+ # A config carrying a fresh UUID but a (database, catalog, schema,
+ # table) identity that already belongs to an active dataset would be
+ # matched-and-updated on that unique key by ``import_from_dict``,
+ # without passing through the overwrite gate below (which only ran on a
+ # UUID match). Resolve that collision to the existing dataset here so
+ # the same gate applies; align the UUID so the subsequent import updates
+ # that row deterministically. (Soft-deleted twins are handled in the
+ # create branch further down.)
+ active_twin = (
+ db.session.query(SqlaTable)
+ .filter_by(
+ database_id=config["database_id"],
+ catalog=config.get("catalog"),
+ schema=config.get("schema"),
+ table_name=config["table_name"],
+ )
+ .first()
Review Comment:
Fixed in d1cc301. The probe now goes through a shared `find_existing_by_import_identity` helper that derives its predicate from the model unique-constraint metadata and drops keys the config leaves null, exactly as `import_from_dict` does, so a catalog-less config still resolves a dataset stored under a catalog. Covered by a parametrized regression test.
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -248,7 +248,30 @@ def import_dataset( # noqa: C901
# implicit-restore re-import is a clean replacement, not a merge.
is_soft_deleted_match = False
- if existing := find_existing_for_import(SqlaTable, config["uuid"]):
+ existing = find_existing_for_import(SqlaTable, config["uuid"])
+ if not existing and can_write:
+ # A config carrying a fresh UUID but a (database, catalog, schema,
+ # table) identity that already belongs to an active dataset would be
+ # matched-and-updated on that unique key by ``import_from_dict``,
+ # without passing through the overwrite gate below (which only ran on a
+ # UUID match). Resolve that collision to the existing dataset here so
+ # the same gate applies; align the UUID so the subsequent import updates
+ # that row deterministically. (Soft-deleted twins are handled in the
+ # create branch further down.)
+ active_twin = (
+ db.session.query(SqlaTable)
+ .filter_by(
+ database_id=config["database_id"],
+ catalog=config.get("catalog"),
+ schema=config.get("schema"),
+ table_name=config["table_name"],
+ )
+ .first()
+ )
+ if active_twin is not None:
+ config["uuid"] = str(active_twin.uuid)
+ existing = find_existing_for_import(SqlaTable, config["uuid"])
Review Comment:
Fixed in d1cc301. The UUID rewrite is gone entirely rather than copied around: `import_from_dict` already resolves the row by that identity, so aligning the UUID was redundant. The caller config is left untouched, with a test asserting it keeps its own UUID after a collision resolves.
--
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(dataset): apply the import overwrite permission check to identity-matched datasets [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #43058:
URL: https://github.com/apache/superset/pull/43058
--
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