Revert command arg functionality to a variation of V3 for API

This change makes the job request and job objects behave pretty much as they did in V3 with the caveat that they limit the number of characters total in the command arguments to 10,000 and throws a precondition exception if that is exceeded.
KRAhn · May 9, 2019 · 5ec49d0 · 5ec49d0
1 parent 8829cce
commit 5ec49d0
Show file tree

Hide file tree

Showing 5 changed files with 69 additions and 52 deletions.
diff --git a/genie-common/src/main/java/com/netflix/genie/common/dto/Job.java b/genie-common/src/main/java/com/netflix/genie/common/dto/Job.java
@@ -22,8 +22,6 @@
 import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
 import com.fasterxml.jackson.databind.annotation.JsonSerialize;
 import com.fasterxml.jackson.databind.ser.std.ToStringSerializer;
-import com.google.common.collect.ImmutableList;
-import com.google.common.collect.Lists;
 import com.netflix.genie.common.util.TimeUtils;
 import lombok.Getter;
 import org.apache.commons.lang3.StringUtils;
@@ -63,7 +61,8 @@ public class Job extends CommonDTO {
     @NotNull
     @JsonSerialize(using = ToStringSerializer.class)
     private final Duration runtime;
-    private final List<String> commandArgs;
+    @Size(max = 10_000, message = "The maximum number of characters for the command arguments is 10,000")
+    private final String commandArgs;
     private final String grouping;
     private final String groupingInstance;
 
@@ -74,7 +73,7 @@ public class Job extends CommonDTO {
      */
     protected Job(@Valid final Builder builder) {
         super(builder);
-        this.commandArgs = ImmutableList.copyOf(builder.bCommandArgs);
+        this.commandArgs = builder.bCommandArgs;
         this.status = builder.bStatus;
         this.statusMsg = builder.bStatusMsg;
         this.started = builder.bStarted;
@@ -94,9 +93,7 @@ protected Job(@Valid final Builder builder) {
      * @return The command arguments
      */
     public Optional<String> getCommandArgs() {
-        return this.commandArgs.isEmpty()
-            ? Optional.empty()
-            : Optional.ofNullable(StringUtils.join(this.commandArgs, StringUtils.SPACE));
+        return Optional.ofNullable(this.commandArgs);
     }
 
     /**
@@ -181,7 +178,7 @@ public Optional<Instant> getFinished() {
      */
     public static class Builder extends CommonDTO.Builder<Builder> {
 
-        private final List<String> bCommandArgs;
+        private String bCommandArgs;
         private JobStatus bStatus = JobStatus.INIT;
         private String bStatusMsg;
         private Instant bStarted;
@@ -207,7 +204,6 @@ public Builder(
             @JsonProperty("version") final String version
         ) {
             super(name, user, version);
-            this.bCommandArgs = Lists.newArrayList();
         }
 
         /**
@@ -218,7 +214,7 @@ public Builder(
          * @param name        The name to use for the Job
          * @param user        The user to use for the Job
          * @param version     The version to use for the Job
-         * @param commandArgs The command arguments used for this job
+         * @param commandArgs The command arguments used for this job. Max length 10,000 characters
          * @see #Builder(String, String, String)
          */
         @Deprecated
@@ -229,9 +225,7 @@ public Builder(
             @Nullable final String commandArgs
         ) {
             super(name, user, version);
-            this.bCommandArgs = commandArgs == null
-                ? Lists.newArrayList()
-                : Lists.newArrayList(commandArgs);
+            this.bCommandArgs = StringUtils.isBlank(commandArgs) ? null : commandArgs;
         }
 
         /**
@@ -240,31 +234,33 @@ public Builder(
          * DEPRECATED: This API will be removed in 4.0.0 in favor of the List based method for improved control over
          * escaping of arguments.
          *
-         * @param commandArgs The command args
+         * @param commandArgs The command args. The max length is 10,000 characters
          * @return The builder
-         * @see #withCommandArgs(List)
          * @since 3.3.0
+         * @deprecated See {@link #withCommandArgs(List)}
          */
         @Deprecated
         public Builder withCommandArgs(@Nullable final String commandArgs) {
-            this.bCommandArgs.clear();
-            if (commandArgs != null) {
-                this.bCommandArgs.add(commandArgs);
-            }
+            this.bCommandArgs = StringUtils.isBlank(commandArgs) ? null : commandArgs;
             return this;
         }
 
         /**
          * The command arguments to use in conjunction with the command executable selected for this job.
          *
-         * @param commandArgs The command args
+         * @param commandArgs The command args. The maximum combined size of the command args plus 1 space character
+         *                    between each argument must be <= 10,000 characters
          * @return The builder
          * @since 3.3.0
          */
         public Builder withCommandArgs(@Nullable final List<String> commandArgs) {
-            this.bCommandArgs.clear();
             if (commandArgs != null) {
-                this.bCommandArgs.addAll(commandArgs);
+                this.bCommandArgs = StringUtils.join(commandArgs, StringUtils.SPACE);
+                if (StringUtils.isBlank(this.bCommandArgs)) {
+                    this.bCommandArgs = null;
+                }
+            } else {
+                this.bCommandArgs = null;
             }
             return this;
         }

diff --git a/genie-common/src/main/java/com/netflix/genie/common/dto/JobRequest.java b/genie-common/src/main/java/com/netflix/genie/common/dto/JobRequest.java
@@ -22,7 +22,6 @@
 import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Lists;
 import lombok.Getter;
 import org.apache.commons.lang3.StringUtils;
 
@@ -55,7 +54,8 @@ public class JobRequest extends ExecutionEnvironmentDTO {
 
     private static final long serialVersionUID = 3163971970144435277L;
 
-    private final List<String> commandArgs;
+    @Size(max = 10_000, message = "The maximum number of characters for the command arguments is 10,000")
+    private final String commandArgs;
     @Valid
     @NotEmpty(message = "At least one cluster criteria is required")
     private final List<ClusterCriteria> clusterCriterias;
@@ -82,10 +82,9 @@ public class JobRequest extends ExecutionEnvironmentDTO {
      *
      * @param builder The builder to use
      */
-    @SuppressWarnings("unchecked")
     JobRequest(@Valid final Builder builder) {
         super(builder);
-        this.commandArgs = ImmutableList.copyOf(builder.bCommandArgs);
+        this.commandArgs = builder.bCommandArgs;
         this.clusterCriterias = ImmutableList.copyOf(builder.bClusterCriterias);
         this.commandCriteria = ImmutableSet.copyOf(builder.bCommandCriteria);
         this.group = builder.bGroup;
@@ -105,9 +104,7 @@ public class JobRequest extends ExecutionEnvironmentDTO {
      * @return The command arguments
      */
     public Optional<String> getCommandArgs() {
-        return this.commandArgs.isEmpty()
-            ? Optional.empty()
-            : Optional.ofNullable(StringUtils.join(this.commandArgs, StringUtils.SPACE));
+        return Optional.ofNullable(this.commandArgs);
     }
 
     /**
@@ -183,10 +180,10 @@ public Optional<String> getGroupingInstance() {
      */
     public static class Builder extends ExecutionEnvironmentDTO.Builder<Builder> {
 
-        private final List<String> bCommandArgs;
         private final List<ClusterCriteria> bClusterCriterias = new ArrayList<>();
         private final Set<String> bCommandCriteria = new HashSet<>();
         private final List<String> bApplications = new ArrayList<>();
+        private String bCommandArgs;
         private String bGroup;
         private boolean bDisableLogArchival;
         private String bEmail;
@@ -215,7 +212,6 @@ public Builder(
             @JsonProperty("commandCriteria") final Set<String> commandCriteria
         ) {
             super(name, user, version);
-            this.bCommandArgs = Lists.newArrayList();
             this.bClusterCriterias.addAll(clusterCriterias);
             commandCriteria.forEach(
                 criteria -> {
@@ -237,7 +233,7 @@ public Builder(
          * @param commandArgs      The command line arguments for the Job
          * @param clusterCriterias The list of cluster criteria for the Job
          * @param commandCriteria  The list of command criteria for the Job
-         * @see #Builder(String, String, String, List, Set)
+         * @deprecated Use {@link #Builder(String, String, String, List, Set)}
          */
         @Deprecated
         public Builder(
@@ -249,9 +245,7 @@ public Builder(
             final Set<String> commandCriteria
         ) {
             super(name, user, version);
-            this.bCommandArgs = commandArgs == null
-                ? Lists.newArrayList()
-                : Lists.newArrayList(commandArgs);
+            this.bCommandArgs = StringUtils.isBlank(commandArgs) ? null : commandArgs;
             this.bClusterCriterias.addAll(clusterCriterias);
             commandCriteria.forEach(
                 criteria -> {
@@ -263,36 +257,38 @@ public Builder(
         }
 
         /**
-         * The command arguments to use in conjunction with the command executable selected for this job.
+         * The command arguments to use in conjunction witOh the command executable selected for this job.
          * <p>
          * DEPRECATED: This API will be removed in 4.0.0 in favor of the List based method for improved control over
          * escaping of arguments.
          *
-         * @param commandArgs The command args
+         * @param commandArgs The command args. Max length is 10,000 characters
          * @return The builder
-         * @see #withCommandArgs(List)
          * @since 3.3.0
+         * @deprecated Use {@link #withCommandArgs(List)}
          */
         @Deprecated
         public Builder withCommandArgs(@Nullable final String commandArgs) {
-            this.bCommandArgs.clear();
-            if (commandArgs != null) {
-                this.bCommandArgs.add(commandArgs);
-            }
+            this.bCommandArgs = StringUtils.isBlank(commandArgs) ? null : commandArgs;
             return this;
         }
 
         /**
          * The command arguments to use in conjunction with the command executable selected for this job.
          *
-         * @param commandArgs The command args
+         * @param commandArgs The command args. The maximum combined length of the command args plus one space between
+         *                    each argument must be <= 10,000 characters
          * @return The builder
          * @since 3.3.0
          */
         public Builder withCommandArgs(@Nullable final List<String> commandArgs) {
-            this.bCommandArgs.clear();
             if (commandArgs != null) {
-                this.bCommandArgs.addAll(commandArgs);
+                this.bCommandArgs = StringUtils.join(commandArgs, StringUtils.SPACE);
+                if (StringUtils.isBlank(this.bCommandArgs)) {
+                    this.bCommandArgs = null;
+                }
+            } else {
+                this.bCommandArgs = null;
             }
             return this;
         }

diff --git a/...rc/integTest/java/com/netflix/genie/web/controllers/JobRestControllerIntegrationTest.java b/...rc/integTest/java/com/netflix/genie/web/controllers/JobRestControllerIntegrationTest.java
@@ -65,6 +65,7 @@
 import org.springframework.restdocs.restassured3.RestAssuredRestDocumentation;
 import org.springframework.restdocs.restassured3.RestDocumentationFilter;
 
+import javax.annotation.Nullable;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.InputStream;
@@ -245,6 +246,34 @@ public void testSubmitJobMethodSuccess() throws Exception {
         this.submitAndCheckJob(1, true);
     }
 
+    /**
+     * Test to make sure command args are limitted to 10,000 characters.
+     *
+     * @throws Exception On error
+     */
+    @Test
+    public void testForTooManyCommandArgs() throws Exception {
+        final JobRequest tooManyCommandArguments = new JobRequest.Builder(
+            JOB_NAME,
+            JOB_USER,
+            JOB_VERSION,
+            Lists.newArrayList(new ClusterCriteria(Sets.newHashSet(LOCALHOST_CLUSTER_TAG))),
+            Sets.newHashSet(BASH_COMMAND_TAG)
+        )
+            .withCommandArgs(StringUtils.leftPad("bad", 10_001))
+            .build();
+
+        RestAssured
+            .given(this.getRequestSpecification())
+            .contentType(MediaType.APPLICATION_JSON_VALUE)
+            .body(GenieObjectMapper.getMapper().writeValueAsBytes(tooManyCommandArguments))
+            .when()
+            .port(this.port)
+            .post(JOBS_API)
+            .then()
+            .statusCode(Matchers.is(HttpStatus.PRECONDITION_FAILED.value()));
+    }
+
     private void submitAndCheckJob(final int documentationId, final boolean archiveJob) throws Exception {
         Assume.assumeTrue(SystemUtils.IS_OS_UNIX);
         final List<String> commandArgs = Lists.newArrayList("-c", "'echo hello world'");
@@ -341,7 +370,7 @@ private void submitAndCheckJob(final int documentationId, final boolean archiveJ
     private String submitJob(
         final int documentationId,
         final JobRequest jobRequest,
-        final List<MockMultipartFile> attachments
+        @Nullable final List<MockMultipartFile> attachments
     ) throws Exception {
         if (attachments != null) {
             final RestDocumentationFilter createResultFilter = RestAssuredRestDocumentation.document(

diff --git a/genie-web/src/main/java/com/netflix/genie/web/jobs/workflow/impl/JobTask.java b/genie-web/src/main/java/com/netflix/genie/web/jobs/workflow/impl/JobTask.java
@@ -145,7 +145,7 @@ public void executeTask(@NotNull final Map<String, Object> context) throws Genie
                 + System.lineSeparator());
 
             writer.write(
-                StringUtils.join(jobExecEnv.getCommand().getExecutable(), ' ')
+                StringUtils.join(jobExecEnv.getCommand().getExecutable(), StringUtils.SPACE)
                     + JobConstants.WHITE_SPACE
                     + jobExecEnv.getJobRequest().getCommandArgs().orElse(EMPTY_STRING)
                     + JobConstants.STDOUT_REDIRECT

diff --git a/genie-web/src/main/java/com/netflix/genie/web/jpa/services/JpaJobPersistenceServiceImpl.java b/genie-web/src/main/java/com/netflix/genie/web/jpa/services/JpaJobPersistenceServiceImpl.java
@@ -66,7 +66,6 @@
 import com.netflix.genie.web.jpa.repositories.JpaJobRepository;
 import com.netflix.genie.web.services.JobPersistenceService;
 import lombok.extern.slf4j.Slf4j;
-import org.apache.commons.lang3.StringUtils;
 import org.springframework.dao.DataIntegrityViolationException;
 import org.springframework.data.domain.PageRequest;
 import org.springframework.data.domain.Pageable;
@@ -670,10 +669,7 @@ private JobEntity toEntity(
             .getMetadata()
             .ifPresent(metadata -> EntityDtoConverters.setJsonField(metadata, jobEntity::setMetadata));
         JpaServiceUtils.setEntityMetadata(GenieObjectMapper.getMapper(), jobRequest, jobEntity);
-        jobRequest.getCommandArgs().ifPresent(
-            commandArgs ->
-                jobEntity.setCommandArgs(Lists.newArrayList(StringUtils.split(commandArgs)))
-        );
+        jobRequest.getCommandArgs().ifPresent(commandArgs -> jobEntity.setCommandArgs(Lists.newArrayList(commandArgs)));
         jobRequest.getGroup().ifPresent(jobEntity::setGenieUserGroup);
         final FileEntity setupFile = jobRequest.getSetupFile().isPresent()
             ? this.createAndGetFileEntity(jobRequest.getSetupFile().get())