Skip to content

Commit

Permalink
[S3] Fix multipart tests (#2985)
Browse files Browse the repository at this point in the history 
Fix a nullptr in one of the tests
  • Loading branch information
@snalli
snalli authored Jan 15, 2025
1 parent 3e96437 commit 7353080
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 37 deletions.
9 changes: 5 additions & 4 deletions ambry-frontend/src/main/java/com/github/ambry/frontend/s3/S3Constants.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
71 changes: 47 additions & 24 deletions ...frontend/src/main/java/com/github/ambry/frontend/s3/S3MultipartCompleteUploadHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
}
}
11 changes: 2 additions & 9 deletions ambry-frontend/src/test/java/com/github/ambry/frontend/S3MultipartUploadTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}

Expand All @@ -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);
}

Expand Down

0 comments on commit 7353080

Please sign in to comment.