From 716b9f9c4d28f0c21ec0ec9a7ec77d0ca9aa4ede Mon Sep 17 00:00:00 2001 From: Nick Lopez Date: Mon, 28 Sep 2026 17:31:29 -0700 Subject: [PATCH 1/5] Classify generated upload ID failures as server errors Clients cannot correct malformed IDs produced during upload finalization. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ambry/frontend/NamedBlobPutHandler.java | 14 +- .../ambry/frontend/PostBlobHandler.java | 20 ++- .../s3/S3MultipartCompleteUploadHandler.java | 10 +- .../s3/S3MultipartUploadPartHandler.java | 13 +- .../FrontendRestRequestServiceTest.java | 128 +++++++++++++++++ .../ambry/router/NonBlockingRouter.java | 11 +- .../ambry/router/UploadIdConversionTest.java | 134 ++++++++++++++++++ 7 files changed, 321 insertions(+), 9 deletions(-) create mode 100644 ambry-router/src/test/java/com/github/ambry/router/UploadIdConversionTest.java diff --git a/ambry-frontend/src/main/java/com/github/ambry/frontend/NamedBlobPutHandler.java b/ambry-frontend/src/main/java/com/github/ambry/frontend/NamedBlobPutHandler.java index ba5ed9fc96..569e81a7ac 100644 --- a/ambry-frontend/src/main/java/com/github/ambry/frontend/NamedBlobPutHandler.java +++ b/ambry-frontend/src/main/java/com/github/ambry/frontend/NamedBlobPutHandler.java @@ -81,6 +81,7 @@ * blob content, accepts a UTF-8 JSON object that includes the signed IDs for the chunks to stitch, and header x-ambry-put-mode should set as "STITCH". *

Request body format

* The body of the request should be a JSON object that conforms to the format described in {@link StitchRequestSerDe}. + * Invalid generated blob IDs are server errors; invalid client-supplied stitch IDs are client errors. */ public class NamedBlobPutHandler { private static final Logger LOGGER = LoggerFactory.getLogger(NamedBlobPutHandler.class); @@ -152,6 +153,7 @@ private class CallbackChain { private final Callback finalCallback; private final Callback deleteDatasetCallback; private final String uri; + private boolean blobIdIsServerGenerated; /** * @param restRequest the {@link RestRequest}. @@ -162,8 +164,14 @@ private CallbackChain(RestRequest restRequest, RestResponseChannel restResponseC Callback finalCallback) { this.restRequest = restRequest; this.restResponseChannel = restResponseChannel; - this.finalCallback = finalCallback; - this.deleteDatasetCallback = deleteDatasetVersionIfUploadFailedCallBack(finalCallback); + this.finalCallback = (result, exception) -> { + if (blobIdIsServerGenerated && exception instanceof RouterException + && ((RouterException) exception).getErrorCode() == RouterErrorCode.InvalidBlobId) { + exception = new RestServiceException(exception, RestServiceErrorCode.InternalServerError); + } + finalCallback.onCompletion(result, exception); + }; + this.deleteDatasetCallback = deleteDatasetVersionIfUploadFailedCallBack(this.finalCallback); this.uri = restRequest.getUri(); } @@ -226,6 +234,7 @@ private Callback securityPostProcessRequestCallback(BlobInfo blobInfo) { addDatasetVersion(blobInfo.getBlobProperties(), restRequest); } PutBlobOptions options = getPutBlobOptionsFromRequest(); + blobIdIsServerGenerated = true; router.putBlob(restRequest, getPropertiesForRouterUpload(blobInfo), blobInfo.getUserMetadata(), restRequest, options, routerPutBlobCallback(blobInfo), QuotaUtils.buildQuotaChargeCallback(restRequest, quotaManager, true)); @@ -291,6 +300,7 @@ private Callback fetchStitchRequestBodyCallback(RetainingAsyncWritableChan */ private Callback routerStitchBlobCallback(BlobInfo blobInfo, BlobProperties propertiesForRouterUpload) { return buildCallback(frontendMetrics.putRouterStitchBlobMetrics, convertedBlobId -> { + blobIdIsServerGenerated = true; // The actual blob size is now present in the instance of BlobProperties passed to the router.stitchBlob(). // Update it in the BlobInfo so that IdConverter can add it to the named blob DB blobInfo.getBlobProperties().setBlobSize(propertiesForRouterUpload.getBlobSize()); diff --git a/ambry-frontend/src/main/java/com/github/ambry/frontend/PostBlobHandler.java b/ambry-frontend/src/main/java/com/github/ambry/frontend/PostBlobHandler.java index 08a8e4f1f1..a07e6e57d9 100644 --- a/ambry-frontend/src/main/java/com/github/ambry/frontend/PostBlobHandler.java +++ b/ambry-frontend/src/main/java/com/github/ambry/frontend/PostBlobHandler.java @@ -33,6 +33,8 @@ import com.github.ambry.router.PutBlobOptions; import com.github.ambry.router.PutBlobOptionsBuilder; import com.github.ambry.router.Router; +import com.github.ambry.router.RouterErrorCode; +import com.github.ambry.router.RouterException; import com.github.ambry.utils.Pair; import com.github.ambry.utils.Time; import com.github.ambry.utils.Utils; @@ -64,6 +66,7 @@ * blob content, accepts a UTF-8 JSON object that includes the signed IDs for the chunks to stitch. *

Request body format

* The body of the request should be a JSON object that conforms to the format described in {@link StitchRequestSerDe}. + * Invalid generated blob IDs are server errors; invalid client-supplied stitch IDs are client errors. */ class PostBlobHandler { /** @@ -128,6 +131,7 @@ private class CallbackChain { private final String uri; private final RestResponseChannel restResponseChannel; private final Callback finalCallback; + private boolean blobIdIsServerGenerated; /** * @param restRequest the {@link RestRequest}. @@ -138,7 +142,13 @@ private CallbackChain(RestRequest restRequest, RestResponseChannel restResponseC Callback finalCallback) { this.restRequest = restRequest; this.restResponseChannel = restResponseChannel; - this.finalCallback = finalCallback; + this.finalCallback = (result, exception) -> { + if (blobIdIsServerGenerated && exception instanceof RouterException + && ((RouterException) exception).getErrorCode() == RouterErrorCode.InvalidBlobId) { + exception = new RestServiceException(exception, RestServiceErrorCode.InternalServerError); + } + finalCallback.onCompletion(result, exception); + }; this.uri = restRequest.getUri(); } @@ -194,6 +204,7 @@ private Callback securityPostProcessRequestCallback(BlobInfo blobInfo) { restRequest.readInto(channel, fetchStitchRequestBodyCallback(channel, blobInfo)); } else { PutBlobOptions options = getPutBlobOptionsFromRequest(); + blobIdIsServerGenerated = true; router.putBlob(null, blobInfo.getBlobProperties(), blobInfo.getUserMetadata(), restRequest, options, routerPutBlobCallback(blobInfo), QuotaUtils.buildQuotaChargeCallback(restRequest, quotaManager, true)); } @@ -223,9 +234,10 @@ private Callback fetchStitchRequestBodyCallback(RetainingAsyncWritableChan * @return a {@link Callback} to be used with {@link Router#putBlob}. */ private Callback routerStitchBlobCallback(BlobInfo blobInfo) { - return buildCallback(frontendMetrics.postRouterStitchBlobMetrics, - blobId -> idConverter.convert(restRequest, blobId, blobInfo.getBlobProperties(), idConverterCallback(blobInfo)), uri, LOGGER, - finalCallback); + return buildCallback(frontendMetrics.postRouterStitchBlobMetrics, blobId -> { + blobIdIsServerGenerated = true; + idConverter.convert(restRequest, blobId, blobInfo.getBlobProperties(), idConverterCallback(blobInfo)); + }, uri, LOGGER, finalCallback); } /** 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 5ab7a831dc..f8a60e518a 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 @@ -136,6 +136,7 @@ private class CallbackChain { private final RestResponseChannel restResponseChannel; private final Callback finalCallback; private final String uri; + private boolean blobIdIsServerGenerated; /** * @param restRequest the {@link RestRequest}. @@ -146,7 +147,13 @@ private CallbackChain(RestRequest restRequest, RestResponseChannel restResponseC Callback finalCallback) { this.restRequest = restRequest; this.restResponseChannel = restResponseChannel; - this.finalCallback = finalCallback; + this.finalCallback = (result, exception) -> { + if (blobIdIsServerGenerated && exception instanceof RouterException + && ((RouterException) exception).getErrorCode() == RouterErrorCode.InvalidBlobId) { + exception = new RestServiceException(exception, RestServiceErrorCode.InternalServerError); + } + finalCallback.onCompletion(result, exception); + }; this.uri = restRequest.getUri(); } @@ -241,6 +248,7 @@ private Callback fetchStitchRequestBodyCallback(RetainingAsyncWritableChan private Callback routerStitchBlobCallback(BlobInfo blobInfo, BlobProperties propertiesPassedInRouterUpload) { return buildCallback(frontendMetrics.putRouterStitchBlobMetrics, blobId -> { + blobIdIsServerGenerated = true; // The actual blob size is now present in the instance of BlobProperties passed to the router.stitchBlob(). // Update it in the BlobInfo so that IdConverter can add it to the named blob DB blobInfo.getBlobProperties().setBlobSize(propertiesPassedInRouterUpload.getBlobSize()); diff --git a/ambry-frontend/src/main/java/com/github/ambry/frontend/s3/S3MultipartUploadPartHandler.java b/ambry-frontend/src/main/java/com/github/ambry/frontend/s3/S3MultipartUploadPartHandler.java index 4b2bfd55f7..2439332c0d 100644 --- a/ambry-frontend/src/main/java/com/github/ambry/frontend/s3/S3MultipartUploadPartHandler.java +++ b/ambry-frontend/src/main/java/com/github/ambry/frontend/s3/S3MultipartUploadPartHandler.java @@ -29,12 +29,15 @@ import com.github.ambry.rest.ResponseStatus; import com.github.ambry.rest.RestRequest; import com.github.ambry.rest.RestResponseChannel; +import com.github.ambry.rest.RestServiceErrorCode; import com.github.ambry.rest.RestServiceException; import com.github.ambry.rest.RestUtils; import com.github.ambry.router.PutBlobOptions; import com.github.ambry.router.PutBlobOptionsBuilder; import com.github.ambry.router.ReadableStreamChannel; import com.github.ambry.router.Router; +import com.github.ambry.router.RouterErrorCode; +import com.github.ambry.router.RouterException; import java.util.HashMap; import java.util.Map; import org.slf4j.Logger; @@ -123,6 +126,7 @@ private class CallbackChain { private final RestResponseChannel restResponseChannel; private final Callback finalCallback; private final String uri; + private boolean blobIdIsServerGenerated; /** * @param restRequest the {@link RestRequest}. @@ -133,7 +137,13 @@ private CallbackChain(RestRequest restRequest, RestResponseChannel restResponseC Callback finalCallback) { this.restRequest = restRequest; this.restResponseChannel = restResponseChannel; - this.finalCallback = finalCallback; + this.finalCallback = (result, exception) -> { + if (blobIdIsServerGenerated && exception instanceof RouterException + && ((RouterException) exception).getErrorCode() == RouterErrorCode.InvalidBlobId) { + exception = new RestServiceException(exception, RestServiceErrorCode.InternalServerError); + } + finalCallback.onCompletion(result, exception); + }; this.uri = restRequest.getUri(); } @@ -174,6 +184,7 @@ private Callback securityPostProcessRequestCallback(BlobInfo blobInfo) { finalCallback.onCompletion(null, null); } else { PutBlobOptions options = getPutBlobOptionsFromRequest(); + blobIdIsServerGenerated = true; router.putBlob(null, blobInfo.getBlobProperties(), blobInfo.getUserMetadata(), restRequest, options, routerPutBlobCallback(blobInfo), QuotaUtils.buildQuotaChargeCallback(restRequest, quotaManager, true)); } diff --git a/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java b/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java index a4f1553316..ad579c256a 100644 --- a/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java +++ b/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java @@ -43,6 +43,7 @@ import com.github.ambry.config.QuotaConfig; import com.github.ambry.config.RouterConfig; import com.github.ambry.config.VerifiableProperties; +import com.github.ambry.frontend.s3.S3MultipartETag; import com.github.ambry.messageformat.BlobInfo; import com.github.ambry.messageformat.BlobProperties; import com.github.ambry.named.DeleteResult; @@ -134,6 +135,7 @@ import org.junit.Assert; import org.junit.Test; import org.mockito.Mockito; +import org.mockito.stubbing.Answer; import static com.github.ambry.rest.RestUtils.*; import static com.github.ambry.rest.RestUtils.Headers.*; @@ -2243,6 +2245,132 @@ public void routerExceptionPipelineTest() throws Exception { doRouterExceptionPipelineTest(testRouter, exceptionMsg + " Error: " + RouterErrorCode.UnexpectedInternalError); } + @Test + public void testUploadBlobIdErrorClassification() throws Exception { + refContainer = new ContainerBuilder(refContainer).setNamedBlobMode(Container.NamedBlobMode.OPTIONAL) + .setTtlRequired(false).build(); + accountService.updateAccounts( + Collections.singletonList(new AccountBuilder(refAccount).addOrUpdateContainer(refContainer).build())); + FrontendTestIdConverterFactory converterFactory = new FrontendTestIdConverterFactory(); + converterFactory.returnInputIfTranslationNull = true; + idConverterFactory = converterFactory; + router = spy(router); + doReturn(converterFactory.getIdConverter()).when(router).getIdConverter(); + frontendRestRequestService = getFrontendRestRequestService(); + frontendRestRequestService.start(); + String namedPath = "/named/" + refAccount.getName() + "/" + refContainer.getName() + "/blob"; + String s3Path = namedPath.replace("/named/", "/s3/"); + String chunkId = new BlobId(blobIdVersion, BlobId.BlobIdType.NATIVE, ClusterMap.UNKNOWN_DATACENTER_ID, + refAccount.getId(), refContainer.getId(), referenceBlobId.getPartition(), false, + BlobId.BlobDataType.DATACHUNK).getID(); + Map metadata = new HashMap<>(); + metadata.put(SESSION, "session"); + metadata.put(BLOB_SIZE, "1"); + metadata.put(PostBlobHandler.EXPIRATION_TIME_MS_KEY, Long.toString(Long.MAX_VALUE)); + metadata.put(RESERVED_METADATA_ID, referenceBlobIdStr); + String stitchBody = StitchRequestSerDe.toJson( + Collections.singletonList(idSigningService.getSignedId(chunkId, metadata))).toString(); + String etag = S3MultipartETag.serialize(new S3MultipartETag(Collections.singletonList(new Pair<>(chunkId, 1L)))); + String completeBody = "1" + etag + + ""; + Object[][] requests = { + {RestMethod.POST, "/", false, ""}, + {RestMethod.PUT, namedPath, false, ""}, + {RestMethod.PUT, s3Path, false, ""}, + {RestMethod.PUT, s3Path + "?uploadId=session&partNumber=1", false, ""}, + {RestMethod.POST, "/stitch", true, stitchBody}, + {RestMethod.PUT, namedPath, true, stitchBody}, + {RestMethod.POST, s3Path + "?uploadId=session", true, completeBody} + }; + for (Object[] requestCase : requests) { + RestMethod method = (RestMethod) requestCase[0]; + String uri = (String) requestCase[1]; + boolean stitch = (boolean) requestCase[2]; + for (String stage : new String[]{"router", "conversion", "ttl"}) { + if (stage.equals("conversion") && method != RestMethod.POST + || stage.equals("ttl") && (uri.equals("/") || uri.equals("/stitch") || uri.contains("partNumber"))) { + continue; + } + for (Exception failure : new Exception[]{ + new RouterException("invalid ID", RouterErrorCode.InvalidBlobId), + new RouterException("too large", RouterErrorCode.BlobTooLarge), + new RestServiceException("invalid input", RestServiceErrorCode.BadRequest)}) { + converterFactory.exceptionToReturn = stage.equals("conversion") ? failure : null; + Answer> upload = invocation -> { + Callback callback = invocation.getArgument(5); + callback.onCompletion("invalid-generated-id", stage.equals("router") ? failure : null); + return CompletableFuture.completedFuture("invalid-generated-id"); + }; + doAnswer(upload).when(router).putBlob(any(), any(), any(), any(), any(), any(), any()); + doAnswer(upload).when(router).stitchBlob(any(), any(), any(), any(), any(), any(), any()); + doAnswer(invocation -> { + assertEquals("invalid-generated-id", invocation.getArgument(1)); + Callback callback = invocation.getArgument(4); + callback.onCompletion(null, failure); + return CompletableFuture.completedFuture(null); + }).when(router).updateBlobTtl(any(), anyString(), any(), anyLong(), any(), any()); + boolean parseGeneratedId = stage.equals("ttl") && failure instanceof RouterException + && ((RouterException) failure).getErrorCode() == RouterErrorCode.InvalidBlobId; + if (parseGeneratedId) { + doCallRealMethod().when(router).updateBlobTtl(any(), anyString(), any(), anyLong(), any(), any()); + } + JSONObject headers = new JSONObject(); + setAmbryHeadersForPut(headers, Utils.Infinite_Time, !refContainer.isCacheable(), "test", + "application/octet-stream", "test", refAccount.getName(), refContainer.getName(), null); + if (stitch && method == RestMethod.PUT) { + headers.put(UPLOAD_NAMED_BLOB_MODE, STITCH); + } + byte[] body = ((String) requestCase[3]).getBytes(StandardCharsets.UTF_8); + clearInvocations(router); + try (RestRequest request = createRestRequest(method, uri, headers, + new LinkedList<>(Arrays.asList(ByteBuffer.wrap(body), null)))) { + RestServiceErrorCode expected; + if (failure instanceof RouterException + && ((RouterException) failure).getErrorCode() == RouterErrorCode.BlobTooLarge) { + expected = RestServiceErrorCode.RequestTooLarge; + } else if (failure instanceof RouterException && !(stitch && stage.equals("router"))) { + expected = RestServiceErrorCode.InternalServerError; + } else { + expected = RestServiceErrorCode.BadRequest; + } + MockRestResponseChannel response = spy(new MockRestResponseChannel()); + assertException(RestServiceException.class, () -> doOperation(request, response), + e -> assertEquals(uri + ":" + stage, expected, e.getErrorCode())); + assertEquals(uri + ":" + stage, ResponseStatus.getResponseStatus(expected), response.getStatus()); + verify(response).onResponseComplete(any()); + if (parseGeneratedId) { + assertEquals(RouterErrorCode.InvalidBlobId, + ((RouterException) response.getException().getCause()).getErrorCode()); + } else { + assertSame(failure, failure instanceof RouterException ? response.getException().getCause() + : response.getException()); + } + verify(router, times(stage.equals("ttl") ? 1 : 0)) + .updateBlobTtl(any(), anyString(), any(), anyLong(), any(), any()); + } + } + } + } + } + + @Test + public void testClientBlobIdErrorsRemainBadRequest() throws Exception { + Properties properties = new Properties(); + properties.setProperty(InMemoryRouter.OPERATION_THROW_ROUTER_EXCEPTION, RouterErrorCode.InvalidBlobId.name()); + router.setVerifiableProperties(new VerifiableProperties(properties)); + for (String blobId : new String[]{"invalid-client-id", referenceBlobIdStr}) { + for (String operation : new String[]{"GET", "HEAD", "DELETE", Operations.UPDATE_TTL, Operations.UNDELETE}) { + boolean put = operation.equals(Operations.UPDATE_TTL) || operation.equals(Operations.UNDELETE); + JSONObject headers = new JSONObject().put(BLOB_ID, blobId).put(SERVICE_ID, "test"); + try (RestRequest request = createRestRequest(put ? RestMethod.PUT : RestMethod.valueOf(operation), + put ? "/" + operation : "/" + blobId, headers, null)) { + assertEquals(ResponseStatus.BadRequest, + verifyOperationFailure(request, RestServiceErrorCode.BadRequest).getStatus()); + } + } + } + } + /** * Test that GET operations fail with the expected error code when a bad range header is provided. * @throws Exception diff --git a/ambry-router/src/main/java/com/github/ambry/router/NonBlockingRouter.java b/ambry-router/src/main/java/com/github/ambry/router/NonBlockingRouter.java index ddd6d98eb5..576e9638d2 100644 --- a/ambry-router/src/main/java/com/github/ambry/router/NonBlockingRouter.java +++ b/ambry-router/src/main/java/com/github/ambry/router/NonBlockingRouter.java @@ -36,6 +36,8 @@ import com.github.ambry.repair.RepairRequestsDbFactory; import com.github.ambry.rest.RequestPath; import com.github.ambry.rest.RestRequest; +import com.github.ambry.rest.RestServiceErrorCode; +import com.github.ambry.rest.RestServiceException; import com.github.ambry.rest.RestUtils; import com.github.ambry.store.StoreKey; import com.github.ambry.utils.Time; @@ -1070,6 +1072,7 @@ int getBackgroundOperationsCount() { /** * Create id converter callback after router put the blob. + * Invalid IDs from conversion of a generated blob ID are server errors. * @param restRequest {@link RestRequest} to put the blob. * @param blobProperties {@link BlobProperties} for the blob. * @return @@ -1088,7 +1091,13 @@ private Callback createIdConverterCallbackForPutAndStitch(RestRequest re blobProperties.setBlobSize(restRequest.getBlobBytesReceived()); } // Call idConverter.convert after putBlob succeeds - idConverter.convert(restRequest, blobId, blobProperties, callback); + idConverter.convert(restRequest, blobId, blobProperties, callback == null ? null : (convertedId, error) -> { + if (error instanceof RouterException + && ((RouterException) error).getErrorCode() == RouterErrorCode.InvalidBlobId) { + error = new RestServiceException(error, RestServiceErrorCode.InternalServerError); + } + callback.onCompletion(convertedId, error); + }); } }; } diff --git a/ambry-router/src/test/java/com/github/ambry/router/UploadIdConversionTest.java b/ambry-router/src/test/java/com/github/ambry/router/UploadIdConversionTest.java new file mode 100644 index 0000000000..bdc9179d5e --- /dev/null +++ b/ambry-router/src/test/java/com/github/ambry/router/UploadIdConversionTest.java @@ -0,0 +1,134 @@ +/* + * Copyright 2026 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.router; + +import com.github.ambry.commons.Callback; +import com.github.ambry.commons.LoggingNotificationSystem; +import com.github.ambry.frontend.IdConverter; +import com.github.ambry.frontend.IdConverterFactory; +import com.github.ambry.messageformat.MessageFormatRecord; +import com.github.ambry.rest.MockRestRequest; +import com.github.ambry.rest.RequestPath; +import com.github.ambry.rest.RestMethod; +import com.github.ambry.rest.RestRequest; +import com.github.ambry.rest.RestServiceErrorCode; +import com.github.ambry.rest.RestServiceException; +import com.github.ambry.rest.RestUtils; +import com.github.ambry.utils.Utils; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import org.json.JSONObject; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +import static com.github.ambry.router.RouterTestHelpers.*; +import static com.github.ambry.utils.TestUtils.*; +import static org.junit.Assert.*; +import static org.mockito.Mockito.*; + + +@RunWith(Parameterized.class) +public class UploadIdConversionTest extends NonBlockingRouterTestBase { + public UploadIdConversionTest(boolean encrypted) throws Exception { + super(encrypted, MessageFormatRecord.Metadata_Content_Version_V3, false); + } + + @Parameterized.Parameters + public static List data() { + return Arrays.asList(new Object[][]{{false}, {true}}); + } + + @Test + public void testGeneratedBlobIdConversionErrors() throws Exception { + IdConverterFactory factory = mock(IdConverterFactory.class); + IdConverter converter = mock(IdConverter.class); + when(factory.getIdConverter()).thenReturn(converter); + setRouterWithIdConverterFactory(getNonBlockingRouterProperties(localDcName), + new MockServerLayout(mockClusterMap), new LoggingNotificationSystem(), factory); + setOperationParams(); + String chunkId = router.putBlob(putBlobProperties, putUserMetadata, putChannel, putOptionsForChunkedUpload) + .get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS); + for (boolean stitch : new boolean[]{false, true}) { + for (Exception failure : new Exception[]{ + null, + new RouterException("invalid ID", RouterErrorCode.InvalidBlobId), + new RestServiceException("invalid input", RestServiceErrorCode.BadRequest)}) { + CompletableFuture> pendingConversion = new CompletableFuture<>(); + FutureResult conversion = new FutureResult<>(); + when(converter.convert(any(), anyString(), any(), any())).thenAnswer(invocation -> { + pendingConversion.complete(invocation.getArgument(3)); + return conversion; + }); + setOperationParams(); + try (RestRequest request = createUploadRequest(stitch)) { + FutureResult result = new FutureResult<>(); + AtomicInteger callbackCount = new AtomicInteger(); + Callback callback = (id, error) -> { + callbackCount.incrementAndGet(); + result.done(id, error); + }; + Future stored = stitch ? router.stitchBlob(request, putBlobProperties, putUserMetadata, + Collections.singletonList(new ChunkInfo(chunkId, PUT_CONTENT_SIZE, Utils.Infinite_Time, null)), + null, callback, null) + : router.putBlob(request, putBlobProperties, putUserMetadata, putChannel, + new PutBlobOptionsBuilder().build(), callback, null); + String blobId = stored.get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS); + assertFalse("Upload callback must wait for ID conversion", result.isDone()); + conversion.done(failure == null ? blobId : null, failure); + pendingConversion.get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS) + .onCompletion(failure == null ? blobId : null, failure); + if (failure == null) { + assertEquals(blobId, result.get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS)); + } else { + assertException(ExecutionException.class, () -> result.get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), e -> { + RestServiceException error = (RestServiceException) e.getCause(); + assertEquals(failure instanceof RouterException ? RestServiceErrorCode.InternalServerError + : RestServiceErrorCode.BadRequest, error.getErrorCode()); + assertSame(failure, failure instanceof RouterException ? error.getCause() : error); + }); + } + assertEquals(1, callbackCount.get()); + GetBlobResult blob = router.getBlob(blobId, new GetBlobOptionsBuilder().build()) + .get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS); + blob.getBlobDataChannel().close(); + } + } + } + clearInvocations(converter); + FutureResult result = new FutureResult<>(); + try (RestRequest request = createUploadRequest(true)) { + router.stitchBlob(request, putBlobProperties, putUserMetadata, + Collections.singletonList(new ChunkInfo("invalid-client-id", 1, Utils.Infinite_Time, null)), + null, result::done, null); + assertException(ExecutionException.class, () -> result.get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), + e -> assertEquals(RouterErrorCode.InvalidBlobId, ((RouterException) e.getCause()).getErrorCode())); + verifyNoInteractions(converter); + } + } + + private RestRequest createUploadRequest(boolean stitch) throws Exception { + JSONObject headers = new JSONObject().put(RestUtils.Headers.UPLOAD_NAMED_BLOB_MODE, stitch ? RestUtils.STITCH : ""); + RestRequest request = new MockRestRequest(new JSONObject().put(MockRestRequest.REST_METHOD_KEY, RestMethod.PUT.name()) + .put(MockRestRequest.URI_KEY, "/named/account/container/blob").put(MockRestRequest.HEADERS_KEY, headers), null); + request.setArg(RestUtils.InternalKeys.REQUEST_PATH, RequestPath.parse(request, Collections.emptyList(), null)); + return request; + } +} From 460f420039f467de006e708155642edf9df4eb92 Mon Sep 17 00:00:00 2001 From: Nick Lopez Date: Wed, 30 Sep 2026 16:10:53 -0700 Subject: [PATCH 2/5] Keep upload errors intact until response classification Classify generated IDs at the existing REST mapping boundary so operation callbacks retain their original exceptions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../java/com/github/ambry/rest/RestUtils.java | 5 ++++ .../frontend/FrontendRestRequestService.java | 8 ++++-- .../ambry/frontend/NamedBlobPutHandler.java | 20 ++++++-------- .../ambry/frontend/PostBlobHandler.java | 17 +++--------- .../s3/S3MultipartCompleteUploadHandler.java | 12 +++------ .../s3/S3MultipartUploadPartHandler.java | 15 +++-------- .../FrontendRestRequestServiceTest.java | 27 +++++++++++++++++-- .../frontend/NamedBlobPutHandlerTest.java | 2 ++ .../ambry/frontend/PostBlobHandlerTest.java | 2 ++ .../ambry/frontend/S3MultipartUploadTest.java | 2 ++ .../ambry/frontend/S3PutHandlerTest.java | 1 + .../ambry/router/NonBlockingRouter.java | 12 ++------- .../ambry/router/UploadIdConversionTest.java | 14 +++++----- 13 files changed, 69 insertions(+), 68 deletions(-) diff --git a/ambry-api/src/main/java/com/github/ambry/rest/RestUtils.java b/ambry-api/src/main/java/com/github/ambry/rest/RestUtils.java index d53ffb4fa6..8627092767 100644 --- a/ambry-api/src/main/java/com/github/ambry/rest/RestUtils.java +++ b/ambry-api/src/main/java/com/github/ambry/rest/RestUtils.java @@ -514,6 +514,11 @@ public static final class InternalKeys { */ public static final String REQUEST_PATH = KEY_PREFIX + "request-path"; + /** + * Boolean context for consuming a server-generated upload ID. Cleared after successful conversion or TTL promotion. + */ + public static final String BLOB_ID_IS_SERVER_GENERATED = KEY_PREFIX + "blob-id-is-server-generated"; + /** * The internal header to determine if the delete request is coming from a dataset deletion. */ diff --git a/ambry-frontend/src/main/java/com/github/ambry/frontend/FrontendRestRequestService.java b/ambry-frontend/src/main/java/com/github/ambry/frontend/FrontendRestRequestService.java index 66904e689b..8a918344d3 100644 --- a/ambry-frontend/src/main/java/com/github/ambry/frontend/FrontendRestRequestService.java +++ b/ambry-frontend/src/main/java/com/github/ambry/frontend/FrontendRestRequestService.java @@ -561,6 +561,7 @@ public void handleOptions(RestRequest restRequest, RestResponseChannel restRespo /** * Submits the response and {@code responseBody} (and any {@code exception})for the {@code restRequest} to the * {@code responseHandler}. + * InvalidBlobId is a server error only while consuming a server-generated upload ID. * @param restRequest the {@link RestRequest} for which a response is ready. * @param restResponseChannel the {@link RestResponseChannel} over which the response can be sent. * @param responseBody the body of the response in the form of a {@link ReadableStreamChannel}. @@ -575,8 +576,11 @@ void submitResponse(RestRequest restRequest, RestResponseChannel restResponseCha restResponseChannel.setHeader(TrackingHeaders.FRONTEND_NAME, hostname); } if (exception instanceof RouterException) { - exception = new RestServiceException(exception, - RestServiceErrorCode.getRestServiceErrorCode(((RouterException) exception).getErrorCode())); + RouterErrorCode routerErrorCode = ((RouterException) exception).getErrorCode(); + RestServiceErrorCode errorCode = routerErrorCode == RouterErrorCode.InvalidBlobId + && Boolean.TRUE.equals(restRequest.getArgs().get(BLOB_ID_IS_SERVER_GENERATED)) + ? RestServiceErrorCode.InternalServerError : RestServiceErrorCode.getRestServiceErrorCode(routerErrorCode); + exception = new RestServiceException(exception, errorCode); } responseHandler.handleResponse(restRequest, restResponseChannel, responseBody, exception); } catch (Exception e) { diff --git a/ambry-frontend/src/main/java/com/github/ambry/frontend/NamedBlobPutHandler.java b/ambry-frontend/src/main/java/com/github/ambry/frontend/NamedBlobPutHandler.java index 569e81a7ac..8e8bcbfda2 100644 --- a/ambry-frontend/src/main/java/com/github/ambry/frontend/NamedBlobPutHandler.java +++ b/ambry-frontend/src/main/java/com/github/ambry/frontend/NamedBlobPutHandler.java @@ -81,7 +81,6 @@ * blob content, accepts a UTF-8 JSON object that includes the signed IDs for the chunks to stitch, and header x-ambry-put-mode should set as "STITCH". *

Request body format

* The body of the request should be a JSON object that conforms to the format described in {@link StitchRequestSerDe}. - * Invalid generated blob IDs are server errors; invalid client-supplied stitch IDs are client errors. */ public class NamedBlobPutHandler { private static final Logger LOGGER = LoggerFactory.getLogger(NamedBlobPutHandler.class); @@ -153,7 +152,6 @@ private class CallbackChain { private final Callback finalCallback; private final Callback deleteDatasetCallback; private final String uri; - private boolean blobIdIsServerGenerated; /** * @param restRequest the {@link RestRequest}. @@ -164,14 +162,8 @@ private CallbackChain(RestRequest restRequest, RestResponseChannel restResponseC Callback finalCallback) { this.restRequest = restRequest; this.restResponseChannel = restResponseChannel; - this.finalCallback = (result, exception) -> { - if (blobIdIsServerGenerated && exception instanceof RouterException - && ((RouterException) exception).getErrorCode() == RouterErrorCode.InvalidBlobId) { - exception = new RestServiceException(exception, RestServiceErrorCode.InternalServerError); - } - finalCallback.onCompletion(result, exception); - }; - this.deleteDatasetCallback = deleteDatasetVersionIfUploadFailedCallBack(this.finalCallback); + this.finalCallback = finalCallback; + this.deleteDatasetCallback = deleteDatasetVersionIfUploadFailedCallBack(finalCallback); this.uri = restRequest.getUri(); } @@ -234,7 +226,6 @@ private Callback securityPostProcessRequestCallback(BlobInfo blobInfo) { addDatasetVersion(blobInfo.getBlobProperties(), restRequest); } PutBlobOptions options = getPutBlobOptionsFromRequest(); - blobIdIsServerGenerated = true; router.putBlob(restRequest, getPropertiesForRouterUpload(blobInfo), blobInfo.getUserMetadata(), restRequest, options, routerPutBlobCallback(blobInfo), QuotaUtils.buildQuotaChargeCallback(restRequest, quotaManager, true)); @@ -253,6 +244,7 @@ private Callback routerPutBlobCallback(BlobInfo blobInfo) { restRequest.getMetricsTracker().setBytesTransferred(restRequest.getBytesReceived()); restResponseChannel.setHeader(RestUtils.Headers.BLOB_SIZE, restRequest.getBlobBytesReceived()); restResponseChannel.setHeader(RestUtils.Headers.LOCATION, blobId); + restRequest.setArg(BLOB_ID_IS_SERVER_GENERATED, true); String blobIdClean = stripPrefixAndExtension(blobId); if (blobInfo.getBlobProperties().getTimeToLiveInSeconds() == Utils.Infinite_Time) { // Do ttl update with retryExecutor. Use the blob ID returned from the router instead of the converted ID @@ -263,6 +255,7 @@ private Callback routerPutBlobCallback(BlobInfo blobInfo) { QuotaUtils.buildQuotaChargeCallback(restRequest, quotaManager, false)), this::isRetriable, routerTtlUpdateCallbackForPut(blobInfo)); } else { + restRequest.removeArg(BLOB_ID_IS_SERVER_GENERATED); if (RestUtils.isDatasetVersionQueryEnabled(restRequest.getArgs())) { //Make sure to process response after delete finished updateVersionStateAndDeleteDatasetVersionOutOfRetentionCount( @@ -300,7 +293,7 @@ private Callback fetchStitchRequestBodyCallback(RetainingAsyncWritableChan */ private Callback routerStitchBlobCallback(BlobInfo blobInfo, BlobProperties propertiesForRouterUpload) { return buildCallback(frontendMetrics.putRouterStitchBlobMetrics, convertedBlobId -> { - blobIdIsServerGenerated = true; + restRequest.setArg(BLOB_ID_IS_SERVER_GENERATED, true); // The actual blob size is now present in the instance of BlobProperties passed to the router.stitchBlob(). // Update it in the BlobInfo so that IdConverter can add it to the named blob DB blobInfo.getBlobProperties().setBlobSize(propertiesForRouterUpload.getBlobSize()); @@ -315,6 +308,7 @@ private Callback routerStitchBlobCallback(BlobInfo blobInfo, BlobPropert QuotaUtils.buildQuotaChargeCallback(restRequest, quotaManager, false)), this::isRetriable, routerTtlUpdateCallbackForStitch(blobInfo)); } else { + restRequest.removeArg(BLOB_ID_IS_SERVER_GENERATED); if (RestUtils.isDatasetVersionQueryEnabled(restRequest.getArgs())) { //Make sure to process response after delete finished updateVersionStateAndDeleteDatasetVersionOutOfRetentionCount( @@ -345,6 +339,7 @@ private boolean isRetriable(Throwable throwable) { */ private Callback routerTtlUpdateCallbackForPut(BlobInfo blobInfo) { return buildCallback(frontendMetrics.updateBlobTtlRouterMetrics, convertedBlobId -> { + restRequest.removeArg(BLOB_ID_IS_SERVER_GENERATED); if (RestUtils.isDatasetVersionQueryEnabled(restRequest.getArgs())) { //Make sure to process response after delete finished updateVersionStateAndDeleteDatasetVersionOutOfRetentionCount( @@ -364,6 +359,7 @@ private Callback routerTtlUpdateCallbackForPut(BlobInfo blobInfo) { */ private Callback routerTtlUpdateCallbackForStitch(BlobInfo blobInfo) { return buildCallback(frontendMetrics.updateBlobTtlRouterMetrics, convertedBlobId -> { + restRequest.removeArg(BLOB_ID_IS_SERVER_GENERATED); if (RestUtils.isDatasetVersionQueryEnabled(restRequest.getArgs())) { //Make sure to process response after delete finished updateVersionStateAndDeleteDatasetVersionOutOfRetentionCount( diff --git a/ambry-frontend/src/main/java/com/github/ambry/frontend/PostBlobHandler.java b/ambry-frontend/src/main/java/com/github/ambry/frontend/PostBlobHandler.java index a07e6e57d9..a6398b8677 100644 --- a/ambry-frontend/src/main/java/com/github/ambry/frontend/PostBlobHandler.java +++ b/ambry-frontend/src/main/java/com/github/ambry/frontend/PostBlobHandler.java @@ -33,8 +33,6 @@ import com.github.ambry.router.PutBlobOptions; import com.github.ambry.router.PutBlobOptionsBuilder; import com.github.ambry.router.Router; -import com.github.ambry.router.RouterErrorCode; -import com.github.ambry.router.RouterException; import com.github.ambry.utils.Pair; import com.github.ambry.utils.Time; import com.github.ambry.utils.Utils; @@ -66,7 +64,6 @@ * blob content, accepts a UTF-8 JSON object that includes the signed IDs for the chunks to stitch. *

Request body format

* The body of the request should be a JSON object that conforms to the format described in {@link StitchRequestSerDe}. - * Invalid generated blob IDs are server errors; invalid client-supplied stitch IDs are client errors. */ class PostBlobHandler { /** @@ -131,7 +128,6 @@ private class CallbackChain { private final String uri; private final RestResponseChannel restResponseChannel; private final Callback finalCallback; - private boolean blobIdIsServerGenerated; /** * @param restRequest the {@link RestRequest}. @@ -142,13 +138,7 @@ private CallbackChain(RestRequest restRequest, RestResponseChannel restResponseC Callback finalCallback) { this.restRequest = restRequest; this.restResponseChannel = restResponseChannel; - this.finalCallback = (result, exception) -> { - if (blobIdIsServerGenerated && exception instanceof RouterException - && ((RouterException) exception).getErrorCode() == RouterErrorCode.InvalidBlobId) { - exception = new RestServiceException(exception, RestServiceErrorCode.InternalServerError); - } - finalCallback.onCompletion(result, exception); - }; + this.finalCallback = finalCallback; this.uri = restRequest.getUri(); } @@ -204,7 +194,6 @@ private Callback securityPostProcessRequestCallback(BlobInfo blobInfo) { restRequest.readInto(channel, fetchStitchRequestBodyCallback(channel, blobInfo)); } else { PutBlobOptions options = getPutBlobOptionsFromRequest(); - blobIdIsServerGenerated = true; router.putBlob(null, blobInfo.getBlobProperties(), blobInfo.getUserMetadata(), restRequest, options, routerPutBlobCallback(blobInfo), QuotaUtils.buildQuotaChargeCallback(restRequest, quotaManager, true)); } @@ -235,7 +224,7 @@ private Callback fetchStitchRequestBodyCallback(RetainingAsyncWritableChan */ private Callback routerStitchBlobCallback(BlobInfo blobInfo) { return buildCallback(frontendMetrics.postRouterStitchBlobMetrics, blobId -> { - blobIdIsServerGenerated = true; + restRequest.setArg(RestUtils.InternalKeys.BLOB_ID_IS_SERVER_GENERATED, true); idConverter.convert(restRequest, blobId, blobInfo.getBlobProperties(), idConverterCallback(blobInfo)); }, uri, LOGGER, finalCallback); } @@ -250,6 +239,7 @@ private Callback routerPutBlobCallback(BlobInfo blobInfo) { return buildCallback(frontendMetrics.postRouterPutBlobMetrics, blobId -> { setSignedIdMetadataAndBlobSize(blobInfo.getBlobProperties()); restRequest.getMetricsTracker().setBytesTransferred(restRequest.getBytesReceived()); + restRequest.setArg(RestUtils.InternalKeys.BLOB_ID_IS_SERVER_GENERATED, true); idConverter.convert(restRequest, blobId, idConverterCallback(blobInfo)); }, uri, LOGGER, finalCallback); } @@ -262,6 +252,7 @@ private Callback routerPutBlobCallback(BlobInfo blobInfo) { */ private Callback idConverterCallback(BlobInfo blobInfo) { return buildCallback(frontendMetrics.postIdConversionMetrics, convertedBlobId -> { + restRequest.removeArg(RestUtils.InternalKeys.BLOB_ID_IS_SERVER_GENERATED); restResponseChannel.setHeader(RestUtils.Headers.LOCATION, convertedBlobId); securityService.processResponse(restRequest, restResponseChannel, blobInfo, securityProcessResponseCallback()); }, uri, LOGGER, finalCallback); 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 f8a60e518a..54cd619df5 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 @@ -136,7 +136,6 @@ private class CallbackChain { private final RestResponseChannel restResponseChannel; private final Callback finalCallback; private final String uri; - private boolean blobIdIsServerGenerated; /** * @param restRequest the {@link RestRequest}. @@ -147,13 +146,7 @@ private CallbackChain(RestRequest restRequest, RestResponseChannel restResponseC Callback finalCallback) { this.restRequest = restRequest; this.restResponseChannel = restResponseChannel; - this.finalCallback = (result, exception) -> { - if (blobIdIsServerGenerated && exception instanceof RouterException - && ((RouterException) exception).getErrorCode() == RouterErrorCode.InvalidBlobId) { - exception = new RestServiceException(exception, RestServiceErrorCode.InternalServerError); - } - finalCallback.onCompletion(result, exception); - }; + this.finalCallback = finalCallback; this.uri = restRequest.getUri(); } @@ -248,7 +241,7 @@ private Callback fetchStitchRequestBodyCallback(RetainingAsyncWritableChan private Callback routerStitchBlobCallback(BlobInfo blobInfo, BlobProperties propertiesPassedInRouterUpload) { return buildCallback(frontendMetrics.putRouterStitchBlobMetrics, blobId -> { - blobIdIsServerGenerated = true; + restRequest.setArg(BLOB_ID_IS_SERVER_GENERATED, true); // The actual blob size is now present in the instance of BlobProperties passed to the router.stitchBlob(). // Update it in the BlobInfo so that IdConverter can add it to the named blob DB blobInfo.getBlobProperties().setBlobSize(propertiesPassedInRouterUpload.getBlobSize()); @@ -285,6 +278,7 @@ private Callback idConverterCallback(BlobInfo blobInfo, String blobId) { */ private Callback routerTtlUpdateCallback(BlobInfo blobInfo, String blobId) { return buildCallback(frontendMetrics.updateBlobTtlRouterMetrics, convertedBlobId -> { + restRequest.removeArg(BLOB_ID_IS_SERVER_GENERATED); // Set the named blob state to be 'READY' after the Ttl update succeed if (!restRequest.getArgs().containsKey(NAMED_BLOB_VERSION)) { throw new RestServiceException( diff --git a/ambry-frontend/src/main/java/com/github/ambry/frontend/s3/S3MultipartUploadPartHandler.java b/ambry-frontend/src/main/java/com/github/ambry/frontend/s3/S3MultipartUploadPartHandler.java index 2439332c0d..c2cf9a9ba7 100644 --- a/ambry-frontend/src/main/java/com/github/ambry/frontend/s3/S3MultipartUploadPartHandler.java +++ b/ambry-frontend/src/main/java/com/github/ambry/frontend/s3/S3MultipartUploadPartHandler.java @@ -29,15 +29,12 @@ import com.github.ambry.rest.ResponseStatus; import com.github.ambry.rest.RestRequest; import com.github.ambry.rest.RestResponseChannel; -import com.github.ambry.rest.RestServiceErrorCode; import com.github.ambry.rest.RestServiceException; import com.github.ambry.rest.RestUtils; import com.github.ambry.router.PutBlobOptions; import com.github.ambry.router.PutBlobOptionsBuilder; import com.github.ambry.router.ReadableStreamChannel; import com.github.ambry.router.Router; -import com.github.ambry.router.RouterErrorCode; -import com.github.ambry.router.RouterException; import java.util.HashMap; import java.util.Map; import org.slf4j.Logger; @@ -126,7 +123,6 @@ private class CallbackChain { private final RestResponseChannel restResponseChannel; private final Callback finalCallback; private final String uri; - private boolean blobIdIsServerGenerated; /** * @param restRequest the {@link RestRequest}. @@ -137,13 +133,7 @@ private CallbackChain(RestRequest restRequest, RestResponseChannel restResponseC Callback finalCallback) { this.restRequest = restRequest; this.restResponseChannel = restResponseChannel; - this.finalCallback = (result, exception) -> { - if (blobIdIsServerGenerated && exception instanceof RouterException - && ((RouterException) exception).getErrorCode() == RouterErrorCode.InvalidBlobId) { - exception = new RestServiceException(exception, RestServiceErrorCode.InternalServerError); - } - finalCallback.onCompletion(result, exception); - }; + this.finalCallback = finalCallback; this.uri = restRequest.getUri(); } @@ -184,7 +174,6 @@ private Callback securityPostProcessRequestCallback(BlobInfo blobInfo) { finalCallback.onCompletion(null, null); } else { PutBlobOptions options = getPutBlobOptionsFromRequest(); - blobIdIsServerGenerated = true; router.putBlob(null, blobInfo.getBlobProperties(), blobInfo.getUserMetadata(), restRequest, options, routerPutBlobCallback(blobInfo), QuotaUtils.buildQuotaChargeCallback(restRequest, quotaManager, true)); } @@ -199,6 +188,7 @@ private Callback securityPostProcessRequestCallback(BlobInfo blobInfo) { */ private Callback routerPutBlobCallback(BlobInfo blobInfo) { return buildCallback(frontendMetrics.putRouterPutBlobMetrics, blobId -> { + restRequest.setArg(BLOB_ID_IS_SERVER_GENERATED, true); restResponseChannel.setHeader(RestUtils.Headers.BLOB_SIZE, restRequest.getBlobBytesReceived()); // TODO [S3] Make changes to sign ETags. Currently they are sent as shown below. // ETag: {"chunks":[{"blob":"AAYQAQBlAAgAAQAAAAAAAAAAw6UGCoNgS8KGgV-SGXAMdQ","size":4194304}, @@ -207,6 +197,7 @@ private Callback routerPutBlobCallback(BlobInfo blobInfo) { S3MultipartETag etag = new S3MultipartETag(putBlobMetaInfo.getOrderedChunkIdSizeList()); String eTagStr = S3MultipartETag.serialize(etag); restResponseChannel.setHeader(RestUtils.Headers.LOCATION, eTagStr); + restRequest.removeArg(BLOB_ID_IS_SERVER_GENERATED); securityService.processResponse(restRequest, restResponseChannel, blobInfo, securityProcessResponseCallback()); }, uri, logger, finalCallback); } diff --git a/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java b/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java index ad579c256a..13ba7dd6aa 100644 --- a/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java +++ b/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java @@ -2245,6 +2245,26 @@ public void routerExceptionPipelineTest() throws Exception { doRouterExceptionPipelineTest(testRouter, exceptionMsg + " Error: " + RouterErrorCode.UnexpectedInternalError); } + @Test + public void testGeneratedIdResponseMapping() throws Exception { + for (Object context : new Object[]{null, false, true, "true"}) { + for (RouterErrorCode code : RouterErrorCode.values()) { + try (RestRequest request = createRestRequest(RestMethod.POST, "/", null, null)) { + if (context != null) { + request.setArg(InternalKeys.BLOB_ID_IS_SERVER_GENERATED, context); + } + RouterException error = new RouterException("router failure", code); + MockRestResponseChannel response = new MockRestResponseChannel(); + frontendRestRequestService.submitResponse(request, response, null, error); + RestServiceErrorCode expected = Boolean.TRUE.equals(context) && code == RouterErrorCode.InvalidBlobId + ? RestServiceErrorCode.InternalServerError : RestServiceErrorCode.getRestServiceErrorCode(code); + assertEquals(ResponseStatus.getResponseStatus(expected), response.getStatus()); + assertSame(error, response.getException().getCause()); + } + } + } + } + @Test public void testUploadBlobIdErrorClassification() throws Exception { refContainer = new ContainerBuilder(refContainer).setNamedBlobMode(Container.NamedBlobMode.OPTIONAL) @@ -2256,7 +2276,7 @@ public void testUploadBlobIdErrorClassification() throws Exception { idConverterFactory = converterFactory; router = spy(router); doReturn(converterFactory.getIdConverter()).when(router).getIdConverter(); - frontendRestRequestService = getFrontendRestRequestService(); + frontendRestRequestService = spy(getFrontendRestRequestService()); frontendRestRequestService.start(); String namedPath = "/named/" + refAccount.getName() + "/" + refContainer.getName() + "/blob"; String s3Path = namedPath.replace("/named/", "/s3/"); @@ -2328,7 +2348,7 @@ public void testUploadBlobIdErrorClassification() throws Exception { if (failure instanceof RouterException && ((RouterException) failure).getErrorCode() == RouterErrorCode.BlobTooLarge) { expected = RestServiceErrorCode.RequestTooLarge; - } else if (failure instanceof RouterException && !(stitch && stage.equals("router"))) { + } else if (failure instanceof RouterException && !stage.equals("router")) { expected = RestServiceErrorCode.InternalServerError; } else { expected = RestServiceErrorCode.BadRequest; @@ -2345,6 +2365,8 @@ public void testUploadBlobIdErrorClassification() throws Exception { assertSame(failure, failure instanceof RouterException ? response.getException().getCause() : response.getException()); } + verify(frontendRestRequestService).submitResponse(request, response, null, + parseGeneratedId ? (RouterException) response.getException().getCause() : failure); verify(router, times(stage.equals("ttl") ? 1 : 0)) .updateBlobTtl(any(), anyString(), any(), anyLong(), any(), any()); } @@ -2362,6 +2384,7 @@ public void testClientBlobIdErrorsRemainBadRequest() throws Exception { for (String operation : new String[]{"GET", "HEAD", "DELETE", Operations.UPDATE_TTL, Operations.UNDELETE}) { boolean put = operation.equals(Operations.UPDATE_TTL) || operation.equals(Operations.UNDELETE); JSONObject headers = new JSONObject().put(BLOB_ID, blobId).put(SERVICE_ID, "test"); + headers.put(InternalKeys.BLOB_ID_IS_SERVER_GENERATED, "true"); try (RestRequest request = createRestRequest(put ? RestMethod.PUT : RestMethod.valueOf(operation), put ? "/" + operation : "/" + blobId, headers, null)) { assertEquals(ResponseStatus.BadRequest, diff --git a/ambry-frontend/src/test/java/com/github/ambry/frontend/NamedBlobPutHandlerTest.java b/ambry-frontend/src/test/java/com/github/ambry/frontend/NamedBlobPutHandlerTest.java index 3416042c08..958a9d3456 100644 --- a/ambry-frontend/src/test/java/com/github/ambry/frontend/NamedBlobPutHandlerTest.java +++ b/ambry-frontend/src/test/java/com/github/ambry/frontend/NamedBlobPutHandlerTest.java @@ -563,6 +563,7 @@ private void stitchBlobAndVerify(byte[] requestBody, List expectedSti namedBlobPutHandler.handle(request, restResponseChannel, future::done); if (errorChecker == null) { future.get(TIMEOUT_SECS, TimeUnit.SECONDS); + assertNull(request.getArgs().get(RestUtils.InternalKeys.BLOB_ID_IS_SERVER_GENERATED)); // in id converter, the callback is invoked before the lastConvertedId is set, we need to wait // for a while before we can check the lastConvertedId. Thread.sleep(100); @@ -731,6 +732,7 @@ private void putBlobAndVerify(ThrowingConsumer errorChecker, namedBlobPutHandler.handle(request, restResponseChannel, future::done); if (errorChecker == null) { future.get(TIMEOUT_SECS, TimeUnit.SECONDS); + assertNull(request.getArgs().get(RestUtils.InternalKeys.BLOB_ID_IS_SERVER_GENERATED)); // in id converter, the callback is invoked before the lastConvertedId is set, we need to wait // for a while before we can check the lastConvertedId. Thread.sleep(100); diff --git a/ambry-frontend/src/test/java/com/github/ambry/frontend/PostBlobHandlerTest.java b/ambry-frontend/src/test/java/com/github/ambry/frontend/PostBlobHandlerTest.java index 50be0d0eeb..03041889ad 100644 --- a/ambry-frontend/src/test/java/com/github/ambry/frontend/PostBlobHandlerTest.java +++ b/ambry-frontend/src/test/java/com/github/ambry/frontend/PostBlobHandlerTest.java @@ -536,6 +536,7 @@ private void doChunkUploadTest(int contentLength, boolean chunkUpload, String up postBlobHandler.handle(request, restResponseChannel, future::done); if (errorChecker == null) { future.get(TIMEOUT_SECS, TimeUnit.SECONDS); + assertNull(request.getArgs().get(BLOB_ID_IS_SERVER_GENERATED)); assertEquals("Unexpected converted ID", CONVERTED_ID, restResponseChannel.getHeader(RestUtils.Headers.LOCATION)); Object metadata = request.getArgs().get(RestUtils.InternalKeys.SIGNED_ID_METADATA_KEY); if (chunkUpload) { @@ -694,6 +695,7 @@ private void stitchBlobAndVerify(byte[] requestBody, List expectedSti postBlobHandler.handle(request, restResponseChannel, future::done); if (errorChecker == null) { future.get(TIMEOUT_SECS, TimeUnit.SECONDS); + assertNull(request.getArgs().get(BLOB_ID_IS_SERVER_GENERATED)); assertEquals("Unexpected converted ID", CONVERTED_ID, restResponseChannel.getHeader(RestUtils.Headers.LOCATION)); InMemoryRouter.InMemoryBlob blob = router.getActiveBlobs().get(idConverterFactory.lastInput); assertEquals("List of chunks stitched does not match expected", expectedStitchedChunks, blob.getStitchedChunks()); 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 8f5a532656..6d5c683b29 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 @@ -143,6 +143,7 @@ public void multiPartUploadTest() throws Exception { FutureResult putResult = new FutureResult<>(); s3PutHandler.handle(request, restResponseChannel, putResult::done); putResult.get(); + assertNull(request.getArgs().get(InternalKeys.BLOB_ID_IS_SERVER_GENERATED)); String etag1 = (String) restResponseChannel.getHeader(Headers.ETAG); String location1 = (String) restResponseChannel.getHeader(Headers.LOCATION); assertEquals("Mismatch on response status", ResponseStatus.Ok, restResponseChannel.getStatus()); @@ -196,6 +197,7 @@ public void multiPartUploadTest() throws Exception { // Verify Upsert header is set by default for S3 multipart uploads. assertTrue("Upsert header must be present", request.getArgs().containsKey(NAMED_UPSERT)); readableStreamChannel = postResult.get(); + assertNull(request.getArgs().get(InternalKeys.BLOB_ID_IS_SERVER_GENERATED)); byteBuffer = ((ByteBufferReadableStreamChannel) readableStreamChannel).getContent(); byte[] byteArray = byteBuffer.array(); CompleteMultipartUploadResult completeMultipartUploadResult = diff --git a/ambry-frontend/src/test/java/com/github/ambry/frontend/S3PutHandlerTest.java b/ambry-frontend/src/test/java/com/github/ambry/frontend/S3PutHandlerTest.java index 48539cb5b2..84a87fcf8a 100644 --- a/ambry-frontend/src/test/java/com/github/ambry/frontend/S3PutHandlerTest.java +++ b/ambry-frontend/src/test/java/com/github/ambry/frontend/S3PutHandlerTest.java @@ -104,6 +104,7 @@ public void putBlobsTestHelper(boolean provideContentType) throws Exception { restResponseChannel.setStatus(ResponseStatus.Created); s3PutHandler.handle(request, restResponseChannel, putResult::done); putResult.get(); + assertNull(request.getArgs().get(BLOB_ID_IS_SERVER_GENERATED)); // Verify Upsert header is set by default for S3 uploads. assertTrue("Upsert header must be present", request.getArgs().containsKey(NAMED_UPSERT)); diff --git a/ambry-router/src/main/java/com/github/ambry/router/NonBlockingRouter.java b/ambry-router/src/main/java/com/github/ambry/router/NonBlockingRouter.java index 576e9638d2..b3ae7ac395 100644 --- a/ambry-router/src/main/java/com/github/ambry/router/NonBlockingRouter.java +++ b/ambry-router/src/main/java/com/github/ambry/router/NonBlockingRouter.java @@ -36,8 +36,6 @@ import com.github.ambry.repair.RepairRequestsDbFactory; import com.github.ambry.rest.RequestPath; import com.github.ambry.rest.RestRequest; -import com.github.ambry.rest.RestServiceErrorCode; -import com.github.ambry.rest.RestServiceException; import com.github.ambry.rest.RestUtils; import com.github.ambry.store.StoreKey; import com.github.ambry.utils.Time; @@ -1072,7 +1070,6 @@ int getBackgroundOperationsCount() { /** * Create id converter callback after router put the blob. - * Invalid IDs from conversion of a generated blob ID are server errors. * @param restRequest {@link RestRequest} to put the blob. * @param blobProperties {@link BlobProperties} for the blob. * @return @@ -1091,13 +1088,8 @@ private Callback createIdConverterCallbackForPutAndStitch(RestRequest re blobProperties.setBlobSize(restRequest.getBlobBytesReceived()); } // Call idConverter.convert after putBlob succeeds - idConverter.convert(restRequest, blobId, blobProperties, callback == null ? null : (convertedId, error) -> { - if (error instanceof RouterException - && ((RouterException) error).getErrorCode() == RouterErrorCode.InvalidBlobId) { - error = new RestServiceException(error, RestServiceErrorCode.InternalServerError); - } - callback.onCompletion(convertedId, error); - }); + restRequest.setArg(RestUtils.InternalKeys.BLOB_ID_IS_SERVER_GENERATED, true); + idConverter.convert(restRequest, blobId, blobProperties, callback); } }; } diff --git a/ambry-router/src/test/java/com/github/ambry/router/UploadIdConversionTest.java b/ambry-router/src/test/java/com/github/ambry/router/UploadIdConversionTest.java index bdc9179d5e..4df2017fd7 100644 --- a/ambry-router/src/test/java/com/github/ambry/router/UploadIdConversionTest.java +++ b/ambry-router/src/test/java/com/github/ambry/router/UploadIdConversionTest.java @@ -93,17 +93,14 @@ public void testGeneratedBlobIdConversionErrors() throws Exception { String blobId = stored.get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS); assertFalse("Upload callback must wait for ID conversion", result.isDone()); conversion.done(failure == null ? blobId : null, failure); - pendingConversion.get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS) - .onCompletion(failure == null ? blobId : null, failure); + Callback conversionCallback = pendingConversion.get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS); + assertEquals(Boolean.TRUE, request.getArgs().get(RestUtils.InternalKeys.BLOB_ID_IS_SERVER_GENERATED)); + conversionCallback.onCompletion(failure == null ? blobId : null, failure); if (failure == null) { assertEquals(blobId, result.get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS)); } else { - assertException(ExecutionException.class, () -> result.get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), e -> { - RestServiceException error = (RestServiceException) e.getCause(); - assertEquals(failure instanceof RouterException ? RestServiceErrorCode.InternalServerError - : RestServiceErrorCode.BadRequest, error.getErrorCode()); - assertSame(failure, failure instanceof RouterException ? error.getCause() : error); - }); + assertException(ExecutionException.class, () -> result.get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), + e -> assertSame(failure, e.getCause())); } assertEquals(1, callbackCount.get()); GetBlobResult blob = router.getBlob(blobId, new GetBlobOptionsBuilder().build()) @@ -121,6 +118,7 @@ public void testGeneratedBlobIdConversionErrors() throws Exception { assertException(ExecutionException.class, () -> result.get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), e -> assertEquals(RouterErrorCode.InvalidBlobId, ((RouterException) e.getCause()).getErrorCode())); verifyNoInteractions(converter); + assertNull(request.getArgs().get(RestUtils.InternalKeys.BLOB_ID_IS_SERVER_GENERATED)); } } From f6b261560fe34d8f24651e59807855ec218f7636 Mon Sep 17 00:00:00 2001 From: Nick Lopez Date: Mon, 5 Oct 2026 11:40:24 -0700 Subject: [PATCH 3/5] Count generated ID failures before enabling server errors Count invalid generated upload IDs independently of a trusted, default-off HTTP classification gate. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../java/com/github/ambry/rest/RestUtils.java | 5 ++ .../ambry/frontend/FrontendMetrics.java | 3 + .../frontend/FrontendRestRequestService.java | 13 +++-- .../FrontendRestRequestServiceTest.java | 58 +++++++++++++++---- 4 files changed, 64 insertions(+), 15 deletions(-) diff --git a/ambry-api/src/main/java/com/github/ambry/rest/RestUtils.java b/ambry-api/src/main/java/com/github/ambry/rest/RestUtils.java index 8627092767..aabc1ecd18 100644 --- a/ambry-api/src/main/java/com/github/ambry/rest/RestUtils.java +++ b/ambry-api/src/main/java/com/github/ambry/rest/RestUtils.java @@ -519,6 +519,11 @@ public static final class InternalKeys { */ public static final String BLOB_ID_IS_SERVER_GENERATED = KEY_PREFIX + "blob-id-is-server-generated"; + /** + * Trusted Boolean gate for HTTP 500 on invalid generated upload IDs. Absent or false preserves HTTP 400. + */ + public static final String GENERATED_BLOB_ID_SERVER_ERROR_ENABLED = KEY_PREFIX + "generatedBlobIdServerErrorEnabled"; + /** * The internal header to determine if the delete request is coming from a dataset deletion. */ diff --git a/ambry-frontend/src/main/java/com/github/ambry/frontend/FrontendMetrics.java b/ambry-frontend/src/main/java/com/github/ambry/frontend/FrontendMetrics.java index 206654ebd7..1915893c9a 100644 --- a/ambry-frontend/src/main/java/com/github/ambry/frontend/FrontendMetrics.java +++ b/ambry-frontend/src/main/java/com/github/ambry/frontend/FrontendMetrics.java @@ -255,6 +255,7 @@ public class FrontendMetrics { // Errors // FrontendRestRequestService public final Counter responseSubmissionError; + public final Counter generatedBlobIdError; public final Counter resourceReleaseError; public final Counter ttlTooLargeError; public final Counter ttlNotCompliantError; @@ -707,6 +708,8 @@ public FrontendMetrics(MetricRegistry metricRegistry, FrontendConfig frontendCon // FrontendRestRequestService responseSubmissionError = metricRegistry.counter(MetricRegistry.name(FrontendRestRequestService.class, "ResponseSubmissionError")); + generatedBlobIdError = + metricRegistry.counter(MetricRegistry.name(FrontendRestRequestService.class, "GeneratedBlobIdError")); resourceReleaseError = metricRegistry.counter(MetricRegistry.name(FrontendRestRequestService.class, "ResourceReleaseError")); // DeleteCallback diff --git a/ambry-frontend/src/main/java/com/github/ambry/frontend/FrontendRestRequestService.java b/ambry-frontend/src/main/java/com/github/ambry/frontend/FrontendRestRequestService.java index 8a918344d3..a3e81d008d 100644 --- a/ambry-frontend/src/main/java/com/github/ambry/frontend/FrontendRestRequestService.java +++ b/ambry-frontend/src/main/java/com/github/ambry/frontend/FrontendRestRequestService.java @@ -561,7 +561,7 @@ public void handleOptions(RestRequest restRequest, RestResponseChannel restRespo /** * Submits the response and {@code responseBody} (and any {@code exception})for the {@code restRequest} to the * {@code responseHandler}. - * InvalidBlobId is a server error only while consuming a server-generated upload ID. + * Invalid generated upload IDs are always counted; server-error classification requires an enabled request gate. * @param restRequest the {@link RestRequest} for which a response is ready. * @param restResponseChannel the {@link RestResponseChannel} over which the response can be sent. * @param responseBody the body of the response in the form of a {@link ReadableStreamChannel}. @@ -577,9 +577,14 @@ void submitResponse(RestRequest restRequest, RestResponseChannel restResponseCha } if (exception instanceof RouterException) { RouterErrorCode routerErrorCode = ((RouterException) exception).getErrorCode(); - RestServiceErrorCode errorCode = routerErrorCode == RouterErrorCode.InvalidBlobId - && Boolean.TRUE.equals(restRequest.getArgs().get(BLOB_ID_IS_SERVER_GENERATED)) - ? RestServiceErrorCode.InternalServerError : RestServiceErrorCode.getRestServiceErrorCode(routerErrorCode); + RestServiceErrorCode errorCode = RestServiceErrorCode.getRestServiceErrorCode(routerErrorCode); + if (routerErrorCode == RouterErrorCode.InvalidBlobId + && Boolean.TRUE.equals(restRequest.getArgs().get(BLOB_ID_IS_SERVER_GENERATED))) { + frontendMetrics.generatedBlobIdError.inc(); + if (Boolean.TRUE.equals(restRequest.getArgs().get(GENERATED_BLOB_ID_SERVER_ERROR_ENABLED))) { + errorCode = RestServiceErrorCode.InternalServerError; + } + } exception = new RestServiceException(exception, errorCode); } responseHandler.handleResponse(restRequest, restResponseChannel, responseBody, exception); diff --git a/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java b/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java index 13ba7dd6aa..c19e73a41f 100644 --- a/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java +++ b/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java @@ -2247,24 +2247,53 @@ public void routerExceptionPipelineTest() throws Exception { @Test public void testGeneratedIdResponseMapping() throws Exception { + assertEquals("ambry-internal-key-generatedBlobIdServerErrorEnabled", + InternalKeys.GENERATED_BLOB_ID_SERVER_ERROR_ENABLED); + assertSame(frontendMetrics.generatedBlobIdError, + metricRegistry.getCounters().get(MetricRegistry.name(FrontendRestRequestService.class, "GeneratedBlobIdError"))); for (Object context : new Object[]{null, false, true, "true"}) { - for (RouterErrorCode code : RouterErrorCode.values()) { - try (RestRequest request = createRestRequest(RestMethod.POST, "/", null, null)) { - if (context != null) { - request.setArg(InternalKeys.BLOB_ID_IS_SERVER_GENERATED, context); + for (Object gate : new Object[]{null, false, true, "true"}) { + for (RouterErrorCode code : RouterErrorCode.values()) { + try (RestRequest request = createRestRequest(RestMethod.POST, "/", null, null)) { + if (context != null) { + request.setArg(InternalKeys.BLOB_ID_IS_SERVER_GENERATED, context); + } + if (gate != null) { + request.setArg(InternalKeys.GENERATED_BLOB_ID_SERVER_ERROR_ENABLED, gate); + } + RouterException error = new RouterException("router failure", code); + MockRestResponseChannel response = spy(new MockRestResponseChannel()); + long countBefore = frontendMetrics.generatedBlobIdError.getCount(); + frontendRestRequestService.submitResponse(request, response, null, error); + boolean generatedIdError = Boolean.TRUE.equals(context) && code == RouterErrorCode.InvalidBlobId; + RestServiceErrorCode expected = generatedIdError && Boolean.TRUE.equals(gate) + ? RestServiceErrorCode.InternalServerError : RestServiceErrorCode.getRestServiceErrorCode(code); + assertEquals(ResponseStatus.getResponseStatus(expected), response.getStatus()); + assertSame(error, response.getException().getCause()); + assertEquals(countBefore + (generatedIdError ? 1 : 0), frontendMetrics.generatedBlobIdError.getCount()); + verify(response).onResponseComplete(any()); } - RouterException error = new RouterException("router failure", code); - MockRestResponseChannel response = new MockRestResponseChannel(); - frontendRestRequestService.submitResponse(request, response, null, error); - RestServiceErrorCode expected = Boolean.TRUE.equals(context) && code == RouterErrorCode.InvalidBlobId - ? RestServiceErrorCode.InternalServerError : RestServiceErrorCode.getRestServiceErrorCode(code); - assertEquals(ResponseStatus.getResponseStatus(expected), response.getStatus()); - assertSame(error, response.getException().getCause()); } } } } + @Test + public void testGeneratedIdCounterIgnoresNonRouterFailures() throws Exception { + for (Exception error : new Exception[]{null, new RestServiceException("invalid input", RestServiceErrorCode.BadRequest), + new RuntimeException("failure")}) { + try (RestRequest request = createRestRequest(RestMethod.POST, "/", null, null)) { + request.setArg(InternalKeys.BLOB_ID_IS_SERVER_GENERATED, true); + request.setArg(InternalKeys.GENERATED_BLOB_ID_SERVER_ERROR_ENABLED, true); + MockRestResponseChannel response = new MockRestResponseChannel(); + long countBefore = frontendMetrics.generatedBlobIdError.getCount(); + frontendRestRequestService.submitResponse(request, response, null, error); + assertSame(error, response.getException()); + assertEquals(countBefore, frontendMetrics.generatedBlobIdError.getCount()); + } + } + } + @Test public void testUploadBlobIdErrorClassification() throws Exception { refContainer = new ContainerBuilder(refContainer).setNamedBlobMode(Container.NamedBlobMode.OPTIONAL) @@ -2344,6 +2373,8 @@ public void testUploadBlobIdErrorClassification() throws Exception { clearInvocations(router); try (RestRequest request = createRestRequest(method, uri, headers, new LinkedList<>(Arrays.asList(ByteBuffer.wrap(body), null)))) { + request.setArg(InternalKeys.GENERATED_BLOB_ID_SERVER_ERROR_ENABLED, true); + long countBefore = frontendMetrics.generatedBlobIdError.getCount(); RestServiceErrorCode expected; if (failure instanceof RouterException && ((RouterException) failure).getErrorCode() == RouterErrorCode.BlobTooLarge) { @@ -2357,6 +2388,8 @@ public void testUploadBlobIdErrorClassification() throws Exception { assertException(RestServiceException.class, () -> doOperation(request, response), e -> assertEquals(uri + ":" + stage, expected, e.getErrorCode())); assertEquals(uri + ":" + stage, ResponseStatus.getResponseStatus(expected), response.getStatus()); + assertEquals(countBefore + (expected == RestServiceErrorCode.InternalServerError ? 1 : 0), + frontendMetrics.generatedBlobIdError.getCount()); verify(response).onResponseComplete(any()); if (parseGeneratedId) { assertEquals(RouterErrorCode.InvalidBlobId, @@ -2387,8 +2420,11 @@ public void testClientBlobIdErrorsRemainBadRequest() throws Exception { headers.put(InternalKeys.BLOB_ID_IS_SERVER_GENERATED, "true"); try (RestRequest request = createRestRequest(put ? RestMethod.PUT : RestMethod.valueOf(operation), put ? "/" + operation : "/" + blobId, headers, null)) { + request.setArg(InternalKeys.GENERATED_BLOB_ID_SERVER_ERROR_ENABLED, true); + long countBefore = frontendMetrics.generatedBlobIdError.getCount(); assertEquals(ResponseStatus.BadRequest, verifyOperationFailure(request, RestServiceErrorCode.BadRequest).getStatus()); + assertEquals(countBefore, frontendMetrics.generatedBlobIdError.getCount()); } } } From 34c1cfbc907f30b56a0df19e9387fb7207bc4e27 Mon Sep 17 00:00:00 2001 From: Nick Lopez Date: Thu, 8 Oct 2026 11:32:04 -0700 Subject: [PATCH 4/5] Configure generated upload ID server errors in the frontend Use a default-off frontend property to control HTTP classification while counting generated ID failures independently. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../github/ambry/config/FrontendConfig.java | 10 ++++++ .../java/com/github/ambry/rest/RestUtils.java | 5 --- .../ambry/config/FrontendConfigTest.java | 20 +++++++++++ .../frontend/FrontendRestRequestService.java | 4 +-- .../FrontendRestRequestServiceTest.java | 33 +++++++++++-------- 5 files changed, 52 insertions(+), 20 deletions(-) diff --git a/ambry-api/src/main/java/com/github/ambry/config/FrontendConfig.java b/ambry-api/src/main/java/com/github/ambry/config/FrontendConfig.java index 399c477c9a..74be8e7d95 100644 --- a/ambry-api/src/main/java/com/github/ambry/config/FrontendConfig.java +++ b/ambry-api/src/main/java/com/github/ambry/config/FrontendConfig.java @@ -41,6 +41,7 @@ public class FrontendConfig { PREFIX + "max.acceptable.ttl.secs.if.ttl.required"; public static final String MAX_JSON_REQUEST_SIZE_BYTES_KEY = PREFIX + "max.json.request.size.bytes"; public static final String ENABLE_UNDELETE = PREFIX + "enable.undelete"; + public static final String GENERATED_BLOB_ID_SERVER_ERROR_ENABLED = PREFIX + "generated.blob.id.server.error.enabled"; public static final String NAMED_BLOB_DB_FACTORY = PREFIX + "named.blob.db.factory"; public static final String CONTAINER_METRICS_EXCLUDED_ACCOUNTS = PREFIX + "container.metrics.excluded.accounts"; public static final String CONTAINER_METRICS_AGGREGATED_ACCOUNTS = PREFIX + "container.metrics.aggregated.accounts"; @@ -312,6 +313,14 @@ public class FrontendConfig { @Default("false") public final boolean enableUndelete; + /** + * {@code frontend.generated.blob.id.server.error.enabled} enables HTTP 500 for invalid generated upload IDs. + * Defaults to {@code false} (HTTP 400); generated-ID errors are counted regardless of this setting. + */ + @Config(GENERATED_BLOB_ID_SERVER_ERROR_ENABLED) + @Default("false") + public final boolean generatedBlobIdServerErrorEnabled; + /** * The {@link AccountStatsStoreFactory}. */ @@ -447,6 +456,7 @@ public FrontendConfig(VerifiableProperties verifiableProperties) { maxJsonRequestSizeBytes = verifiableProperties.getIntInRange(MAX_JSON_REQUEST_SIZE_BYTES_KEY, 20 * 1024 * 1024, 0, Integer.MAX_VALUE); enableUndelete = verifiableProperties.getBoolean(ENABLE_UNDELETE, false); + generatedBlobIdServerErrorEnabled = verifiableProperties.getBoolean(GENERATED_BLOB_ID_SERVER_ERROR_ENABLED, false); accountStatsStoreFactory = verifiableProperties.getString(ACCOUNT_STATS_STORE_FACTORY, DEFAULT_ACCOUNT_STATS_STORE_FACTORY); namedBlobDbFactory = verifiableProperties.getString(NAMED_BLOB_DB_FACTORY, null); diff --git a/ambry-api/src/main/java/com/github/ambry/rest/RestUtils.java b/ambry-api/src/main/java/com/github/ambry/rest/RestUtils.java index aabc1ecd18..8627092767 100644 --- a/ambry-api/src/main/java/com/github/ambry/rest/RestUtils.java +++ b/ambry-api/src/main/java/com/github/ambry/rest/RestUtils.java @@ -519,11 +519,6 @@ public static final class InternalKeys { */ public static final String BLOB_ID_IS_SERVER_GENERATED = KEY_PREFIX + "blob-id-is-server-generated"; - /** - * Trusted Boolean gate for HTTP 500 on invalid generated upload IDs. Absent or false preserves HTTP 400. - */ - public static final String GENERATED_BLOB_ID_SERVER_ERROR_ENABLED = KEY_PREFIX + "generatedBlobIdServerErrorEnabled"; - /** * The internal header to determine if the delete request is coming from a dataset deletion. */ diff --git a/ambry-api/src/test/java/com/github/ambry/config/FrontendConfigTest.java b/ambry-api/src/test/java/com/github/ambry/config/FrontendConfigTest.java index 270dae8bc5..0ff4103b2d 100644 --- a/ambry-api/src/test/java/com/github/ambry/config/FrontendConfigTest.java +++ b/ambry-api/src/test/java/com/github/ambry/config/FrontendConfigTest.java @@ -26,6 +26,26 @@ */ public class FrontendConfigTest { + @Test + public void testGeneratedBlobIdServerErrorEnabled() { + Assert.assertEquals("frontend.generated.blob.id.server.error.enabled", + FrontendConfig.GENERATED_BLOB_ID_SERVER_ERROR_ENABLED); + Properties properties = new Properties(); + Assert.assertFalse(new FrontendConfig(new VerifiableProperties(properties)).generatedBlobIdServerErrorEnabled); + for (boolean enabled : new boolean[]{false, true}) { + properties.setProperty(FrontendConfig.GENERATED_BLOB_ID_SERVER_ERROR_ENABLED, Boolean.toString(enabled)); + Assert.assertEquals(enabled, + new FrontendConfig(new VerifiableProperties(properties)).generatedBlobIdServerErrorEnabled); + } + } + + @Test(expected = IllegalArgumentException.class) + public void testGeneratedBlobIdServerErrorEnabledRejectsInvalidValue() { + Properties properties = new Properties(); + properties.setProperty(FrontendConfig.GENERATED_BLOB_ID_SERVER_ERROR_ENABLED, "enabled"); + new FrontendConfig(new VerifiableProperties(properties)); + } + @Test public void testNamedBlobCleanupContainerDelaySeconds() { Properties properties = new Properties(); diff --git a/ambry-frontend/src/main/java/com/github/ambry/frontend/FrontendRestRequestService.java b/ambry-frontend/src/main/java/com/github/ambry/frontend/FrontendRestRequestService.java index a3e81d008d..7e16deab06 100644 --- a/ambry-frontend/src/main/java/com/github/ambry/frontend/FrontendRestRequestService.java +++ b/ambry-frontend/src/main/java/com/github/ambry/frontend/FrontendRestRequestService.java @@ -561,7 +561,7 @@ public void handleOptions(RestRequest restRequest, RestResponseChannel restRespo /** * Submits the response and {@code responseBody} (and any {@code exception})for the {@code restRequest} to the * {@code responseHandler}. - * Invalid generated upload IDs are always counted; server-error classification requires an enabled request gate. + * Invalid generated upload IDs are always counted; frontend configuration controls server-error classification. * @param restRequest the {@link RestRequest} for which a response is ready. * @param restResponseChannel the {@link RestResponseChannel} over which the response can be sent. * @param responseBody the body of the response in the form of a {@link ReadableStreamChannel}. @@ -581,7 +581,7 @@ void submitResponse(RestRequest restRequest, RestResponseChannel restResponseCha if (routerErrorCode == RouterErrorCode.InvalidBlobId && Boolean.TRUE.equals(restRequest.getArgs().get(BLOB_ID_IS_SERVER_GENERATED))) { frontendMetrics.generatedBlobIdError.inc(); - if (Boolean.TRUE.equals(restRequest.getArgs().get(GENERATED_BLOB_ID_SERVER_ERROR_ENABLED))) { + if (frontendConfig.generatedBlobIdServerErrorEnabled) { errorCode = RestServiceErrorCode.InternalServerError; } } diff --git a/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java b/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java index c19e73a41f..7f42b0eab1 100644 --- a/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java +++ b/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java @@ -2247,30 +2247,27 @@ public void routerExceptionPipelineTest() throws Exception { @Test public void testGeneratedIdResponseMapping() throws Exception { - assertEquals("ambry-internal-key-generatedBlobIdServerErrorEnabled", - InternalKeys.GENERATED_BLOB_ID_SERVER_ERROR_ENABLED); assertSame(frontendMetrics.generatedBlobIdError, metricRegistry.getCounters().get(MetricRegistry.name(FrontendRestRequestService.class, "GeneratedBlobIdError"))); - for (Object context : new Object[]{null, false, true, "true"}) { - for (Object gate : new Object[]{null, false, true, "true"}) { + for (String configuredValue : new String[]{null, "false", "true"}) { + setupGeneratedIdConfig(configuredValue); + for (Object context : new Object[]{null, false, true, "true"}) { for (RouterErrorCode code : RouterErrorCode.values()) { try (RestRequest request = createRestRequest(RestMethod.POST, "/", null, null)) { if (context != null) { request.setArg(InternalKeys.BLOB_ID_IS_SERVER_GENERATED, context); } - if (gate != null) { - request.setArg(InternalKeys.GENERATED_BLOB_ID_SERVER_ERROR_ENABLED, gate); - } RouterException error = new RouterException("router failure", code); MockRestResponseChannel response = spy(new MockRestResponseChannel()); long countBefore = frontendMetrics.generatedBlobIdError.getCount(); frontendRestRequestService.submitResponse(request, response, null, error); boolean generatedIdError = Boolean.TRUE.equals(context) && code == RouterErrorCode.InvalidBlobId; - RestServiceErrorCode expected = generatedIdError && Boolean.TRUE.equals(gate) + RestServiceErrorCode expected = generatedIdError && Boolean.parseBoolean(configuredValue) ? RestServiceErrorCode.InternalServerError : RestServiceErrorCode.getRestServiceErrorCode(code); assertEquals(ResponseStatus.getResponseStatus(expected), response.getStatus()); assertSame(error, response.getException().getCause()); assertEquals(countBefore + (generatedIdError ? 1 : 0), frontendMetrics.generatedBlobIdError.getCount()); + assertEquals(0, frontendMetrics.responseSubmissionError.getCount()); verify(response).onResponseComplete(any()); } } @@ -2280,11 +2277,11 @@ public void testGeneratedIdResponseMapping() throws Exception { @Test public void testGeneratedIdCounterIgnoresNonRouterFailures() throws Exception { + setupGeneratedIdConfig("true"); for (Exception error : new Exception[]{null, new RestServiceException("invalid input", RestServiceErrorCode.BadRequest), new RuntimeException("failure")}) { try (RestRequest request = createRestRequest(RestMethod.POST, "/", null, null)) { request.setArg(InternalKeys.BLOB_ID_IS_SERVER_GENERATED, true); - request.setArg(InternalKeys.GENERATED_BLOB_ID_SERVER_ERROR_ENABLED, true); MockRestResponseChannel response = new MockRestResponseChannel(); long countBefore = frontendMetrics.generatedBlobIdError.getCount(); frontendRestRequestService.submitResponse(request, response, null, error); @@ -2305,8 +2302,8 @@ public void testUploadBlobIdErrorClassification() throws Exception { idConverterFactory = converterFactory; router = spy(router); doReturn(converterFactory.getIdConverter()).when(router).getIdConverter(); - frontendRestRequestService = spy(getFrontendRestRequestService()); - frontendRestRequestService.start(); + setupGeneratedIdConfig("true"); + frontendRestRequestService = spy(frontendRestRequestService); String namedPath = "/named/" + refAccount.getName() + "/" + refContainer.getName() + "/blob"; String s3Path = namedPath.replace("/named/", "/s3/"); String chunkId = new BlobId(blobIdVersion, BlobId.BlobIdType.NATIVE, ClusterMap.UNKNOWN_DATACENTER_ID, @@ -2373,7 +2370,6 @@ public void testUploadBlobIdErrorClassification() throws Exception { clearInvocations(router); try (RestRequest request = createRestRequest(method, uri, headers, new LinkedList<>(Arrays.asList(ByteBuffer.wrap(body), null)))) { - request.setArg(InternalKeys.GENERATED_BLOB_ID_SERVER_ERROR_ENABLED, true); long countBefore = frontendMetrics.generatedBlobIdError.getCount(); RestServiceErrorCode expected; if (failure instanceof RouterException @@ -2410,6 +2406,7 @@ public void testUploadBlobIdErrorClassification() throws Exception { @Test public void testClientBlobIdErrorsRemainBadRequest() throws Exception { + setupGeneratedIdConfig("true"); Properties properties = new Properties(); properties.setProperty(InMemoryRouter.OPERATION_THROW_ROUTER_EXCEPTION, RouterErrorCode.InvalidBlobId.name()); router.setVerifiableProperties(new VerifiableProperties(properties)); @@ -2420,7 +2417,6 @@ public void testClientBlobIdErrorsRemainBadRequest() throws Exception { headers.put(InternalKeys.BLOB_ID_IS_SERVER_GENERATED, "true"); try (RestRequest request = createRestRequest(put ? RestMethod.PUT : RestMethod.valueOf(operation), put ? "/" + operation : "/" + blobId, headers, null)) { - request.setArg(InternalKeys.GENERATED_BLOB_ID_SERVER_ERROR_ENABLED, true); long countBefore = frontendMetrics.generatedBlobIdError.getCount(); assertEquals(ResponseStatus.BadRequest, verifyOperationFailure(request, RestServiceErrorCode.BadRequest).getStatus()); @@ -2430,6 +2426,17 @@ public void testClientBlobIdErrorsRemainBadRequest() throws Exception { } } + private void setupGeneratedIdConfig(String value) throws InstantiationException { + if (value == null) { + configProps.remove(FrontendConfig.GENERATED_BLOB_ID_SERVER_ERROR_ENABLED); + } else { + configProps.setProperty(FrontendConfig.GENERATED_BLOB_ID_SERVER_ERROR_ENABLED, value); + } + frontendConfig = new FrontendConfig(new VerifiableProperties(configProps)); + frontendRestRequestService = getFrontendRestRequestService(); + frontendRestRequestService.start(); + } + /** * Test that GET operations fail with the expected error code when a bad range header is provided. * @throws Exception From cdd5d3342e4856508e04c7dc3d746fb91a7b827e Mon Sep 17 00:00:00 2001 From: Nick Lopez Date: Thu, 8 Oct 2026 13:36:27 -0700 Subject: [PATCH 5/5] Measure generated upload ID failures without changing responses Count server-generated invalid IDs separately while preserving HTTP status mapping and callback exceptions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../github/ambry/config/FrontendConfig.java | 10 ---- .../ambry/config/FrontendConfigTest.java | 20 ------- .../frontend/FrontendRestRequestService.java | 8 +-- .../FrontendRestRequestServiceTest.java | 60 +++++++------------ 4 files changed, 23 insertions(+), 75 deletions(-) diff --git a/ambry-api/src/main/java/com/github/ambry/config/FrontendConfig.java b/ambry-api/src/main/java/com/github/ambry/config/FrontendConfig.java index 74be8e7d95..399c477c9a 100644 --- a/ambry-api/src/main/java/com/github/ambry/config/FrontendConfig.java +++ b/ambry-api/src/main/java/com/github/ambry/config/FrontendConfig.java @@ -41,7 +41,6 @@ public class FrontendConfig { PREFIX + "max.acceptable.ttl.secs.if.ttl.required"; public static final String MAX_JSON_REQUEST_SIZE_BYTES_KEY = PREFIX + "max.json.request.size.bytes"; public static final String ENABLE_UNDELETE = PREFIX + "enable.undelete"; - public static final String GENERATED_BLOB_ID_SERVER_ERROR_ENABLED = PREFIX + "generated.blob.id.server.error.enabled"; public static final String NAMED_BLOB_DB_FACTORY = PREFIX + "named.blob.db.factory"; public static final String CONTAINER_METRICS_EXCLUDED_ACCOUNTS = PREFIX + "container.metrics.excluded.accounts"; public static final String CONTAINER_METRICS_AGGREGATED_ACCOUNTS = PREFIX + "container.metrics.aggregated.accounts"; @@ -313,14 +312,6 @@ public class FrontendConfig { @Default("false") public final boolean enableUndelete; - /** - * {@code frontend.generated.blob.id.server.error.enabled} enables HTTP 500 for invalid generated upload IDs. - * Defaults to {@code false} (HTTP 400); generated-ID errors are counted regardless of this setting. - */ - @Config(GENERATED_BLOB_ID_SERVER_ERROR_ENABLED) - @Default("false") - public final boolean generatedBlobIdServerErrorEnabled; - /** * The {@link AccountStatsStoreFactory}. */ @@ -456,7 +447,6 @@ public FrontendConfig(VerifiableProperties verifiableProperties) { maxJsonRequestSizeBytes = verifiableProperties.getIntInRange(MAX_JSON_REQUEST_SIZE_BYTES_KEY, 20 * 1024 * 1024, 0, Integer.MAX_VALUE); enableUndelete = verifiableProperties.getBoolean(ENABLE_UNDELETE, false); - generatedBlobIdServerErrorEnabled = verifiableProperties.getBoolean(GENERATED_BLOB_ID_SERVER_ERROR_ENABLED, false); accountStatsStoreFactory = verifiableProperties.getString(ACCOUNT_STATS_STORE_FACTORY, DEFAULT_ACCOUNT_STATS_STORE_FACTORY); namedBlobDbFactory = verifiableProperties.getString(NAMED_BLOB_DB_FACTORY, null); diff --git a/ambry-api/src/test/java/com/github/ambry/config/FrontendConfigTest.java b/ambry-api/src/test/java/com/github/ambry/config/FrontendConfigTest.java index 0ff4103b2d..270dae8bc5 100644 --- a/ambry-api/src/test/java/com/github/ambry/config/FrontendConfigTest.java +++ b/ambry-api/src/test/java/com/github/ambry/config/FrontendConfigTest.java @@ -26,26 +26,6 @@ */ public class FrontendConfigTest { - @Test - public void testGeneratedBlobIdServerErrorEnabled() { - Assert.assertEquals("frontend.generated.blob.id.server.error.enabled", - FrontendConfig.GENERATED_BLOB_ID_SERVER_ERROR_ENABLED); - Properties properties = new Properties(); - Assert.assertFalse(new FrontendConfig(new VerifiableProperties(properties)).generatedBlobIdServerErrorEnabled); - for (boolean enabled : new boolean[]{false, true}) { - properties.setProperty(FrontendConfig.GENERATED_BLOB_ID_SERVER_ERROR_ENABLED, Boolean.toString(enabled)); - Assert.assertEquals(enabled, - new FrontendConfig(new VerifiableProperties(properties)).generatedBlobIdServerErrorEnabled); - } - } - - @Test(expected = IllegalArgumentException.class) - public void testGeneratedBlobIdServerErrorEnabledRejectsInvalidValue() { - Properties properties = new Properties(); - properties.setProperty(FrontendConfig.GENERATED_BLOB_ID_SERVER_ERROR_ENABLED, "enabled"); - new FrontendConfig(new VerifiableProperties(properties)); - } - @Test public void testNamedBlobCleanupContainerDelaySeconds() { Properties properties = new Properties(); diff --git a/ambry-frontend/src/main/java/com/github/ambry/frontend/FrontendRestRequestService.java b/ambry-frontend/src/main/java/com/github/ambry/frontend/FrontendRestRequestService.java index 7e16deab06..906c5265fd 100644 --- a/ambry-frontend/src/main/java/com/github/ambry/frontend/FrontendRestRequestService.java +++ b/ambry-frontend/src/main/java/com/github/ambry/frontend/FrontendRestRequestService.java @@ -561,7 +561,7 @@ public void handleOptions(RestRequest restRequest, RestResponseChannel restRespo /** * Submits the response and {@code responseBody} (and any {@code exception})for the {@code restRequest} to the * {@code responseHandler}. - * Invalid generated upload IDs are always counted; frontend configuration controls server-error classification. + * Counts invalid IDs encountered while consuming server-generated upload results. * @param restRequest the {@link RestRequest} for which a response is ready. * @param restResponseChannel the {@link RestResponseChannel} over which the response can be sent. * @param responseBody the body of the response in the form of a {@link ReadableStreamChannel}. @@ -577,15 +577,11 @@ void submitResponse(RestRequest restRequest, RestResponseChannel restResponseCha } if (exception instanceof RouterException) { RouterErrorCode routerErrorCode = ((RouterException) exception).getErrorCode(); - RestServiceErrorCode errorCode = RestServiceErrorCode.getRestServiceErrorCode(routerErrorCode); if (routerErrorCode == RouterErrorCode.InvalidBlobId && Boolean.TRUE.equals(restRequest.getArgs().get(BLOB_ID_IS_SERVER_GENERATED))) { frontendMetrics.generatedBlobIdError.inc(); - if (frontendConfig.generatedBlobIdServerErrorEnabled) { - errorCode = RestServiceErrorCode.InternalServerError; - } } - exception = new RestServiceException(exception, errorCode); + exception = new RestServiceException(exception, RestServiceErrorCode.getRestServiceErrorCode(routerErrorCode)); } responseHandler.handleResponse(restRequest, restResponseChannel, responseBody, exception); } catch (Exception e) { diff --git a/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java b/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java index 7f42b0eab1..77a27eb94f 100644 --- a/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java +++ b/ambry-frontend/src/test/java/com/github/ambry/frontend/FrontendRestRequestServiceTest.java @@ -2249,27 +2249,23 @@ public void routerExceptionPipelineTest() throws Exception { public void testGeneratedIdResponseMapping() throws Exception { assertSame(frontendMetrics.generatedBlobIdError, metricRegistry.getCounters().get(MetricRegistry.name(FrontendRestRequestService.class, "GeneratedBlobIdError"))); - for (String configuredValue : new String[]{null, "false", "true"}) { - setupGeneratedIdConfig(configuredValue); - for (Object context : new Object[]{null, false, true, "true"}) { - for (RouterErrorCode code : RouterErrorCode.values()) { - try (RestRequest request = createRestRequest(RestMethod.POST, "/", null, null)) { - if (context != null) { - request.setArg(InternalKeys.BLOB_ID_IS_SERVER_GENERATED, context); - } - RouterException error = new RouterException("router failure", code); - MockRestResponseChannel response = spy(new MockRestResponseChannel()); - long countBefore = frontendMetrics.generatedBlobIdError.getCount(); - frontendRestRequestService.submitResponse(request, response, null, error); - boolean generatedIdError = Boolean.TRUE.equals(context) && code == RouterErrorCode.InvalidBlobId; - RestServiceErrorCode expected = generatedIdError && Boolean.parseBoolean(configuredValue) - ? RestServiceErrorCode.InternalServerError : RestServiceErrorCode.getRestServiceErrorCode(code); - assertEquals(ResponseStatus.getResponseStatus(expected), response.getStatus()); - assertSame(error, response.getException().getCause()); - assertEquals(countBefore + (generatedIdError ? 1 : 0), frontendMetrics.generatedBlobIdError.getCount()); - assertEquals(0, frontendMetrics.responseSubmissionError.getCount()); - verify(response).onResponseComplete(any()); + for (Object context : new Object[]{null, false, true, "true"}) { + for (RouterErrorCode code : RouterErrorCode.values()) { + try (RestRequest request = createRestRequest(RestMethod.POST, "/", null, null)) { + if (context != null) { + request.setArg(InternalKeys.BLOB_ID_IS_SERVER_GENERATED, context); } + RouterException error = new RouterException("router failure", code); + MockRestResponseChannel response = spy(new MockRestResponseChannel()); + long countBefore = frontendMetrics.generatedBlobIdError.getCount(); + frontendRestRequestService.submitResponse(request, response, null, error); + boolean generatedIdError = Boolean.TRUE.equals(context) && code == RouterErrorCode.InvalidBlobId; + assertEquals(ResponseStatus.getResponseStatus(RestServiceErrorCode.getRestServiceErrorCode(code)), + response.getStatus()); + assertSame(error, response.getException().getCause()); + assertEquals(countBefore + (generatedIdError ? 1 : 0), frontendMetrics.generatedBlobIdError.getCount()); + assertEquals(0, frontendMetrics.responseSubmissionError.getCount()); + verify(response).onResponseComplete(any()); } } } @@ -2277,7 +2273,6 @@ public void testGeneratedIdResponseMapping() throws Exception { @Test public void testGeneratedIdCounterIgnoresNonRouterFailures() throws Exception { - setupGeneratedIdConfig("true"); for (Exception error : new Exception[]{null, new RestServiceException("invalid input", RestServiceErrorCode.BadRequest), new RuntimeException("failure")}) { try (RestRequest request = createRestRequest(RestMethod.POST, "/", null, null)) { @@ -2302,8 +2297,8 @@ public void testUploadBlobIdErrorClassification() throws Exception { idConverterFactory = converterFactory; router = spy(router); doReturn(converterFactory.getIdConverter()).when(router).getIdConverter(); - setupGeneratedIdConfig("true"); - frontendRestRequestService = spy(frontendRestRequestService); + frontendRestRequestService = spy(getFrontendRestRequestService()); + frontendRestRequestService.start(); String namedPath = "/named/" + refAccount.getName() + "/" + refContainer.getName() + "/blob"; String s3Path = namedPath.replace("/named/", "/s3/"); String chunkId = new BlobId(blobIdVersion, BlobId.BlobIdType.NATIVE, ClusterMap.UNKNOWN_DATACENTER_ID, @@ -2375,17 +2370,16 @@ public void testUploadBlobIdErrorClassification() throws Exception { if (failure instanceof RouterException && ((RouterException) failure).getErrorCode() == RouterErrorCode.BlobTooLarge) { expected = RestServiceErrorCode.RequestTooLarge; - } else if (failure instanceof RouterException && !stage.equals("router")) { - expected = RestServiceErrorCode.InternalServerError; } else { expected = RestServiceErrorCode.BadRequest; } + boolean generatedIdError = !stage.equals("router") && failure instanceof RouterException + && ((RouterException) failure).getErrorCode() == RouterErrorCode.InvalidBlobId; MockRestResponseChannel response = spy(new MockRestResponseChannel()); assertException(RestServiceException.class, () -> doOperation(request, response), e -> assertEquals(uri + ":" + stage, expected, e.getErrorCode())); assertEquals(uri + ":" + stage, ResponseStatus.getResponseStatus(expected), response.getStatus()); - assertEquals(countBefore + (expected == RestServiceErrorCode.InternalServerError ? 1 : 0), - frontendMetrics.generatedBlobIdError.getCount()); + assertEquals(countBefore + (generatedIdError ? 1 : 0), frontendMetrics.generatedBlobIdError.getCount()); verify(response).onResponseComplete(any()); if (parseGeneratedId) { assertEquals(RouterErrorCode.InvalidBlobId, @@ -2406,7 +2400,6 @@ public void testUploadBlobIdErrorClassification() throws Exception { @Test public void testClientBlobIdErrorsRemainBadRequest() throws Exception { - setupGeneratedIdConfig("true"); Properties properties = new Properties(); properties.setProperty(InMemoryRouter.OPERATION_THROW_ROUTER_EXCEPTION, RouterErrorCode.InvalidBlobId.name()); router.setVerifiableProperties(new VerifiableProperties(properties)); @@ -2426,17 +2419,6 @@ public void testClientBlobIdErrorsRemainBadRequest() throws Exception { } } - private void setupGeneratedIdConfig(String value) throws InstantiationException { - if (value == null) { - configProps.remove(FrontendConfig.GENERATED_BLOB_ID_SERVER_ERROR_ENABLED); - } else { - configProps.setProperty(FrontendConfig.GENERATED_BLOB_ID_SERVER_ERROR_ENABLED, value); - } - frontendConfig = new FrontendConfig(new VerifiableProperties(configProps)); - frontendRestRequestService = getFrontendRestRequestService(); - frontendRestRequestService.start(); - } - /** * Test that GET operations fail with the expected error code when a bad range header is provided. * @throws Exception