fix: handle some edge cases in the createOrReplaceArtifactBranch oper…

…ation
Apicurio · Jan 25, 2024 · e6bde8c · e6bde8c
1 parent c1fa0a3
commit e6bde8c
Show file tree

Hide file tree

Showing 4 changed files with 70 additions and 4 deletions.
diff --git a/app/src/main/java/io/apicurio/registry/storage/impl/sql/AbstractSqlRegistryStorage.java b/app/src/main/java/io/apicurio/registry/storage/impl/sql/AbstractSqlRegistryStorage.java
@@ -27,6 +27,7 @@
 import jakarta.enterprise.event.Event;
 import jakarta.inject.Inject;
 import jakarta.transaction.Transactional;
+import jakarta.validation.ValidationException;
 import org.apache.commons.lang3.tuple.Pair;
 import org.eclipse.microprofile.config.inject.ConfigProperty;
 import org.slf4j.Logger;
@@ -3346,6 +3347,10 @@ public void createOrReplaceArtifactBranch(GA ga, BranchId branchId, List<Version
         if (BranchId.LATEST.equals(branchId)) {
             throw new NotAllowedException("Artifact branch 'latest' cannot be replaced.");
         }
+        if (versions.isEmpty()) {
+            throw new ValidationException("Artifact branch replace operation requires at least one version. " +
+                    "Use the delete operation instead if this is intentional.");
+        }
 
         handles.withHandleNoException(handle -> {
 
@@ -3357,7 +3362,11 @@ public void createOrReplaceArtifactBranch(GA ga, BranchId branchId, List<Version
                 }
             }
 
-            deleteArtifactBranchRaw(ga, branchId);
+            try {
+                deleteArtifactBranchRaw(ga, branchId);
+            } catch (ArtifactBranchNotFoundException ignored) {
+                // Branch does not exist, it will be created
+            }
 
             var reversed = new ArrayDeque<>(versions).descendingIterator();
             while (reversed.hasNext()) {

diff --git a/.../main/resources-unfiltered/META-INF/resources/api-specifications/registry/v3/openapi.json b/.../main/resources-unfiltered/META-INF/resources/api-specifications/registry/v3/openapi.json
@@ -3272,6 +3272,9 @@
                         },
                         "description": "List of versions in an artifact branch."
                     },
+                    "400": {
+                        "$ref": "#/components/responses/BadRequest"
+                    },
                     "404": {
                         "$ref": "#/components/responses/NotFound"
                     },

diff --git a/app/src/test/java/io/apicurio/registry/noprofile/rest/v3/GroupsResourceTest.java b/app/src/test/java/io/apicurio/registry/noprofile/rest/v3/GroupsResourceTest.java
@@ -3183,6 +3183,55 @@ public void testBranches() throws Exception {
                 ArtifactBranch.builder().groupId(GROUP).artifactId(artifactId).branchId("branch3").versions(List.of("3", "2")).build()
         ), convert(branches));
 
+        // Try to replace a non-existent branch
+
+        branch = clientV3
+                .groups()
+                .byGroupId(GROUP)
+                .artifacts()
+                .byArtifactId(artifactId)
+                .branches()
+                .byBranchId("branch4")
+                .put(convert(ArtifactBranch.builder().versions(List.of("1", "2", "3", "4")).build())); // We support omitting the repeated data
+
+        assertEquals(
+                ArtifactBranch.builder().groupId(GROUP).artifactId(artifactId).branchId("branch4").versions(List.of("1", "2", "3", "4")).build(),
+                convert(branch)
+        );
+
+        branches = clientV3
+                .groups()
+                .byGroupId(GROUP)
+                .artifacts()
+                .byArtifactId(artifactId)
+                .branches()
+                .get();
+
+        assertEquals(Set.of(
+                ArtifactBranch.builder().groupId(GROUP).artifactId(artifactId).branchId(BranchId.LATEST.getRawBranchId()).versions(List.of("4", "3", "2", "1")).build(),
+                ArtifactBranch.builder().groupId(GROUP).artifactId(artifactId).branchId("branch1").versions(List.of("4", "1")).build(),
+                ArtifactBranch.builder().groupId(GROUP).artifactId(artifactId).branchId("branch2").versions(List.of("4", "2")).build(),
+                ArtifactBranch.builder().groupId(GROUP).artifactId(artifactId).branchId("branch3").versions(List.of("3", "2")).build(),
+                ArtifactBranch.builder().groupId(GROUP).artifactId(artifactId).branchId("branch4").versions(List.of("1", "2", "3", "4")).build()
+        ), convert(branches));
+
+        // Failure mode: Try to replace a branch with empty version sequence
+
+        var error = assertThrows(Error.class, () -> {
+            clientV3
+                    .groups()
+                    .byGroupId(GROUP)
+                    .artifacts()
+                    .byArtifactId(artifactId)
+                    .branches()
+                    .byBranchId("branch4")
+                    .put(convert(ArtifactBranch.builder().versions(List.of()).build()));
+        });
+
+        assertNotNull(error);
+        assertEquals(400, error.getErrorCode());
+        assertEquals("ValidationException", error.getName());
+
         // Adding existing version is allowed, but not recommended
 
         branch = clientV3
@@ -3203,7 +3252,7 @@ public void testBranches() throws Exception {
         // It could be used to "hide" a bad latest version, but it's probably better to use the artifact state feature.
         // The latest branch is used in a lot of internal features, that currently do not expect duplicates or any updates.
 
-        var error = assertThrows(Error.class, () -> {
+        error = assertThrows(Error.class, () -> {
             clientV3
                     .groups()
                     .byGroupId(GROUP)
@@ -3353,7 +3402,8 @@ public void testBranches() throws Exception {
         assertEquals(Set.of(
                 ArtifactBranch.builder().groupId(GROUP).artifactId(artifactId).branchId(BranchId.LATEST.getRawBranchId()).versions(List.of("4", "3", "2", "1")).build(),
                 ArtifactBranch.builder().groupId(GROUP).artifactId(artifactId).branchId("branch1").versions(List.of("4", "1")).build(),
-                ArtifactBranch.builder().groupId(GROUP).artifactId(artifactId).branchId("branch3").versions(List.of("3", "2")).build()
+                ArtifactBranch.builder().groupId(GROUP).artifactId(artifactId).branchId("branch3").versions(List.of("3", "2")).build(),
+                ArtifactBranch.builder().groupId(GROUP).artifactId(artifactId).branchId("branch4").versions(List.of("1", "2", "3", "4")).build()
         ), convert(branches));
 
         clientV3
@@ -3393,7 +3443,8 @@ public void testBranches() throws Exception {
         assertEquals(Set.of(
                 ArtifactBranch.builder().groupId(GROUP).artifactId(artifactId).branchId(BranchId.LATEST.getRawBranchId()).versions(List.of("4", "3", "1")).build(),
                 ArtifactBranch.builder().groupId(GROUP).artifactId(artifactId).branchId("branch1").versions(List.of("4", "1")).build(),
-                ArtifactBranch.builder().groupId(GROUP).artifactId(artifactId).branchId("branch3").versions(List.of("3")).build()
+                ArtifactBranch.builder().groupId(GROUP).artifactId(artifactId).branchId("branch3").versions(List.of("3")).build(),
+                ArtifactBranch.builder().groupId(GROUP).artifactId(artifactId).branchId("branch4").versions(List.of("1", "3", "4")).build()
         ), convert(branches));
 
         // Delete the entire artifact and recreate it. Make sure branches have been cleaned up.

diff --git a/common/src/main/resources/META-INF/openapi.json b/common/src/main/resources/META-INF/openapi.json
@@ -3272,6 +3272,9 @@
                         },
                         "description": "List of versions in an artifact branch."
                     },
+                    "400": {
+                        "$ref": "#/components/responses/BadRequest"
+                    },
                     "404": {
                         "$ref": "#/components/responses/NotFound"
                     },