bito-code-review[bot] commented on code in PR #40130: URL: https://github.com/apache/superset/pull/40130#discussion_r3365903803
########## tests/integration_tests/datasets/soft_delete_tests.py: ########## @@ -0,0 +1,300 @@ +# 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. +"""Integration tests for dataset soft-delete and restore.""" + +from superset.connectors.sqla.models import SqlaTable +from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES +from superset.extensions import db +from superset.models.slice import Slice +from superset.utils import json +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.constants import ADMIN_USERNAME, ALPHA_USERNAME + + +class TestDatasetSoftDelete(SupersetTestCase): + """Tests for dataset soft-delete behaviour (T015, T018).""" + + def _get_example_dataset_id(self) -> int: + """Get an existing example dataset ID for testing.""" + dataset = db.session.query(SqlaTable).first() + assert dataset is not None, "No datasets found — load examples first" + return dataset.id + + def _restore_dataset(self, dataset_id: int) -> None: + """Restore a soft-deleted dataset (cleanup helper). + + Used in ``finally`` blocks so a failed assertion can't strand a + soft-deleted row and leak it into later tests; re-queries with the + visibility-filter bypass and only restores if still soft-deleted. + """ + row = ( + db.session.query(SqlaTable) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {SqlaTable}}) + .filter(SqlaTable.id == dataset_id) + .one_or_none() + ) + if row is not None and row.deleted_at is not None: + row.restore() + db.session.commit() + + def test_delete_dataset_soft_deletes(self) -> None: + """DELETE should set deleted_at instead of removing the row.""" + dataset_id = self._get_example_dataset_id() + self.login(ADMIN_USERNAME) + + try: + rv = self.client.delete(f"/api/v1/dataset/{dataset_id}") + assert rv.status_code == 200 + + row = ( + db.session.query(SqlaTable) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {SqlaTable}}) + .filter(SqlaTable.id == dataset_id) + .one_or_none() + ) + assert row is not None + assert row.deleted_at is not None + finally: + self._restore_dataset(dataset_id) + + def test_soft_deleted_dataset_excluded_from_list(self) -> None: + """GET /api/v1/dataset/ should not include soft-deleted datasets.""" + dataset_id = self._get_example_dataset_id() + self.login(ADMIN_USERNAME) + + try: + self.client.delete(f"/api/v1/dataset/{dataset_id}") Review Comment: <div> <div id="suggestion"> <div id="issue"><b>CWE-483: Missing DELETE status assertion</b></div> <div id="fix"> Missing assertion for DELETE response status code. The delete call at line 80 should be captured in a variable and asserted to be 200, consistent with the pattern in `test_delete_dataset_soft_deletes` at line 61. Without this, a failed delete (e.g., 403, 404, 500) silently passes through, leaving the dataset alive and causing subsequent assertions to fail for the wrong reason. (See also: [CWE-483](https://cwe.mitre.org/data/definitions/483.html)) </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` --- tests/integration_tests/datasets/soft_delete_tests.py +++ tests/integration_tests/datasets/soft_delete_tests.py @@ -77,7 +77,8 @@ self.login(ADMIN_USERNAME) try: - self.client.delete(f"/api/v1/dataset/{dataset_id}") + rv = self.client.delete(f"/api/v1/dataset/{dataset_id}") + assert rv.status_code == 200 rv = self.client.get("/api/v1/dataset/") ``` </div> </details> </div> <small><i>Code Review Run #227619</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 ########## tests/integration_tests/datasets/soft_delete_tests.py: ########## @@ -0,0 +1,300 @@ +# 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. +"""Integration tests for dataset soft-delete and restore.""" + +from superset.connectors.sqla.models import SqlaTable +from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES +from superset.extensions import db +from superset.models.slice import Slice +from superset.utils import json +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.constants import ADMIN_USERNAME, ALPHA_USERNAME + + +class TestDatasetSoftDelete(SupersetTestCase): + """Tests for dataset soft-delete behaviour (T015, T018).""" + + def _get_example_dataset_id(self) -> int: + """Get an existing example dataset ID for testing.""" + dataset = db.session.query(SqlaTable).first() + assert dataset is not None, "No datasets found — load examples first" + return dataset.id + + def _restore_dataset(self, dataset_id: int) -> None: + """Restore a soft-deleted dataset (cleanup helper). + + Used in ``finally`` blocks so a failed assertion can't strand a + soft-deleted row and leak it into later tests; re-queries with the + visibility-filter bypass and only restores if still soft-deleted. + """ + row = ( + db.session.query(SqlaTable) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {SqlaTable}}) + .filter(SqlaTable.id == dataset_id) + .one_or_none() + ) + if row is not None and row.deleted_at is not None: + row.restore() + db.session.commit() + + def test_delete_dataset_soft_deletes(self) -> None: + """DELETE should set deleted_at instead of removing the row.""" + dataset_id = self._get_example_dataset_id() + self.login(ADMIN_USERNAME) + + try: + rv = self.client.delete(f"/api/v1/dataset/{dataset_id}") + assert rv.status_code == 200 + + row = ( + db.session.query(SqlaTable) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {SqlaTable}}) + .filter(SqlaTable.id == dataset_id) + .one_or_none() + ) + assert row is not None + assert row.deleted_at is not None + finally: + self._restore_dataset(dataset_id) + + def test_soft_deleted_dataset_excluded_from_list(self) -> None: + """GET /api/v1/dataset/ should not include soft-deleted datasets.""" + dataset_id = self._get_example_dataset_id() + self.login(ADMIN_USERNAME) + + try: + self.client.delete(f"/api/v1/dataset/{dataset_id}") + + rv = self.client.get("/api/v1/dataset/") + data = json.loads(rv.data) + ids = [d["id"] for d in data["result"]] + assert dataset_id not in ids + finally: + self._restore_dataset(dataset_id) + + def test_soft_deleted_dataset_included_in_list_when_requested(self) -> None: + """GET /api/v1/dataset/ with dataset_deleted_state=include returns deleted datasets.""" # noqa: E501 + dataset_id = self._get_example_dataset_id() + self.login(ADMIN_USERNAME) + + try: + self.client.delete(f"/api/v1/dataset/{dataset_id}") + + rison_query = ( + "(filters:!((col:id,opr:dataset_deleted_state,value:include)))" + ) + rv = self.client.get(f"/api/v1/dataset/?q={rison_query}") + assert rv.status_code == 200 + + data = json.loads(rv.data) + deleted_row = next( + (row for row in data["result"] if row["id"] == dataset_id), + None, + ) + assert deleted_row is not None + assert deleted_row["deleted_at"] is not None + finally: + self._restore_dataset(dataset_id) + + def test_only_filter_returns_only_soft_deleted_datasets(self) -> None: + """dataset_deleted_state=only excludes live rows and returns only deleted ones.""" # noqa: E501 + ids = [row.id for row in db.session.query(SqlaTable).limit(2).all()] + assert len(ids) >= 2, "Need at least two example datasets for this test" + live_id, deleted_id = ids[0], ids[1] + self.login(ADMIN_USERNAME) + + try: + self.client.delete(f"/api/v1/dataset/{deleted_id}") Review Comment: <div> <div id="suggestion"> <div id="issue"><b>CWE-483: Missing DELETE status assertion</b></div> <div id="fix"> Missing assertion for DELETE response status code. The delete call at line 121 should be asserted to return 200, consistent with `test_delete_dataset_soft_deletes`. Without this, a failed delete silently passes and the test assertions at lines 129-130 may pass incorrectly. (See also: [CWE-483](https://cwe.mitre.org/data/definitions/483.html)) </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` --- tests/integration_tests/datasets/soft_delete_tests.py +++ tests/integration_tests/datasets/soft_delete_tests.py @@ -118,7 +118,8 @@ self.login(ADMIN_USERNAME) try: - self.client.delete(f"/api/v1/dataset/{deleted_id}") + rv = self.client.delete(f"/api/v1/dataset/{deleted_id}") + assert rv.status_code == 200 rison_query = "(filters:!((col:id,opr:dataset_deleted_state,value:only)))" ``` </div> </details> </div> <small><i>Code Review Run #227619</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 ########## tests/integration_tests/datasets/soft_delete_tests.py: ########## @@ -0,0 +1,300 @@ +# 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. +"""Integration tests for dataset soft-delete and restore.""" + +from superset.connectors.sqla.models import SqlaTable +from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES +from superset.extensions import db +from superset.models.slice import Slice +from superset.utils import json +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.constants import ADMIN_USERNAME, ALPHA_USERNAME + + +class TestDatasetSoftDelete(SupersetTestCase): + """Tests for dataset soft-delete behaviour (T015, T018).""" + + def _get_example_dataset_id(self) -> int: + """Get an existing example dataset ID for testing.""" + dataset = db.session.query(SqlaTable).first() + assert dataset is not None, "No datasets found — load examples first" + return dataset.id + + def _restore_dataset(self, dataset_id: int) -> None: + """Restore a soft-deleted dataset (cleanup helper). + + Used in ``finally`` blocks so a failed assertion can't strand a + soft-deleted row and leak it into later tests; re-queries with the + visibility-filter bypass and only restores if still soft-deleted. + """ + row = ( + db.session.query(SqlaTable) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {SqlaTable}}) + .filter(SqlaTable.id == dataset_id) + .one_or_none() + ) + if row is not None and row.deleted_at is not None: + row.restore() + db.session.commit() + + def test_delete_dataset_soft_deletes(self) -> None: + """DELETE should set deleted_at instead of removing the row.""" + dataset_id = self._get_example_dataset_id() + self.login(ADMIN_USERNAME) + + try: + rv = self.client.delete(f"/api/v1/dataset/{dataset_id}") + assert rv.status_code == 200 + + row = ( + db.session.query(SqlaTable) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {SqlaTable}}) + .filter(SqlaTable.id == dataset_id) + .one_or_none() + ) + assert row is not None + assert row.deleted_at is not None + finally: + self._restore_dataset(dataset_id) + + def test_soft_deleted_dataset_excluded_from_list(self) -> None: + """GET /api/v1/dataset/ should not include soft-deleted datasets.""" + dataset_id = self._get_example_dataset_id() + self.login(ADMIN_USERNAME) + + try: + self.client.delete(f"/api/v1/dataset/{dataset_id}") + + rv = self.client.get("/api/v1/dataset/") + data = json.loads(rv.data) + ids = [d["id"] for d in data["result"]] + assert dataset_id not in ids + finally: + self._restore_dataset(dataset_id) + + def test_soft_deleted_dataset_included_in_list_when_requested(self) -> None: + """GET /api/v1/dataset/ with dataset_deleted_state=include returns deleted datasets.""" # noqa: E501 + dataset_id = self._get_example_dataset_id() + self.login(ADMIN_USERNAME) + + try: + self.client.delete(f"/api/v1/dataset/{dataset_id}") Review Comment: <div> <div id="suggestion"> <div id="issue"><b>CWE-483: Missing DELETE status assertion</b></div> <div id="fix"> Missing assertion for DELETE response status code. The delete call at line 95 should be asserted to return 200, consistent with `test_delete_dataset_soft_deletes`. Without this, a failed delete silently passes, causing the subsequent assertions to fail for an unintended reason. (See also: [CWE-483](https://cwe.mitre.org/data/definitions/483.html)) </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` --- tests/integration_tests/datasets/soft_delete_tests.py +++ tests/integration_tests/datasets/soft_delete_tests.py @@ -92,7 +92,8 @@ self.login(ADMIN_USERNAME) try: - self.client.delete(f"/api/v1/dataset/{dataset_id}") + rv = self.client.delete(f"/api/v1/dataset/{dataset_id}") + assert rv.status_code == 200 rison_query = ( ``` </div> </details> </div> <small><i>Code Review Run #227619</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]
