fix: address code review comments

Resolved: - remove format restrictions on group and artifact ID - more consistent naming - support specifying branches on artifact (version) creation - support branch "rebase" - add tests - small bug fixes
Apicurio · Jan 25, 2024 · c1fa0a3 · c1fa0a3
1 parent 8876226
commit c1fa0a3
Show file tree

Hide file tree

Showing 46 changed files with 2,780 additions and 1,842 deletions.
diff --git a/app/src/main/java/io/apicurio/registry/rest/v2/GroupsResourceImpl.java b/app/src/main/java/io/apicurio/registry/rest/v2/GroupsResourceImpl.java
@@ -590,7 +590,7 @@ public Comment addArtifactVersionComment(String groupId, String artifactId, Stri
      * @see io.apicurio.registry.rest.v2.GroupsResource#deleteArtifactVersionComment(java.lang.String, java.lang.String, java.lang.String, java.lang.String)
      */
     @Override
-    @Audited(extractParameters = {"0", KEY_GROUP_ID, "1", KEY_ARTIFACT_ID, "2", KEY_VERSION, "3", "comment_id"})
+    @Audited(extractParameters = {"0", KEY_GROUP_ID, "1", KEY_ARTIFACT_ID, "2", KEY_VERSION, "3", "comment_id"}) // TODO
     @Authorized(style = AuthorizedStyle.GroupAndArtifact, level = AuthorizedLevel.Write)
     public void deleteArtifactVersionComment(String groupId, String artifactId, String version, String commentId) {
         requireParameter("groupId", groupId);
@@ -620,7 +620,7 @@ public List<Comment> getArtifactVersionComments(String groupId, String artifactI
      * @see io.apicurio.registry.rest.v2.GroupsResource#updateArtifactVersionComment(java.lang.String, java.lang.String, java.lang.String, java.lang.String, io.apicurio.registry.rest.v2.beans.NewComment)
      */
     @Override
-    @Audited(extractParameters = {"0", KEY_GROUP_ID, "1", KEY_ARTIFACT_ID, "2", KEY_VERSION, "3", "comment_id"})
+    @Audited(extractParameters = {"0", KEY_GROUP_ID, "1", KEY_ARTIFACT_ID, "2", KEY_VERSION, "3", "comment_id"}) // TODO
     @Authorized(style = AuthorizedStyle.GroupAndArtifact, level = AuthorizedLevel.Write)
     public void updateArtifactVersionComment(String groupId, String artifactId, String version, String commentId, NewComment data) {
         requireParameter("groupId", groupId);
@@ -705,7 +705,7 @@ public ArtifactMetaData createArtifact(String groupId, String xRegistryArtifactT
      * @see io.apicurio.registry.rest.v2.GroupsResource#createArtifact(String, String, String, String, IfExists, Boolean, String, String, String, String, String, String, ArtifactContent)
      */
     @Override
-    @Audited(extractParameters = {"0", KEY_GROUP_ID, "1", KEY_ARTIFACT_TYPE, "2", KEY_ARTIFACT_ID, "3", KEY_VERSION, "4", KEY_IF_EXISTS, "5", KEY_CANONICAL, "6", KEY_DESCRIPTION, "7", KEY_DESCRIPTION_ENCODED, "8", KEY_NAME, "9", KEY_NAME_ENCODED, "10", "from_url" /*KEY_FROM_URL*/, "11", "artifact_sha" /*KEY_SHA*/})
+    @Audited(extractParameters = {"0", KEY_GROUP_ID, "1", KEY_ARTIFACT_TYPE, "2", KEY_ARTIFACT_ID, "3", KEY_VERSION, "4", KEY_IF_EXISTS, "5", KEY_CANONICAL, "6", KEY_DESCRIPTION, "7", KEY_DESCRIPTION_ENCODED, "8", KEY_NAME, "9", KEY_NAME_ENCODED, "10", KEY_FROM_URL, "11", KEY_SHA})
     @Authorized(style = AuthorizedStyle.GroupOnly, level = AuthorizedLevel.Write)
     public ArtifactMetaData createArtifact(String groupId, String xRegistryArtifactType, String xRegistryArtifactId,
                                            String xRegistryVersion, IfExists ifExists, Boolean canonical,

diff --git a/app/src/main/java/io/apicurio/registry/rest/v3/GroupsResourceImpl.java b/app/src/main/java/io/apicurio/registry/rest/v3/GroupsResourceImpl.java
diff --git a/app/src/main/java/io/apicurio/registry/services/http/RegistryExceptionMapperService.java b/app/src/main/java/io/apicurio/registry/services/http/RegistryExceptionMapperService.java
@@ -64,8 +64,8 @@ public class RegistryExceptionMapperService {
         map.put(ArtifactAlreadyExistsException.class, HTTP_CONFLICT);
         map.put(ArtifactNotFoundException.class, HTTP_NOT_FOUND);
         map.put(BadRequestException.class, HTTP_BAD_REQUEST);
-        map.put(BranchNotFoundException.class, HTTP_NOT_FOUND);
-        map.put(BranchVersionAlreadyExistsException.class, HTTP_CONFLICT);
+        map.put(ArtifactBranchNotFoundException.class, HTTP_NOT_FOUND);
+        map.put(ArtifactBranchAlreadyContainsVersionException.class, HTTP_CONFLICT);
         map.put(ConfigPropertyNotFoundException.class, HTTP_NOT_FOUND);
         map.put(ConflictException.class, HTTP_CONFLICT);
         map.put(ContentNotFoundException.class, HTTP_NOT_FOUND);

diff --git a/app/src/main/java/io/apicurio/registry/storage/RegistryStorage.java b/app/src/main/java/io/apicurio/registry/storage/RegistryStorage.java
@@ -3,6 +3,7 @@
 import io.apicurio.common.apps.config.DynamicConfigPropertyDto;
 import io.apicurio.common.apps.config.DynamicConfigStorage;
 import io.apicurio.registry.content.ContentHandle;
+import io.apicurio.registry.model.VersionId;
 import io.apicurio.registry.storage.dto.*;
 import io.apicurio.registry.storage.error.*;
 import io.apicurio.registry.storage.impexp.EntityInputStream;
@@ -870,7 +871,7 @@ CommentDto createArtifactVersionCommentRaw(String groupId, String artifactId, St
     void importArtifactRule(ArtifactRuleEntity entity);
 
 
-    void importArtifactBranch(ArtifactVersionBranchEntity entity);
+    void importArtifactBranch(ArtifactBranchEntity entity);
 
 
     boolean isContentExists(String contentHash) throws RegistryStorageException;
@@ -904,31 +905,40 @@ ArtifactMetaDataDto createArtifactWithMetadata(String groupId, String artifactId
 
 
     /**
-     * @return map from a branch to a sorted list of GAVs, leaf (latest) version first.
+     * @return map from an artifact branch to a sorted list of GAVs, branch tip (latest) version first.
      */
     Map<BranchId, List<GAV>> getArtifactBranches(GA ga);
 
 
     /**
-     * @return sorted list of GAVs, leaf (latest) version first.
+     * @return sorted list of GAVs, branch tip (latest) version first.
      */
     List<GAV> getArtifactBranch(GA ga, BranchId branchId, ArtifactRetrievalBehavior behavior);
 
 
     /**
-     * Add a version to the artifact branch. The branch is created if it does not exist. The version becomes a new leaf (latest).
+     * Add a version to the artifact branch. The branch is created if it does not exist. The version becomes a new branch tip (latest).
+     * Not supported for the "latest" branch.
      */
     void createOrUpdateArtifactBranch(GAV gav, BranchId branchId);
 
 
     /**
-     * @return GAV identifier of the leaf (latest) version in the artifact branch.
+     * Replace the content of the artifact branch with a new sequence of versions.
+     * Not supported for the "latest" branch.
      */
-    GAV getArtifactBranchLeaf(GA ga, BranchId branchId, ArtifactRetrievalBehavior behavior);
+    void createOrReplaceArtifactBranch(GA ga, BranchId branchId, List<VersionId> versions);
+
+
+    /**
+     * @return GAV identifier of the branch tip (latest) version in the artifact branch.
+     */
+    GAV getArtifactBranchTip(GA ga, BranchId branchId, ArtifactRetrievalBehavior behavior);
 
 
     /**
      * Delete artifact branch.
+     * Not supported for the "latest" branch.
      */
     void deleteArtifactBranch(GA ga, BranchId branchId);
 

diff --git a/...rc/main/java/io/apicurio/registry/storage/decorator/ReadOnlyRegistryStorageDecorator.java b/...rc/main/java/io/apicurio/registry/storage/decorator/ReadOnlyRegistryStorageDecorator.java
@@ -4,6 +4,7 @@
 import io.apicurio.common.apps.config.DynamicConfigPropertyDto;
 import io.apicurio.common.apps.config.Info;
 import io.apicurio.registry.content.ContentHandle;
+import io.apicurio.registry.model.VersionId;
 import io.apicurio.registry.storage.RegistryStorage;
 import io.apicurio.registry.storage.dto.*;
 import io.apicurio.registry.storage.error.*;
@@ -423,7 +424,7 @@ public void importArtifactRule(ArtifactRuleEntity entity) {
 
 
     @Override
-    public void importArtifactBranch(ArtifactVersionBranchEntity entity) {
+    public void importArtifactBranch(ArtifactBranchEntity entity) {
         checkReadOnly();
         delegate.importArtifactBranch(entity);
     }
@@ -487,6 +488,13 @@ public void createOrUpdateArtifactBranch(GAV gav, BranchId branchId) {
     }
 
 
+    @Override
+    public void createOrReplaceArtifactBranch(GA ga, BranchId branchId, List<VersionId> versions) {
+        checkReadOnly();
+        delegate.createOrReplaceArtifactBranch(ga, branchId, versions);
+    }
+
+
     @Override
     public void deleteArtifactBranch(GA ga, BranchId branchId) {
         checkReadOnly();

diff --git a/app/src/main/java/io/apicurio/registry/storage/decorator/RegistryStorageDecoratorBase.java b/app/src/main/java/io/apicurio/registry/storage/decorator/RegistryStorageDecoratorBase.java
@@ -2,6 +2,7 @@
 
 import io.apicurio.common.apps.config.DynamicConfigPropertyDto;
 import io.apicurio.registry.content.ContentHandle;
+import io.apicurio.registry.model.VersionId;
 import io.apicurio.registry.storage.dto.*;
 import io.apicurio.registry.storage.error.*;
 import io.apicurio.registry.storage.impexp.EntityInputStream;
@@ -339,7 +340,7 @@ public void importArtifactRule(ArtifactRuleEntity entity) {
 
 
     @Override
-    public void importArtifactBranch(ArtifactVersionBranchEntity entity) {
+    public void importArtifactBranch(ArtifactBranchEntity entity) {
         delegate.importArtifactBranch(entity);
     }
 
@@ -395,6 +396,12 @@ public void createOrUpdateArtifactBranch(GAV gav, BranchId branchId) {
     }
 
 
+    @Override
+    public void createOrReplaceArtifactBranch(GA ga, BranchId branchId, List<VersionId> versions) {
+        delegate.createOrReplaceArtifactBranch(ga, branchId, versions);
+    }
+
+
     @Override
     public void deleteArtifactBranch(GA ga, BranchId branchId) {
         delegate.deleteArtifactBranch(ga, branchId);

diff --git a/...ain/java/io/apicurio/registry/storage/decorator/RegistryStorageDecoratorReadOnlyBase.java b/...ain/java/io/apicurio/registry/storage/decorator/RegistryStorageDecoratorReadOnlyBase.java
@@ -390,8 +390,8 @@ public List<String> getArtifactVersions(String groupId, String artifactId, Artif
 
 
     @Override
-    public GAV getArtifactBranchLeaf(GA ga, BranchId branchId, ArtifactRetrievalBehavior behavior) {
-        return delegate.getArtifactBranchLeaf(ga, branchId, behavior);
+    public GAV getArtifactBranchTip(GA ga, BranchId branchId, ArtifactRetrievalBehavior behavior) {
+        return delegate.getArtifactBranchTip(ga, branchId, behavior);
     }
 
 

diff --git a/...curio/registry/storage/dto/BranchDto.java → ...gistry/storage/dto/ArtifactBranchDto.java b/...curio/registry/storage/dto/BranchDto.java → ...gistry/storage/dto/ArtifactBranchDto.java
@@ -10,13 +10,13 @@
 @Setter
 @EqualsAndHashCode
 @ToString
-public class BranchDto {
+public class ArtifactBranchDto {
 
     private String groupId;
 
     private String artifactId;
 
-    private String branch;
+    private String branchId;
 
     private int branchOrder;
 

diff --git a/.../BranchVersionAlreadyExistsException.java → ...ranchAlreadyContainsVersionException.java b/.../BranchVersionAlreadyExistsException.java → ...ranchAlreadyContainsVersionException.java
@@ -4,7 +4,7 @@
 import io.apicurio.registry.model.GAV;
 import lombok.Getter;
 
-public class BranchVersionAlreadyExistsException extends AlreadyExistsException {
+public class ArtifactBranchAlreadyContainsVersionException extends AlreadyExistsException {
 
     private static final long serialVersionUID = -2869727219770505486L;
 
@@ -15,14 +15,14 @@ public class BranchVersionAlreadyExistsException extends AlreadyExistsException
     private final BranchId branchId;
 
 
-    public BranchVersionAlreadyExistsException(GAV gav, BranchId branchId) {
+    public ArtifactBranchAlreadyContainsVersionException(GAV gav, BranchId branchId) {
         super(message(gav, branchId));
         this.gav = gav;
         this.branchId = branchId;
     }
 
 
     private static String message(GAV gav, BranchId branchId) {
-        return "Branch '" + branchId + "' already contains version '" + gav + "'.";
+        return "Artifact branch '" + branchId + "' already contains version '" + gav + "'.";
     }
 }
diff --git a/...torage/error/BranchNotFoundException.java → ...rror/ArtifactBranchNotFoundException.java b/...torage/error/BranchNotFoundException.java → ...rror/ArtifactBranchNotFoundException.java
@@ -5,7 +5,7 @@
 import lombok.Getter;
 
 
-public class BranchNotFoundException extends NotFoundException {
+public class ArtifactBranchNotFoundException extends NotFoundException {
 
     private static final long serialVersionUID = -5382272137668348037L;
 
@@ -16,7 +16,7 @@ public class BranchNotFoundException extends NotFoundException {
     private final BranchId branchId;
 
 
-    public BranchNotFoundException(GA ga, BranchId branchId) {
+    public ArtifactBranchNotFoundException(GA ga, BranchId branchId) {
         super(message(ga, branchId));
         this.ga = ga;
         this.branchId = branchId;

diff --git a/...c/main/java/io/apicurio/registry/storage/impl/gitops/AbstractReadOnlyRegistryStorage.java b/...c/main/java/io/apicurio/registry/storage/impl/gitops/AbstractReadOnlyRegistryStorage.java
@@ -3,6 +3,7 @@
 import io.apicurio.common.apps.config.DynamicConfigPropertyDto;
 import io.apicurio.registry.content.ContentHandle;
 import io.apicurio.registry.exception.UnreachableCodeException;
+import io.apicurio.registry.model.VersionId;
 import io.apicurio.registry.storage.RegistryStorage;
 import io.apicurio.registry.storage.dto.*;
 import io.apicurio.registry.storage.error.RegistryStorageException;
@@ -341,7 +342,7 @@ public void importArtifactRule(ArtifactRuleEntity entity) {
 
 
     @Override
-    public void importArtifactBranch(ArtifactVersionBranchEntity entity) {
+    public void importArtifactBranch(ArtifactBranchEntity entity) {
         readOnlyViolation();
     }
 
@@ -393,6 +394,12 @@ public void createOrUpdateArtifactBranch(GAV gav, BranchId branchId) {
     }
 
 
+    @Override
+    public void createOrReplaceArtifactBranch(GA ga, BranchId branchId, List<VersionId> versions) {
+        readOnlyViolation();
+    }
+
+
     @Override
     public void deleteArtifactBranch(GA ga, BranchId branchId) {
         readOnlyViolation();

diff --git a/app/src/main/java/io/apicurio/registry/storage/impl/gitops/GitOpsRegistryStorage.java b/app/src/main/java/io/apicurio/registry/storage/impl/gitops/GitOpsRegistryStorage.java
@@ -502,8 +502,8 @@ public Map<BranchId, List<GAV>> getArtifactBranches(GA ga) {
 
 
     @Override
-    public GAV getArtifactBranchLeaf(GA ga, BranchId branchId, ArtifactRetrievalBehavior behavior) {
-        return proxy(storage -> storage.getArtifactBranchLeaf(ga, branchId, behavior));
+    public GAV getArtifactBranchTip(GA ga, BranchId branchId, ArtifactRetrievalBehavior behavior) {
+        return proxy(storage -> storage.getArtifactBranchTip(ga, branchId, behavior));
     }
 
 

diff --git a/app/src/main/java/io/apicurio/registry/storage/impl/kafkasql/KafkaSqlRegistryStorage.java b/app/src/main/java/io/apicurio/registry/storage/impl/kafkasql/KafkaSqlRegistryStorage.java
@@ -6,6 +6,10 @@
 import io.apicurio.registry.metrics.StorageMetricsApply;
 import io.apicurio.registry.metrics.health.liveness.PersistenceExceptionLivenessApply;
 import io.apicurio.registry.metrics.health.readiness.PersistenceTimeoutReadinessApply;
+import io.apicurio.registry.model.BranchId;
+import io.apicurio.registry.model.GA;
+import io.apicurio.registry.model.GAV;
+import io.apicurio.registry.model.VersionId;
 import io.apicurio.registry.storage.ArtifactStateExt;
 import io.apicurio.registry.storage.StorageEvent;
 import io.apicurio.registry.storage.StorageEventType;
@@ -24,9 +28,6 @@
 import io.apicurio.registry.storage.impl.sql.SqlUtil;
 import io.apicurio.registry.storage.importing.DataImporter;
 import io.apicurio.registry.storage.importing.SqlDataImporter;
-import io.apicurio.registry.model.BranchId;
-import io.apicurio.registry.model.GA;
-import io.apicurio.registry.model.GAV;
 import io.apicurio.registry.types.ArtifactState;
 import io.apicurio.registry.types.RuleType;
 import io.apicurio.registry.utils.ConcurrentUtil;
@@ -56,7 +57,6 @@
  * An implementation of a registry artifactStore that extends the basic SQL artifactStore but federates 'write' operations
  * to other nodes in a cluster using a Kafka topic.  As a result, all reads are performed locally but all
  * writes are published to a topic for consumption by all nodes.
- *
  */
 @ApplicationScoped
 @PersistenceExceptionLivenessApply
@@ -813,8 +813,8 @@ public void importGroup(GroupEntity entity) {
 
 
     @Override
-    public void importArtifactBranch(ArtifactVersionBranchEntity entity) {
-        submitter.submitBranchImport(entity);
+    public void importArtifactBranch(ArtifactBranchEntity entity) {
+        submitter.submitArtifactBranchImport(entity);
     }
 
 
@@ -900,14 +900,21 @@ public ArtifactMetaDataDto updateArtifact(String groupId, String artifactId, Str
 
     @Override
     public void createOrUpdateArtifactBranch(GAV gav, BranchId branchId) {
-        var uuid = ConcurrentUtil.get(submitter.submitBranch(ActionType.CREATE_OR_UPDATE, gav, branchId));
+        var uuid = ConcurrentUtil.get(submitter.submitArtifactBranch(ActionType.CREATE_OR_UPDATE, gav, branchId));
+        coordinator.waitForResponse(uuid);
+    }
+
+
+    @Override
+    public void createOrReplaceArtifactBranch(GA ga, BranchId branchId, List<VersionId> versions) {
+        var uuid = ConcurrentUtil.get(submitter.submitArtifactBranchCreateOrReplace(ga, branchId, versions));
         coordinator.waitForResponse(uuid);
     }
 
 
     @Override
     public void deleteArtifactBranch(GA ga, BranchId branchId) {
-        var uuid = ConcurrentUtil.get(submitter.submitBranch(ActionType.DELETE, ga, branchId));
+        var uuid = ConcurrentUtil.get(submitter.submitArtifactBranch(ActionType.DELETE, ga, branchId));
         coordinator.waitForResponse(uuid);
     }
 }