Repository navigation
Reject existing ref names in create_branch and create_tag - #4319
Conversation
ManageSnapshots.create_branch and create_tag set the ref without checking
whether the name is taken, so an existing branch was reset to the given
snapshot and lost its commits, and an existing tag moved. remove_branch and
remove_tag checked nothing either: a missing name was accepted, a tag could
be removed as a branch, and remove_branch("main") emptied the table.
Check the names against the refs as they stand after the earlier
operations in the same chain, and raise ValueError with the messages the
Java UpdateSnapshotReferencesOperation uses: "Ref x already exists",
"Cannot remove main branch", "Branch does not exist: x", "Tag does not
exist: x", "Ref x is a tag not a branch", "Ref x is a branch not a tag".
Closes apache#4187
| """ | ||
| ref = self._effective_refs().get(tag_name) | ||
| if ref is None: | ||
| raise ValueError(f"Tag does not exist: {tag_name}") |
There was a problem hiding this comment.
The rest of this class raises domain exceptions for ref problems. fast_forward_branch uses NoSuchSnapshotRefError and SnapshotRefTypeError for the same conditions. Could we do the same here (and in remove_branch)?
Tag does not exist: x/Branch does not exist: x->NoSuchSnapshotRefErrorRef x is a branch not a tag/Ref x is a tag not a branch->SnapshotRefTypeError
This follows the project convention to use domain-specific exceptions for Iceberg-specific conditions. If you'd rather keep ValueError, please say so in the PR description, since the raised type is part of the user-facing change.
There was a problem hiding this comment.
Done. remove_branch and remove_tag now raise NoSuchSnapshotRefError and SnapshotRefTypeError, with the same message shape as fast_forward_branch. create_branch, create_tag, and the main check keep ValueError as invalid arguments; I added that to the description.
| table_v2_main_behind.catalog.commit_table.assert_not_called() | ||
|
|
||
|
|
||
| def test_remove_then_create_same_name_in_one_chain(table_v2_main_behind: Table) -> None: |
There was a problem hiding this comment.
Could we add a case where the ref kind changes within a chain, e.g. remove_tag("test").create_branch(..., "test")? It would confirm _effective_refs handles the type switch, not just same-kind replacement.
There was a problem hiding this comment.
Added test_remove_then_create_same_name_as_other_kind_in_one_chain: remove_tag("test").create_branch(..., "test") commits a RemoveSnapshotRefUpdate followed by a SetSnapshotRefUpdate of type branch.
| for method, name, message in [ | ||
| (ms.remove_branch, "main", "Cannot remove main branch"), | ||
| (ms.remove_branch, "missing", "Branch does not exist: missing"), | ||
| (ms.remove_branch, "test", "Ref test is a tag not a branch"), | ||
| (ms.remove_tag, "missing", "Tag does not exist: missing"), | ||
| (ms.remove_tag, "audit", "Ref audit is a branch not a tag"), | ||
| ]: |
There was a problem hiding this comment.
nit: The loop over (bound_method, name, message) tuples is hard to read, and a failure won't say which case broke. Plain consecutive pytest.raises blocks, one per case, would be clearer. Five short blocks cost little.
There was a problem hiding this comment.
Replaced the loop with five pytest.raises blocks, each asserting the specific exception.
…nch and remove_tag The rest of ManageSnapshots raises these domain exceptions for a missing ref and for a ref of the wrong kind, so the remove methods now do the same. Each refusal gets its own test, and a chain that removes a tag and creates a branch under the same name is covered.
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
All review comments addressed, the exception types now match the rest of the class. LGTM! 🙌 Thanks @deepdave98!
Closes #4187
Rationale for this change
ManageSnapshots.create_branchandcreate_tagset the ref without checking whether the name exists, so an existing branch was reset to the given snapshot and lost its commits, and an existing tag moved.remove_branchandremove_tagchecked nothing either: a missing name was accepted, a tag could be removed as a branch, andremove_branch("main")left the table reading as empty.The names are now checked against the refs as they stand after earlier operations in the same chain (
_effective_refs), and the error messages match Java'sUpdateSnapshotReferencesOperation:Ref x already exists,Cannot remove main branch,Branch does not exist: x,Tag does not exist: x,Ref x is a tag not a branch,Ref x is a branch not a tag. Replacing a ref on purpose is still possible withremove_branch(...).create_branch(...)in one chain.AI assistance was used for the analysis and drafting; the change was reviewed and tested by the author.
Are these changes tested?
Four unit tests in
tests/table/test_manage_snapshots.py: an existing name is refused for both kinds, a name staged earlier in the same chain is refused, everyremove_*refusal, and remove-then-create of the same name in one chain still commits both updates in order. The script from the issue now raisesRef dev already existsandRef v1 already exists, anddevkeeps[1, 2].make lintandmake testpass.Are there any user-facing changes?
Yes. Calls that silently replaced or removed a ref now raise.
remove_branchandremove_tagraiseNoSuchSnapshotRefErrorandSnapshotRefTypeError(bothValueErrorsubclasses), asfast_forward_branchdoes.create_branch,create_tag, and removingmainraise plainValueError. This needs the changelog label.