You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "goldjee (via GitHub)" <gi...@apache.org> on 2026/02/12 15:10:36 UTC
[PR] fix(dataset): resolve duplicate virtual dataset validation error [superset]
goldjee opened a new pull request, #37932:
URL: https://github.com/apache/superset/pull/37932
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
Imagine a user who doesn't have "all database access" or "all datasource access" permissions, but has access to a certain set of database connections and can create/edit datasources pointing to them. Current implementation does not allow such users to duplicate their datasets, displaying vague 500 error.
This PR fixes duplication checks that are present in `superset/commands/dataset/duplicate.py` and allows users to create copies of their datasets.
### TESTING INSTRUCTIONS
Assuming that the test would be carried out on an installation of Superset with stock example (meta)data,
1. Create a copy of Alpha role and remove "all database access" and "all datasource access" permissions from it. Add "database access [examples]" permission to this role.
2. Create a new user and assign this role and "sql_lab" to it.
3. Log into Superset as this user and verify it has access to example datasets.
4. Create a new virtual dataset with some query (`select 1 as c`, for example).
5. On "Datasets" page, find your newly created dataset and hit "Duplicate" button.
6. Fill in a new unique name for the copy and hit "Duplicate" button.
Superset should create a copy of your test dataset with a new name, displaying no errors.
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes [#27712](https://github.com/apache/superset/issues/27712)
- [ ] 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(dataset): resolve duplicate virtual dataset validation error [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #37932:
URL: https://github.com/apache/superset/pull/37932#issuecomment-4732418477
Thanks @goldjee, and nice root-cause writeup. Reusing `validate_uniqueness` the way `CreateDatasetCommand` does is great, and copying `catalog` is a good catch. It conflicts with master now, so a rebase would set it up for review. I'll reopen #27712 since folks are still hitting this on 5.0 and 6.0.
--
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): resolve duplicate virtual dataset validation error [superset]
Posted by "goldjee (via GitHub)" <gi...@apache.org>.
goldjee commented on code in PR #37932:
URL: https://github.com/apache/superset/pull/37932#discussion_r2800328840
##########
tests/unit_tests/commands/dataset/test_duplicate.py:
##########
@@ -0,0 +1,363 @@
+# 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.
+from unittest.mock import MagicMock, Mock, patch
+
+import pytest
+
+from superset.commands.dataset.duplicate import DuplicateDatasetCommand
+from superset.commands.dataset.exceptions import (
+ DatasetExistsValidationError,
+ DatasetInvalidError,
+ DatasetNotFoundError,
+)
+from superset.commands.exceptions import DatasourceTypeInvalidError
+from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn
+from superset.models.core import Database
+
+
+def test_duplicate_dataset_success():
+ """Test successful duplication of a virtual dataset."""
+ # Create a mock base model (virtual dataset)
+ mock_base_model = Mock(spec=SqlaTable)
+ mock_base_model.id = 1
+ mock_base_model.database_id = 1
+ mock_base_model.table_name = "original_dataset"
+ mock_base_model.schema = "public"
+ mock_base_model.catalog = None
+ mock_base_model.kind = "virtual"
+ mock_base_model.sql = "SELECT 1 as c"
+ mock_base_model.template_params = None
+ mock_base_model.normalize_columns = False
+ mock_base_model.always_filter_main_dttm = False
+ mock_base_model.columns = []
+ mock_base_model.metrics = []
+
+ mock_database = Mock(spec=Database)
+ mock_database.id = 1
+ mock_database.get_default_catalog.return_value = None
+
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=mock_base_model,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.db.session.query"
+ ) as mock_query:
+ mock_query.return_value.get.return_value = mock_database
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.validate_uniqueness",
+ return_value=True,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DuplicateDatasetCommand.populate_owners",
+ return_value=[],
+ ):
+ with patch("superset.commands.dataset.duplicate.db.session.add"):
+ command = DuplicateDatasetCommand(
+ {
+ "base_model_id": 1,
+ "table_name": "duplicated_dataset",
+ "owners": [],
+ }
+ )
+
+ # Should not raise any errors
+ command.validate()
Review Comment:
Fixed
--
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): resolve duplicate virtual dataset validation error [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #37932:
URL: https://github.com/apache/superset/pull/37932
--
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): resolve duplicate virtual dataset validation error [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 #37932:
URL: https://github.com/apache/superset/pull/37932#discussion_r2799467494
##########
superset/commands/dataset/duplicate.py:
##########
@@ -66,6 +66,7 @@ def run(self) -> Model:
table = SqlaTable(table_name=table_name, owners=owners)
table.database = database
table.schema = self._base_model.schema
+ table.catalog = self._base_model.catalog
Review Comment:
**Suggestion:** When duplicating a virtual dataset whose `catalog` is not explicitly set, validation uses the database's default catalog to check uniqueness, but the duplicated dataset itself is saved with a `None` catalog, which is inconsistent with the uniqueness logic and the CreateDatasetCommand behavior and can lead to later duplicate-name checks misbehaving for catalog-aware databases. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dataset duplication may overlook existing duplicates in catalogs.
- ⚠️ Catalog-aware DBs get inconsistent uniqueness vs stored metadata.
- ⚠️ Dataset management UI may allow logically duplicate datasets.
```
</details>
```suggestion
table.catalog = self._base_model.catalog or database.get_default_catalog()
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a catalog-aware database (e.g. Trino) in Superset with a non-null default
catalog (via Database model at `superset/models/core.py`, then referenced in
`Database.get_default_catalog()` used in `superset/commands/dataset/duplicate.py:121`).
2. Create a virtual dataset on this database so that its `SqlaTable` record has `catalog`
unset/NULL (this dataset later becomes `self._base_model` inside `DuplicateDatasetCommand`
in `superset/commands/dataset/duplicate.py`).
3. From the Datasets list UI, click "Duplicate" for that virtual dataset and submit a new
name; this triggers `DuplicateDatasetCommand.run()` at
`superset/commands/dataset/duplicate.py:51`, which calls `validate()` at line 105.
4. In `validate()`, uniqueness is checked using `catalog = self._base_model.catalog or
database.get_default_catalog()` at line 121 (i.e. the database default catalog), but in
`run()` the new `SqlaTable` is persisted with `table.catalog = self._base_model.catalog`
at line 69 (NULL), so the stored catalog for the duplicated dataset does not match the
catalog used for uniqueness checks. This mismatch can cause later duplicate-name
validation (using the resolved catalog) to overlook this dataset for catalog-aware
databases, allowing logically duplicate datasets pointing at the same catalog/schema/table
to be created.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/dataset/duplicate.py
**Line:** 69:69
**Comment:**
*Logic Error: When duplicating a virtual dataset whose `catalog` is not explicitly set, validation uses the database's default catalog to check uniqueness, but the duplicated dataset itself is saved with a `None` catalog, which is inconsistent with the uniqueness logic and the CreateDatasetCommand behavior and can lead to later duplicate-name checks misbehaving for catalog-aware databases.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37932&comment_hash=133252f12f563e5a75f2df5fb845e805439b8efa2b7123896b28cb861f58c64f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37932&comment_hash=133252f12f563e5a75f2df5fb845e805439b8efa2b7123896b28cb861f58c64f&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): resolve duplicate virtual dataset validation error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37932:
URL: https://github.com/apache/superset/pull/37932#issuecomment-3891515498
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): resolve duplicate virtual dataset validation error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #37932:
URL: https://github.com/apache/superset/pull/37932#discussion_r2800330747
##########
tests/unit_tests/commands/dataset/test_duplicate.py:
##########
@@ -0,0 +1,363 @@
+# 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.
+from unittest.mock import MagicMock, Mock, patch
+
+import pytest
+
+from superset.commands.dataset.duplicate import DuplicateDatasetCommand
+from superset.commands.dataset.exceptions import (
+ DatasetExistsValidationError,
+ DatasetInvalidError,
+ DatasetNotFoundError,
+)
+from superset.commands.exceptions import DatasourceTypeInvalidError
+from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn
+from superset.models.core import Database
+
+
+def test_duplicate_dataset_success():
+ """Test successful duplication of a virtual dataset."""
+ # Create a mock base model (virtual dataset)
+ mock_base_model = Mock(spec=SqlaTable)
+ mock_base_model.id = 1
+ mock_base_model.database_id = 1
+ mock_base_model.table_name = "original_dataset"
+ mock_base_model.schema = "public"
+ mock_base_model.catalog = None
+ mock_base_model.kind = "virtual"
+ mock_base_model.sql = "SELECT 1 as c"
+ mock_base_model.template_params = None
+ mock_base_model.normalize_columns = False
+ mock_base_model.always_filter_main_dttm = False
+ mock_base_model.columns = []
+ mock_base_model.metrics = []
+
+ mock_database = Mock(spec=Database)
+ mock_database.id = 1
+ mock_database.get_default_catalog.return_value = None
+
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=mock_base_model,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.db.session.query"
+ ) as mock_query:
+ mock_query.return_value.get.return_value = mock_database
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.validate_uniqueness",
+ return_value=True,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DuplicateDatasetCommand.populate_owners",
+ return_value=[],
+ ):
+ with patch("superset.commands.dataset.duplicate.db.session.add"):
+ command = DuplicateDatasetCommand(
+ {
+ "base_model_id": 1,
+ "table_name": "duplicated_dataset",
+ "owners": [],
+ }
+ )
+
+ # Should not raise any errors
+ command.validate()
Review Comment:
<!-- Bito Reply -->
This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.
--
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): resolve duplicate virtual dataset validation error [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #37932:
URL: https://github.com/apache/superset/pull/37932#issuecomment-3892573595
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 8f0d4c770dab0461aea0631eb6119f20c2ec131b |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/698e16499ee3a900082829cb |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-37932--superset-docs-preview.netlify.app](https://deploy-preview-37932--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> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-37932--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): resolve duplicate virtual dataset validation error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37932:
URL: https://github.com/apache/superset/pull/37932#issuecomment-4917263050
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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): resolve duplicate virtual dataset validation error [superset]
Posted by "goldjee (via GitHub)" <gi...@apache.org>.
goldjee commented on code in PR #37932:
URL: https://github.com/apache/superset/pull/37932#discussion_r2799510255
##########
superset/commands/dataset/duplicate.py:
##########
@@ -66,6 +66,7 @@ def run(self) -> Model:
table = SqlaTable(table_name=table_name, owners=owners)
table.database = database
table.schema = self._base_model.schema
+ table.catalog = self._base_model.catalog
Review Comment:
This check is performed in this exact manner at [line 121](https://github.com/apache/superset/pull/37932/changes#diff-38934e7029af81490bbc931c312e2e8970c7988b269b55bdd37d07e24f57009cR121).
##########
superset/commands/dataset/duplicate.py:
##########
@@ -66,6 +66,7 @@ def run(self) -> Model:
table = SqlaTable(table_name=table_name, owners=owners)
table.database = database
table.schema = self._base_model.schema
+ table.catalog = self._base_model.catalog
Review Comment:
This check is performed in this exact manner at [line 121](https://github.com/apache/superset/pull/37932/changes#diff-38934e7029af81490bbc931c312e2e8970c7988b269b55bdd37d07e24f57009cR121).
--
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): resolve duplicate virtual dataset validation error [superset]
Posted by "goldjee (via GitHub)" <gi...@apache.org>.
goldjee commented on code in PR #37932:
URL: https://github.com/apache/superset/pull/37932#discussion_r2800338528
##########
superset/commands/dataset/duplicate.py:
##########
@@ -66,6 +66,7 @@ def run(self) -> Model:
table = SqlaTable(table_name=table_name, owners=owners)
table.database = database
table.schema = self._base_model.schema
+ table.catalog = self._base_model.catalog
Review Comment:
Duplicates should preserve base model properties. This is intuitive and expected behavior. I have fixed the inconsistency, though, by removing `or database.get_default_catalog()` from the validation function.
--
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): resolve duplicate virtual dataset validation error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37932:
URL: https://github.com/apache/superset/pull/37932#issuecomment-3892883614
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 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): resolve duplicate virtual dataset validation error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #37932:
URL: https://github.com/apache/superset/pull/37932#discussion_r2803959121
##########
tests/unit_tests/commands/dataset/test_duplicate.py:
##########
@@ -0,0 +1,373 @@
+# 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.
+from unittest.mock import MagicMock, Mock, patch
+
+import pytest
+
+from superset.commands.dataset.duplicate import DuplicateDatasetCommand
+from superset.commands.dataset.exceptions import (
+ DatasetExistsValidationError,
+ DatasetInvalidError,
+ DatasetNotFoundError,
+)
+from superset.commands.exceptions import DatasourceTypeInvalidError
+from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn
+from superset.models.core import Database
+
+
+def test_duplicate_dataset_success():
+ """Test successful duplication of a virtual dataset."""
+ # Create a mock base model (virtual dataset)
+ mock_base_model = Mock(spec=SqlaTable)
+ mock_base_model.id = 1
+ mock_base_model.database_id = 1
+ mock_base_model.table_name = "original_dataset"
+ mock_base_model.schema = "public"
+ mock_base_model.catalog = None
+ mock_base_model.kind = "virtual"
+ mock_base_model.sql = "SELECT 1 as c"
+ mock_base_model.template_params = None
+ mock_base_model.normalize_columns = False
+ mock_base_model.always_filter_main_dttm = False
+ mock_base_model.columns = []
+ mock_base_model.metrics = []
+
+ # Create a mock database without spec to allow SQLAlchemy relationship assignment
+ mock_database = MagicMock()
+ mock_database.id = 1
+ mock_database.get_default_catalog.return_value = None
+ # Add _sa_instance_state to allow SQLAlchemy relationship assignment
+ mock_database._sa_instance_state = MagicMock()
+
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=mock_base_model,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.db.session.query"
+ ) as mock_query:
+ mock_query.return_value.get.return_value = mock_database
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.validate_uniqueness",
+ return_value=True,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DuplicateDatasetCommand.populate_owners",
+ return_value=[],
+ ):
+ with patch("superset.commands.dataset.duplicate.db.session.add"):
+ command = DuplicateDatasetCommand(
+ {
+ "base_model_id": 1,
+ "table_name": "duplicated_dataset",
+ "owners": [],
+ }
+ )
+
+ # Should not raise any errors
+ command.validate()
+ # Test the actual duplication
+ result = command.run()
+ assert result.table_name == "duplicated_dataset"
+ assert result.database.id == 1
+ assert result.schema == "public"
+ assert result.is_sqllab_view is True
+ assert result.sql == "SELECT 1 as c"
+
+
+def test_duplicate_dataset_not_found():
+ """Test duplication when base dataset doesn't exist."""
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=None,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DuplicateDatasetCommand.populate_owners",
+ return_value=[],
+ ):
+ command = DuplicateDatasetCommand(
+ {
+ "base_model_id": 999,
+ "table_name": "duplicated_dataset",
+ "owners": [],
+ }
+ )
+
+ with pytest.raises(DatasetInvalidError) as exc_info:
+ command.validate()
+
+ # Verify the exception contains DatasetNotFoundError
+ validation_errors = exc_info.value._exceptions
+ assert any(
+ isinstance(error, DatasetNotFoundError) for error in validation_errors
+ )
+
+
+def test_duplicate_dataset_not_virtual():
+ """Test that duplication fails for physical (non-virtual) datasets."""
+ mock_base_model = Mock(spec=SqlaTable)
+ mock_base_model.id = 1
+ mock_base_model.database_id = 1
+ mock_base_model.kind = "physical" # Not virtual
+
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=mock_base_model,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DuplicateDatasetCommand.populate_owners",
+ return_value=[],
+ ):
+ command = DuplicateDatasetCommand(
+ {
+ "base_model_id": 1,
+ "table_name": "duplicated_dataset",
+ "owners": [],
+ }
+ )
+
+ with pytest.raises(DatasetInvalidError) as exc_info:
+ command.validate()
+
+ # Verify the exception contains DatasourceTypeInvalidError
+ validation_errors = exc_info.value._exceptions
+ assert len(validation_errors) == 1
+ assert isinstance(validation_errors[0], DatasourceTypeInvalidError)
+
+
+def test_duplicate_dataset_name_exists_same_database_schema():
+ """Test that duplication fails when name exists in same database/schema."""
+ mock_base_model = Mock(spec=SqlaTable)
+ mock_base_model.id = 1
+ mock_base_model.database_id = 1
+ mock_base_model.table_name = "original_dataset"
+ mock_base_model.schema = "public"
+ mock_base_model.catalog = None
+ mock_base_model.kind = "virtual"
+
+ mock_database = Mock(spec=Database)
+ mock_database.id = 1
+ mock_database.get_default_catalog.return_value = None
+
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=mock_base_model,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.db.session.query"
+ ) as mock_query:
+ mock_query.return_value.get.return_value = mock_database
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.validate_uniqueness",
+ return_value=False, # Name already exists
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DuplicateDatasetCommand.populate_owners",
+ return_value=[],
+ ):
+ command = DuplicateDatasetCommand(
+ {
+ "base_model_id": 1,
+ "table_name": "existing_dataset",
+ "owners": [],
+ }
+ )
+
+ with pytest.raises(DatasetInvalidError) as exc_info:
+ command.validate()
+
+ # Verify the exception contains DatasetExistsValidationError
+ validation_errors = exc_info.value._exceptions
+ assert len(validation_errors) == 1
+ assert isinstance(
+ validation_errors[0], DatasetExistsValidationError
+ )
+
+
+def test_duplicate_dataset_catalog_preserved():
+ """Test that catalog is preserved during duplication."""
+ mock_base_model = Mock(spec=SqlaTable)
+ mock_base_model.id = 1
+ mock_base_model.database_id = 1
+ mock_base_model.table_name = "original_dataset"
+ mock_base_model.schema = "public"
+ mock_base_model.catalog = "prod_catalog"
+ mock_base_model.kind = "virtual"
+ mock_base_model.sql = "SELECT 1 as c"
+ mock_base_model.template_params = None
+ mock_base_model.normalize_columns = False
+ mock_base_model.always_filter_main_dttm = False
+ mock_base_model.columns = []
+ mock_base_model.metrics = []
+
+ # Create a mock database without spec to allow SQLAlchemy relationship assignment
+ mock_database = MagicMock()
+ mock_database.id = 1
+ mock_database.get_default_catalog.return_value = None
+ # Add _sa_instance_state to allow SQLAlchemy relationship assignment
+ mock_database._sa_instance_state = MagicMock()
+
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=mock_base_model,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.db.session.query"
+ ) as mock_query:
+ mock_query.return_value.get.return_value = mock_database
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.validate_uniqueness",
+ return_value=True,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DuplicateDatasetCommand.populate_owners",
+ return_value=[],
+ ):
+ with patch("superset.commands.dataset.duplicate.db.session.add"):
+ command = DuplicateDatasetCommand(
+ {
+ "base_model_id": 1,
+ "table_name": "duplicated_dataset",
+ "owners": [],
+ }
+ )
+
+ result = command.run()
+
+ # Verify catalog was preserved
+ assert result.catalog == "prod_catalog"
+
+
+def test_duplicate_dataset_default_catalog_used():
+ """Test that catalog=None is preserved when base model has no catalog."""
+ mock_base_model = Mock(spec=SqlaTable)
+ mock_base_model.id = 1
+ mock_base_model.database_id = 1
+ mock_base_model.table_name = "original_dataset"
+ mock_base_model.schema = "public"
+ mock_base_model.catalog = None
+ mock_base_model.kind = "virtual"
+
+ mock_database = Mock(spec=Database)
+ mock_database.id = 1
+ mock_database.get_default_catalog.return_value = "default_catalog"
+
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=mock_base_model,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.db.session.query"
+ ) as mock_query:
+ mock_query.return_value.get.return_value = mock_database
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.validate_uniqueness",
+ return_value=True,
+ ) as mock_validate:
+ with patch(
+ "superset.commands.dataset.duplicate.DuplicateDatasetCommand.populate_owners",
+ return_value=[],
+ ):
+ command = DuplicateDatasetCommand(
+ {
+ "base_model_id": 1,
+ "table_name": "duplicated_dataset",
+ "owners": [],
+ }
+ )
+
+ command.validate()
+
+ # Verify catalog=None is resolved to default catalog for validation
+ mock_validate.assert_called_once()
+ call_args = mock_validate.call_args
+ table_arg = call_args[0][1] # Second argument is the Table object
+ assert table_arg.catalog == "default_catalog"
+
+
+def test_duplicate_dataset_with_columns_and_metrics():
+ """Test duplication preserves columns and metrics."""
+ mock_column = Mock(spec=TableColumn)
+ mock_column.column_name = "col1"
+ mock_column.verbose_name = "Column 1"
+ mock_column.expression = None
+ mock_column.is_dttm = False
+ mock_column.type = "VARCHAR"
+ mock_column.description = "Test column"
+
+ mock_metric = Mock(spec=SqlMetric)
+ mock_metric.metric_name = "count"
+ mock_metric.verbose_name = "Count"
+ mock_metric.expression = "COUNT(*)"
+ mock_metric.metric_type = "count"
+ mock_metric.description = "Row count"
+
+ mock_base_model = Mock(spec=SqlaTable)
+ mock_base_model.id = 1
+ mock_base_model.database_id = 1
+ mock_base_model.table_name = "original_dataset"
+ mock_base_model.schema = "public"
+ mock_base_model.catalog = None
+ mock_base_model.kind = "virtual"
+ mock_base_model.sql = "SELECT 1 as c"
+ mock_base_model.template_params = None
+ mock_base_model.normalize_columns = False
+ mock_base_model.always_filter_main_dttm = False
+ mock_base_model.columns = [mock_column]
+ mock_base_model.metrics = [mock_metric]
+
+ # Create a mock database without spec to allow SQLAlchemy relationship assignment
+ mock_database = MagicMock()
+ mock_database.id = 1
+ mock_database.get_default_catalog.return_value = None
+ # Add _sa_instance_state to allow SQLAlchemy relationship assignment
+ mock_database._sa_instance_state = MagicMock()
+
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=mock_base_model,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.db.session.query"
+ ) as mock_query:
+ mock_query.return_value.get.return_value = mock_database
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.validate_uniqueness",
+ return_value=True,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DuplicateDatasetCommand.populate_owners",
+ return_value=[],
+ ):
+ with patch("superset.commands.dataset.duplicate.db.session.add"):
+ command = DuplicateDatasetCommand(
+ {
+ "base_model_id": 1,
+ "table_name": "duplicated_dataset",
+ "owners": [],
+ }
+ )
+
+ result = command.run()
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incomplete test assertions</b></div>
<div id="fix">
The test 'test_duplicate_dataset_with_columns_and_metrics' executes the duplication but lacks assertions to confirm that columns and metrics are preserved in the result. Per BITO.md rule [6262], tests must assert the actual behavior they claim to verify. Add checks like assert len(result.columns) == 1, assert result.columns[0].column_name == 'col1', and similar for metrics to ensure copying logic works.
</div>
</div>
<small><i>Code Review Run #cba412</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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): resolve duplicate virtual dataset validation error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37932:
URL: https://github.com/apache/superset/pull/37932#issuecomment-3896873607
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 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): resolve duplicate virtual dataset validation error [superset]
Posted by "goldjee (via GitHub)" <gi...@apache.org>.
goldjee commented on PR #37932:
URL: https://github.com/apache/superset/pull/37932#issuecomment-4915991172
Seems #40130 has already fixed this very issue. I've resolved the conflict again and will leave this PR open for the sake of the unit tests.
--
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): resolve duplicate virtual dataset validation error [superset]
Posted by "goldjee (via GitHub)" <gi...@apache.org>.
goldjee commented on PR #37932:
URL: https://github.com/apache/superset/pull/37932#issuecomment-4875651486
Hi @rusackas, I've resolved the conflicts. Could you please take a look?
--
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): resolve duplicate virtual dataset validation error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37932:
URL: https://github.com/apache/superset/pull/37932#issuecomment-4876463404
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 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): resolve duplicate virtual dataset validation error [superset]
Posted by "goldjee (via GitHub)" <gi...@apache.org>.
goldjee commented on code in PR #37932:
URL: https://github.com/apache/superset/pull/37932#discussion_r2799512238
##########
superset/commands/dataset/duplicate.py:
##########
@@ -66,6 +66,7 @@ def run(self) -> Model:
table = SqlaTable(table_name=table_name, owners=owners)
table.database = database
table.schema = self._base_model.schema
+ table.catalog = self._base_model.catalog
Review Comment:
This check is performed in this exact manner at [line 121](https://github.com/apache/superset/pull/37932/changes#diff-38934e7029af81490bbc931c312e2e8970c7988b269b55bdd37d07e24f57009cR121). Is it sufficient?
--
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): resolve duplicate virtual dataset validation error [superset]
Posted by "goldjee (via GitHub)" <gi...@apache.org>.
goldjee commented on code in PR #37932:
URL: https://github.com/apache/superset/pull/37932#discussion_r2800339593
##########
superset/commands/dataset/duplicate.py:
##########
@@ -66,6 +66,7 @@ def run(self) -> Model:
table = SqlaTable(table_name=table_name, owners=owners)
table.database = database
table.schema = self._base_model.schema
+ table.catalog = self._base_model.catalog
Review Comment:
Duplicates should preserve base model properties. This is intuitive and expected behavior. I have fixed the inconsistency, though, by removing `or database.get_default_catalog()` from the validation function.
--
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): resolve duplicate virtual dataset validation error [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #37932:
URL: https://github.com/apache/superset/pull/37932#issuecomment-4916418390
Always love having more tests! In she goes!!
--
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): resolve duplicate virtual dataset validation error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #37932:
URL: https://github.com/apache/superset/pull/37932#discussion_r3519978490
##########
tests/unit_tests/commands/dataset/test_duplicate.py:
##########
@@ -84,20 +100,387 @@ def test_duplicate_dataset_access_check_passes_through() -> None:
"superset.commands.dataset.duplicate.security_manager.raise_for_access"
) as mock_access:
with patch(
- "superset.commands.dataset.duplicate.DatasetDAO.find_one_or_none",
- return_value=None,
+ "superset.commands.dataset.duplicate.db.session.query"
+ ) as mock_query:
+ mock_query.return_value.get.return_value = mock_database
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.validate_uniqueness",
+ return_value=True,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DuplicateDatasetCommand.populate_owners",
+ return_value=[],
+ ):
+ command = DuplicateDatasetCommand(
+ {
+ "base_model_id": 1,
+ "table_name": "new_unique_name",
+ "is_managed_externally": False,
+ }
+ )
+ command.validate() # should not raise
+ # Confirm access check was called with the base dataset
+ mock_access.assert_called_once_with(datasource=mock_dataset)
+
+
+def test_duplicate_dataset_success():
+ """Test successful duplication of a virtual dataset."""
+ # Create a mock base model (virtual dataset)
+ mock_base_model = Mock(spec=SqlaTable)
+ mock_base_model.id = 1
+ mock_base_model.database_id = 1
+ mock_base_model.table_name = "original_dataset"
+ mock_base_model.schema = "public"
+ mock_base_model.catalog = None
+ mock_base_model.kind = "virtual"
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Consolidate duplicated test setup mocks</b></div>
<div id="fix">
Code duplication detected in test file `tests/unit_tests/commands/dataset/test_duplicate.py` across multiple locations (135-176, 312-358, 441-481). The mock setup pattern for base_model and database is repeated with minor variations. Consider extracting into a shared fixture to improve maintainability.
</div>
</div>
<small><i>Code Review Run #6fb8dc</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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): resolve duplicate virtual dataset validation error [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #37932:
URL: https://github.com/apache/superset/pull/37932#issuecomment-3891575720
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/37932?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 65.93%. Comparing base ([`7714827`](https://app.codecov.io/gh/apache/superset/commit/77148277b96b1a3b1a60b11d364d2f92659cbd04?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`106190e`](https://app.codecov.io/gh/apache/superset/commit/106190e0f100f62298f91e36f3093102dbec9788?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 3 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #37932 +/- ##
===========================================
+ Coverage 0 65.93% +65.93%
===========================================
Files 0 668 +668
Lines 0 51269 +51269
Branches 0 5771 +5771
===========================================
+ Hits 0 33804 +33804
- Misses 0 16070 +16070
- Partials 0 1395 +1395
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/37932/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/37932/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.50% <0.00%> (?)` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/37932/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.52% <0.00%> (?)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/37932/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `65.92% <100.00%> (?)` | |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/37932/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `64.26% <100.00%> (?)` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/37932/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (?)` | |
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 Sentry](https://app.codecov.io/gh/apache/superset/pull/37932?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): resolve duplicate virtual dataset validation error [superset]
Posted by "goldjee (via GitHub)" <gi...@apache.org>.
goldjee commented on code in PR #37932:
URL: https://github.com/apache/superset/pull/37932#discussion_r2799512238
##########
superset/commands/dataset/duplicate.py:
##########
@@ -66,6 +66,7 @@ def run(self) -> Model:
table = SqlaTable(table_name=table_name, owners=owners)
table.database = database
table.schema = self._base_model.schema
+ table.catalog = self._base_model.catalog
Review Comment:
This check is performed in this exact manner at [line 121](https://github.com/apache/superset/pull/37932/changes#diff-38934e7029af81490bbc931c312e2e8970c7988b269b55bdd37d07e24f57009cR121). Is it sufficient?
--
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): resolve duplicate virtual dataset validation error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37932:
URL: https://github.com/apache/superset/pull/37932#issuecomment-3892192491
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 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): resolve duplicate virtual dataset validation error [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 #37932:
URL: https://github.com/apache/superset/pull/37932#discussion_r3544341247
##########
tests/unit_tests/commands/dataset/test_duplicate.py:
##########
@@ -165,3 +170,360 @@ def test_duplicate_dataset_blocked_by_soft_deleted_twin(session: Session) -> Non
)
with pytest.raises(DatasetInvalidError):
command.validate()
+
+
+def test_duplicate_dataset_success():
Review Comment:
**Suggestion:** Add an explicit return type annotation to this test function signature (for example, `-> None`). [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The function signature in the final file has no return type annotation. The custom rule requires new or modified Python functions to include type hints, so this is a real violation.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=197e45d7df7c4ddb9ba15f7f4f0cf0eb&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=197e45d7df7c4ddb9ba15f7f4f0cf0eb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/commands/dataset/test_duplicate.py
**Line:** 175:175
**Comment:**
*Custom Rule: Add an explicit return type annotation to this test function signature (for example, `-> None`).
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%2F37932&comment_hash=e3e43473f32cb0c2a47a6da2c886d501afe19e7c3e933a3ee11b83a518bc619a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37932&comment_hash=e3e43473f32cb0c2a47a6da2c886d501afe19e7c3e933a3ee11b83a518bc619a&reaction=dislike'>👎</a>
##########
tests/unit_tests/commands/dataset/test_duplicate.py:
##########
@@ -165,3 +170,360 @@ def test_duplicate_dataset_blocked_by_soft_deleted_twin(session: Session) -> Non
)
with pytest.raises(DatasetInvalidError):
command.validate()
+
+
+def test_duplicate_dataset_success():
+ """Test successful duplication of a virtual dataset."""
+ # Create a mock base model (virtual dataset)
+ mock_base_model = Mock(spec=SqlaTable)
+ mock_base_model.id = 1
+ mock_base_model.database_id = 1
+ mock_base_model.table_name = "original_dataset"
+ mock_base_model.schema = "public"
+ mock_base_model.catalog = None
+ mock_base_model.kind = "virtual"
+ mock_base_model.sql = "SELECT 1 as c"
+ mock_base_model.template_params = None
+ mock_base_model.normalize_columns = False
+ mock_base_model.always_filter_main_dttm = False
+ mock_base_model.columns = []
+ mock_base_model.metrics = []
+
+ # Create a mock database without spec to allow SQLAlchemy relationship assignment
+ mock_database = MagicMock()
+ mock_database.id = 1
+ mock_database.get_default_catalog.return_value = None
+ # Add _sa_instance_state to allow SQLAlchemy relationship assignment
+ mock_database._sa_instance_state = MagicMock()
+
+ with (
+ patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=mock_base_model,
+ ),
+ patch("superset.commands.dataset.duplicate.security_manager.raise_for_access"),
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.db.session.query"
+ ) as mock_query:
+ mock_query.return_value.get.return_value = mock_database
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.validate_uniqueness",
+ return_value=True,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DuplicateDatasetCommand.populate_owners",
+ return_value=[],
+ ):
+ with patch("superset.commands.dataset.duplicate.db.session.add"):
+ command = DuplicateDatasetCommand(
+ {
+ "base_model_id": 1,
+ "table_name": "duplicated_dataset",
+ "owners": [],
+ }
+ )
+
+ # Should not raise any errors
+ command.validate()
+ # Test the actual duplication
+ result = command.run()
+ assert result.table_name == "duplicated_dataset"
+ assert result.database.id == 1
+ assert result.schema == "public"
+ assert result.is_sqllab_view is True
+ assert result.sql == "SELECT 1 as c"
+
+
+def test_duplicate_dataset_not_found():
+ """Test duplication when base dataset doesn't exist."""
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=None,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DuplicateDatasetCommand.populate_owners",
+ return_value=[],
+ ):
+ command = DuplicateDatasetCommand(
+ {
+ "base_model_id": 999,
+ "table_name": "duplicated_dataset",
+ "owners": [],
+ }
+ )
+
+ with pytest.raises(DatasetInvalidError) as exc_info:
+ command.validate()
+
+ # Verify the exception contains DatasetNotFoundError
+ validation_errors = exc_info.value._exceptions
+ assert any(
+ isinstance(error, DatasetNotFoundError) for error in validation_errors
+ )
+
+
+def test_duplicate_dataset_not_virtual():
+ """Test that duplication fails for physical (non-virtual) datasets."""
+ mock_base_model = Mock(spec=SqlaTable)
+ mock_base_model.id = 1
+ mock_base_model.database_id = 1
+ mock_base_model.kind = "physical" # Not virtual
+
+ with (
+ patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=mock_base_model,
+ ),
+ patch("superset.commands.dataset.duplicate.security_manager.raise_for_access"),
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.validate_uniqueness",
+ return_value=True,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DuplicateDatasetCommand.populate_owners",
+ return_value=[],
+ ):
+ command = DuplicateDatasetCommand(
+ {
+ "base_model_id": 1,
+ "table_name": "duplicated_dataset",
+ "owners": [],
+ }
+ )
+
+ with pytest.raises(DatasetInvalidError) as exc_info:
+ command.validate()
+
+ # Verify the exception contains DatasourceTypeInvalidError
+ validation_errors = exc_info.value._exceptions
+ assert len(validation_errors) == 1
+ assert isinstance(validation_errors[0], DatasourceTypeInvalidError)
+
+
+def test_duplicate_dataset_name_exists_same_database_schema():
Review Comment:
**Suggestion:** Add an explicit return type annotation to this test function signature (for example, `-> None`). [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added test function lacks an explicit return type annotation in the final file, which matches the custom typing rule violation.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bf4f8ee8d0fc4863a17fefa9ae08596c&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=bf4f8ee8d0fc4863a17fefa9ae08596c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/commands/dataset/test_duplicate.py
**Line:** 305:305
**Comment:**
*Custom Rule: Add an explicit return type annotation to this test function signature (for example, `-> None`).
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%2F37932&comment_hash=72dbb29542640733d53648c68a06bf261e909a382c657a8f55c16c551e4279b6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37932&comment_hash=72dbb29542640733d53648c68a06bf261e909a382c657a8f55c16c551e4279b6&reaction=dislike'>👎</a>
##########
tests/unit_tests/commands/dataset/test_duplicate.py:
##########
@@ -165,3 +170,360 @@ def test_duplicate_dataset_blocked_by_soft_deleted_twin(session: Session) -> Non
)
with pytest.raises(DatasetInvalidError):
command.validate()
+
+
+def test_duplicate_dataset_success():
+ """Test successful duplication of a virtual dataset."""
+ # Create a mock base model (virtual dataset)
+ mock_base_model = Mock(spec=SqlaTable)
+ mock_base_model.id = 1
+ mock_base_model.database_id = 1
+ mock_base_model.table_name = "original_dataset"
+ mock_base_model.schema = "public"
+ mock_base_model.catalog = None
+ mock_base_model.kind = "virtual"
+ mock_base_model.sql = "SELECT 1 as c"
+ mock_base_model.template_params = None
+ mock_base_model.normalize_columns = False
+ mock_base_model.always_filter_main_dttm = False
+ mock_base_model.columns = []
+ mock_base_model.metrics = []
+
+ # Create a mock database without spec to allow SQLAlchemy relationship assignment
+ mock_database = MagicMock()
+ mock_database.id = 1
+ mock_database.get_default_catalog.return_value = None
+ # Add _sa_instance_state to allow SQLAlchemy relationship assignment
+ mock_database._sa_instance_state = MagicMock()
+
+ with (
+ patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=mock_base_model,
+ ),
+ patch("superset.commands.dataset.duplicate.security_manager.raise_for_access"),
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.db.session.query"
+ ) as mock_query:
+ mock_query.return_value.get.return_value = mock_database
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.validate_uniqueness",
+ return_value=True,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DuplicateDatasetCommand.populate_owners",
+ return_value=[],
+ ):
+ with patch("superset.commands.dataset.duplicate.db.session.add"):
+ command = DuplicateDatasetCommand(
+ {
+ "base_model_id": 1,
+ "table_name": "duplicated_dataset",
+ "owners": [],
+ }
+ )
+
+ # Should not raise any errors
+ command.validate()
+ # Test the actual duplication
+ result = command.run()
+ assert result.table_name == "duplicated_dataset"
+ assert result.database.id == 1
+ assert result.schema == "public"
+ assert result.is_sqllab_view is True
+ assert result.sql == "SELECT 1 as c"
+
+
+def test_duplicate_dataset_not_found():
+ """Test duplication when base dataset doesn't exist."""
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=None,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DuplicateDatasetCommand.populate_owners",
+ return_value=[],
+ ):
+ command = DuplicateDatasetCommand(
+ {
+ "base_model_id": 999,
+ "table_name": "duplicated_dataset",
+ "owners": [],
+ }
+ )
+
+ with pytest.raises(DatasetInvalidError) as exc_info:
+ command.validate()
+
+ # Verify the exception contains DatasetNotFoundError
+ validation_errors = exc_info.value._exceptions
+ assert any(
+ isinstance(error, DatasetNotFoundError) for error in validation_errors
+ )
+
+
+def test_duplicate_dataset_not_virtual():
Review Comment:
**Suggestion:** Add an explicit return type annotation to this test function signature (for example, `-> None`). [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The final code shows this test function without a return type hint. That violates the Python type-hint requirement for modified code.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1c24770e9cc54d85a40ff94cc56e8ac1&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=1c24770e9cc54d85a40ff94cc56e8ac1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/commands/dataset/test_duplicate.py
**Line:** 266:266
**Comment:**
*Custom Rule: Add an explicit return type annotation to this test function signature (for example, `-> None`).
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%2F37932&comment_hash=66eb17a8bea4ccd35edae9f72589fdc850a1c85fb758904f19a76cb866e0633f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37932&comment_hash=66eb17a8bea4ccd35edae9f72589fdc850a1c85fb758904f19a76cb866e0633f&reaction=dislike'>👎</a>
##########
tests/unit_tests/commands/dataset/test_duplicate.py:
##########
@@ -165,3 +170,360 @@ def test_duplicate_dataset_blocked_by_soft_deleted_twin(session: Session) -> Non
)
with pytest.raises(DatasetInvalidError):
command.validate()
+
+
+def test_duplicate_dataset_success():
+ """Test successful duplication of a virtual dataset."""
+ # Create a mock base model (virtual dataset)
+ mock_base_model = Mock(spec=SqlaTable)
+ mock_base_model.id = 1
+ mock_base_model.database_id = 1
+ mock_base_model.table_name = "original_dataset"
+ mock_base_model.schema = "public"
+ mock_base_model.catalog = None
+ mock_base_model.kind = "virtual"
+ mock_base_model.sql = "SELECT 1 as c"
+ mock_base_model.template_params = None
+ mock_base_model.normalize_columns = False
+ mock_base_model.always_filter_main_dttm = False
+ mock_base_model.columns = []
+ mock_base_model.metrics = []
+
+ # Create a mock database without spec to allow SQLAlchemy relationship assignment
+ mock_database = MagicMock()
+ mock_database.id = 1
+ mock_database.get_default_catalog.return_value = None
+ # Add _sa_instance_state to allow SQLAlchemy relationship assignment
+ mock_database._sa_instance_state = MagicMock()
+
+ with (
+ patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=mock_base_model,
+ ),
+ patch("superset.commands.dataset.duplicate.security_manager.raise_for_access"),
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.db.session.query"
+ ) as mock_query:
+ mock_query.return_value.get.return_value = mock_database
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.validate_uniqueness",
+ return_value=True,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DuplicateDatasetCommand.populate_owners",
+ return_value=[],
+ ):
+ with patch("superset.commands.dataset.duplicate.db.session.add"):
+ command = DuplicateDatasetCommand(
+ {
+ "base_model_id": 1,
+ "table_name": "duplicated_dataset",
+ "owners": [],
+ }
+ )
+
+ # Should not raise any errors
+ command.validate()
+ # Test the actual duplication
+ result = command.run()
+ assert result.table_name == "duplicated_dataset"
+ assert result.database.id == 1
+ assert result.schema == "public"
+ assert result.is_sqllab_view is True
+ assert result.sql == "SELECT 1 as c"
+
+
+def test_duplicate_dataset_not_found():
+ """Test duplication when base dataset doesn't exist."""
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=None,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DuplicateDatasetCommand.populate_owners",
+ return_value=[],
+ ):
+ command = DuplicateDatasetCommand(
+ {
+ "base_model_id": 999,
+ "table_name": "duplicated_dataset",
+ "owners": [],
+ }
+ )
+
+ with pytest.raises(DatasetInvalidError) as exc_info:
+ command.validate()
+
+ # Verify the exception contains DatasetNotFoundError
+ validation_errors = exc_info.value._exceptions
+ assert any(
+ isinstance(error, DatasetNotFoundError) for error in validation_errors
+ )
+
+
+def test_duplicate_dataset_not_virtual():
+ """Test that duplication fails for physical (non-virtual) datasets."""
+ mock_base_model = Mock(spec=SqlaTable)
+ mock_base_model.id = 1
+ mock_base_model.database_id = 1
+ mock_base_model.kind = "physical" # Not virtual
+
+ with (
+ patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=mock_base_model,
+ ),
+ patch("superset.commands.dataset.duplicate.security_manager.raise_for_access"),
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.validate_uniqueness",
+ return_value=True,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DuplicateDatasetCommand.populate_owners",
+ return_value=[],
+ ):
+ command = DuplicateDatasetCommand(
+ {
+ "base_model_id": 1,
+ "table_name": "duplicated_dataset",
+ "owners": [],
+ }
+ )
+
+ with pytest.raises(DatasetInvalidError) as exc_info:
+ command.validate()
+
+ # Verify the exception contains DatasourceTypeInvalidError
+ validation_errors = exc_info.value._exceptions
+ assert len(validation_errors) == 1
+ assert isinstance(validation_errors[0], DatasourceTypeInvalidError)
+
+
+def test_duplicate_dataset_name_exists_same_database_schema():
+ """Test that duplication fails when name exists in same database/schema."""
+ mock_base_model = Mock(spec=SqlaTable)
+ mock_base_model.id = 1
+ mock_base_model.database_id = 1
+ mock_base_model.table_name = "original_dataset"
+ mock_base_model.schema = "public"
+ mock_base_model.catalog = None
+ mock_base_model.kind = "virtual"
+
+ with (
+ patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=mock_base_model,
+ ),
+ patch("superset.commands.dataset.duplicate.security_manager.raise_for_access"),
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.validate_uniqueness",
+ return_value=False, # Name already exists
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DuplicateDatasetCommand.populate_owners",
+ return_value=[],
+ ):
+ command = DuplicateDatasetCommand(
+ {
+ "base_model_id": 1,
+ "table_name": "existing_dataset",
+ "owners": [],
+ }
+ )
+
+ with pytest.raises(DatasetInvalidError) as exc_info:
+ command.validate()
+
+ # Verify the exception contains DatasetExistsValidationError
+ validation_errors = exc_info.value._exceptions
+ assert len(validation_errors) == 1
+ assert isinstance(validation_errors[0], DatasetExistsValidationError)
+
+
+def test_duplicate_dataset_catalog_preserved():
Review Comment:
**Suggestion:** Add an explicit return type annotation to this test function signature (for example, `-> None`). [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The test function is missing a return type annotation in the final state of the file, so the suggestion correctly identifies a type-hint violation.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=49c04788558448219566de6b7138aa1d&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=49c04788558448219566de6b7138aa1d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/commands/dataset/test_duplicate.py
**Line:** 347:347
**Comment:**
*Custom Rule: Add an explicit return type annotation to this test function signature (for example, `-> None`).
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%2F37932&comment_hash=d6e9ce27c5f4976fd14252bcb868e129ba7bdff435a2230efd1b4dd5a7df0c0f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37932&comment_hash=d6e9ce27c5f4976fd14252bcb868e129ba7bdff435a2230efd1b4dd5a7df0c0f&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): resolve duplicate virtual dataset validation error [superset]
Posted by "goldjee (via GitHub)" <gi...@apache.org>.
goldjee commented on code in PR #37932:
URL: https://github.com/apache/superset/pull/37932#discussion_r2799492503
##########
superset/commands/dataset/duplicate.py:
##########
@@ -66,6 +66,7 @@ def run(self) -> Model:
table = SqlaTable(table_name=table_name, owners=owners)
table.database = database
table.schema = self._base_model.schema
+ table.catalog = self._base_model.catalog
Review Comment:
This check is performed in this exact manner at [line 121](https://github.com/apache/superset/pull/37932/changes#diff-38934e7029af81490bbc931c312e2e8970c7988b269b55bdd37d07e24f57009cR121).
--
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): resolve duplicate virtual dataset validation error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #37932:
URL: https://github.com/apache/superset/pull/37932#discussion_r2799509799
##########
tests/unit_tests/commands/dataset/test_duplicate.py:
##########
@@ -0,0 +1,363 @@
+# 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.
+from unittest.mock import MagicMock, Mock, patch
+
+import pytest
+
+from superset.commands.dataset.duplicate import DuplicateDatasetCommand
+from superset.commands.dataset.exceptions import (
+ DatasetExistsValidationError,
+ DatasetInvalidError,
+ DatasetNotFoundError,
+)
+from superset.commands.exceptions import DatasourceTypeInvalidError
+from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn
+from superset.models.core import Database
+
+
+def test_duplicate_dataset_success():
+ """Test successful duplication of a virtual dataset."""
+ # Create a mock base model (virtual dataset)
+ mock_base_model = Mock(spec=SqlaTable)
+ mock_base_model.id = 1
+ mock_base_model.database_id = 1
+ mock_base_model.table_name = "original_dataset"
+ mock_base_model.schema = "public"
+ mock_base_model.catalog = None
+ mock_base_model.kind = "virtual"
+ mock_base_model.sql = "SELECT 1 as c"
+ mock_base_model.template_params = None
+ mock_base_model.normalize_columns = False
+ mock_base_model.always_filter_main_dttm = False
+ mock_base_model.columns = []
+ mock_base_model.metrics = []
+
+ mock_database = Mock(spec=Database)
+ mock_database.id = 1
+ mock_database.get_default_catalog.return_value = None
+
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=mock_base_model,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.db.session.query"
+ ) as mock_query:
+ mock_query.return_value.get.return_value = mock_database
+ with patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.validate_uniqueness",
+ return_value=True,
+ ):
+ with patch(
+ "superset.commands.dataset.duplicate.DuplicateDatasetCommand.populate_owners",
+ return_value=[],
+ ):
+ with patch("superset.commands.dataset.duplicate.db.session.add"):
+ command = DuplicateDatasetCommand(
+ {
+ "base_model_id": 1,
+ "table_name": "duplicated_dataset",
+ "owners": [],
+ }
+ )
+
+ # Should not raise any errors
+ command.validate()
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incomplete duplication test</b></div>
<div id="fix">
The test test_duplicate_dataset_success validates that command.validate() passes without errors, but it does not call command.run() or assert on the duplication result. This misses verifying the actual duplication behavior, such as copying attributes from the base model to the new dataset. Per BITO rule [6262], tests should assert the business logic they claim to test.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
# Should not raise any errors
command.validate()
# Test the actual duplication
result = command.run()
assert result.table_name == "duplicated_dataset"
assert result.database_id == 1
assert result.schema == "public"
assert result.kind == "virtual"
assert result.sql == "SELECT 1 as c"
````
</div>
</details>
</div>
<small><i>Code Review Run #2d522b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/commands/dataset/duplicate.py:
##########
@@ -66,6 +66,7 @@ def run(self) -> Model:
table = SqlaTable(table_name=table_name, owners=owners)
table.database = database
table.schema = self._base_model.schema
+ table.catalog = self._base_model.catalog
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Catalog Resolution Inconsistency</b></div>
<div id="fix">
The catalog assignment here uses self._base_model.catalog directly, but in validate, uniqueness checks against the resolved catalog (default if None). This inconsistency means the created table might have catalog=None while validation assumed default, potentially allowing invalid duplicates.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
table.catalog = self._base_model.catalog or database.get_default_catalog()
````
</div>
</details>
</div>
<small><i>Code Review Run #2d522b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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): resolve duplicate virtual dataset validation error [superset]
Posted by "goldjee (via GitHub)" <gi...@apache.org>.
goldjee commented on code in PR #37932:
URL: https://github.com/apache/superset/pull/37932#discussion_r2799492503
##########
superset/commands/dataset/duplicate.py:
##########
@@ -66,6 +66,7 @@ def run(self) -> Model:
table = SqlaTable(table_name=table_name, owners=owners)
table.database = database
table.schema = self._base_model.schema
+ table.catalog = self._base_model.catalog
Review Comment:
This check is performed in this exact manner at [line 121](https://github.com/apache/superset/pull/37932/changes#diff-38934e7029af81490bbc931c312e2e8970c7988b269b55bdd37d07e24f57009cR121).
--
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): resolve duplicate virtual dataset validation error [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #37932:
URL: https://github.com/apache/superset/pull/37932#discussion_r2800344242
##########
superset/commands/dataset/duplicate.py:
##########
@@ -66,6 +66,7 @@ def run(self) -> Model:
table = SqlaTable(table_name=table_name, owners=owners)
table.database = database
table.schema = self._base_model.schema
+ table.catalog = self._base_model.catalog
Review Comment:
<!-- Bito Reply -->
Your change removes the fallback to `database.get_default_catalog()` from the validation function, ensuring that both the uniqueness check and the catalog assignment in the duplicated dataset use `self._base_model.catalog` directly. This maintains consistency by preserving the base model's catalog property, aligning with the expectation that duplicates should retain original attributes. The fix addresses the inconsistency flagged in the review comments without altering the run method.
**superset/commands/dataset/duplicate.py**
```
catalog = self._base_model.catalog
table = Table(duplicate_name, self._base_model.schema, catalog)
```
**superset/commands/dataset/duplicate.py**
```
table.catalog = self._base_model.catalog
```
--
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