From e1ad4d663c62b4bfb189acba4261ab7444c25802 Mon Sep 17 00:00:00 2001 From: mmuller Date: Tue, 27 Mar 2018 12:02:50 -0700 Subject: [PATCH] Remove Registrar caching from all console actions Caching turns out to be an anti-pattern for the console. If we use it, changes from the user just get obliterated by the older, cached version the next time the console refreshes (and it happens to refresh after every update). Caching is also not very useful here, as the amount of database access driven by the console is very small. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=190650931 --- .../ui/server/registrar/RegistrarPaymentAction.java | 2 +- .../ui/server/registrar/RegistrarPaymentSetupAction.java | 2 +- .../server/registrar/RegistrarPremiumPriceAckAction.java | 2 +- .../ui/server/registrar/RegistrarSettingsAction.java | 2 +- java/google/registry/ui/server/registrar/SessionUtils.java | 7 ++----- .../ui/server/registrar/RegistrarPaymentActionTest.java | 3 +-- .../server/registrar/RegistrarPaymentSetupActionTest.java | 5 ++--- .../ui/server/registrar/RegistrarSettingsActionTest.java | 3 +-- .../server/registrar/RegistrarSettingsActionTestCase.java | 2 +- .../registry/ui/server/registrar/SecuritySettingsTest.java | 2 +- 10 files changed, 12 insertions(+), 18 deletions(-) diff --git a/java/google/registry/ui/server/registrar/RegistrarPaymentAction.java b/java/google/registry/ui/server/registrar/RegistrarPaymentAction.java index ecf615750..4e6284bbd 100644 --- a/java/google/registry/ui/server/registrar/RegistrarPaymentAction.java +++ b/java/google/registry/ui/server/registrar/RegistrarPaymentAction.java @@ -157,7 +157,7 @@ public final class RegistrarPaymentAction implements Runnable, JsonAction { @Override public Map handleJsonRequest(Map json) { - Registrar registrar = sessionUtils.getRegistrarForAuthResult(request, authResult, false); + Registrar registrar = sessionUtils.getRegistrarForAuthResult(request, authResult); logger.infofmt("Processing payment: %s", json); String paymentMethodNonce; Money amount; diff --git a/java/google/registry/ui/server/registrar/RegistrarPaymentSetupAction.java b/java/google/registry/ui/server/registrar/RegistrarPaymentSetupAction.java index 6d735e240..3ccf791d4 100644 --- a/java/google/registry/ui/server/registrar/RegistrarPaymentSetupAction.java +++ b/java/google/registry/ui/server/registrar/RegistrarPaymentSetupAction.java @@ -91,7 +91,7 @@ public final class RegistrarPaymentSetupAction implements Runnable, JsonAction { @Override public Map handleJsonRequest(Map json) { - Registrar registrar = sessionUtils.getRegistrarForAuthResult(request, authResult, false); + Registrar registrar = sessionUtils.getRegistrarForAuthResult(request, authResult); if (!json.isEmpty()) { return JsonResponseHelper.create(ERROR, "JSON request object must be empty"); diff --git a/java/google/registry/ui/server/registrar/RegistrarPremiumPriceAckAction.java b/java/google/registry/ui/server/registrar/RegistrarPremiumPriceAckAction.java index e6feaa392..5079f4560 100644 --- a/java/google/registry/ui/server/registrar/RegistrarPremiumPriceAckAction.java +++ b/java/google/registry/ui/server/registrar/RegistrarPremiumPriceAckAction.java @@ -81,7 +81,7 @@ public class RegistrarPremiumPriceAckAction implements Runnable, JsonActionRunne } // Get the registrar - Registrar initialRegistrar = sessionUtils.getRegistrarForAuthResult(request, authResult, true); + Registrar initialRegistrar = sessionUtils.getRegistrarForAuthResult(request, authResult); // Process the operation. Though originally derived from a CRUD handler, registrar-settings // and registrar-premium-price-action really only support read and update. diff --git a/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java b/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java index ec10a763c..f24141ea8 100644 --- a/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java +++ b/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java @@ -93,7 +93,7 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA throw new BadRequestException("Malformed JSON"); } - Registrar initialRegistrar = sessionUtils.getRegistrarForAuthResult(request, authResult, false); + Registrar initialRegistrar = sessionUtils.getRegistrarForAuthResult(request, authResult); // Process the operation. Though originally derived from a CRUD // handler, registrar-settings really only supports read and update. String op = Optional.ofNullable((String) input.get(OP_PARAM)).orElse("read"); diff --git a/java/google/registry/ui/server/registrar/SessionUtils.java b/java/google/registry/ui/server/registrar/SessionUtils.java index cae043336..ceca85d35 100644 --- a/java/google/registry/ui/server/registrar/SessionUtils.java +++ b/java/google/registry/ui/server/registrar/SessionUtils.java @@ -59,8 +59,7 @@ public class SessionUtils { * the registrar console. */ @CheckReturnValue - Registrar getRegistrarForAuthResult( - HttpServletRequest request, AuthResult authResult, boolean bypassCache) { + Registrar getRegistrarForAuthResult(HttpServletRequest request, AuthResult authResult) { if (!authResult.userAuthInfo().isPresent()) { throw new ForbiddenException("Not logged in"); } @@ -69,9 +68,7 @@ public class SessionUtils { } String clientId = getRegistrarClientId(request); return checkArgumentPresent( - bypassCache - ? Registrar.loadByClientId(clientId) - : Registrar.loadByClientIdCached(clientId), + Registrar.loadByClientId(clientId), "Registrar %s not found", clientId); } diff --git a/javatests/google/registry/ui/server/registrar/RegistrarPaymentActionTest.java b/javatests/google/registry/ui/server/registrar/RegistrarPaymentActionTest.java index 0f68eaa86..73558cf21 100644 --- a/javatests/google/registry/ui/server/registrar/RegistrarPaymentActionTest.java +++ b/javatests/google/registry/ui/server/registrar/RegistrarPaymentActionTest.java @@ -19,7 +19,6 @@ import static google.registry.testing.DatastoreHelper.loadRegistrar; import static google.registry.testing.ReflectiveFieldExtractor.extractField; import static java.util.Arrays.asList; import static org.mockito.Matchers.any; -import static org.mockito.Matchers.anyBoolean; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -94,7 +93,7 @@ public class RegistrarPaymentActionTest { when(braintreeGateway.transaction()).thenReturn(transactionGateway); when(transactionGateway.sale(any(TransactionRequest.class))).thenReturn(result); when(sessionUtils.getRegistrarForAuthResult( - any(HttpServletRequest.class), any(AuthResult.class), anyBoolean())) + any(HttpServletRequest.class), any(AuthResult.class))) .thenReturn(loadRegistrar("TheRegistrar")); } diff --git a/javatests/google/registry/ui/server/registrar/RegistrarPaymentSetupActionTest.java b/javatests/google/registry/ui/server/registrar/RegistrarPaymentSetupActionTest.java index f393009b1..aad2ec8f0 100644 --- a/javatests/google/registry/ui/server/registrar/RegistrarPaymentSetupActionTest.java +++ b/javatests/google/registry/ui/server/registrar/RegistrarPaymentSetupActionTest.java @@ -19,7 +19,6 @@ import static google.registry.testing.DatastoreHelper.loadRegistrar; import static google.registry.testing.DatastoreHelper.persistResource; import static java.util.Arrays.asList; import static org.mockito.Matchers.any; -import static org.mockito.Matchers.anyBoolean; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -69,7 +68,7 @@ public class RegistrarPaymentSetupActionTest { .setBillingMethod(Registrar.BillingMethod.BRAINTREE) .build()); when(sessionUtils.getRegistrarForAuthResult( - any(HttpServletRequest.class), any(AuthResult.class), anyBoolean())) + any(HttpServletRequest.class), any(AuthResult.class))) .thenReturn(registrar); when(braintreeGateway.clientToken()).thenReturn(clientTokenGateway); } @@ -112,7 +111,7 @@ public class RegistrarPaymentSetupActionTest { .setBillingMethod(Registrar.BillingMethod.EXTERNAL) .build()); when(sessionUtils.getRegistrarForAuthResult( - any(HttpServletRequest.class), any(AuthResult.class), anyBoolean())) + any(HttpServletRequest.class), any(AuthResult.class))) .thenReturn(registrar); assertThat(action.handleJsonRequest(ImmutableMap.of())) .containsExactly( diff --git a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java index 0ced02779..b1a639883 100644 --- a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java +++ b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java @@ -22,7 +22,6 @@ import static google.registry.testing.TaskQueueHelper.assertTasksEnqueued; import static google.registry.util.ResourceUtils.readResourceUtf8; import static java.util.Arrays.asList; import static org.mockito.Matchers.any; -import static org.mockito.Matchers.anyBoolean; import static org.mockito.Matchers.anyInt; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; @@ -72,7 +71,7 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase @Test public void testRead_notAuthorized_failure() throws Exception { when(sessionUtils.getRegistrarForAuthResult( - any(HttpServletRequest.class), any(AuthResult.class), anyBoolean())) + any(HttpServletRequest.class), any(AuthResult.class))) .thenThrow(new ForbiddenException("Not authorized to access Registrar Console")); assertThrows(ForbiddenException.class, () -> action.handleJsonRequest(ImmutableMap.of())); assertNoTasksEnqueued("sheet"); diff --git a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java index 3db236aa4..433fc5fd5 100644 --- a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java +++ b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java @@ -106,7 +106,7 @@ public class RegistrarSettingsActionTestCase { when(rsp.getWriter()).thenReturn(new PrintWriter(writer)); when(req.getContentType()).thenReturn("application/json"); when(req.getReader()).thenReturn(createJsonPayload(ImmutableMap.of("op", "read"))); - when(sessionUtils.getRegistrarForAuthResult(req, action.authResult, false)) + when(sessionUtils.getRegistrarForAuthResult(req, action.authResult)) .thenReturn(loadRegistrar(CLIENT_ID)); when(modulesService.getVersionHostname("backend", null)).thenReturn("backend.hostname"); } diff --git a/javatests/google/registry/ui/server/registrar/SecuritySettingsTest.java b/javatests/google/registry/ui/server/registrar/SecuritySettingsTest.java index 2b5cb8b1c..12c59ae70 100644 --- a/javatests/google/registry/ui/server/registrar/SecuritySettingsTest.java +++ b/javatests/google/registry/ui/server/registrar/SecuritySettingsTest.java @@ -112,7 +112,7 @@ public class SecuritySettingsTest extends RegistrarSettingsActionTestCase { .setClientCertificate(SAMPLE_CERT, START_OF_TIME) .setFailoverClientCertificate(SAMPLE_CERT2, START_OF_TIME) .build()); - when(sessionUtils.getRegistrarForAuthResult(req, action.authResult, false)) + when(sessionUtils.getRegistrarForAuthResult(req, action.authResult)) .thenReturn(initialRegistrar); Map jsonMap = initialRegistrar.toJsonMap(); jsonMap.put("clientCertificate", null);