From ccb209ad376fef0dfa1fc85086b5b882238bfb19 Mon Sep 17 00:00:00 2001 From: Chris Eager Date: Thu, 22 May 2025 14:50:07 -0500 Subject: [PATCH] Consolidate avatar deletion logic in ProfilesManager --- .../textsecuregcm/WhisperServerService.java | 6 ++-- .../controllers/ProfileController.java | 14 +------- .../grpc/ProfileGrpcService.java | 15 ++------ .../storage/ProfilesManager.java | 36 ++++++++++++++----- .../controllers/ProfileControllerTest.java | 34 +++++++++--------- .../grpc/ProfileGrpcServiceTest.java | 22 +++--------- .../storage/ProfilesManagerTest.java | 4 ++- 7 files changed, 57 insertions(+), 74 deletions(-) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java index 9d52029bd..f721131d7 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java @@ -872,7 +872,7 @@ public class WhisperServerService extends Application dynamicConfigurationManager, ProfileBadgeConverter profileBadgeConverter, BadgesConfiguration badgesConfiguration, - S3Client s3client, PostPolicyGenerator policyGenerator, PolicySigner policySigner, - String bucket, ServerSecretParams serverSecretParams, ServerZkProfileOperations zkProfileOperations, Executor batchIdentityCheckExecutor) { @@ -151,8 +144,6 @@ public class ProfileController { BadgeConfiguration::getId, Function.identity())); this.serverSecretParams = serverSecretParams; this.zkProfileOperations = zkProfileOperations; - this.bucket = bucket; - this.s3client = s3client; this.policyGenerator = policyGenerator; this.policySigner = policySigner; this.batchIdentityCheckExecutor = Preconditions.checkNotNull(batchIdentityCheckExecutor); @@ -200,10 +191,7 @@ public class ProfileController { request.commitment().serialize())); if (request.getAvatarChange() != CreateProfileRequest.AvatarChange.UNCHANGED) { - currentAvatar.ifPresent(s -> s3client.deleteObject(DeleteObjectRequest.builder() - .bucket(bucket) - .key(s) - .build())); + currentAvatar.ifPresent(s -> profilesManager.deleteAvatar(s).join()); } accountsManager.update(auth.getAccount(), a -> { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/grpc/ProfileGrpcService.java b/service/src/main/java/org/whispersystems/textsecuregcm/grpc/ProfileGrpcService.java index 49462afeb..0419282b5 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/grpc/ProfileGrpcService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/grpc/ProfileGrpcService.java @@ -50,8 +50,6 @@ import org.whispersystems.textsecuregcm.util.Pair; import org.whispersystems.textsecuregcm.util.ProfileHelper; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; -import software.amazon.awssdk.services.s3.S3AsyncClient; -import software.amazon.awssdk.services.s3.model.DeleteObjectRequest; public class ProfileGrpcService extends ReactorProfileGrpc.ProfileImplBase { @@ -60,13 +58,11 @@ public class ProfileGrpcService extends ReactorProfileGrpc.ProfileImplBase { private final ProfilesManager profilesManager; private final DynamicConfigurationManager dynamicConfigurationManager; private final Map badgeConfigurationMap; - private final S3AsyncClient asyncS3client; private final PostPolicyGenerator policyGenerator; private final PolicySigner policySigner; private final ProfileBadgeConverter profileBadgeConverter; private final RateLimiters rateLimiters; private final ServerZkProfileOperations zkProfileOperations; - private final String bucket; private record AvatarData(Optional currentAvatar, Optional finalAvatar, @@ -78,26 +74,22 @@ public class ProfileGrpcService extends ReactorProfileGrpc.ProfileImplBase { final ProfilesManager profilesManager, final DynamicConfigurationManager dynamicConfigurationManager, final BadgesConfiguration badgesConfiguration, - final S3AsyncClient asyncS3client, final PostPolicyGenerator policyGenerator, final PolicySigner policySigner, final ProfileBadgeConverter profileBadgeConverter, final RateLimiters rateLimiters, - final ServerZkProfileOperations zkProfileOperations, - final String bucket) { + final ServerZkProfileOperations zkProfileOperations) { this.clock = clock; this.accountsManager = accountsManager; this.profilesManager = profilesManager; this.dynamicConfigurationManager = dynamicConfigurationManager; this.badgeConfigurationMap = badgesConfiguration.getBadges().stream().collect(Collectors.toMap( BadgeConfiguration::getId, Function.identity())); - this.asyncS3client = asyncS3client; this.policyGenerator = policyGenerator; this.policySigner = policySigner; this.profileBadgeConverter = profileBadgeConverter; this.rateLimiters = rateLimiters; this.zkProfileOperations = zkProfileOperations; - this.bucket = bucket; } @Override @@ -157,10 +149,7 @@ public class ProfileGrpcService extends ReactorProfileGrpc.ProfileImplBase { }))); if (request.getAvatarChange() != AvatarChange.AVATAR_CHANGE_UNCHANGED && avatarData.currentAvatar().isPresent()) { - updates.add(Mono.fromFuture(() -> asyncS3client.deleteObject(DeleteObjectRequest.builder() - .bucket(bucket) - .key(avatarData.currentAvatar().get()) - .build()))); + updates.add(Mono.fromFuture(() -> profilesManager.deleteAvatar(avatarData.currentAvatar.get()))); } return profileSetMono.thenMany(Flux.merge(updates)).then(Mono.just(avatarData)); }) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/ProfilesManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/ProfilesManager.java index 3c893a0e1..48666d991 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/ProfilesManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/ProfilesManager.java @@ -5,6 +5,8 @@ package org.whispersystems.textsecuregcm.storage; +import static org.whispersystems.textsecuregcm.metrics.MetricsUtil.name; + import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.annotations.VisibleForTesting; @@ -15,6 +17,7 @@ import java.util.UUID; import java.util.concurrent.CompletableFuture; import java.util.function.Function; import javax.annotation.Nullable; +import io.micrometer.core.instrument.Metrics; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.whispersystems.textsecuregcm.redis.FaultTolerantRedisClusterClient; @@ -36,8 +39,7 @@ public class ProfilesManager { private final String bucket; private final ObjectMapper mapper; - private static final CompletableFuture[] EMPTY_FUTURE_ARRAY = new CompletableFuture[0]; - + private static final String DELETE_AVATAR_COUNTER_NAME = name(ProfilesManager.class, "deleteAvatar"); public ProfilesManager(final Profiles profiles, final FaultTolerantRedisClusterClient cacheCluster, final S3AsyncClient s3Client, final String bucket) { @@ -63,19 +65,35 @@ public class ProfilesManager { final CompletableFuture profilesAndAvatars = Mono.fromFuture(profiles.deleteAll(uuid)) .flatMapIterable(Function.identity()) .flatMap(avatar -> - Mono.fromFuture(s3Client.deleteObject(DeleteObjectRequest.builder() - .bucket(bucket) - .key(avatar) - .build())) + Mono.fromFuture(deleteAvatar(avatar)) // this is best-effort .retry(3) - .onErrorComplete() - .then() - ).then().toFuture(); + .onErrorComplete()) + .then().toFuture(); return CompletableFuture.allOf(redisDelete(uuid), profilesAndAvatars); } + public CompletableFuture deleteAvatar(String avatar) { + return s3Client.deleteObject(DeleteObjectRequest.builder() + .bucket(bucket) + .key(avatar) + .build()) + .handle((ignored, throwable) -> { + final String outcome; + if (throwable != null) { + logger.warn("Error deleting avatar", throwable); + outcome = "error"; + } else { + outcome = "success"; + } + + Metrics.counter(DELETE_AVATAR_COUNTER_NAME, "outcome", outcome).increment(); + return null; + }) + .thenRun(Util.NOOP); + } + public Optional get(UUID uuid, String version) { Optional profile = redisGet(uuid, version); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ProfileControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ProfileControllerTest.java index 1c3aaee84..fb1c40f4b 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ProfileControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ProfileControllerTest.java @@ -8,6 +8,7 @@ package org.whispersystems.textsecuregcm.controllers; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatNoException; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.refEq; import static org.mockito.Mockito.any; import static org.mockito.Mockito.clearInvocations; @@ -112,8 +113,6 @@ import org.whispersystems.textsecuregcm.util.SystemMapper; import org.whispersystems.textsecuregcm.util.TestClock; import org.whispersystems.textsecuregcm.util.TestRandomUtil; import org.whispersystems.textsecuregcm.util.Util; -import software.amazon.awssdk.services.s3.S3Client; -import software.amazon.awssdk.services.s3.model.DeleteObjectRequest; @ExtendWith(DropwizardExtensionsSupport.class) class ProfileControllerTest { @@ -125,7 +124,6 @@ class ProfileControllerTest { private static final RateLimiter rateLimiter = mock(RateLimiter.class); private static final RateLimiter usernameRateLimiter = mock(RateLimiter.class); - private static final S3Client s3client = mock(S3Client.class); private static final PostPolicyGenerator postPolicyGenerator = new PostPolicyGenerator("us-west-1", "profile-bucket", "accessKey"); private static final PolicySigner policySigner = new PolicySigner("accessSecret", "us-west-1"); @@ -169,10 +167,8 @@ class ProfileControllerTest { new BadgeConfiguration("TEST2", "testing", List.of("l", "m", "h", "x", "xx", "xxx"), "SVG", List.of(new BadgeSvg("sl", "sd"), new BadgeSvg("ml", "md"), new BadgeSvg("ll", "ld"))), new BadgeConfiguration("TEST3", "testing", List.of("l", "m", "h", "x", "xx", "xxx"), "SVG", List.of(new BadgeSvg("sl", "sd"), new BadgeSvg("ml", "md"), new BadgeSvg("ll", "ld"))) ), List.of("TEST1"), Map.of(1L, "TEST1", 2L, "TEST2", 3L, "TEST3")), - s3client, postPolicyGenerator, policySigner, - "profilesBucket", serverSecretParams, zkProfileOperations, Executors.newSingleThreadExecutor())) @@ -180,7 +176,7 @@ class ProfileControllerTest { @BeforeEach void setup() { - reset(s3client); + reset(profilesManager); clock.pin(Instant.ofEpochSecond(42)); AccountsHelper.setupMockUpdate(accountsManager); @@ -234,6 +230,8 @@ class ProfileControllerTest { when(profilesManager.get(eq(AuthHelper.VALID_UUID_TWO), eq(versionHex("validversion")))).thenReturn(Optional.of(new VersionedProfile( versionHex("validversion"), name, "profiles/validavatar", emoji, about, null, phoneNumberSharing, "validcommitment".getBytes()))); + when(profilesManager.deleteAvatar(anyString())).thenReturn(CompletableFuture.completedFuture(null)); + clearInvocations(rateLimiter); clearInvocations(accountsManager); clearInvocations(usernameRateLimiter); @@ -474,7 +472,7 @@ class ProfileControllerTest { verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID), eq(versionHex("someversion"))); verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID), profileArgumentCaptor.capture()); - verifyNoMoreInteractions(s3client); + verifyNoMoreInteractions(profilesManager); assertThat(profileArgumentCaptor.getValue().commitment()).isEqualTo(commitment.serialize()); assertThat(profileArgumentCaptor.getValue().avatar()).isEqualTo(uploadAttributes.getKey()); @@ -523,7 +521,7 @@ class ProfileControllerTest { verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO), eq(versionHex("anotherversion"))); verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture()); - verifyNoMoreInteractions(s3client); + verifyNoMoreInteractions(profilesManager); assertThat(profileArgumentCaptor.getValue().commitment()).isEqualTo(commitment.serialize()); assertThat(profileArgumentCaptor.getValue().avatar()).isNull(); @@ -552,7 +550,7 @@ class ProfileControllerTest { verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO), eq(versionHex("validversion"))); verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture()); - verify(s3client, times(1)).deleteObject(eq(DeleteObjectRequest.builder().bucket("profilesBucket").key("profiles/validavatar").build())); + verify(profilesManager, times(1)).deleteAvatar("profiles/validavatar"); assertThat(profileArgumentCaptor.getValue().commitment()).isEqualTo(commitment.serialize()); assertThat(profileArgumentCaptor.getValue().avatar()).startsWith("profiles/"); @@ -581,7 +579,7 @@ class ProfileControllerTest { verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO), eq(versionHex("validversion"))); verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture()); - verify(s3client, times(1)).deleteObject(eq(DeleteObjectRequest.builder().bucket("profilesBucket").key("profiles/validavatar").build())); + verify(profilesManager, times(1)).deleteAvatar(eq("profiles/validavatar")); assertThat(profileArgumentCaptor.getValue().commitment()).isEqualTo(commitment.serialize()); assertThat(profileArgumentCaptor.getValue().avatar()).isNull(); @@ -611,7 +609,7 @@ class ProfileControllerTest { verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO), eq(versionHex("validversion"))); verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture()); - verify(s3client, never()).deleteObject(any(DeleteObjectRequest.class)); + verify(profilesManager, never()).deleteAvatar(anyString()); assertThat(profileArgumentCaptor.getValue().commitment()).isEqualTo(commitment.serialize()); assertThat(profileArgumentCaptor.getValue().avatar()).isEqualTo("profiles/validavatar"); @@ -639,7 +637,7 @@ class ProfileControllerTest { verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO), eq(versionHex("validversion"))); verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture()); - verify(s3client, times(1)).deleteObject(eq(DeleteObjectRequest.builder().bucket("profilesBucket").key("profiles/validavatar").build())); + verify(profilesManager, times(1)).deleteAvatar(eq("profiles/validavatar")); assertThat(profileArgumentCaptor.getValue().commitment()).isEqualTo(commitment.serialize()); assertThat(profileArgumentCaptor.getValue().avatar()).isNull(); @@ -669,7 +667,7 @@ class ProfileControllerTest { verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID), eq(version)); verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID), profileArgumentCaptor.capture()); - verify(s3client, never()).deleteObject(any(DeleteObjectRequest.class)); + verify(profilesManager, never()).deleteAvatar(anyString()); assertThat(profileArgumentCaptor.getValue().commitment()).isEqualTo(commitment.serialize()); assertThat(profileArgumentCaptor.getValue().avatar()).isNull(); @@ -700,7 +698,7 @@ class ProfileControllerTest { verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO), eq(version)); verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture()); - verify(s3client, times(1)).deleteObject(eq(DeleteObjectRequest.builder().bucket("profilesBucket").key("profiles/validavatar").build())); + verify(profilesManager, times(1)).deleteAvatar("profiles/validavatar"); assertThat(profileArgumentCaptor.getValue().commitment()).isEqualTo(commitment.serialize()); assertThat(profileArgumentCaptor.getValue().avatar()).startsWith("profiles/"); @@ -738,7 +736,7 @@ class ProfileControllerTest { verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO), eq(version)); verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture()); - verifyNoMoreInteractions(s3client); + verifyNoMoreInteractions(profilesManager); final VersionedProfile profile = profileArgumentCaptor.getValue(); assertThat(profile.commitment()).isEqualTo(commitment.serialize()); @@ -778,7 +776,7 @@ class ProfileControllerTest { verify(profilesManager).get(eq(AuthHelper.VALID_UUID_TWO), eq(version)); verify(profilesManager).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture()); - verifyNoMoreInteractions(s3client); + verifyNoMoreInteractions(profilesManager); final VersionedProfile profile = profileArgumentCaptor.getValue(); assertThat(profile.commitment()).isEqualTo(commitment.serialize()); @@ -859,7 +857,7 @@ class ProfileControllerTest { verify(profilesManager).get(eq(AuthHelper.VALID_UUID_TWO), eq(version)); verify(profilesManager).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture()); - verifyNoMoreInteractions(s3client); + verifyNoMoreInteractions(profilesManager); final VersionedProfile profile = profileArgumentCaptor.getValue(); assertThat(profile.commitment()).isEqualTo(commitment.serialize()); @@ -902,7 +900,7 @@ class ProfileControllerTest { verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO), eq(version)); verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture()); - verifyNoMoreInteractions(s3client); + verifyNoMoreInteractions(profilesManager); assertThat(profileArgumentCaptor.getValue().commitment()).isEqualTo(commitment.serialize()); assertThat(profileArgumentCaptor.getValue().avatar()).isNull(); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/grpc/ProfileGrpcServiceTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/grpc/ProfileGrpcServiceTest.java index 8a80b9ce8..6ac222db1 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/grpc/ProfileGrpcServiceTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/grpc/ProfileGrpcServiceTest.java @@ -17,9 +17,9 @@ import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.refEq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; import static org.whispersystems.textsecuregcm.grpc.GrpcTestUtils.assertRateLimitExceeded; import static org.whispersystems.textsecuregcm.grpc.GrpcTestUtils.assertStatusException; @@ -110,13 +110,9 @@ import org.whispersystems.textsecuregcm.util.MockUtils; import org.whispersystems.textsecuregcm.util.TestRandomUtil; import org.whispersystems.textsecuregcm.util.UUIDUtil; import reactor.core.publisher.Mono; -import software.amazon.awssdk.services.s3.S3AsyncClient; -import software.amazon.awssdk.services.s3.model.DeleteObjectRequest; public class ProfileGrpcServiceTest extends SimpleBaseGrpcTest { - private static final String S3_BUCKET = "profileBucket"; - private static final String VERSION = "someVersion"; private static final byte[] VALID_NAME = new byte[81]; @@ -130,9 +126,6 @@ public class ProfileGrpcServiceTest extends SimpleBaseGrpcTest