bito-code-review[bot] commented on code in PR #34993:
URL: https://github.com/apache/superset/pull/34993#discussion_r2322782702
##########
tests/unit_tests/commands/dataset/update_test.py:
##########
@@ -253,38 +276,42 @@ def test_validate_folders_inter_cycle(mocker:
MockerFixture) -> None:
)
with pytest.raises(ValidationError) as excinfo:
- validate_folders(folders=folders, metrics=[], columns=[])
- assert str(excinfo.value) == "Duplicate UUID in folder structure: uuid2"
+ validate_folders(folders=folders, valid_uuids=set())
+ assert str(excinfo.value) == f"Duplicate UUID in folder structure:
{folder_uuid2}"
@with_feature_flags(DATASET_FOLDERS=True)
def test_validate_folders_duplicates(mocker: MockerFixture) -> None:
"""
Test that metrics and columns belong to a single folder.
"""
- metrics = [mocker.MagicMock(metric_name="count", uuid="uuid2")]
+ from uuid import UUID
+
+ metric_uuid = UUID("22222222-2222-2222-2222-222222222222")
+ folder_uuid1 = UUID("11111111-1111-1111-1111-111111111111")
+ metrics = [mocker.MagicMock(metric_name="count", uuid=metric_uuid)]
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Folder UUID conflicts with child metric UUID</b></div>
<div id="fix">
The second folder incorrectly uses `metric_uuid` as its UUID, creating a
logical inconsistency where a folder has the same UUID as the metric it
contains. This violates folder structure semantics and causes the validation to
fail for the wrong reason. Use a separate `folder_uuid2` variable instead.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
folder_uuid1 = UUID("11111111-1111-1111-1111-111111111111")
folder_uuid2 = UUID("33333333-3333-3333-3333-333333333333")
metrics = [mocker.MagicMock(metric_name="count", uuid=metric_uuid)]
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/34993#issuecomment-3254544545>#ecca9d</a></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/update.py:
##########
@@ -175,8 +176,17 @@
self._validate_metrics(metrics, exceptions)
if folders := self._properties.get("folders"):
+ valid_uuids: set[UUID] = set()
+ if metrics:
+ valid_uuids.update(UUID(metric["uuid"]) for metric in metrics)
+ else:
+ valid_uuids.update(metric.uuid for metric in
self._model.metrics)
+ if columns:
+ valid_uuids.update(UUID(column["uuid"]) for column in columns)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing uuid key validation for columns</b></div>
<div id="fix">
Potential KeyError when accessing `column["uuid"]` if the uuid key is
missing from columns dictionaries. Add conditional check:
`valid_uuids.update(UUID(column["uuid"]) for column in columns if "uuid" in
column)`
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
valid_uuids.update(UUID(column["uuid"]) for column in
columns if "uuid" in column)
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/34993#issuecomment-3254544545>#ecca9d</a></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/update.py:
##########
@@ -175,8 +176,17 @@ def _validate_semantics(self, exceptions:
list[ValidationError]) -> None:
self._validate_metrics(metrics, exceptions)
if folders := self._properties.get("folders"):
+ valid_uuids: set[UUID] = set()
+ if metrics:
+ valid_uuids.update(UUID(metric["uuid"]) for metric in metrics)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing uuid key validation for metrics</b></div>
<div id="fix">
Potential KeyError when accessing `metric["uuid"]` if the uuid key is
missing from metrics dictionaries. Add conditional check:
`valid_uuids.update(UUID(metric["uuid"]) for metric in metrics if "uuid" in
metric)`
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
valid_uuids.update(UUID(metric["uuid"]) for metric in
metrics if "uuid" in metric)
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/34993#issuecomment-3254544545>#ecca9d</a></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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
