[fix][broker] Should not throw NotFoundException when metadata alread…

…y exists. (apache#20539) ## Motivation Make the Exception more clear. ## Modification Return the `409 Conflict` instead of `404 NoFound` when metadata already exists.
lhotari · Jun 9, 2023 · aa1d5d9 · aa1d5d9
1 parent 362ece3
commit aa1d5d9
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 2 deletions.
diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PackagesBase.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PackagesBase.java
@@ -19,6 +19,7 @@
 package org.apache.pulsar.broker.admin.impl;
 
 import java.io.InputStream;
+import java.nio.file.FileAlreadyExistsException;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ExecutionException;
 import javax.ws.rs.WebApplicationException;
@@ -64,6 +65,8 @@ private Void handleError(Throwable throwable, AsyncResponse asyncResponse) {
             asyncResponse.resume(throwable);
         } else if (throwable instanceof UnsupportedOperationException) {
             asyncResponse.resume(new RestException(Response.Status.SERVICE_UNAVAILABLE, throwable.getMessage()));
+        } else if (throwable instanceof FileAlreadyExistsException) {
+            asyncResponse.resume(new RestException(Response.Status.CONFLICT, throwable.getMessage()));
         } else {
             log.error("Encountered unexpected error", throwable);
             asyncResponse.resume(new RestException(Response.Status.INTERNAL_SERVER_ERROR, throwable.getMessage()));

diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/v3/PackagesApiTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/v3/PackagesApiTest.java
@@ -56,6 +56,23 @@ protected void cleanup() throws Exception {
         super.internalCleanup();
     }
 
+    @Test
+    public void testRepeatUploadThrowConflictException() throws Exception {
+        // create a temp file for testing
+        File file = File.createTempFile("package-api-test", ".package");
+
+        // testing upload api
+        String packageName = "function://public/default/test@v1";
+        PackageMetadata originalMetadata = PackageMetadata.builder().description("test").build();
+        admin.packages().upload(originalMetadata, packageName, file.getPath());
+        try {
+            admin.packages().upload(originalMetadata, packageName, file.getPath());
+            fail();
+        } catch (PulsarAdminException e) {
+            assertEquals(e.getStatusCode(), 409);
+        }
+    }
+
     @Test(timeOut = 60000)
     public void testPackagesOperations() throws Exception {
         // create a temp file for testing

diff --git a/...src/main/java/org/apache/pulsar/packages/management/core/impl/PackagesManagementImpl.java b/...src/main/java/org/apache/pulsar/packages/management/core/impl/PackagesManagementImpl.java
@@ -23,6 +23,7 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.nio.file.FileAlreadyExistsException;
 import java.util.List;
 import java.util.concurrent.CompletableFuture;
 import org.apache.pulsar.packages.management.core.PackagesManagement;
@@ -187,8 +188,8 @@ private CompletableFuture<Void> checkMetadataExistsAndThrowException(PackageName
                 if (!exist) {
                     future.complete(null);
                 } else {
-                    future.completeExceptionally(
-                        new NotFoundException(String.format("Package '%s' metadata already exists", packageName)));
+                    future.completeExceptionally(new FileAlreadyExistsException(
+                            String.format("Package '%s' metadata already exists", packageName)));
                 }
             });
         return future;