[S3] Fix multipart tests

ambry-frontend/src/main/java/com/github/ambry/frontend/s3/S3Constants.java

@@ -17,13 +17,14 @@
 public class S3Constants {
   public static final int MIN_PART_NUM = 1;
   public static final int MAX_PART_NUM = 10000;
-  public static final int MAX_LIST_SIZE = 10000;
+  public static final int MAX_LIST_SIZE = MAX_PART_NUM; // since parts are contiguous, the list size cannot exceed the max part number
 
   // Error Messages
+  public static final String ERR_INVALID_MULTIPART_UPLOAD = "Invalid multipart upload.";
   public static final String ERR_INVALID_PART_NUMBER =
-      "Invalid part number: %d. Part number must be an integer between %d and %d.";
-  public static final String ERR_DUPLICATE_PART_NUMBER = "Duplicate part number found: %d.";
+      "Invalid part number: %s. " + String.format("Part number must be an integer between %s and %s.", MIN_PART_NUM, MAX_PART_NUM);
+  public static final String ERR_DUPLICATE_PART_NUMBER = "Duplicate part number found: %s.";
   public static final String ERR_DUPLICATE_ETAG = "Duplicate eTag found: %s.";
   public static final String ERR_EMPTY_REQUEST_BODY = "Xml request body cannot be empty.";
-  public static final String ERR_PART_LIST_TOO_LONG = String.format("Parts list size cannot exceed %d.", MAX_LIST_SIZE);
+  public static final String ERR_PART_LIST_TOO_LONG = String.format("Parts list size cannot exceed %s.", MAX_LIST_SIZE);
 }

ambry-frontend/src/main/java/com/github/ambry/frontend/s3/S3MultipartCompleteUploadHandler.java

@@ -83,9 +83,6 @@
 public class S3MultipartCompleteUploadHandler<R> {
   private static final Logger LOGGER = LoggerFactory.getLogger(S3MultipartCompleteUploadHandler.class);
   private static final ObjectMapper objectMapper = new XmlMapper();
-  private static final int MIN_PART_NUM = 1;
-  private static final int MAX_PART_NUM = 10000;
-  private static final int MAX_LIST_SIZE = 10000;
   private final SecurityService securityService;
   private final FrontendMetrics frontendMetrics;
   private final AccountAndContainerInjector accountAndContainerInjector;
@@ -385,11 +382,10 @@ private boolean isRetriable(Throwable throwable) {
      */
     List<ChunkInfo> getChunksToStitch(CompleteMultipartUpload completeMultipartUpload) throws RestServiceException {
       // Get parts in order from CompleteMultipartUpload, deserialize each part id to get data chunk ids.
+      List<Part> parts = validatePartsOrThrow(completeMultipartUpload);
       List<ChunkInfo> chunkInfos = new ArrayList<>();
       try {
         // sort the list in order
-        List<Part> parts = Arrays.asList(completeMultipartUpload.getPart());
-        validatePartsOrThrow(parts);
         Collections.sort(parts, Comparator.comparingInt(Part::getPartNumber));
         String reservedMetadataId = null;
         for (Part part : parts) {
@@ -427,41 +423,68 @@ List<ChunkInfo> getChunksToStitch(CompleteMultipartUpload completeMultipartUploa
    * 2. Disallow duplicate etags
    * 3. Check for list size 10000
    * 4. Check for part numbers integer 1-10000
-   * @param parts sorted parts list
+   * @param request the {@link CompleteMultipartUpload} request
    * @return the bad request error
    */
-  private static void validatePartsOrThrow(List<Part> parts) throws RestServiceException {
-    if (parts == null || parts.isEmpty()) {
-      throw new RestServiceException(S3Constants.ERR_EMPTY_REQUEST_BODY, RestServiceErrorCode.BadRequest);
-    }
-
-    if (parts.size() > S3Constants.MAX_LIST_SIZE) {
-      String error = S3Constants.ERR_PART_LIST_TOO_LONG;
-      throw new RestServiceException(error, RestServiceErrorCode.BadRequest);
-    }
-
+  List<Part> validatePartsOrThrow(CompleteMultipartUpload request) throws RestServiceException {
+    List<Part> parts = getParts(request);
     Set<Integer> partNumbers = new HashSet<>();
     Set<String> etags = new HashSet<>();
-
     for (Part part : parts) {
-      int partNumber = part.getPartNumber();
-      String etag = part.geteTag();
-
+      int partNumber = getPartNumber(part);
       if (partNumber < S3Constants.MIN_PART_NUM || partNumber > S3Constants.MAX_PART_NUM) {
-        String error = String.format(S3Constants.ERR_INVALID_PART_NUMBER, partNumber, S3Constants.MIN_PART_NUM,
-            S3Constants.MAX_PART_NUM);
+        String error = String.format(S3Constants.ERR_INVALID_PART_NUMBER, partNumber);
         throw new RestServiceException(error, RestServiceErrorCode.BadRequest);
       }
-
       if (!partNumbers.add(partNumber)) {
         String error = String.format(S3Constants.ERR_DUPLICATE_PART_NUMBER, partNumber);
         throw new RestServiceException(error, RestServiceErrorCode.BadRequest);
       }
-
+      String etag = part.geteTag();
       if (!etags.add(etag)) {
         String error = String.format(S3Constants.ERR_DUPLICATE_ETAG, etag);
         throw new RestServiceException(error, RestServiceErrorCode.BadRequest);
       }
     }
+    return parts;
+  }
+
+  /**
+   * Get the part number from the part object
+   * @param part
+   * @return
+   * @throws RestServiceException
+   */
+  int getPartNumber(Part part) throws RestServiceException {
+    try {
+      return part.getPartNumber();
+    } catch (NumberFormatException e) {
+      // cannot use getPartNumber() here as it would cause another exception
+      String error = String.format(S3Constants.ERR_INVALID_PART_NUMBER, part);
+      throw new RestServiceException(error, RestServiceErrorCode.BadRequest);
+    } catch (Throwable e) {
+      // any other exception is invalid
+      throw new RestServiceException(S3Constants.ERR_INVALID_MULTIPART_UPLOAD, RestServiceErrorCode.BadRequest);
+    }
+  }
+
+  /**
+   * Get the list of parts from the request
+   * @param request
+   * @return
+   * @throws RestServiceException
+   */
+  List<Part> getParts(CompleteMultipartUpload request) throws RestServiceException {
+    Part[] part = request.getPart();
+    if (part == null) {
+      throw new RestServiceException(S3Constants.ERR_EMPTY_REQUEST_BODY, RestServiceErrorCode.BadRequest);
+    }
+    // Arrays.asList() can return an empty list, but only if it is called with no arguments.
+    // Therefore, the list below will always have at least one element as we are passing an argument.
+    List<Part> parts = Arrays.asList(part);
+    if (parts.size() > S3Constants.MAX_LIST_SIZE) {
+      throw new RestServiceException(S3Constants.ERR_PART_LIST_TOO_LONG, RestServiceErrorCode.BadRequest);
+    }
+    return parts;
   }
 }

ambry-frontend/src/test/java/com/github/ambry/frontend/S3MultipartUploadTest.java

@@ -19,7 +19,6 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.dataformat.xml.XmlMapper;
 import com.github.ambry.account.Account;
-import com.github.ambry.account.AccountService;
 import com.github.ambry.account.Container;
 import com.github.ambry.account.ContainerBuilder;
 import com.github.ambry.account.InMemAccountService;
@@ -46,24 +45,18 @@
 import com.github.ambry.rest.RestMethod;
 import com.github.ambry.rest.RestRequest;
 import com.github.ambry.rest.RestResponseChannel;
-import com.github.ambry.rest.RestServiceException;
 import com.github.ambry.router.ByteBufferRSC;
 import com.github.ambry.router.FutureResult;
 import com.github.ambry.router.InMemoryRouter;
 import com.github.ambry.router.ReadableStreamChannel;
-import com.github.ambry.router.Router;
 import com.github.ambry.utils.TestUtils;
 import com.github.ambry.utils.Utils;
 import java.io.ByteArrayOutputStream;
-import java.io.IOException;
-import java.net.URISyntaxException;
 import java.nio.ByteBuffer;
 import java.nio.charset.StandardCharsets;
-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.LinkedList;
-import java.util.List;
 import java.util.Properties;
 import org.json.JSONObject;
 import org.junit.Test;
@@ -274,7 +267,7 @@ public void testInvalidPartNumLessThanMin() throws Exception {
     Part part1 = new Part("0", "etag1");
     Part part2 = new Part("1", "etag2");
     Part[] parts = {part2, part1};
-    String expectedMessage = String.format(S3Constants.ERR_INVALID_PART_NUMBER, 0, S3Constants.MIN_PART_NUM, S3Constants.MAX_PART_NUM);
+    String expectedMessage = String.format(S3Constants.ERR_INVALID_PART_NUMBER, 0);
     testMultipartUploadWithInvalidParts(parts, expectedMessage);
   }
 
@@ -284,7 +277,7 @@ public void testPartNumberInvalidExceedsMax() throws Exception {
     Part part1 = new Part("2", "etag1");
     Part part2 = new Part(String.valueOf(invalidPartNumber), "etag2");
     Part[] parts = {part2, part1};
-    String expectedMessage = String.format(S3Constants.ERR_INVALID_PART_NUMBER, invalidPartNumber, S3Constants.MIN_PART_NUM, S3Constants.MAX_PART_NUM);
+    String expectedMessage = String.format(S3Constants.ERR_INVALID_PART_NUMBER, invalidPartNumber);
     testMultipartUploadWithInvalidParts(parts, expectedMessage);
   }