From 5ec49d0d75039b09e8568af87ea943d99b2e2771 Mon Sep 17 00:00:00 2001 From: Tom Gianos Date: Wed, 8 May 2019 19:17:29 -0700 Subject: [PATCH] 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. --- .../com/netflix/genie/common/dto/Job.java | 40 ++++++++---------- .../netflix/genie/common/dto/JobRequest.java | 42 +++++++++---------- .../JobRestControllerIntegrationTest.java | 31 +++++++++++++- .../genie/web/jobs/workflow/impl/JobTask.java | 2 +- .../JpaJobPersistenceServiceImpl.java | 6 +-- 5 files changed, 69 insertions(+), 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 index 21242743253..a9210cbecd9 100644 --- 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 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 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 getFinished() { */ public static class Builder extends CommonDTO.Builder { - private final List 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 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 index 10a3f6360b0..09656572813 100644 --- 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 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 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 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 getGroupingInstance() { */ public static class Builder extends ExecutionEnvironmentDTO.Builder { - private final List bCommandArgs; private final List bClusterCriterias = new ArrayList<>(); private final Set bCommandCriteria = new HashSet<>(); private final List 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 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 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. *

* 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 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-web/src/integTest/java/com/netflix/genie/web/controllers/JobRestControllerIntegrationTest.java b/genie-web/src/integTest/java/com/netflix/genie/web/controllers/JobRestControllerIntegrationTest.java index f795357c8e8..7759c8c0858 100644 --- a/genie-web/src/integTest/java/com/netflix/genie/web/controllers/JobRestControllerIntegrationTest.java +++ b/genie-web/src/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 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 attachments + @Nullable final List 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 index 03ee56672fc..a210c655e80 100644 --- 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 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 index 719d0c09658..f225fa8d671 100644 --- 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())