fix: consider Storage#delete(BlobId) idempotent when id has generation (

#2222) Update logic to consider an object delete request to be idempotent if the objects generation is specified in the BlobId.
googleapis · Sep 25, 2023 · 453dd63 · 453dd63
1 parent f092c4e
commit 453dd63
Show file tree

Hide file tree

Showing 3 changed files with 18 additions and 3 deletions.
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcRetryAlgorithmManager.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcRetryAlgorithmManager.java
@@ -96,6 +96,9 @@ public ResultRetryAlgorithm<?> getFor(DeleteNotificationConfigRequest req) {
   }
 
   public ResultRetryAlgorithm<?> getFor(DeleteObjectRequest req) {
+    if (req.getGeneration() > 0 || req.hasIfGenerationMatch()) {
+      return retryStrategy.getIdempotentHandler();
+    }
     return retryStrategy.getNonidempotentHandler();
   }
 

diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/HttpRetryAlgorithmManager.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/HttpRetryAlgorithmManager.java
@@ -203,6 +203,7 @@ public ResultRetryAlgorithm<?> getForObjectsCreate(
   public ResultRetryAlgorithm<?> getForObjectsDelete(
       StorageObject pb, Map<StorageRpc.Option, ?> optionsMap) {
     return optionsMap.containsKey(StorageRpc.Option.IF_GENERATION_MATCH)
+            || (pb.getGeneration() != null && pb.getGeneration() > 0)
         ? retryStrategy.getIdempotentHandler()
         : retryStrategy.getNonidempotentHandler();
   }

diff --git a/...d-storage/src/test/java/com/google/cloud/storage/conformance/retry/RpcMethodMappings.java b/...d-storage/src/test/java/com/google/cloud/storage/conformance/retry/RpcMethodMappings.java
@@ -1126,8 +1126,9 @@ private static void delete(ArrayList<RpcMethodMapping> a) {
                     (ctx, c) ->
                         ctx.map(
                             state -> {
-                              boolean success =
-                                  ctx.getStorage().delete(state.getBlob().getBlobId());
+                              BlobId id = state.getBlob().getBlobId();
+                              BlobId idWithoutGeneration = BlobId.of(id.getBucket(), id.getName());
+                              boolean success = ctx.getStorage().delete(idWithoutGeneration);
                               assertTrue(success);
                               return state.with(success);
                             }))
@@ -1169,7 +1170,17 @@ private static void delete(ArrayList<RpcMethodMapping> a) {
         a.add(
             RpcMethodMapping.newBuilder(67, objects.delete)
                 .withApplicable(not(TestRetryConformance::isPreconditionsProvided))
-                .withTest((ctx, c) -> ctx.peek(state -> state.getBlob().delete()))
+                .withTest(
+                    (ctx, c) ->
+                        ctx.peek(
+                            state -> {
+                              Blob blob = state.getBlob();
+                              Blob blobWithoutGeneration =
+                                  blob.toBuilder()
+                                      .setBlobId(BlobId.of(blob.getBucket(), blob.getName()))
+                                      .build();
+                              blobWithoutGeneration.delete();
+                            }))
                 .build());
         a.add(
             RpcMethodMapping.newBuilder(68, objects.delete)