Skip to content

Commit

Permalink
return explicit Response rather than Void from async controllers with…
Browse files Browse the repository at this point in the history
… no expected body content
  • Loading branch information
jkt-signal authored Nov 15, 2023
1 parent d4ef2ad commit 7764185
Show file tree
Hide file tree
Showing 8 changed files with 42 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -274,9 +274,9 @@ private AccountIdentityResponse buildAccountIdentityResponse(AccountAndAuthentic
)
@ApiResponse(responseCode = "204", description = "Username successfully deleted.", useReturnTypeSchema = true)
@ApiResponse(responseCode = "401", description = "Account authentication check failed.")
public CompletableFuture<Void> deleteUsernameHash(@Auth final AuthenticatedAccount auth) {
public CompletableFuture<Response> deleteUsernameHash(@Auth final AuthenticatedAccount auth) {
return accounts.clearUsernameHash(auth.getAccount())
.thenRun(Util.NOOP);
.thenApply(Util.ASYNC_EMPTY_RESPONSE);
}

@PUT
Expand Down Expand Up @@ -518,8 +518,8 @@ public Response accountExists(

@DELETE
@Path("/me")
public CompletableFuture<Void> deleteAccount(@Auth DisabledPermittedAuthenticatedAccount auth) throws InterruptedException {
return accounts.delete(auth.getAccount(), AccountsManager.DeletionReason.USER_REQUEST);
public CompletableFuture<Response> deleteAccount(@Auth DisabledPermittedAuthenticatedAccount auth) throws InterruptedException {
return accounts.delete(auth.getAccount(), AccountsManager.DeletionReason.USER_REQUEST).thenApply(Util.ASYNC_EMPTY_RESPONSE);
}

private void clearUsernameLink(final Account account) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ public CompletableFuture<PreKeyCount> getStatus(@Auth final AuthenticatedAccount
@ApiResponse(responseCode = "401", description = "Account authentication check failed.")
@ApiResponse(responseCode = "403", description = "Attempt to change identity key from a non-primary device.")
@ApiResponse(responseCode = "422", description = "Invalid request format.")
public CompletableFuture<Void> setKeys(@Auth final DisabledPermittedAuthenticatedAccount disabledPermittedAuth,
public CompletableFuture<Response> setKeys(@Auth final DisabledPermittedAuthenticatedAccount disabledPermittedAuth,
@RequestBody @NotNull @Valid final PreKeyState preKeys,

@Parameter(allowEmptyValue=true)
Expand Down Expand Up @@ -189,7 +189,8 @@ public CompletableFuture<Void> setKeys(@Auth final DisabledPermittedAuthenticate
}

return keys.store(getIdentifier(account, identityType), device.getId(),
preKeys.getPreKeys(), preKeys.getPqPreKeys(), preKeys.getSignedPreKey(), preKeys.getPqLastResortPreKey());
preKeys.getPreKeys(), preKeys.getPqPreKeys(), preKeys.getSignedPreKey(), preKeys.getPqLastResortPreKey())
.thenApply(Util.ASYNC_EMPTY_RESPONSE);
}

@GET
Expand Down Expand Up @@ -299,7 +300,7 @@ public PreKeyResponse getDeviceKeys(@Auth Optional<AuthenticatedAccount> auth,
@ApiResponse(responseCode = "200", description = "Indicates that new prekey was successfully stored.")
@ApiResponse(responseCode = "401", description = "Account authentication check failed.")
@ApiResponse(responseCode = "422", description = "Invalid request format.")
public CompletableFuture<Void> setSignedKey(@Auth final AuthenticatedAccount auth,
public CompletableFuture<Response> setSignedKey(@Auth final AuthenticatedAccount auth,
@Valid final ECSignedPreKey signedPreKey,
@QueryParam("identity") final Optional<String> identityType) {

Expand All @@ -314,7 +315,8 @@ public CompletableFuture<Void> setSignedKey(@Auth final AuthenticatedAccount aut
});

return keys.storeEcSignedPreKeys(getIdentifier(auth.getAccount(), identityType),
Map.of(device.getId(), signedPreKey));
Map.of(device.getId(), signedPreKey))
.thenApply(Util.ASYNC_EMPTY_RESPONSE);
}

private static boolean usePhoneNumberIdentity(final Optional<String> identityType) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -581,7 +581,7 @@ private static long estimateMessageListSizeBytes(final OutgoingMessageEntityList
@Timed
@DELETE
@Path("/uuid/{uuid}")
public CompletableFuture<Void> removePendingMessage(@Auth AuthenticatedAccount auth, @PathParam("uuid") UUID uuid) {
public CompletableFuture<Response> removePendingMessage(@Auth AuthenticatedAccount auth, @PathParam("uuid") UUID uuid) {
return messagesManager.delete(
auth.getAccount().getUuid(),
auth.getAuthenticatedDevice().getId(),
Expand All @@ -603,7 +603,8 @@ public CompletableFuture<Void> removePendingMessage(@Auth AuthenticatedAccount a
}
}
});
});
})
.thenApply(Util.ASYNC_EMPTY_RESPONSE);
}

@Timed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,12 @@
import java.util.Locale.LanguageRange;
import java.util.Optional;
import java.util.concurrent.TimeUnit;
import java.util.function.Function;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import javax.ws.rs.core.Response;

import org.apache.commons.lang3.StringUtils;

public class Util {
Expand All @@ -28,6 +32,12 @@ public class Util {

public static final Runnable NOOP = () -> {};

// Use `CompletableFuture#thenApply(ASYNC_EMPTY_RESPONSE) to convert futures to
// CompletableFuture<Response> instead of using NOOP to convert them to CompletableFuture<Void>
// for jersey controllers; https://github.com/eclipse-ee4j/jersey/issues/3901 causes controllers
// returning Void futures to behave differently than synchronous controllers returning void
public static final Function<Object, Response> ASYNC_EMPTY_RESPONSE = ignored -> Response.noContent().build();

/**
* Checks that the given number is a valid, E164-normalized phone number.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import java.util.Optional;
import java.util.UUID;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.TimeUnit;
import java.util.stream.Stream;
import javax.ws.rs.client.Entity;
import javax.ws.rs.client.Invocation;
Expand Down Expand Up @@ -90,6 +91,7 @@
import org.whispersystems.textsecuregcm.storage.UsernameReservationNotFoundException;
import org.whispersystems.textsecuregcm.tests.util.AccountsHelper;
import org.whispersystems.textsecuregcm.tests.util.AuthHelper;
import org.whispersystems.textsecuregcm.util.CompletableFutureTestUtil;
import org.whispersystems.textsecuregcm.util.MockUtils;
import org.whispersystems.textsecuregcm.util.SystemMapper;
import org.whispersystems.textsecuregcm.util.UsernameHashZkProofVerifier;
Expand Down Expand Up @@ -737,7 +739,7 @@ void testCommitUsernameHashWithInvalidProof() throws BaseUsernameException {
@Test
void testDeleteUsername() {
when(accountsManager.clearUsernameHash(any()))
.thenAnswer(invocation -> CompletableFuture.completedFuture(invocation.getArgument(0)));
.thenAnswer(invocation -> CompletableFutureTestUtil.almostCompletedFuture(invocation.getArgument(0)));

Response response =
resources.getJerseyTest()
Expand All @@ -746,6 +748,7 @@ void testDeleteUsername() {
.header(HttpHeaders.AUTHORIZATION, AuthHelper.getAuthHeader(AuthHelper.VALID_UUID, AuthHelper.VALID_PASSWORD))
.delete();

assertThat(response.readEntity(String.class)).isEqualTo("");
assertThat(response.getStatus()).isEqualTo(204);
verify(accountsManager).clearUsernameHash(AuthHelper.VALID_ACCOUNT);
}
Expand Down Expand Up @@ -828,6 +831,8 @@ void testSetAccountAttributesBadUnidentifiedKeyLength() {

@Test
void testDeleteAccount() {
when(accountsManager.delete(any(), any())).thenReturn(CompletableFutureTestUtil.almostCompletedFuture(null));

Response response =
resources.getJerseyTest()
.target("/v1/accounts/me")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@
import org.whispersystems.textsecuregcm.tests.util.AuthHelper;
import org.whispersystems.textsecuregcm.tests.util.KeysHelper;
import org.whispersystems.textsecuregcm.util.ByteArrayAdapter;
import org.whispersystems.textsecuregcm.util.CompletableFutureTestUtil;

@ExtendWith(DropwizardExtensionsSupport.class)
class KeysControllerTest {
Expand Down Expand Up @@ -235,9 +236,9 @@ void setup() {

when(rateLimiters.getPreKeysLimiter()).thenReturn(rateLimiter);

when(KEYS.store(any(), anyByte(), any(), any(), any(), any())).thenReturn(CompletableFuture.completedFuture(null));
when(KEYS.store(any(), anyByte(), any(), any(), any(), any())).thenReturn(CompletableFutureTestUtil.almostCompletedFuture(null));
when(KEYS.getEcSignedPreKey(any(), anyByte())).thenReturn(CompletableFuture.completedFuture(Optional.empty()));
when(KEYS.storeEcSignedPreKeys(any(), any())).thenReturn(CompletableFuture.completedFuture(null));
when(KEYS.storeEcSignedPreKeys(any(), any())).thenReturn(CompletableFutureTestUtil.almostCompletedFuture(null));

when(KEYS.takeEC(EXISTS_UUID, sampleDeviceId)).thenReturn(
CompletableFuture.completedFuture(Optional.of(SAMPLE_KEY)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@
import org.whispersystems.textsecuregcm.tests.util.AccountsHelper;
import org.whispersystems.textsecuregcm.tests.util.AuthHelper;
import org.whispersystems.textsecuregcm.tests.util.KeysHelper;
import org.whispersystems.textsecuregcm.util.CompletableFutureTestUtil;
import org.whispersystems.textsecuregcm.util.Pair;
import org.whispersystems.textsecuregcm.util.SystemMapper;
import org.whispersystems.textsecuregcm.util.UUIDUtil;
Expand Down Expand Up @@ -610,19 +611,19 @@ void testDeleteMessages() {
UUID uuid1 = UUID.randomUUID();
when(messagesManager.delete(AuthHelper.VALID_UUID, (byte) 1, uuid1, null))
.thenReturn(
CompletableFuture.completedFuture(Optional.of(generateEnvelope(uuid1, Envelope.Type.CIPHERTEXT_VALUE,
CompletableFutureTestUtil.almostCompletedFuture(Optional.of(generateEnvelope(uuid1, Envelope.Type.CIPHERTEXT_VALUE,
timestamp, sourceUuid, (byte) 1, AuthHelper.VALID_UUID, null, "hi".getBytes(), 0))));

UUID uuid2 = UUID.randomUUID();
when(messagesManager.delete(AuthHelper.VALID_UUID, (byte) 1, uuid2, null))
.thenReturn(
CompletableFuture.completedFuture(Optional.of(generateEnvelope(
CompletableFutureTestUtil.almostCompletedFuture(Optional.of(generateEnvelope(
uuid2, Envelope.Type.SERVER_DELIVERY_RECEIPT_VALUE,
System.currentTimeMillis(), sourceUuid, (byte) 1, AuthHelper.VALID_UUID, null, null, 0))));

UUID uuid3 = UUID.randomUUID();
when(messagesManager.delete(AuthHelper.VALID_UUID, (byte) 1, uuid3, null))
.thenReturn(CompletableFuture.completedFuture(Optional.empty()));
.thenReturn(CompletableFutureTestUtil.almostCompletedFuture(Optional.empty()));

UUID uuid4 = UUID.randomUUID();
when(messagesManager.delete(AuthHelper.VALID_UUID, (byte) 1, uuid4, null))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionException;
import java.util.concurrent.TimeUnit;

public class CompletableFutureTestUtil {

Expand All @@ -24,4 +25,9 @@ public static <T extends Throwable> void assertFailsWithCause(final Class<T> exp
final CompletionException completionException = assertThrows(CompletionException.class, completableFuture::join, message);
assertTrue(ExceptionUtils.unwrap(completionException).getClass().isAssignableFrom(expectedCause), message);
}

public static <T> CompletableFuture<T> almostCompletedFuture(T result) {
return new CompletableFuture<T>().completeOnTimeout(result, 5, TimeUnit.MILLISECONDS);
}

}

0 comments on commit 7764185

Please sign in to comment.