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

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<String> 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<String, Object> 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<String, ?> 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,
+        MediaType.FORM_DATA,
-        new byte[0]);
+        requestBody.getBytes());
 
     return extractServerResponse(response);
   }

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 org.mockito.ArgumentCaptor;
-import java.nio.file.Paths;
+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<T> {
+
+  @Captor
+  ArgumentCaptor<ImmutableMap<String, String>> urlParamCaptor;
+
+  @Captor
+  ArgumentCaptor<byte[]> requestBodyCaptor;
 
   static String generateInputData(String premiumTermsPath) throws Exception {
-    Path inputFile = Paths.get(premiumTermsPath);
+    return Files.toString(new File(premiumTermsPath), StandardCharsets.UTF_8);
-    String data = new String(java.nio.file.Files.readAllBytes(inputFile));
-    return data;
   }
 
-  static void verifySentParams(
+  void verifySentParams(
       Connection connection, String path, ImmutableMap<String, String> parameterMap)
           throws Exception {
     verify(connection).send(
         eq(path),
-        eq(parameterMap),
+        urlParamCaptor.capture(),
-        eq(MediaType.PLAIN_TEXT_UTF_8),
+        eq(MediaType.FORM_DATA),
-        eq(new byte[0]));
+        requestBodyCaptor.capture());
+    assertThat(new ImmutableMap.Builder<String, String>()
+        .putAll(urlParamCaptor.getValue())
+        .putAll(UriParameters.parse(new String(requestBodyCaptor.getValue(), UTF_8)).entries())
+        .build())
+            .containsExactlyEntriesIn(parameterMap);
   }
 }

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<C extends CreatePremiumListCommand>
-    extends CommandTestCase<C> {
+    extends CreateOrUpdatePremiumListCommandTestCase<C> {
 
   @Mock
   Connection connection;
@@ -50,25 +48,39 @@ public class CreatePremiumListCommandTest<C extends CreatePremiumListCommand>
   @Before
   public void init() throws Exception {
     command.setConnection(connection);
-    premiumTermsPath = writeToTmpFile(readResourceUtf8(
+    premiumTermsPath = writeToNamedTmpFile(
-        CreatePremiumListCommandTest.class,
+        "example_premium_terms.csv",
-        "testdata/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<String, String> 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<C extends CreatePremiumListCommand>
     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");
   }

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<C extends UpdatePremiumListCommand>
-    extends CommandTestCase<C> {
+    extends CreateOrUpdatePremiumListCommandTestCase<C> {
 
   @Mock
   Connection connection;
@@ -47,22 +45,36 @@ public class UpdatePremiumListCommandTest<C extends UpdatePremiumListCommand>
   public void init() throws Exception {
     command.setConnection(connection);
     servletPath = "/_dr/admin/updatePremiumList";
-    premiumTermsPath = writeToTmpFile(readResourceUtf8(
+    premiumTermsPath = writeToNamedTmpFile(
-        UpdatePremiumListCommandTest.class,
+        "example_premium_terms.csv",
-        "testdata/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<String, String> 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)));
   }
 }