Skip to content

Commit

Permalink
Revert command arg functionality to a variation of V3 for API
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@tgianos @mprimi
tgianos authored and mprimi committed May 9, 2019
1 parent 8829cce commit 5ec49d0
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 52 deletions.
40 changes: 18 additions & 22 deletions genie-common/src/main/java/com/netflix/genie/common/dto/Job.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand All @@ -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;
Expand All @@ -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);
}

/**
Expand Down Expand Up @@ -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;
Expand All @@ -207,7 +204,6 @@ public Builder(
@JsonProperty("version") final String version
) {
super(name, user, version);
this.bCommandArgs = Lists.newArrayList();
}

/**
Expand All @@ -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
Expand All @@ -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;
}

/**
Expand All @@ -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;
}
Expand Down
42 changes: 19 additions & 23 deletions genie-common/src/main/java/com/netflix/genie/common/dto/JobRequest.java
View file
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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);
}

/**
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 -> {
Expand All @@ -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(
Expand All @@ -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 -> {
Expand All @@ -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;
}
Expand Down
31 changes: 30 additions & 1 deletion ...rc/integTest/java/com/netflix/genie/web/controllers/JobRestControllerIntegrationTest.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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'");
Expand Down Expand Up @@ -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(
Expand Down
2 changes: 1 addition & 1 deletion genie-web/src/main/java/com/netflix/genie/web/jobs/workflow/impl/JobTask.java
View file
Expand Up @@ -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
Expand Down
6 changes: 1 addition & 5 deletions genie-web/src/main/java/com/netflix/genie/web/jpa/services/JpaJobPersistenceServiceImpl.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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())
Expand Down

0 comments on commit 5ec49d0

Please sign in to comment.