diff --git a/ambry-frontend/src/integration-test/java/com/github/ambry/frontend/S3IntegrationTest.java b/ambry-frontend/src/integration-test/java/com/github/ambry/frontend/S3IntegrationTest.java index 678caadbf1..4cfd86447c 100644 --- a/ambry-frontend/src/integration-test/java/com/github/ambry/frontend/S3IntegrationTest.java +++ b/ambry-frontend/src/integration-test/java/com/github/ambry/frontend/S3IntegrationTest.java @@ -327,6 +327,65 @@ private void doListBlob(String account, String container, String key) throws Exc assertEquals("Mismatch in encoding type", "url", listBucketResult.getEncodingType()); } + /** + * Reproduces the request shape that the AWS S3 SDK emits when the caller sets an unset/empty + * prefix on ListObjectsRequest: the query string contains {@code prefix=} (parameter present, + * value empty) rather than omitting the parameter entirely. This is the exact shape that + * caused the original empty-prefix regression on option 4 (LIST_WITH_PREFIX_SQL evaluated + * {@code blob_name LIKE '%'} over a full container scan, timing out at MAX_EXECUTION_TIME); + * fixed in linkedin/ambry#3265 by collapsing empty {@code prefix} to {@code null} in + * {@link NamedBlobPath#parseS3}. + * + * This test asserts the request path returns 200 OK end-to-end through the S3 handler stack + * (HTTP → Netty → {@code S3ListHandler} → {@code NamedBlobPath.parseS3} → {@code + * NamedBlobListHandler} → {@code NamedBlobDb#list}). The named-blob DB in this integration + * test is {@code InMemNamedBlobDbFactory}, so this test specifically catches regressions in + * the S3-handler routing layer (empty-prefix collapse, parseS3 logic) — not SQL-side + * regressions, which are covered by + * {@code MySqlNamedBlobDbListOperationIntegrationTest#testListNamedBlobsWithNullPrefix} + * against a real MySQL backend. + */ + @Test + public void s3ListEmptyPrefixTest() throws Exception { + Container container = ACCOUNT.getAllContainers().iterator().next(); + String account = ACCOUNT.getName(); + String containerName = container.getName(); + + // Seed a couple of blobs so the LIST has something to return — the test focuses on the + // request shape and routing, not on the content of the response. + String[] keys = new String[]{"empty_prefix_seed_a", "empty_prefix_seed_b"}; + int contentSize = 64; + for (String key : keys) { + byte[] content = TestUtils.getRandomBytes(contentSize); + doPutBlob(account, containerName, key, contentSize, content); + } + + // V1 LIST: GET /s3/{account}/{container}?prefix= (explicit empty value) + String uriV1 = String.format("/s3/%s/%s?prefix=", account, containerName); + HttpHeaders headers = new DefaultHttpHeaders(); + FullHttpRequest reqV1 = buildRequest(HttpMethod.GET, uriV1, headers, null); + NettyClient.ResponseParts partsV1 = nettyClient.sendRequest(reqV1, null, null).get(); + HttpResponse respV1 = getHttpResponse(partsV1); + assertEquals("LIST v1 with explicit empty prefix should return 200 OK end-to-end through " + + "the S3 handler stack; regression in parseS3 empty-prefix collapse would surface here", + HttpResponseStatus.OK, respV1.status()); + + // V2 LIST: GET /s3/{account}/{container}?prefix=&list-type=2 + String uriV2 = String.format("/s3/%s/%s?prefix=&list-type=2", account, containerName); + FullHttpRequest reqV2 = buildRequest(HttpMethod.GET, uriV2, new DefaultHttpHeaders(), null); + NettyClient.ResponseParts partsV2 = nettyClient.sendRequest(reqV2, null, null).get(); + HttpResponse respV2 = getHttpResponse(partsV2); + assertEquals("LIST v2 with explicit empty prefix should return 200 OK end-to-end", + HttpResponseStatus.OK, respV2.status()); + + // Cleanup + for (String key : keys) { + String deleteUri = String.format("/s3/%s/%s/%s", account, containerName, key); + FullHttpRequest delReq = buildRequest(HttpMethod.DELETE, deleteUri, new DefaultHttpHeaders(), null); + nettyClient.sendRequest(delReq, null, null).get(); + } + } + /** * Builds properties required to start a {@link RestServer} as an Ambry frontend server. * @param trustStoreFile the trust store file to add certificates to for SSL testing. diff --git a/ambry-named-mysql/src/integration-test/java/com/github/ambry/named/MySqlNamedBlobDbListOperationIntegrationTest.java b/ambry-named-mysql/src/integration-test/java/com/github/ambry/named/MySqlNamedBlobDbListOperationIntegrationTest.java index a5970c593b..babaf1cdd2 100644 --- a/ambry-named-mysql/src/integration-test/java/com/github/ambry/named/MySqlNamedBlobDbListOperationIntegrationTest.java +++ b/ambry-named-mysql/src/integration-test/java/com/github/ambry/named/MySqlNamedBlobDbListOperationIntegrationTest.java @@ -30,6 +30,7 @@ import java.util.Set; import java.util.TimeZone; import java.util.concurrent.TimeUnit; +import org.junit.Assume; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -224,6 +225,101 @@ public void testListHidesBlobWhenLatestVersionIsExpired() throws Exception { + page.getEntries(), 0, page.getEntries().size()); } + /** + * Test case for list named blobs with a null prefix — the LIST_ALL_QUERY code path. + * + * Closes a coverage gap surfaced after #3265: prior to this test, every list() invocation in + * this integration suite passed a non-null prefix, so the no-prefix LIST code path + * (LIST_ALL_QUERY for options 2/3, LIST_ALL_SQL window-function variant for option 4) was + * never exercised by the int-test matrix. The TMC regression that motivated #3265 specifically + * crossed this path (S3 SDK sends `prefix=` empty → parseS3 collapses to null → list() with + * null prefix → LIST_ALL_QUERY under option 4), and option 4's window-function variant only + * shipped with unit-test coverage. This test runs against options 2/3/4 × hard-delete on/off + * via the existing parameterized matrix. + * + * Option-agnostic: PUT N distinct blobs in one container, LIST with null prefix, expect all N + * back in blob_name order. No multi-version / deleted-latest scenario here — see the + * follow-up testListAllNullPrefixHidesDeletedLatestUnderOption4 for that, which has + * different semantics across options 2/3 vs option 4 and so is option-4-only. + */ + @Test + public void testListNamedBlobsWithNullPrefix() throws Exception { + Calendar calendar = Calendar.getInstance(TimeZone.getTimeZone("UTC")); + time.setCurrentMilliseconds(calendar.getTimeInMillis()); + + Account account = accountService.getAllAccounts().iterator().next(); + Container container = account.getAllContainers().iterator().next(); + + // Seed N blobs with stable lexicographic names so we can assert ordering deterministically. + final int N = 5; + final String[] blobNames = new String[N]; + for (int i = 0; i < N; i++) { + blobNames[i] = String.format("testListAllNullPrefix-%02d-%s", i, TestUtils.getRandomKey(8)); + } + Arrays.sort(blobNames); + for (String name : blobNames) { + NamedBlobRecord record = new NamedBlobRecord(account.getName(), container.getName(), name, + getBlobId(account, container), + calendar.getTimeInMillis() + TimeUnit.HOURS.toMillis(1)); + namedBlobDb.put(record, NamedBlobState.READY, true).get(); + } + + // null prefix triggers LIST_ALL_QUERY (or LIST_ALL_SQL under option 4). This is the path + // TMC's empty-prefix S3 LIST collapses to via parseS3. + Page page = + namedBlobDb.list(account.getName(), container.getName(), null, null, null).get(); + + assertEquals("Null-prefix LIST should return all " + N + " seeded blobs", + N, page.getEntries().size()); + for (int i = 0; i < N; i++) { + assertEquals("Null-prefix LIST should return blobs in blob_name ascending order", + blobNames[i], page.getEntries().get(i).getBlobName()); + } + } + + /** + * Option-4-only invariant test for the null-prefix path: if the latest READY version of a + * blob is TTL-expired (or soft-deleted), the blob must be hidden entirely from a null-prefix + * LIST. Under option 4, LIST_ALL_SQL applies the deleted_ts filter on the OUTER select after + * the window operator, so the second-latest non-deleted version is NOT surfaced — matching + * the LIST_WITH_PREFIX_SQL contract (already covered by testListHidesBlobWhenLatestVersionIsExpired). + * + * Options 2/3 LIST_ALL_QUERY has the opposite semantic (deleted_ts filter inside the inner + * subquery → second-latest surfaces). That divergence is intentional and is preserved by + * #3265 to avoid changing default behavior for fabrics still on options 2/3. So this test + * only runs under option 4. + */ + @Test + public void testListAllNullPrefixHidesDeletedLatestUnderOption4() throws Exception { + Assume.assumeTrue("Semantic only holds for option 4 LIST_ALL_SQL; options 2/3 surface " + + "the second-latest non-deleted version by design", listSqlOption == 4); + Calendar calendar = Calendar.getInstance(TimeZone.getTimeZone("UTC")); + time.setCurrentMilliseconds(calendar.getTimeInMillis()); + + Account account = accountService.getAllAccounts().iterator().next(); + Container container = account.getAllContainers().iterator().next(); + final String blobName = "testListAllNullPrefixHidesDeletedLatest"; + + // v1: older version, expires far in the future. + NamedBlobRecord v1 = new NamedBlobRecord(account.getName(), container.getName(), blobName, + getBlobId(account, container), calendar.getTimeInMillis() + TimeUnit.HOURS.toMillis(1)); + namedBlobDb.put(v1, NamedBlobState.READY, true).get(); + + // Advance the mock clock so v2's generated version is strictly greater than v1's. + time.sleep(100); + + // v2: latest version, already expired at LIST time. + NamedBlobRecord v2 = new NamedBlobRecord(account.getName(), container.getName(), blobName, + getBlobId(account, container), calendar.getTimeInMillis() - TimeUnit.HOURS.toMillis(1)); + namedBlobDb.put(v2, NamedBlobState.READY, true).get(); + + // null prefix → LIST_ALL_SQL under option 4. The blob must be hidden entirely. + Page page = + namedBlobDb.list(account.getName(), container.getName(), null, null, null).get(); + assertEquals("Option 4: latest version expired; null-prefix LIST must hide the blob entirely " + + "(no older-version leak). Got " + page.getEntries(), 0, page.getEntries().size()); + } + /** * Test case for list named blobs with prefix. * @throws Exception