From f20b1d89a9c5e85afe8cd6a3c7ef5839385875b0 Mon Sep 17 00:00:00 2001 From: nickfelt Date: Thu, 14 Apr 2016 18:05:56 -0700 Subject: [PATCH] Fix premium list command regressions This CL fixes bugs introduced when the premium list commands were moved to be server-side commands. Fixes: - omitting the --name parameter actually works now; before it was failing in the local build() call since it uses the name as the @Id and tries to create a key for PremiumListRevision, but key IDs cannot be null. - premium list data larger than about 2K works now (see [] before it was stuffing the list data all into a POST query parameter, and URLs of more than 2K in length were all getting 404s. Fix was changing it to put the inputData param in the POST body. - misc test cleanup ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=119912494 --- .../CreateOrUpdatePremiumListCommand.java | 14 +++++-- ...ateOrUpdatePremiumListCommandTestCase.java | 37 +++++++++++++----- .../tools/CreatePremiumListCommandTest.java | 39 ++++++++++++------- .../tools/UpdatePremiumListCommandTest.java | 34 ++++++++++------ 4 files changed, 86 insertions(+), 38 deletions(-) diff --git a/java/com/google/domain/registry/tools/CreateOrUpdatePremiumListCommand.java b/java/com/google/domain/registry/tools/CreateOrUpdatePremiumListCommand.java index 0ed26c32e..d81e08ab8 100644 --- a/java/com/google/domain/registry/tools/CreateOrUpdatePremiumListCommand.java +++ b/java/com/google/domain/registry/tools/CreateOrUpdatePremiumListCommand.java @@ -21,6 +21,7 @@ import static com.google.domain.registry.tools.server.CreateOrUpdatePremiumListA import static com.google.domain.registry.util.ListNamingUtils.convertFilePathToName; import static java.nio.charset.StandardCharsets.UTF_8; +import com.google.common.base.Joiner; import com.google.common.base.Verify; import com.google.common.collect.ImmutableMap; import com.google.common.net.MediaType; @@ -31,6 +32,7 @@ import com.beust.jcommander.Parameter; import org.json.simple.JSONValue; +import java.net.URLEncoder; import java.nio.file.Files; import java.nio.file.Path; import java.util.List; @@ -75,7 +77,9 @@ abstract class CreateOrUpdatePremiumListCommand extends ConfirmingCommand @Override protected void init() throws Exception { + name = isNullOrEmpty(name) ? convertFilePathToName(inputFile) : name; List lines = Files.readAllLines(inputFile, UTF_8); + // Try constructing the premium list locally to check up front for validation errors. new PremiumList.Builder() .setName(name) .setPremiumListMapFromLines(lines) @@ -91,10 +95,12 @@ abstract class CreateOrUpdatePremiumListCommand extends ConfirmingCommand @Override public String execute() throws Exception { - name = isNullOrEmpty(name) ? convertFilePathToName(inputFile) : name; ImmutableMap.Builder params = new ImmutableMap.Builder<>(); params.put(NAME_PARAM, name); - params.put(INPUT_PARAM, new String(Files.readAllBytes(inputFile), UTF_8)); + String inputFileContents = new String(Files.readAllBytes(inputFile), UTF_8); + String requestBody = + Joiner.on('&').withKeyValueSeparator("=").join( + ImmutableMap.of(INPUT_PARAM, URLEncoder.encode(inputFileContents, UTF_8.toString()))); ImmutableMap extraParams = getParameterMap(); if (extraParams != null) { @@ -105,8 +111,8 @@ abstract class CreateOrUpdatePremiumListCommand extends ConfirmingCommand String response = connection.send( getCommandPath(), params.build(), - MediaType.PLAIN_TEXT_UTF_8, - new byte[0]); + MediaType.FORM_DATA, + requestBody.getBytes()); return extractServerResponse(response); } diff --git a/javatests/com/google/domain/registry/tools/CreateOrUpdatePremiumListCommandTestCase.java b/javatests/com/google/domain/registry/tools/CreateOrUpdatePremiumListCommandTestCase.java index d4d9158ae..fd3ae7812 100644 --- a/javatests/com/google/domain/registry/tools/CreateOrUpdatePremiumListCommandTestCase.java +++ b/javatests/com/google/domain/registry/tools/CreateOrUpdatePremiumListCommandTestCase.java @@ -14,34 +14,51 @@ package com.google.domain.registry.tools; +import static com.google.common.truth.Truth.assertThat; +import static java.nio.charset.StandardCharsets.UTF_8; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.verify; import com.google.common.collect.ImmutableMap; +import com.google.common.io.Files; import com.google.common.net.MediaType; +import com.google.domain.registry.testing.UriParameters; import com.google.domain.registry.tools.ServerSideCommand.Connection; -import java.nio.file.Path; -import java.nio.file.Paths; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; + +import java.io.File; +import java.nio.charset.StandardCharsets; /** * Base class for common testing setup for create and update commands for Premium Lists. */ -public final class CreateOrUpdatePremiumListCommandTestCase { +public abstract class CreateOrUpdatePremiumListCommandTestCase< + T extends CreateOrUpdatePremiumListCommand> extends CommandTestCase { + + @Captor + ArgumentCaptor> urlParamCaptor; + + @Captor + ArgumentCaptor requestBodyCaptor; static String generateInputData(String premiumTermsPath) throws Exception { - Path inputFile = Paths.get(premiumTermsPath); - String data = new String(java.nio.file.Files.readAllBytes(inputFile)); - return data; + return Files.toString(new File(premiumTermsPath), StandardCharsets.UTF_8); } - static void verifySentParams( + void verifySentParams( Connection connection, String path, ImmutableMap parameterMap) throws Exception { verify(connection).send( eq(path), - eq(parameterMap), - eq(MediaType.PLAIN_TEXT_UTF_8), - eq(new byte[0])); + urlParamCaptor.capture(), + eq(MediaType.FORM_DATA), + requestBodyCaptor.capture()); + assertThat(new ImmutableMap.Builder() + .putAll(urlParamCaptor.getValue()) + .putAll(UriParameters.parse(new String(requestBodyCaptor.getValue(), UTF_8)).entries()) + .build()) + .containsExactlyEntriesIn(parameterMap); } } diff --git a/javatests/com/google/domain/registry/tools/CreatePremiumListCommandTest.java b/javatests/com/google/domain/registry/tools/CreatePremiumListCommandTest.java index ee3fc5058..1e2cb7b31 100644 --- a/javatests/com/google/domain/registry/tools/CreatePremiumListCommandTest.java +++ b/javatests/com/google/domain/registry/tools/CreatePremiumListCommandTest.java @@ -15,8 +15,6 @@ package com.google.domain.registry.tools; import static com.google.domain.registry.request.JsonResponse.JSON_SAFETY_PREFIX; -import static com.google.domain.registry.tools.CreateOrUpdatePremiumListCommandTestCase.generateInputData; -import static com.google.domain.registry.tools.CreateOrUpdatePremiumListCommandTestCase.verifySentParams; import static com.google.domain.registry.util.ResourceUtils.readResourceUtf8; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyMapOf; @@ -38,7 +36,7 @@ import org.mockito.Mock; /** Unit tests for {@link CreatePremiumListCommand}. */ public class CreatePremiumListCommandTest - extends CommandTestCase { + extends CreateOrUpdatePremiumListCommandTestCase { @Mock Connection connection; @@ -50,25 +48,39 @@ public class CreatePremiumListCommandTest @Before public void init() throws Exception { command.setConnection(connection); - premiumTermsPath = writeToTmpFile(readResourceUtf8( - CreatePremiumListCommandTest.class, - "testdata/example_premium_terms.csv")); + premiumTermsPath = writeToNamedTmpFile( + "example_premium_terms.csv", + readResourceUtf8( + CreatePremiumListCommandTest.class, + "testdata/example_premium_terms.csv")); servletPath = "/_dr/admin/createPremiumList"; when(connection.send( eq(CreatePremiumListAction.PATH), anyMapOf(String.class, String.class), - eq(MediaType.PLAIN_TEXT_UTF_8), + any(MediaType.class), any(byte[].class))) - .thenReturn(JSON_SAFETY_PREFIX + "{\"status\":\"success\",\"lines\":[]}"); + .thenReturn(JSON_SAFETY_PREFIX + "{\"status\":\"success\",\"lines\":[]}"); } @Test public void testRun() throws Exception { - ImmutableMap params = - ImmutableMap.of("name", "foo", "inputData", generateInputData(premiumTermsPath)); runCommandForced("-i=" + premiumTermsPath, "-n=foo"); assertInStdout("Successfully"); - verifySentParams(connection, servletPath, params); + verifySentParams( + connection, + servletPath, + ImmutableMap.of("name", "foo", "inputData", generateInputData(premiumTermsPath))); + } + + @Test + public void testRun_noProvidedName_usesBasenameOfInputFile() throws Exception { + runCommandForced("-i=" + premiumTermsPath); + assertInStdout("Successfully"); + verifySentParams( + connection, + servletPath, + ImmutableMap.of( + "name", "example_premium_terms", "inputData", generateInputData(premiumTermsPath))); } @Test @@ -78,9 +90,10 @@ public class CreatePremiumListCommandTest when(connection.send( eq(CreatePremiumListAction.PATH), anyMapOf(String.class, String.class), - eq(MediaType.PLAIN_TEXT_UTF_8), + any(MediaType.class), any(byte[].class))) - .thenReturn(JSON_SAFETY_PREFIX + "{\"status\":\"error\",\"error\":\"foo already exists\"}"); + .thenReturn( + JSON_SAFETY_PREFIX + "{\"status\":\"error\",\"error\":\"foo already exists\"}"); thrown.expect(VerifyException.class, "Server error:"); runCommandForced("-i=" + premiumTermsPath, "-n=foo"); } diff --git a/javatests/com/google/domain/registry/tools/UpdatePremiumListCommandTest.java b/javatests/com/google/domain/registry/tools/UpdatePremiumListCommandTest.java index a69ddd7e9..ad52e5f6a 100644 --- a/javatests/com/google/domain/registry/tools/UpdatePremiumListCommandTest.java +++ b/javatests/com/google/domain/registry/tools/UpdatePremiumListCommandTest.java @@ -15,8 +15,6 @@ package com.google.domain.registry.tools; import static com.google.domain.registry.request.JsonResponse.JSON_SAFETY_PREFIX; -import static com.google.domain.registry.tools.CreateOrUpdatePremiumListCommandTestCase.generateInputData; -import static com.google.domain.registry.tools.CreateOrUpdatePremiumListCommandTestCase.verifySentParams; import static com.google.domain.registry.util.ResourceUtils.readResourceUtf8; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyMapOf; @@ -34,7 +32,7 @@ import org.mockito.Mock; /** Unit tests for {@link UpdatePremiumListCommand}. */ public class UpdatePremiumListCommandTest - extends CommandTestCase { + extends CreateOrUpdatePremiumListCommandTestCase { @Mock Connection connection; @@ -47,22 +45,36 @@ public class UpdatePremiumListCommandTest public void init() throws Exception { command.setConnection(connection); servletPath = "/_dr/admin/updatePremiumList"; - premiumTermsPath = writeToTmpFile(readResourceUtf8( - UpdatePremiumListCommandTest.class, - "testdata/example_premium_terms.csv")); + premiumTermsPath = writeToNamedTmpFile( + "example_premium_terms.csv", + readResourceUtf8( + UpdatePremiumListCommandTest.class, + "testdata/example_premium_terms.csv")); when(connection.send( eq(UpdatePremiumListAction.PATH), anyMapOf(String.class, String.class), - eq(MediaType.PLAIN_TEXT_UTF_8), + any(MediaType.class), any(byte[].class))) - .thenReturn(JSON_SAFETY_PREFIX + "{\"status\":\"success\",\"lines\":[]}"); + .thenReturn(JSON_SAFETY_PREFIX + "{\"status\":\"success\",\"lines\":[]}"); } @Test public void testRun() throws Exception { - ImmutableMap params = - ImmutableMap.of("name", "foo", "inputData", generateInputData(premiumTermsPath)); runCommandForced("-i=" + premiumTermsPath, "-n=foo"); - verifySentParams(connection, servletPath, params); + verifySentParams( + connection, + servletPath, + ImmutableMap.of("name", "foo", "inputData", generateInputData(premiumTermsPath))); + } + + @Test + public void testRun_noProvidedName_usesBasenameOfInputFile() throws Exception { + runCommandForced("-i=" + premiumTermsPath); + assertInStdout("Successfully"); + verifySentParams( + connection, + servletPath, + ImmutableMap.of( + "name", "example_premium_terms", "inputData", generateInputData(premiumTermsPath))); } }