From 43d0eeb72cee499316ba8be6a7df6532c3492d0b Mon Sep 17 00:00:00 2001 From: Artur Makutunowicz Date: Tue, 16 Dec 2025 23:03:31 -0800 Subject: [PATCH] Fix missing return statements Two methods in InMemNamedBlobDb completed futures exceptionally but didn't return, allowing execution to continue and potentially complete the future again, causing index out of bounds (OOB) errors. I also added respective unit tests. --- .../ambry/commons/InMemNamedBlobDb.java | 3 + .../ambry/commons/InMemNamedBlobDbTest.java | 158 ++++++++++++++++++ 2 files changed, 161 insertions(+) create mode 100644 ambry-commons/src/test/java/com/github/ambry/commons/InMemNamedBlobDbTest.java diff --git a/ambry-commons/src/main/java/com/github/ambry/commons/InMemNamedBlobDb.java b/ambry-commons/src/main/java/com/github/ambry/commons/InMemNamedBlobDb.java index 9f3c985307..bd1d655310 100644 --- a/ambry-commons/src/main/java/com/github/ambry/commons/InMemNamedBlobDb.java +++ b/ambry-commons/src/main/java/com/github/ambry/commons/InMemNamedBlobDb.java @@ -69,6 +69,7 @@ public CompletableFuture get(String accountName, String contain List rows = getInternal(accountName, containerName, blobName, NamedBlobState.READY); if (rows.isEmpty()) { future.completeExceptionally(new RestServiceException("NotFound", RestServiceErrorCode.NotFound)); + return future; } NamedBlobRecord recordWithDelete = rows.get(rows.size() - 1).getRecord(); @@ -76,6 +77,7 @@ public CompletableFuture get(String accountName, String contain && recordWithDelete.getExpirationTimeMs() < time.milliseconds() && !includeDeletedOrExpiredOptions.contains( option)) { future.completeExceptionally(new RestServiceException("Deleted", RestServiceErrorCode.Deleted)); + return future; } else { future.complete(recordWithDelete); } @@ -192,6 +194,7 @@ public CompletableFuture delete(String accountName, String contain List rows = getInternal(accountName, containerName, blobName); if (rows.isEmpty()) { future.completeExceptionally(new RestServiceException("NotFound", RestServiceErrorCode.NotFound)); + return future; } else { List blobVersions = new ArrayList<>(); for (NamedBlobRow row : rows) { diff --git a/ambry-commons/src/test/java/com/github/ambry/commons/InMemNamedBlobDbTest.java b/ambry-commons/src/test/java/com/github/ambry/commons/InMemNamedBlobDbTest.java new file mode 100644 index 0000000000..9e7ba1d461 --- /dev/null +++ b/ambry-commons/src/test/java/com/github/ambry/commons/InMemNamedBlobDbTest.java @@ -0,0 +1,158 @@ +/** + * Copyright 2025 LinkedIn Corp. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + */ +package com.github.ambry.commons; + +import com.github.ambry.named.DeleteResult; +import com.github.ambry.named.NamedBlobRecord; +import com.github.ambry.named.PutResult; +import com.github.ambry.protocol.GetOption; +import com.github.ambry.protocol.NamedBlobState; +import com.github.ambry.rest.RestServiceErrorCode; +import com.github.ambry.rest.RestServiceException; +import com.github.ambry.utils.MockTime; +import com.github.ambry.utils.Utils; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; +import org.junit.Test; + +import static org.junit.Assert.*; + + +/** + * Test class for {@link InMemNamedBlobDb}. + */ +public class InMemNamedBlobDbTest { + private static final String ACCOUNT_NAME = "testAccount"; + private static final String CONTAINER_NAME = "testContainer"; + private static final String BLOB_NAME = "testBlob"; + private static final String BLOB_ID = "testBlobId"; + private static final long BLOB_SIZE = 1024L; + + /** + * Test that getting an expired blob without Include_Expired_Blobs option returns Deleted error + * and does not complete the future successfully (tests the bug fix for missing return statement). + */ + @Test + public void testGetExpiredBlobReturnsDeletedError() throws Exception { + MockTime mockTime = new MockTime(1000L); + InMemNamedBlobDb db = new InMemNamedBlobDb(mockTime, 100, false); + + // Create a blob with expiration time in the past + long expirationTime = 500L; // Expired (current time is 1000L) + NamedBlobRecord record = new NamedBlobRecord(ACCOUNT_NAME, CONTAINER_NAME, BLOB_NAME, BLOB_ID, expirationTime, + NamedBlobRecord.UNINITIALIZED_VERSION, BLOB_SIZE, 0, false); + + // Put the blob + CompletableFuture putFuture = db.put(record, NamedBlobState.READY, false); + putFuture.get(); // Wait for put to complete + + // Try to get the expired blob without Include_Expired_Blobs option + CompletableFuture getFuture = db.get(ACCOUNT_NAME, CONTAINER_NAME, BLOB_NAME, GetOption.None, + false); + + // Verify that the future completes exceptionally with Deleted error + try { + getFuture.get(); + fail("Expected RestServiceException with Deleted error code"); + } catch (ExecutionException e) { + assertTrue("Exception should be RestServiceException", e.getCause() instanceof RestServiceException); + RestServiceException restException = (RestServiceException) e.getCause(); + assertEquals("Error code should be Deleted", RestServiceErrorCode.Deleted, restException.getErrorCode()); + } + + // Verify that the future is not completed successfully (this would happen if the return was missing) + assertTrue("Future should be completed exceptionally", getFuture.isCompletedExceptionally()); + assertFalse("Future should not be completed normally", getFuture.isDone() && !getFuture.isCompletedExceptionally()); + } + + /** + * Test that getting an expired blob with Include_Expired_Blobs option returns the blob successfully. + */ + @Test + public void testGetExpiredBlobWithIncludeExpiredOption() throws Exception { + MockTime mockTime = new MockTime(1000L); + InMemNamedBlobDb db = new InMemNamedBlobDb(mockTime, 100, false); + + // Create a blob with expiration time in the past + long expirationTime = 500L; // Expired (current time is 1000L) + NamedBlobRecord record = new NamedBlobRecord(ACCOUNT_NAME, CONTAINER_NAME, BLOB_NAME, BLOB_ID, expirationTime, + NamedBlobRecord.UNINITIALIZED_VERSION, BLOB_SIZE, 0, false); + + // Put the blob + CompletableFuture putFuture = db.put(record, NamedBlobState.READY, false); + putFuture.get(); // Wait for put to complete + + // Get the expired blob with Include_Expired_Blobs option + CompletableFuture getFuture = db.get(ACCOUNT_NAME, CONTAINER_NAME, BLOB_NAME, + GetOption.Include_Expired_Blobs, false); + + // Verify that the future completes successfully + NamedBlobRecord result = getFuture.get(); + assertNotNull("Result should not be null", result); + assertEquals("Blob name should match", BLOB_NAME, result.getBlobName()); + assertEquals("Expiration time should match", expirationTime, result.getExpirationTimeMs()); + } + + /** + * Test that deleting a non-existent blob returns NotFound error and does not complete successfully + * (tests the bug fix for missing return statement in delete method). + */ + @Test + public void testDeleteNonExistentBlobReturnsNotFoundError() throws Exception { + MockTime mockTime = new MockTime(1000L); + InMemNamedBlobDb db = new InMemNamedBlobDb(mockTime, 100, false); + + // Try to delete a blob that doesn't exist + CompletableFuture deleteFuture = db.delete(ACCOUNT_NAME, CONTAINER_NAME, BLOB_NAME); + + // Verify that the future completes exceptionally with NotFound error + try { + deleteFuture.get(); + fail("Expected RestServiceException with NotFound error code"); + } catch (ExecutionException e) { + assertTrue("Exception should be RestServiceException", e.getCause() instanceof RestServiceException); + RestServiceException restException = (RestServiceException) e.getCause(); + assertEquals("Error code should be NotFound", RestServiceErrorCode.NotFound, restException.getErrorCode()); + } + + // Verify that the future is not completed successfully (this would happen if the return was missing) + assertTrue("Future should be completed exceptionally", deleteFuture.isCompletedExceptionally()); + assertFalse("Future should not be completed normally", + deleteFuture.isDone() && !deleteFuture.isCompletedExceptionally()); + } + + /** + * Test that deleting an existing blob completes successfully. + */ + @Test + public void testDeleteExistingBlob() throws Exception { + MockTime mockTime = new MockTime(1000L); + InMemNamedBlobDb db = new InMemNamedBlobDb(mockTime, 100, false); + + // Create and put a blob + NamedBlobRecord record = new NamedBlobRecord(ACCOUNT_NAME, CONTAINER_NAME, BLOB_NAME, BLOB_ID, + Utils.Infinite_Time, NamedBlobRecord.UNINITIALIZED_VERSION, BLOB_SIZE, 0, false); + CompletableFuture putFuture = db.put(record, NamedBlobState.READY, false); + putFuture.get(); // Wait for put to complete + + // Delete the blob + CompletableFuture deleteFuture = db.delete(ACCOUNT_NAME, CONTAINER_NAME, BLOB_NAME); + + // Verify that the future completes successfully + DeleteResult result = deleteFuture.get(); + assertNotNull("Result should not be null", result); + assertFalse("Blob versions should not be empty", result.getBlobVersions().isEmpty()); + } +} +