Fix update-requirement check to handle "no current snapshot" requirem…

…ent properly The current implementation of the `assert-ref-snapshot-id` update requirement neglects the `null` requirement case, which means "there must be no current snapshot". The fix is simple. Also added a check that the ref-name in that requirement-check is `main`. Also updated a few cases to eliminate IDE warnings in the same class. Fixes projectnessie#10062
snazy · Dec 9, 2024 · afe554a · afe554a
1 parent 2ce4592
commit afe554a
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 7 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -20,6 +20,8 @@ as necessary. Empty sections will not end in the release notes.
 
 ### Fixes
 
+- Fix handling of Iceberg update-requirement "no current snapshot"
+
 ### Commits
 
 ## [0.101.0] Release (2024-12-06)

diff --git a/...g/src/main/java/org/projectnessie/catalog/formats/iceberg/rest/IcebergMetadataUpdate.java b/...g/src/main/java/org/projectnessie/catalog/formats/iceberg/rest/IcebergMetadataUpdate.java
@@ -702,6 +702,15 @@ interface SetSnapshotRef extends IcebergMetadataUpdate {
 
     @Override
     default void applyToTable(IcebergTableMetadataUpdateState state) {
+      checkState(
+          "main".equals(refName()) && "branch".equals(type()),
+          "Nessie only supports the current snapshot-ref 'main', use Nessie's branches instead");
+      var currentSnapshotId = state.snapshot().icebergSnapshotId();
+      checkState(
+          Objects.equals(snapshotId(), state.snapshot().icebergSnapshotId()),
+          "Snapshot ID mismatch, must be %s, but is %s",
+          currentSnapshotId,
+          snapshotId());
       state.addCatalogOp(CatalogOps.META_SET_SNAPSHOT_REF);
       // NOP - This class is used for JSON deserialization only.
       // Nessie has catalog-level branches and tags.
@@ -718,6 +727,9 @@ interface RemoveSnapshotRef extends IcebergMetadataUpdate {
 
     @Override
     default void applyToTable(IcebergTableMetadataUpdateState state) {
+      checkState(
+          "main".equals(refName()),
+          "Nessie only supports the Iceberg snapshot-ref 'main', use Nessie's branches instead");
       state.addCatalogOp(CatalogOps.META_REMOVE_SNAPSHOT_REF);
       // NOP - This class is used for JSON deserialization only.
       // Nessie has catalog-level branches and tags.

diff --git a/...rc/main/java/org/projectnessie/catalog/formats/iceberg/rest/IcebergUpdateRequirement.java b/...rc/main/java/org/projectnessie/catalog/formats/iceberg/rest/IcebergUpdateRequirement.java
@@ -16,6 +16,7 @@
 package org.projectnessie.catalog.formats.iceberg.rest;
 
 import static java.lang.String.format;
+import static org.projectnessie.catalog.formats.iceberg.meta.IcebergTableMetadata.NO_SNAPSHOT_ID;
 
 import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
 import com.fasterxml.jackson.annotation.JsonSubTypes;
@@ -96,7 +97,7 @@ interface AssertUUID extends IcebergUpdateRequirement {
     default void check(ContentKey key, NessieEntitySnapshot<?> snapshot) {
       String tableUuid = snapshot.entity().icebergUuid();
       checkState(
-          uuid().equals(tableUuid),
+          Objects.equals(uuid(), tableUuid),
           key,
           "Requirement failed: UUID does not match: expected %s != %s",
           tableUuid,
@@ -172,13 +173,27 @@ default void checkForTable(
         NessieTableSnapshot snapshot, boolean tableExists, ContentKey contentKey) {
       // Cannot really check the reference name, because the ref-name in a table-metadata is
       // something very different from Nessie references
-      Long id = snapshotId();
-      if (id != null) {
+      checkState(
+          "main".equals(ref()),
+          contentKey,
+          "Requirement failed: ref must be 'main', but is '%s'",
+          ref());
+
+      Long expectedId = snapshotId();
+      Long currentId = snapshot.icebergSnapshotId();
+      if (expectedId != null) {
         checkState(
-            Objects.equals(id, snapshot.icebergSnapshotId()),
+            Objects.equals(expectedId, currentId),
             contentKey,
             "Requirement failed: snapshot id changed: expected %s != %s",
-            id,
+            expectedId,
+            snapshotId());
+      } else {
+        // 'null' means "no current snapshot"
+        checkState(
+            currentId == null || currentId == NO_SNAPSHOT_ID,
+            contentKey,
+            "Requirement failed: snapshot id mismatch: expected no current snapshot, but has %s",
             snapshotId());
       }
     }
@@ -196,7 +211,7 @@ default void checkForTable(
         NessieTableSnapshot snapshot, boolean tableExists, ContentKey contentKey) {
       Integer id = snapshot.icebergLastColumnId();
       checkState(
-          lastAssignedFieldId() == id,
+          Objects.equals(id, lastAssignedFieldId()),
           contentKey,
           "Requirement failed: last assigned field id changed: expected %s != %s",
           id,
@@ -246,7 +261,7 @@ default void checkForTable(
         NessieTableSnapshot snapshot, boolean tableExists, ContentKey contentKey) {
       Integer id = snapshot.icebergLastPartitionId();
       checkState(
-          lastAssignedPartitionId() == id,
+          Objects.equals(lastAssignedPartitionId(), id),
           contentKey,
           "Requirement failed: last assigned partition id changed: expected %s != %s",
           id,