diff --git a/ambry-frontend/src/main/java/com/github/ambry/frontend/s3/S3Constants.java b/ambry-frontend/src/main/java/com/github/ambry/frontend/s3/S3Constants.java index e93c113241..47b52963ea 100644 --- a/ambry-frontend/src/main/java/com/github/ambry/frontend/s3/S3Constants.java +++ b/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); } diff --git a/ambry-frontend/src/main/java/com/github/ambry/frontend/s3/S3MultipartCompleteUploadHandler.java b/ambry-frontend/src/main/java/com/github/ambry/frontend/s3/S3MultipartCompleteUploadHandler.java index 06c89dc70d..6484cab2af 100644 --- a/ambry-frontend/src/main/java/com/github/ambry/frontend/s3/S3MultipartCompleteUploadHandler.java +++ b/ambry-frontend/src/main/java/com/github/ambry/frontend/s3/S3MultipartCompleteUploadHandler.java @@ -83,9 +83,6 @@ public class S3MultipartCompleteUploadHandler { 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 getChunksToStitch(CompleteMultipartUpload completeMultipartUpload) throws RestServiceException { // Get parts in order from CompleteMultipartUpload, deserialize each part id to get data chunk ids. + List parts = validatePartsOrThrow(completeMultipartUpload); List chunkInfos = new ArrayList<>(); try { // sort the list in order - List 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 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 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 validatePartsOrThrow(CompleteMultipartUpload request) throws RestServiceException { + List parts = getParts(request); Set partNumbers = new HashSet<>(); Set 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 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 parts = Arrays.asList(part); + if (parts.size() > S3Constants.MAX_LIST_SIZE) { + throw new RestServiceException(S3Constants.ERR_PART_LIST_TOO_LONG, RestServiceErrorCode.BadRequest); + } + return parts; } } diff --git a/ambry-frontend/src/test/java/com/github/ambry/frontend/S3MultipartUploadTest.java b/ambry-frontend/src/test/java/com/github/ambry/frontend/S3MultipartUploadTest.java index 44546f7941..af09b3608a 100644 --- a/ambry-frontend/src/test/java/com/github/ambry/frontend/S3MultipartUploadTest.java +++ b/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); }