codeant-ai-for-open-source[bot] commented on code in PR #42797:
URL: https://github.com/apache/superset/pull/42797#discussion_r3721465269
##########
superset/versioning/queries.py:
##########
@@ -423,7 +449,7 @@ def resolve_version(
ver_cls = version_class(model_cls)
tx_ids = (
db.session.query(ver_cls.transaction_id)
- .filter(ver_cls.id == entity.id)
+ .filter(_identity_filter(ver_cls, entity.id, getattr(entity, "uuid",
None)))
Review Comment:
**Suggestion:** The UUID-pinned lookup is used to calculate `version_num`,
but `get_version` still fetches the snapshot with an id-only predicate. After
id reuse, the offset calculated from the successor's rows can therefore select
a predecessor row, returning predecessor content while labeling it with the
requested successor version UUID. Apply the same `(id, uuid)` identity filter
to the snapshot query, and likewise to the history-list query. [incomplete
implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Version-history endpoints can return another entity's historical content.
- ⚠️ Returned version UUIDs and snapshot fields can describe different
entities.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7262fa96aad34e0882be2b88d7e4177f&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=7262fa96aad34e0882be2b88d7e4177f&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:** superset/versioning/queries.py
**Line:** 449:452
**Comment:**
*Incomplete Implementation: The UUID-pinned lookup is used to calculate
`version_num`, but `get_version` still fetches the snapshot with an id-only
predicate. After id reuse, the offset calculated from the successor's rows can
therefore select a predecessor row, returning predecessor content while
labeling it with the requested successor version UUID. Apply the same `(id,
uuid)` identity filter to the snapshot query, and likewise to the history-list
query.
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%2F42797&comment_hash=b879723143789f73cb36090966cf1bd637126f2f6b5cc1928ec60f5d95347bba&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42797&comment_hash=b879723143789f73cb36090966cf1bd637126f2f6b5cc1928ec60f5d95347bba&reaction=dislike'>👎</a>
##########
tests/integration_tests/versioning/id_reuse_tests.py:
##########
@@ -0,0 +1,218 @@
+# 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.
+"""Version reads and restore must not resolve a *predecessor's* shadow rows.
+
+A hard delete frees the entity's integer id and the database may hand it to
+the next row inserted — guaranteed on SQLite ROWID tables, and reachable
+elsewhere whenever a sequence is reset. Matching shadow rows on the id alone
+therefore lets a successor entity inherit the deleted predecessor's version
+history, and lets a restore write the predecessor's content over it.
+
+These tests recreate an entity under a recycled id and assert the successor
+has no history and cannot be restored to its predecessor's version. The same
+defect was found and fixed twice on the soft-delete side (``_purge_one``'s
+identity guard, and ``_identity_predicates`` on the purge cascade's locked
+claim); this pins the equivalent guarantee for the versioning read/restore
+paths.
+"""
+
+from __future__ import annotations
+
+import uuid as uuid_module
+
+import pytest
+import sqlalchemy as sa
+
+from superset import db
+from superset.daos.version import VersionDAO
+from superset.models.slice import Slice
+from superset.versioning.restore import restore_version
+from tests.integration_tests.test_app import app
+
+
+def _make_chart(name: str, chart_uuid: uuid_module.UUID) -> Slice:
+ chart = Slice(
+ slice_name=name,
+ datasource_type="table",
+ datasource_id=1,
+ viz_type="table",
+ uuid=chart_uuid,
+ )
+ db.session.add(chart)
+ db.session.commit()
+ return chart
+
+
[email protected]
+def recycled_id_charts():
+ """Create a chart, capture history, hard-delete it, then create a second
+ chart that lands on the freed id.
+
+ Yields ``(entity_id, predecessor_uuid, successor)`` or skips when the
+ backend did not actually recycle the id — the assertions are only
+ meaningful when it did, and forcing reuse portably is not possible.
+ """
+ with app.app_context():
+ predecessor_uuid = uuid_module.uuid4()
+ predecessor = _make_chart("id_reuse_predecessor", predecessor_uuid)
+ entity_id = predecessor.id
+
+ # Give the predecessor a second version so history is non-trivial.
+ predecessor.slice_name = "id_reuse_predecessor_v2"
+ db.session.commit()
+
+ # Hard delete — frees the id. The shadow rows deliberately survive:
+ # that persistence is the whole point of the version tables, and it
+ # is what makes the successor's inheritance possible.
+ db.session.delete(predecessor)
+ db.session.commit()
+
+ successor_uuid = uuid_module.uuid4()
+ successor = _make_chart("id_reuse_successor", successor_uuid)
+ if successor.id != entity_id:
+ db.session.delete(successor)
+ db.session.commit()
+ pytest.skip(
+ f"backend did not recycle the id ({entity_id} -> "
+ f"{successor.id}); reuse is deterministic on SQLite ROWID "
+ "tables only"
+ )
+
+ yield entity_id, predecessor_uuid, successor
+
+ db.session.delete(successor)
+ db.session.commit()
+
+
+def test_successor_under_recycled_id_has_no_inherited_history(
+ recycled_id_charts,
+) -> None:
+ """The successor's version count must reflect its own writes only.
+
+ Before the ``(id, uuid)`` fix, ``current_version_number`` counted every
+ shadow row carrying the integer id — including the predecessor's — so a
+ freshly created chart reported the deleted chart's history as its own.
+ """
+ entity_id, _predecessor_uuid, successor = recycled_id_charts
+
+ version = VersionDAO.current_version_number(Slice, entity_id,
successor.uuid)
+ id_only_version = VersionDAO.current_version_number(Slice, entity_id)
+
+ # The successor has exactly one version of its own (its INSERT).
+ assert version == 0, (
+ f"successor should report only its own single version; got {version}"
+ )
+ # Control: the id-only lookup still sees the predecessor's rows, which is
+ # precisely the inheritance the uuid pin removes. If this ever equals the
+ # uuid-pinned result the fixture stopped exercising id reuse.
+ assert id_only_version is not None
+ assert id_only_version > version, (
+ "expected the id-only lookup to over-count via the predecessor's "
+ f"rows (id_only={id_only_version}, pinned={version}); the fixture "
+ "may no longer be recycling the id"
+ )
+
+
+def test_live_transaction_id_is_not_the_predecessors(recycled_id_charts) ->
None:
+ """The live transaction resolved for the successor must be its own.
+
+ ``current_live_version_uuid`` derives the client-visible version uuid from
+ this transaction id, so a predecessor's id here produces an ETag naming a
+ version the caller can never legitimately hold.
+ """
+ entity_id, predecessor_uuid, successor = recycled_id_charts
+
+ successor_tx = VersionDAO.current_live_transaction_id(
+ Slice, entity_id, successor.uuid
+ )
+ # Asking about the hard-deleted predecessor must not answer with the
+ # successor's transaction. Without the uuid pin the query cannot tell the
Review Comment:
**Suggestion:** The test assumes a hard-deleted predecessor has no row with
`end_transaction_id IS NULL`, but Continuum records the delete as an operation
row whose validity window can remain open. `current_live_transaction_id`
filters only on the end transaction and does not exclude delete operations, so
this assertion can fail because of the existing delete-row representation
rather than because UUID pinning is broken. Select the predecessor transaction
directly from its shadow rows, or assert against the expected delete operation
semantics. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ The id-reuse integration test can fail on every backend that retains
open DELETE shadows.
- ⚠️ CI can report a false regression in `current_live_transaction_id()`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9824a95c27304141a5e01c205881e734&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=9824a95c27304141a5e01c205881e734&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/integration_tests/versioning/id_reuse_tests.py
**Line:** 139:143
**Comment:**
*Incorrect Condition Logic: The test assumes a hard-deleted predecessor
has no row with `end_transaction_id IS NULL`, but Continuum records the delete
as an operation row whose validity window can remain open.
`current_live_transaction_id` filters only on the end transaction and does not
exclude delete operations, so this assertion can fail because of the existing
delete-row representation rather than because UUID pinning is broken. Select
the predecessor transaction directly from its shadow rows, or assert against
the expected delete operation semantics.
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%2F42797&comment_hash=a012dbfd091faaf37cf45305e7a5576a9b3a380c5f4cea5b5febed40b24fccc5&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42797&comment_hash=a012dbfd091faaf37cf45305e7a5576a9b3a380c5f4cea5b5febed40b24fccc5&reaction=dislike'>👎</a>
##########
superset/versioning/api_helpers.py:
##########
@@ -115,8 +115,10 @@ def current_entity_version_info(
else None
)
return EntityVersionInfo(
- version=VersionDAO.current_version_number(model_cls, entity_id),
- transaction_id=VersionDAO.current_live_transaction_id(model_cls,
entity_id),
+ version=VersionDAO.current_version_number(model_cls, entity_id,
entity_uuid),
+ transaction_id=VersionDAO.current_live_transaction_id(
+ model_cls, entity_id, entity_uuid
+ ),
Review Comment:
**Suggestion:** These three values are now read by separate database
queries, so a concurrent save can commit after `version_uuid` is resolved but
before `version` and `transaction_id` are read. The response can consequently
contain an old version UUID paired with a newer version number and transaction
ID, producing inconsistent optimistic-concurrency metadata. Read all
live-version fields from one consistent query or transaction snapshot. [race
condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Update responses can contain mismatched version metadata.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b0b39b9f73b0440b91fa05adec74643e&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=b0b39b9f73b0440b91fa05adec74643e&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:** superset/versioning/api_helpers.py
**Line:** 118:121
**Comment:**
*Race Condition: These three values are now read by separate database
queries, so a concurrent save can commit after `version_uuid` is resolved but
before `version` and `transaction_id` are read. The response can consequently
contain an old version UUID paired with a newer version number and transaction
ID, producing inconsistent optimistic-concurrency metadata. Read all
live-version fields from one consistent query or transaction snapshot.
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%2F42797&comment_hash=9bda8edc85bfd6afd7ae5d17983b72cfffe78d90b5b5492f3283fdfe26de3f7f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42797&comment_hash=9bda8edc85bfd6afd7ae5d17983b72cfffe78d90b5b5492f3283fdfe26de3f7f&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: [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]
