From d014c7faf984a866753263f0a4031848a888f35f Mon Sep 17 00:00:00 2001 From: David Roazen Date: Mon, 28 Nov 2011 13:35:14 -0500 Subject: [PATCH] Queue now properly escapes all shell arguments in generated shell scripts This has implications for both Qscript authors and CommandLineFunction authors. Qscript authors: You no longer need to (and in fact must not) manually escape String values to avoid interpretation by the shell when setting up Walker parameters. Queue will safely escape all of your Strings for you so that they'll be interpreted literally. Eg., Old way: filterSNPs.filterExpression = List("\"QD<2.0\"", "\"MQ<40.0\"", "\"HaplotypeScore>13.0\"") New way: filterSNPs.filterExpression = List("QD<2.0", "MQ<40.0", "HaplotypeScore>13.0") CommandLineFunction authors: If you're writing a one-off CommandLineFunction in a Qscript and don't really care about quoting issues, just keep doing things the direct, simple way: def commandLine = "cat %s | grep -v \"#\" > %s".format(files, out) If you're writing a CommandLineFunction that will become part of Queue and will be used by other QScripts, however, it's advisable to do things the newer, safer way, ie.: When you construct your commandLine, you should do so ONLY using the API methods required(), optional(), conditional(), and repeat(). These will manage quoting and whitespace separation for you, so you shouldn't insert quotes/extraneous whitespace in your Strings. By default you get both (quoting and whitespace separation), but you can disable either of these via parameters. Eg., override def commandLine = super.commandLine + required("eff") + conditional(verbose, "-v") + optional("-c", config) + required("-i", "vcf") + required("-o", "vcf") + required(genomeVersion) + required(inVcf) + required(">", escape=false) + // This will be shell-interpreted required(outVcf) I've ported the Picard/Samtools/SnpEff CommandLineFunction classes to the new system, so you'll get free shell escaping when you use those in Qscripts just like with walkers. --- .../gatk/ArgumentDefinitionField.java | 20 +- .../gatk/GATKExtensionsGenerator.java | 3 + .../queue/extensions/gatk/RodBindField.java | 129 ----------- .../queue/qscripts/GATKResourcesBundle.scala | 2 +- .../MethodsDevelopmentCallingPipeline.scala | 4 +- .../examples/ExampleUnifiedGenotyper.scala | 2 +- .../sting/queue/extensions/gatk/RodBind.scala | 17 +- .../queue/extensions/gatk/TaggedFile.scala | 18 +- .../picard/AddOrReplaceReadGroups.scala | 14 +- .../extensions/picard/MarkDuplicates.scala | 10 +- .../extensions/picard/MergeSamFiles.scala | 8 +- .../extensions/picard/PicardBamFunction.scala | 19 +- .../queue/extensions/picard/ReorderSam.scala | 6 +- .../queue/extensions/picard/RevertSam.scala | 12 +- .../queue/extensions/picard/SamToFastq.scala | 26 +-- .../extensions/picard/ValidateSamFile.scala | 14 +- .../samtools/SamtoolsIndexFunction.scala | 5 +- .../samtools/SamtoolsMergeFunction.scala | 8 +- .../queue/extensions/snpeff/SnpEff.scala | 17 +- .../queue/function/CommandLineFunction.scala | 209 ++++++++++++++---- .../function/JavaCommandLineFunction.scala | 34 +-- .../sting/queue/util/ShellUtils.scala | 36 +++ .../CommandLineFunctionUnitTest.scala | 171 ++++++++++++++ .../sting/queue/util/ShellUtilsUnitTest.scala | 57 +++++ 24 files changed, 545 insertions(+), 296 deletions(-) delete mode 100644 public/java/src/org/broadinstitute/sting/queue/extensions/gatk/RodBindField.java create mode 100644 public/scala/src/org/broadinstitute/sting/queue/util/ShellUtils.scala create mode 100644 public/scala/test/org/broadinstitute/sting/queue/function/CommandLineFunctionUnitTest.scala create mode 100644 public/scala/test/org/broadinstitute/sting/queue/util/ShellUtilsUnitTest.scala diff --git a/public/java/src/org/broadinstitute/sting/queue/extensions/gatk/ArgumentDefinitionField.java b/public/java/src/org/broadinstitute/sting/queue/extensions/gatk/ArgumentDefinitionField.java index cb5bad4ae..cdfc329e8 100644 --- a/public/java/src/org/broadinstitute/sting/queue/extensions/gatk/ArgumentDefinitionField.java +++ b/public/java/src/org/broadinstitute/sting/queue/extensions/gatk/ArgumentDefinitionField.java @@ -83,10 +83,10 @@ public abstract class ArgumentDefinitionField extends ArgumentField { getShortFieldSetter()); } - protected static final String REQUIRED_TEMPLATE = " + \" %1$s \" + %2$s.format(%3$s)"; - protected static final String REPEAT_TEMPLATE = " + repeat(\" %1$s \", %3$s, format=formatValue(%2$s))"; - protected static final String OPTIONAL_TEMPLATE = " + optional(\" %1$s \", %3$s, format=formatValue(%2$s))"; - protected static final String FLAG_TEMPLATE = " + (if (%3$s) \" %1$s\" else \"\")"; + protected static final String REQUIRED_TEMPLATE = " + required(\"%1$s\", %3$s, spaceSeparated=true, escape=true, format=%2$s)"; + protected static final String REPEAT_TEMPLATE = " + repeat(\"%1$s\", %3$s, spaceSeparated=true, escape=true, format=%2$s)"; + protected static final String OPTIONAL_TEMPLATE = " + optional(\"%1$s\", %3$s, spaceSeparated=true, escape=true, format=%2$s)"; + protected static final String FLAG_TEMPLATE = " + conditional(%3$s, \"%1$s\", escape=true, format=%2$s)"; public final String getCommandLineAddition() { return String.format(getCommandLineTemplate(), getCommandLineParam(), getCommandLineFormat(), getFieldName()); @@ -136,7 +136,7 @@ public abstract class ArgumentDefinitionField extends ArgumentField { new IntervalStringArgumentField(argumentDefinition)); // ROD Bindings are set by the RodBindField - } else if (RodBindField.ROD_BIND_FIELD.equals(argumentDefinition.fullName) && argumentDefinition.ioType == ArgumentIOType.INPUT) { + } else if (RodBindArgumentField.ROD_BIND_FIELD.equals(argumentDefinition.fullName) && argumentDefinition.ioType == ArgumentIOType.INPUT) { // TODO: Once everyone is using @Allows and @Requires correctly, we can stop blindly allowing Triplets return Arrays.asList(new RodBindArgumentField(argumentDefinition), new InputIndexesArgumentField(argumentDefinition, Tribble.STANDARD_INDEX_EXTENSION)); //return Collections.emptyList(); @@ -337,6 +337,8 @@ public abstract class ArgumentDefinitionField extends ArgumentField { // Allows the user to specify the track name, track type, and the file. public static class RodBindArgumentField extends ArgumentDefinitionField { + public static final String ROD_BIND_FIELD = "rodBind"; + public RodBindArgumentField(ArgumentDefinition argumentDefinition) { super(argumentDefinition); } @@ -344,7 +346,7 @@ public abstract class ArgumentDefinitionField extends ArgumentField { @Override protected String getFieldType() { return "List[RodBind]"; } @Override protected String getDefaultValue() { return "Nil"; } @Override protected String getCommandLineTemplate() { - return " + repeat(\"\", %3$s, format=RodBind.formatCommandLine(\"%1$s\"))"; + return " + repeat(\"%1$s\", %3$s, formatPrefix=RodBind.formatCommandLineParameter, spaceSeparated=true, escape=true, format=%2$s)"; } } @@ -358,11 +360,11 @@ public abstract class ArgumentDefinitionField extends ArgumentField { @Override protected String getDefaultValue() { return argumentDefinition.isMultiValued ? "Nil" : "_"; } @Override protected String getCommandLineTemplate() { if (argumentDefinition.isMultiValued) { - return " + repeat(\"\", %3$s, format=TaggedFile.formatCommandLine(\"%1$s\"))"; + return " + repeat(\"%1$s\", %3$s, formatPrefix=TaggedFile.formatCommandLineParameter, spaceSeparated=true, escape=true, format=%2$s)"; } else if (!argumentDefinition.required) { - return " + optional(\"\", %3$s, format=TaggedFile.formatCommandLine(\"%1$s\"))"; + return " + optional(TaggedFile.formatCommandLineParameter(\"%1$s\", %3$s), %3$s, spaceSeparated=true, escape=true, format=%2$s)"; } else { - return " + TaggedFile.formatCommandLine(\"%1$s\")(\"\", %3$s, \"\")"; + return " + required(TaggedFile.formatCommandLineParameter(\"%1$s\", %3$s), %3$s, spaceSeparated=true, escape=true, format=%2$s)"; } } } diff --git a/public/java/src/org/broadinstitute/sting/queue/extensions/gatk/GATKExtensionsGenerator.java b/public/java/src/org/broadinstitute/sting/queue/extensions/gatk/GATKExtensionsGenerator.java index 9578eda84..67010c4d5 100644 --- a/public/java/src/org/broadinstitute/sting/queue/extensions/gatk/GATKExtensionsGenerator.java +++ b/public/java/src/org/broadinstitute/sting/queue/extensions/gatk/GATKExtensionsGenerator.java @@ -374,6 +374,9 @@ public class GATKExtensionsGenerator extends CommandLineProgram { if (isGather) importSet.add("import org.broadinstitute.sting.commandline.Gather"); + // Needed for ShellUtils.escapeShellArgument() + importSet.add("import org.broadinstitute.sting.queue.util.ShellUtils"); + // Sort the imports so that the are always in the same order. List sortedImports = new ArrayList(importSet); Collections.sort(sortedImports); diff --git a/public/java/src/org/broadinstitute/sting/queue/extensions/gatk/RodBindField.java b/public/java/src/org/broadinstitute/sting/queue/extensions/gatk/RodBindField.java deleted file mode 100644 index baf083575..000000000 --- a/public/java/src/org/broadinstitute/sting/queue/extensions/gatk/RodBindField.java +++ /dev/null @@ -1,129 +0,0 @@ -/* - * Copyright (c) 2010, The Broad Institute - * - * Permission is hereby granted, free of charge, to any person - * obtaining a copy of this software and associated documentation - * files (the "Software"), to deal in the Software without - * restriction, including without limitation the rights to use, - * copy, modify, merge, publish, distribute, sublicense, and/or sell - * copies of the Software, and to permit persons to whom the - * Software is furnished to do so, subject to the following - * conditions: - * - * The above copyright notice and this permission notice shall be - * included in all copies or substantial portions of the Software. - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, - * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES - * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND - * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT - * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, - * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR - * OTHER DEALINGS IN THE SOFTWARE. - */ - -package org.broadinstitute.sting.queue.extensions.gatk; - -import org.broadinstitute.sting.commandline.Input; -import org.broadinstitute.sting.gatk.WalkerManager; -import org.broadinstitute.sting.gatk.refdata.tracks.RMDTrackBuilder; -import org.broadinstitute.sting.gatk.walkers.RMD; -import org.broadinstitute.sting.gatk.walkers.Walker; - -import java.io.File; -import java.lang.annotation.Annotation; -import java.util.ArrayList; -import java.util.List; - -/** - * Allows user to specify the rod file but locks in the track name and the track type. - */ -public class RodBindField extends ArgumentField { - public static final String ROD_BIND_FIELD = "rodBind"; - - private final String trackName; - private final String typeName; - private final List relatedFields; - private final boolean isRequired; - - public RodBindField(String trackName, String typeName, List relatedFields, boolean isRequired) { - this.trackName = trackName; - this.typeName = typeName; - this.relatedFields = relatedFields; - this.isRequired = isRequired; - } - - @SuppressWarnings("unchecked") - @Override protected Class getAnnotationIOClass() { return Input.class; } - @Override protected Class getInnerType() { return File.class; } - @Override protected String getFullName() { return escape(getRawFieldName()); } - @Override protected String getFieldType() { return "File"; } - @Override protected String getDefaultValue() { return "_"; } - @Override protected String getRawFieldName() { return this.trackName + this.typeName; } - @Override protected String getDoc() { return escape(this.typeName + " " + this.trackName); } - @Override protected boolean isRequired() { return this.isRequired; } - - @Override public String getCommandLineAddition() { - // TODO: Stop allowing the generic "rodBind" triplets to satisfy the requirement after @Requires are fixed. - return String.format(" + optional(\" -B:%s,%s \", %s)", - /* - return String.format(this.useOption() - ? " + optional(\" -B:%s,%s \", %s)" - : " + \" -B:%s,%s \" + %s", - */ - this.trackName, this.typeName, getFieldName()); - } - - private boolean useOption() { - return !this.isRequired || (relatedFields.size() > 1); - } - - @Override protected String getExclusiveOf() { - StringBuilder exclusiveOf = new StringBuilder(); - // TODO: Stop allowing the generic "rodBind" triplets to satisfy the requirement after @Requires are fixed. - if (this.isRequired) - exclusiveOf.append(ROD_BIND_FIELD); - for (RodBindField relatedField: relatedFields) - if (relatedField != this) { - if (exclusiveOf.length() > 0) - exclusiveOf.append(","); - exclusiveOf.append(relatedField.getFieldName()); - } - return exclusiveOf.toString(); - } -// -// public static List getRodArguments(Class walkerClass, RMDTrackBuilder trackBuilder) { -// List argumentFields = new ArrayList(); -// -// List requires = WalkerManager.getRequiredMetaData(walkerClass); -// List allows = WalkerManager.getAllowsMetaData(walkerClass); -// -// for (RMD required: requires) { -// List fields = new ArrayList(); -// String trackName = required.name(); -// if ("*".equals(trackName)) { -// // TODO: Add the field triplet for name=* after @Allows and @Requires are fixed on walkers -// //fields.add(new RodBindArgumentField(argumentDefinition, true)); -// } else { -// for (String typeName: trackBuilder.getFeatureManager().getTrackRecordTypeNames(required.type())) -// fields.add(new RodBindField(trackName, typeName, fields, true)); -// } -// argumentFields.addAll(fields); -// } -// -// for (RMD allowed: allows) { -// List fields = new ArrayList(); -// String trackName = allowed.name(); -// if ("*".equals(trackName)) { -// // TODO: Add the field triplet for name=* after @Allows and @Requires are fixed on walkers -// //fields.add(new RodBindArgumentField(argumentDefinition, false)); -// } else { -// for (String typeName: trackBuilder.getFeatureManager().getTrackRecordTypeNames(allowed.type())) -// fields.add(new RodBindField(trackName, typeName, fields, true)); -// } -// argumentFields.addAll(fields); -// } -// -// return argumentFields; -// } -} diff --git a/public/scala/qscript/org/broadinstitute/sting/queue/qscripts/GATKResourcesBundle.scala b/public/scala/qscript/org/broadinstitute/sting/queue/qscripts/GATKResourcesBundle.scala index 036a77b58..8c9063c29 100755 --- a/public/scala/qscript/org/broadinstitute/sting/queue/qscripts/GATKResourcesBundle.scala +++ b/public/scala/qscript/org/broadinstitute/sting/queue/qscripts/GATKResourcesBundle.scala @@ -314,7 +314,7 @@ class GATKResourcesBundle extends QScript { class MakeDBSNP129(@Input dbsnp: File, @Input ref: File, @Output dbsnp129: File) extends SelectVariants with UNIVERSAL_GATK_ARGS { this.variant = dbsnp - this.select ++= List("\"dbSNPBuildID <= 129\"") + this.select ++= List("dbSNPBuildID <= 129") this.reference_sequence = ref this.out = dbsnp129 } diff --git a/public/scala/qscript/org/broadinstitute/sting/queue/qscripts/MethodsDevelopmentCallingPipeline.scala b/public/scala/qscript/org/broadinstitute/sting/queue/qscripts/MethodsDevelopmentCallingPipeline.scala index da02c8ac5..c06601a2d 100755 --- a/public/scala/qscript/org/broadinstitute/sting/queue/qscripts/MethodsDevelopmentCallingPipeline.scala +++ b/public/scala/qscript/org/broadinstitute/sting/queue/qscripts/MethodsDevelopmentCallingPipeline.scala @@ -248,10 +248,10 @@ class MethodsDevelopmentCallingPipeline extends QScript { this.V = t.rawIndelVCF this.out = t.filteredIndelVCF this.filterName ++= List("IndelQD", "IndelReadPosRankSum", "IndelFS") - this.filterExpression ++= List("\"QD < 2.0\"", "\"ReadPosRankSum < -20.0\"", "\"FS > 200.0\"") + this.filterExpression ++= List("QD < 2.0", "ReadPosRankSum < -20.0", "FS > 200.0") if (t.nSamples >= 10) { this.filterName ++= List("IndelInbreedingCoeff") - this.filterExpression ++= List("\"InbreedingCoeff < -0.8\"") + this.filterExpression ++= List("InbreedingCoeff < -0.8") } this.analysisName = t.name + "_VF" this.jobName = queueLogDir + t.name + ".indelfilter" diff --git a/public/scala/qscript/org/broadinstitute/sting/queue/qscripts/examples/ExampleUnifiedGenotyper.scala b/public/scala/qscript/org/broadinstitute/sting/queue/qscripts/examples/ExampleUnifiedGenotyper.scala index 9bddfd97c..2f4aea755 100644 --- a/public/scala/qscript/org/broadinstitute/sting/queue/qscripts/examples/ExampleUnifiedGenotyper.scala +++ b/public/scala/qscript/org/broadinstitute/sting/queue/qscripts/examples/ExampleUnifiedGenotyper.scala @@ -62,7 +62,7 @@ class ExampleUnifiedGenotyper extends QScript { variantFilter.variant = genotyper.out variantFilter.out = swapExt(qscript.bamFile, "bam", "filtered.vcf") variantFilter.filterName = filterNames - variantFilter.filterExpression = filterExpressions.map("\"" + _ + "\"") + variantFilter.filterExpression = filterExpressions evalFiltered.eval :+= variantFilter.out evalFiltered.out = swapExt(variantFilter.out, "vcf", "eval") diff --git a/public/scala/src/org/broadinstitute/sting/queue/extensions/gatk/RodBind.scala b/public/scala/src/org/broadinstitute/sting/queue/extensions/gatk/RodBind.scala index 9af4d9bcf..deb83bf5a 100644 --- a/public/scala/src/org/broadinstitute/sting/queue/extensions/gatk/RodBind.scala +++ b/public/scala/src/org/broadinstitute/sting/queue/extensions/gatk/RodBind.scala @@ -28,21 +28,14 @@ object RodBind { def apply(trackName: String, trackType: String, file: File, tag: String) = new RodBind(trackName, trackType, file, tag) def apply(trackName: String, trackType: String, file: File) = new RodBind(trackName, trackType, file, null) - /** - * Formats the rod binding on the command line. - * Used for optional and repeat. - * @param cmdLineParam command line parameter, ex: -B - * @param prefix unused - * @param value RodBind to add. - * @param suffix unused - * @return The command line addition. - */ - def formatCommandLine(cmdLineParam: String)(prefix: String, value: Any, suffix: String) = { + def formatCommandLineParameter( cmdLineParam: String, value: Any ) = { value match { case rodBind: RodBind if (rodBind.tag != null) => - " %s:%s,%s,%s %s".format(cmdLineParam, rodBind.trackName, rodBind.trackType, rodBind.tag, rodBind.getPath) + "%s:%s,%s,%s".format(cmdLineParam, rodBind.trackName, rodBind.trackType, rodBind.tag) case rodBind: RodBind => - " %s:%s,%s %s".format(cmdLineParam, rodBind.trackName, rodBind.trackType, rodBind.getPath) + "%s:%s,%s".format(cmdLineParam, rodBind.trackName, rodBind.trackType) + case x => + "" } } } diff --git a/public/scala/src/org/broadinstitute/sting/queue/extensions/gatk/TaggedFile.scala b/public/scala/src/org/broadinstitute/sting/queue/extensions/gatk/TaggedFile.scala index 295199993..940985f92 100644 --- a/public/scala/src/org/broadinstitute/sting/queue/extensions/gatk/TaggedFile.scala +++ b/public/scala/src/org/broadinstitute/sting/queue/extensions/gatk/TaggedFile.scala @@ -2,6 +2,7 @@ package org.broadinstitute.sting.queue.extensions.gatk import java.io.File import org.broadinstitute.sting.utils.io.FileExtension +import org.broadinstitute.sting.queue.util.ShellUtils /** * Used to provide tagged -I input_file arguments to the GATK. @@ -19,21 +20,14 @@ object TaggedFile { def apply(path: String, tag: String) = new TaggedFile(path, tag) def apply(file: File, tag: String) = new TaggedFile(file, tag) - /** - * Formats the rod binding on the command line. - * Used for optional and repeat. - * @param cmdLineParam command line parameter, ex: -I - * @param prefix unused - * @param value TaggedFile to add. - * @param suffix unused - * @return The command line addition. - */ - def formatCommandLine(cmdLineParam: String)(prefix: String, value: Any, suffix: String) = { + def formatCommandLineParameter(cmdLineParam: String, value: Any) = { value match { case taggedFile: TaggedFile if (taggedFile.tag != null) => - " %s:%s %s".format(cmdLineParam, taggedFile.tag, taggedFile.getPath) + "%s:%s".format(cmdLineParam, taggedFile.tag) case file: File => - " %s %s".format(cmdLineParam, file.getPath) + cmdLineParam + case x => + "" } } } diff --git a/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/AddOrReplaceReadGroups.scala b/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/AddOrReplaceReadGroups.scala index 5456ed02c..93735e4ac 100644 --- a/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/AddOrReplaceReadGroups.scala +++ b/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/AddOrReplaceReadGroups.scala @@ -55,11 +55,11 @@ class AddOrReplaceReadGroups extends org.broadinstitute.sting.queue.function.Jav override def outputBam = output this.createIndex = Some(true) override def commandLine = super.commandLine + - " RGID=" + RGID + - " RGLB=" + RGLB + - " RGPL=" + RGPL + - " RGPU=" + RGPU + - " RGSM=" + RGSM + - conditionalParameter(RGCN != null && !RGCN.isEmpty, " RGCN=" + RGCN) + - conditionalParameter(RGDS != null && !RGDS.isEmpty, " RGDS=" + RGDS) + required("RGID=" + RGID) + + required("RGLB=" + RGLB) + + required("RGPL=" + RGPL) + + required("RGPU=" + RGPU) + + required("RGSM=" + RGSM) + + conditional(RGCN != null && !RGCN.isEmpty, "RGCN=" + RGCN) + + conditional(RGDS != null && !RGDS.isEmpty, "RGDS=" + RGDS) } \ No newline at end of file diff --git a/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/MarkDuplicates.scala b/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/MarkDuplicates.scala index d44d5e004..d73c556af 100644 --- a/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/MarkDuplicates.scala +++ b/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/MarkDuplicates.scala @@ -47,10 +47,8 @@ class MarkDuplicates extends org.broadinstitute.sting.queue.function.JavaCommand this.sortOrder = null this.createIndex = Some(true) override def commandLine = super.commandLine + - " M=" + metrics + - conditionalParameter(REMOVE_DUPLICATES, " REMOVE_DUPLICATES=true") + - conditionalParameter(MAX_FILE_HANDLES_FOR_READ_ENDS_MAP > 0, " MAX_FILE_HANDLES_FOR_READ_ENDS_MAP=" + MAX_FILE_HANDLES_FOR_READ_ENDS_MAP.toString) + - conditionalParameter(SORTING_COLLECTION_SIZE_RATIO > 0, " SORTING_COLLECTION_SIZE_RATIO=" + SORTING_COLLECTION_SIZE_RATIO.toString) - - + required("M=" + metrics) + + conditional(REMOVE_DUPLICATES, "REMOVE_DUPLICATES=true") + + conditional(MAX_FILE_HANDLES_FOR_READ_ENDS_MAP > 0, "MAX_FILE_HANDLES_FOR_READ_ENDS_MAP=" + MAX_FILE_HANDLES_FOR_READ_ENDS_MAP.toString) + + conditional(SORTING_COLLECTION_SIZE_RATIO > 0, "SORTING_COLLECTION_SIZE_RATIO=" + SORTING_COLLECTION_SIZE_RATIO.toString) } \ No newline at end of file diff --git a/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/MergeSamFiles.scala b/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/MergeSamFiles.scala index fd107890e..036932cc6 100644 --- a/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/MergeSamFiles.scala +++ b/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/MergeSamFiles.scala @@ -44,9 +44,7 @@ class MergeSamFiles extends org.broadinstitute.sting.queue.function.JavaCommandL override def outputBam = output this.createIndex = Some(true) override def commandLine = super.commandLine + - conditionalParameter(MERGE_SEQUENCE_DICTIONARIES, " MERGE_SEQUENCE_DICTIONARIES=true") + - conditionalParameter(USE_THREADING, " USE_THREADING=true") + - conditionalParameter(COMMENT != null && !COMMENT.isEmpty, " COMMENT=" + COMMENT) - - + conditional(MERGE_SEQUENCE_DICTIONARIES, "MERGE_SEQUENCE_DICTIONARIES=true") + + conditional(USE_THREADING, "USE_THREADING=true") + + conditional(COMMENT != null && !COMMENT.isEmpty, "COMMENT=" + COMMENT) } \ No newline at end of file diff --git a/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/PicardBamFunction.scala b/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/PicardBamFunction.scala index 427c09f82..76856dc36 100644 --- a/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/PicardBamFunction.scala +++ b/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/PicardBamFunction.scala @@ -48,14 +48,13 @@ trait PicardBamFunction extends JavaCommandLineFunction { protected def outputBam: File abstract override def commandLine = super.commandLine + - Array( - repeat(" INPUT=", inputBams), - " TMP_DIR=" + jobTempDir, - optional(" OUTPUT=", outputBam), - optional(" COMPRESSION_LEVEL=", compressionLevel), - optional(" VALIDATION_STRINGENCY=", validationStringency), - optional(" SO=", sortOrder), - optional(" MAX_RECORDS_IN_RAM=", maxRecordsInRam), - optional(" ASSUME_SORTED=", assumeSorted), - optional(" CREATE_INDEX=", createIndex)).mkString + repeat("INPUT=", inputBams, spaceSeparated=false) + + required("TMP_DIR=" + jobTempDir) + + optional("OUTPUT=", outputBam, spaceSeparated=false) + + optional("COMPRESSION_LEVEL=", compressionLevel, spaceSeparated=false) + + optional("VALIDATION_STRINGENCY=", validationStringency, spaceSeparated=false) + + optional("SO=", sortOrder, spaceSeparated=false) + + optional("MAX_RECORDS_IN_RAM=", maxRecordsInRam, spaceSeparated=false) + + optional("ASSUME_SORTED=", assumeSorted, spaceSeparated=false) + + optional("CREATE_INDEX=", createIndex, spaceSeparated=false) } diff --git a/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/ReorderSam.scala b/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/ReorderSam.scala index 72489dc87..b1968bee5 100644 --- a/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/ReorderSam.scala +++ b/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/ReorderSam.scala @@ -42,7 +42,7 @@ class ReorderSam extends org.broadinstitute.sting.queue.function.JavaCommandLine this.createIndex = Some(true) this.sortOrder = null override def commandLine = super.commandLine + - " REFERENCE=" + sortReference + - optional(" ALLOW_INCOMPLETE_DICT_CONCORDANCE=", ALLOW_INCOMPLETE_DICT_CONCORDANCE) - optional(" ALLOW_CONTIG_LENGTH_DISCORDANCE=", ALLOW_CONTIG_LENGTH_DISCORDANCE) + required("REFERENCE=" + sortReference) + + optional("ALLOW_INCOMPLETE_DICT_CONCORDANCE=", ALLOW_INCOMPLETE_DICT_CONCORDANCE, spaceSeparated=false) + optional("ALLOW_CONTIG_LENGTH_DISCORDANCE=", ALLOW_CONTIG_LENGTH_DISCORDANCE, spaceSeparated=false) } \ No newline at end of file diff --git a/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/RevertSam.scala b/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/RevertSam.scala index 746ce609e..60d8bfaf8 100644 --- a/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/RevertSam.scala +++ b/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/RevertSam.scala @@ -52,10 +52,10 @@ class RevertSam extends org.broadinstitute.sting.queue.function.JavaCommandLineF override def outputBam = output this.createIndex = Some(true) override def commandLine = super.commandLine + - conditionalParameter(!restoreOriginalQualities, " RESTORE_ORIGINAL_QUALITIES=false") + - conditionalParameter(!removeDuplicateInformation, " REMOVE_DUPLICATE_INFORMATION=false") + - conditionalParameter(!removeAlignmentInformation, " REMOVE_ALIGNMENT_INFORMATION=false") + - conditionalParameter(!attributesToClear.isEmpty, repeat(" ATTRIBUTE_TO_CLEAR=", attributesToClear)) + - conditionalParameter(sampleAlias != null, " SAMPLE_ALIAS=" + sampleAlias) + - conditionalParameter(libraryName != null, " LIBRARY_NAME=" + libraryName) + conditional(!restoreOriginalQualities, "RESTORE_ORIGINAL_QUALITIES=false") + + conditional(!removeDuplicateInformation, "REMOVE_DUPLICATE_INFORMATION=false") + + conditional(!removeAlignmentInformation, "REMOVE_ALIGNMENT_INFORMATION=false") + + repeat("ATTRIBUTE_TO_CLEAR=", attributesToClear, spaceSeparated=false) + // repeat() returns "" for null/empty list + conditional(sampleAlias != null, "SAMPLE_ALIAS=" + sampleAlias) + + conditional(libraryName != null, "LIBRARY_NAME=" + libraryName) } \ No newline at end of file diff --git a/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/SamToFastq.scala b/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/SamToFastq.scala index 3a4217e60..3eb4e8e06 100644 --- a/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/SamToFastq.scala +++ b/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/SamToFastq.scala @@ -61,17 +61,17 @@ class SamToFastq extends org.broadinstitute.sting.queue.function.JavaCommandLine this.sortOrder = null override def commandLine = super.commandLine + - " FASTQ=" + fastq + - optional(" SECOND_END_FASTQ=", secondEndFastQ) + - conditionalParameter(outputPerReadGroup, optional(" OUTPUT_PER_RG=", outputPerReadGroup)) + - optional(" OUTPUT_DIR=", outputDir) + - conditionalParameter(!reReverse, optional(" RE_REVERSE=", reReverse)) + - conditionalParameter(includeNonPFReads, optional(" INCLUDE_NON_PF_READS=", includeNonPFReads)) + - optional(" CLIPPING_ATTRIBUTE=", clippingAttribute) + - optional(" CLIPPING_ACTION=", clippingAction) + - conditionalParameter (readOneTrim >= 0, optional(" READ1_TRIM=", readOneTrim)) + - conditionalParameter (readOneMaxBasesToWrite >= 0, optional(" READ1_MAX_BASES_TO_WRITE=", readOneMaxBasesToWrite)) + - conditionalParameter (readTwoTrim >= 0, optional(" READ2_TRIM=", readTwoTrim)) + - conditionalParameter (readTwoMaxBasesToWrite >=0, optional(" READ2_MAX_BASES_TO_WRITE=", readTwoMaxBasesToWrite)) + - conditionalParameter (includeNonPrimaryAlignments, optional(" INCLUDE_NON_PRIMARY_ALIGNMENTS=", includeNonPrimaryAlignments)) + required("FASTQ=" + fastq) + + optional("SECOND_END_FASTQ=", secondEndFastQ, spaceSeparated=false) + + conditional(outputPerReadGroup, "OUTPUT_PER_RG=" + outputPerReadGroup) + + optional("OUTPUT_DIR=", outputDir, spaceSeparated=false) + + conditional(!reReverse, "RE_REVERSE=" + reReverse) + + conditional(includeNonPFReads, "INCLUDE_NON_PF_READS=" + includeNonPFReads) + + optional("CLIPPING_ATTRIBUTE=", clippingAttribute, spaceSeparated=false) + + optional("CLIPPING_ACTION=", clippingAction, spaceSeparated=false) + + conditional(readOneTrim >= 0, "READ1_TRIM=" + readOneTrim) + + conditional(readOneMaxBasesToWrite >= 0, "READ1_MAX_BASES_TO_WRITE=" + readOneMaxBasesToWrite) + + conditional(readTwoTrim >= 0, "READ2_TRIM=" + readTwoTrim) + + conditional(readTwoMaxBasesToWrite >= 0, "READ2_MAX_BASES_TO_WRITE=" + readTwoMaxBasesToWrite) + + conditional(includeNonPrimaryAlignments, "INCLUDE_NON_PRIMARY_ALIGNMENTS=" + includeNonPrimaryAlignments) } \ No newline at end of file diff --git a/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/ValidateSamFile.scala b/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/ValidateSamFile.scala index 2c8fbc6d9..030e4b07d 100644 --- a/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/ValidateSamFile.scala +++ b/public/scala/src/org/broadinstitute/sting/queue/extensions/picard/ValidateSamFile.scala @@ -50,11 +50,11 @@ class ValidateSamFile extends org.broadinstitute.sting.queue.function.JavaComman override def inputBams = input override def outputBam = output override def commandLine = super.commandLine + - " MODE=" + MODE + - " MAX_OUTPUT=" + MAX_OUTPUT + - " MAX_OPEN_TEMP_FILES=" + MAX_OPEN_TEMP_FILES + - conditionalParameter(!VALIDATE_INDEX, " VALIDATE_INDEX=false") + - conditionalParameter(IGNORE_WARNINGS, " IGNORE_WARNINGS=true") + - conditionalParameter(IS_BISULFITE_SEQUENCED, " IS_BISULFITE_SEQUENCED=true") + - conditionalParameter(IGNORE != null && !IGNORE.isEmpty, repeat(" IGNORE=", IGNORE)) + required("MODE=" + MODE) + + required("MAX_OUTPUT=" + MAX_OUTPUT) + + required("MAX_OPEN_TEMP_FILES=" + MAX_OPEN_TEMP_FILES) + + conditional(!VALIDATE_INDEX, "VALIDATE_INDEX=false") + + conditional(IGNORE_WARNINGS, "IGNORE_WARNINGS=true") + + conditional(IS_BISULFITE_SEQUENCED, "IS_BISULFITE_SEQUENCED=true") + + repeat("IGNORE=", IGNORE, spaceSeparated=false) // repeat() returns "" for null/empty list } \ No newline at end of file diff --git a/public/scala/src/org/broadinstitute/sting/queue/extensions/samtools/SamtoolsIndexFunction.scala b/public/scala/src/org/broadinstitute/sting/queue/extensions/samtools/SamtoolsIndexFunction.scala index 801a152ec..83a03b904 100644 --- a/public/scala/src/org/broadinstitute/sting/queue/extensions/samtools/SamtoolsIndexFunction.scala +++ b/public/scala/src/org/broadinstitute/sting/queue/extensions/samtools/SamtoolsIndexFunction.scala @@ -48,7 +48,10 @@ class SamtoolsIndexFunction extends SamtoolsCommandLineFunction { bamFileIndex = new File(bamFile.getPath + ".bai") } - def commandLine = "%s index %s %s".format(samtools, bamFile, bamFileIndex) + def commandLine = required(samtools) + + required("index") + + required(bamFile) + + required(bamFileIndex) override def dotString = "Index: %s".format(bamFile.getName) } diff --git a/public/scala/src/org/broadinstitute/sting/queue/extensions/samtools/SamtoolsMergeFunction.scala b/public/scala/src/org/broadinstitute/sting/queue/extensions/samtools/SamtoolsMergeFunction.scala index 2b864def6..aff9a25c0 100644 --- a/public/scala/src/org/broadinstitute/sting/queue/extensions/samtools/SamtoolsMergeFunction.scala +++ b/public/scala/src/org/broadinstitute/sting/queue/extensions/samtools/SamtoolsMergeFunction.scala @@ -55,7 +55,9 @@ class SamtoolsMergeFunction extends SamtoolsCommandLineFunction { )) } - def commandLine = "%s merge%s %s%s".format( - samtools, optional(" -R ", region), - outputBam, repeat(" ", inputBams)) + def commandLine = required(samtools) + + required("merge") + + optional("-R", region) + + required(outputBam) + + repeat(inputBams) } diff --git a/public/scala/src/org/broadinstitute/sting/queue/extensions/snpeff/SnpEff.scala b/public/scala/src/org/broadinstitute/sting/queue/extensions/snpeff/SnpEff.scala index 62f66ec06..259856c17 100644 --- a/public/scala/src/org/broadinstitute/sting/queue/extensions/snpeff/SnpEff.scala +++ b/public/scala/src/org/broadinstitute/sting/queue/extensions/snpeff/SnpEff.scala @@ -50,11 +50,14 @@ class SnpEff extends JavaCommandLineFunction { @Output(doc="snp eff output") var outVcf: File = _ - override def commandLine = Array( - super.commandLine, - " eff", - if (verbose) " -v" else "", - optional(" -c ", config), - " -i vcf -o vcf %s %s > %s".format(genomeVersion, inVcf, outVcf) - ).mkString + override def commandLine = super.commandLine + + required("eff") + + conditional(verbose, "-v") + + optional("-c", config) + + required("-i", "vcf") + + required("-o", "vcf") + + required(genomeVersion) + + required(inVcf) + + required(">", escape=false) + + required(outVcf) } diff --git a/public/scala/src/org/broadinstitute/sting/queue/function/CommandLineFunction.scala b/public/scala/src/org/broadinstitute/sting/queue/function/CommandLineFunction.scala index ff77503ac..b08832f22 100644 --- a/public/scala/src/org/broadinstitute/sting/queue/function/CommandLineFunction.scala +++ b/public/scala/src/org/broadinstitute/sting/queue/function/CommandLineFunction.scala @@ -110,61 +110,178 @@ trait CommandLineFunction extends QFunction with Logging { } /** - * Repeats parameters with a prefix/suffix if they are set otherwise returns "". - * Skips null, Nil, None. Unwraps Some(x) to x. Everything else is called with x.toString. - * @param prefix Command line prefix per parameter. - * @param params Traversable parameters. - * @param suffix Optional suffix per parameter. - * @param separator Optional separator per parameter. - * @param format Format function if the value has a value - * @return The generated string + * Safely construct a full required command-line argument with consistent quoting, whitespace separation, etc. + * + * @param prefix Prefix to insert before the argument value (eg., "-f") + * @param param The argument value itself + * @param suffix Suffix to append after the argument value + * @param spaceSeparated If true, insert a space between the prefix, param, and suffix + * @param escape If true, quote the generated argument to avoid interpretation by the shell + * @param format Format String used to convert param to a String + * @return The combined and formatted argument, surrounded by whitespace */ - protected def repeat(prefix: String, params: Traversable[_], suffix: String = "", separator: String = "", - format: (String, Any, String) => String = formatValue("%s")) = - if (params == null) - "" - else - params.filter(param => hasValue(param)).map(param => format(prefix, param, suffix)).mkString(separator) + protected def required( prefix: String, param: Any, suffix: String = "", spaceSeparated: Boolean = true, + escape: Boolean = true, format: String = "%s" ): String = { + " %s ".format(formatArgument(prefix, param, suffix, spaceSeparated, escape, format)) + } /** - * Returns parameter with a prefix/suffix if it is set otherwise returns "". - * Does not output null, Nil, None. Unwraps Some(x) to x. Everything else is called with x.toString. - * @param prefix Command line prefix per parameter. - * @param param Parameter to check for a value. - * @param suffix Optional suffix per parameter. - * @param format Format function if the value has a value - * @return The generated string + * Safely construct a one-token required command-line argument with quoting + * + * @param param The command-line argument value + * @return The argument value quoted and surrounded by whitespace */ - protected def optional(prefix: String, param: Any, suffix: String = "", - format: (String, Any, String) => String = formatValue("%s")) = - if (hasValue(param)) format(prefix, param, suffix) else "" + protected def required( param: Any ): String = { + required("", param) + } /** - * Returns "" if the value is null or an empty collection, otherwise return the value.toString. - * @param format Format string if the value has a value - * @param prefix Command line prefix per parameter. - * @param param Parameter to check for a value. - * @param suffix Optional suffix per parameter. - * @return "" if the value is null, or "" if the collection is empty, otherwise the value.toString. + * Safely construct a one-token required command-line argument, and specify whether you want quoting + * + * @param param The command-line argument value + * @param escape If true, quote the generated argument to avoid interpretation by the shell + * @return The argument value, quoted if quoting was requested, and surrounded by whitespace */ - protected def formatValue(format: String)(prefix: String, param: Any, suffix: String): String = - if (CollectionUtils.isNullOrEmpty(param)) - "" - else - prefix + (param match { - case Some(x) => format.format(x) - case x => format.format(x) - }) + suffix + protected def required( param: Any, escape: Boolean ): String = { + required("", param, escape=escape) + } /** - * Returns the parameter if the condition is true. Useful for long string of parameters - * @param condition the condition to validate - * @param param the string to be returned in case condition is true - * @return param if condition is true, "" otherwise + * Safely construct a full optional command-line argument with consistent quoting, whitespace separation, etc. + * If the argument has no value, returns an empty String. + * + * @param prefix Prefix to insert before the argument value (eg., "-f") + * @param param The argument value itself (if null/empty, the method returns empty String) + * @param suffix Suffix to append after the argument value + * @param spaceSeparated If true, insert a space between the prefix, param, and suffix + * @param escape If true, quote the generated argument to avoid interpretation by the shell + * @param format Format String used to convert param to a String + * @return The combined and formatted argument, surrounded by whitespace, or an empty String + * if the argument has no value */ - protected def conditionalParameter(condition: Boolean, param: String): String = - if (condition == true) - param - else + protected def optional( prefix: String, param: Any, suffix: String = "", spaceSeparated: Boolean = true, + escape: Boolean = true, format: String = "%s" ): String = { + if ( hasValue(param) ) " %s ".format(formatArgument(prefix, param, suffix, spaceSeparated, escape, format)) else "" + } + + /** + * Safely construct a one-token optional command-line argument with quoting. + * If the argument has no value, returns an empty String. + * + * @param param The command-line argument value + * @return The argument value quoted and surrounded by whitespace, or an empty String + * if the argument has no value + */ + protected def optional( param: Any ): String = { + optional("", param) + } + + /** + * Safely construct a one-token conditional command-line argument. If the provided condition + * is false, an empty String is returned. + * + * @param condition The condition to check + * @param param The command-line argument value + * @param escape If true, quote the generated argument to avoid interpretation by the shell + * @param format Format String used to convert param to a String + * @return The command-line argument value, quoted if quoting was requested and surrounded + * by whitespace, or an empty String if the argument has no value. + */ + protected def conditional( condition: Boolean, param: Any, escape: Boolean = true, format: String = "%s" ): String = { + if ( condition ) { + " %s ".format(formatArgument("", param, "", false, escape, format)) + } + else { "" + } + } + + /** + * Safely construct a series of full command-line arguments with consistent quoting, whitespace separation, etc. + * + * Each argument value is preceded by a prefix/suffix if they are set. A function can be provided to vary + * each prefix for each argument value (eg., -f:tag1 file1 -f:tag2 file2) -- the default is to use + * the same prefix for all arguments. + * + * @param prefix Prefix to insert before each argument value (eg., "-f") + * @param params The collection of argument values + * @param suffix Suffix to append after each argument value + * @param separator Specifies how to separate the various arguments from each other + * (eg., what should go between '-f' 'file1' and '-f' 'file2'?) + * Default is one space character. + * @param spaceSeparated If true, insert a space between each individual prefix, param, and suffix + * @param escape If true, quote the generated argument to avoid interpretation by the shell + * @param format Format String used to convert each individual param within params to a String + * @param formatPrefix Function mapping (prefix, argumentValue) pairs to prefixes. Can be used to + * vary each prefix depending on the argument value (useful for tags, etc.). + * Default is to use the same prefix for all argument values. + * @return The series of command-line arguments, quoted and whitespace-delimited as requested, + * or an empty String if params was null/Nil/None. + */ + protected def repeat(prefix: String, params: Traversable[_], suffix: String = "", separator: String = " ", + spaceSeparated: Boolean = true, escape: Boolean = true, format: String = "%s", + formatPrefix: (String, Any) => String = (prefix, value) => prefix): String = { + if (CollectionUtils.isNullOrEmpty(params)) + "" + else + " %s ".format(params.filter(param => hasValue(param)).map(param => formatArgument(formatPrefix(prefix, param), param, suffix, spaceSeparated, escape, format)).mkString(separator)) + } + + /** + * Safely construct a series of one-token command-line arguments with quoting and space separation. + * + * @param params The collection of argument values + * @return The argument values quoted and space-delimited, or an empty String if params was null/Nil/None + */ + protected def repeat( params: Traversable[_] ): String = { + repeat("", params) + } + + /** + * Given an (optional) prefix, an argument value, and an (optional) suffix, formats a command-line + * argument with the specified level of quoting and space-separation. + * + * Helper method for required(), optional(), conditional(), and repeat() -- do not use this + * method directly! + * + * @param prefix Prefix to insert before the argument value (eg., "-f"). Ignored if empty/null. + * @param param The argument value itself. If this is Some(x), it is unwrapped to x before processing. + * @param suffix Suffix to append after the argument value. Ignored if empty/null. + * @param spaceSeparated If true, insert a space between the prefix, param, and suffix + * @param escape If true, quote the generated argument to avoid interpretation by the shell + * @param paramFormat Format string used to convert param to a String + * @return The combined and formatted argument, NOT surrounded by any whitespace. + * Returns an empty String if param was null/empty. + */ + protected def formatArgument( prefix: String, param: Any, suffix: String, spaceSeparated: Boolean, escape: Boolean, + paramFormat: String ): String = { + if (CollectionUtils.isNullOrEmpty(param)) { + return "" + } + + // Trim leading and trailing whitespace off our three tokens, and unwrap Some(x) to x for the param + val trimmedValues : List[String] = List((if ( prefix != null ) prefix.trim else ""), + (param match { + case Some(x) => paramFormat.format(x).trim + case x => paramFormat.format(x).trim + }), + (if ( suffix != null ) suffix.trim else "")) + var joinedArgument : String = null + + // If the user requested space-separation, join the tokens with a space, and escape individual + // NON-EMPTY tokens if escaping was requested (eg., ("-f", "foo", "") -> "'-f' 'foo'") + if ( spaceSeparated ) { + joinedArgument = trimmedValues.map(x => if ( x.length > 0 && escape ) ShellUtils.escapeShellArgument(x) else x).mkString(" ").trim() + } + + // Otherwise join the tokens without any intervening whitespace, and if quoting was requested + // quote the entire concatenated value (eg., ("-Xmx", "4", "G") -> "'-Xmx4G'") + else { + joinedArgument = if ( escape ) ShellUtils.escapeShellArgument(trimmedValues.mkString("")) else trimmedValues.mkString("") + } + + // If the user requested escaping and we ended up with an empty String after joining, quote the empty + // String to preserve the command line token. Otherwise just return the joined argument + if ( joinedArgument.length == 0 && escape ) ShellUtils.escapeShellArgument(joinedArgument) else joinedArgument + } } diff --git a/public/scala/src/org/broadinstitute/sting/queue/function/JavaCommandLineFunction.scala b/public/scala/src/org/broadinstitute/sting/queue/function/JavaCommandLineFunction.scala index 4a50a72ac..8cbc2c577 100644 --- a/public/scala/src/org/broadinstitute/sting/queue/function/JavaCommandLineFunction.scala +++ b/public/scala/src/org/broadinstitute/sting/queue/function/JavaCommandLineFunction.scala @@ -49,18 +49,6 @@ trait JavaCommandLineFunction extends CommandLineFunction { */ var javaMemoryLimit: Option[Double] = None - /** - * Returns the java executable to run. - */ - def javaExecutable: String = { - if (jarFile != null) - "-jar " + jarFile - else if (javaMainClass != null) - "-cp \"%s\" %s".format(javaClasspath.mkString(File.pathSeparator), javaMainClass) - else - null - } - override def freezeFieldValues() { super.freezeFieldValues() @@ -71,11 +59,25 @@ trait JavaCommandLineFunction extends CommandLineFunction { javaClasspath = JavaCommandLineFunction.currentClasspath } - def javaOpts = "%s -Djava.io.tmpdir=%s" - .format(optional(" -Xmx", javaMemoryLimit.map(gb => (gb * 1024).ceil.toInt), "m"), jobTempDir) + /** + * Returns the java executable to run. + */ + def javaExecutable: String = { + if (jarFile != null) + required("-jar", jarFile) + else if (javaMainClass != null) + required("-cp", javaClasspath.mkString(File.pathSeparator)) + + required(javaMainClass) + else + null + } - def commandLine = "java%s %s" - .format(javaOpts, javaExecutable) + def javaOpts = required("-Xmx", javaMemoryLimit.map(gb => (gb * 1024).ceil.toInt), "m", spaceSeparated=false) + + required("-Djava.io.tmpdir=", jobTempDir, spaceSeparated=false) + + def commandLine = required("java") + + javaOpts + + javaExecutable } object JavaCommandLineFunction { diff --git a/public/scala/src/org/broadinstitute/sting/queue/util/ShellUtils.scala b/public/scala/src/org/broadinstitute/sting/queue/util/ShellUtils.scala new file mode 100644 index 000000000..3cb1a705a --- /dev/null +++ b/public/scala/src/org/broadinstitute/sting/queue/util/ShellUtils.scala @@ -0,0 +1,36 @@ +package org.broadinstitute.sting.queue.util + +import java.lang.IllegalArgumentException + +object ShellUtils { + + /** + * Escapes the String it's passed so that it will be interpreted literally when + * parsed by sh/bash. Can correctly escape all characters except \0, \r, and \n + * + * Replaces all instances of ' with '\'', and then surrounds the resulting String + * with single quotes. + * + * Examples: + * ab -> 'ab' + * a'b -> 'a'\''b' + * '' -> ''\'''\''' + * + * Since \' is not supported inside single quotes in the shell (ie., '\'' does not work), + * whenever we encounter a single quote we need to terminate the existing single-quoted + * string, place the \' outside of single quotes, and then start a new single-quoted + * string. As long as we don't insert spaces between the separate strings, the shell will + * concatenate them together into a single argument value for us. + * + * @param str the String to escape + * @return the same String quoted so that it will be interpreted literally when + * parsed by sh/bash + */ + def escapeShellArgument ( str : String ) : String = { + if ( str == null ) { + throw new IllegalArgumentException("escapeShellArgument() was passed a null String") + } + + "'" + str.replaceAll("'", "'\\\\''") + "'" + } +} \ No newline at end of file diff --git a/public/scala/test/org/broadinstitute/sting/queue/function/CommandLineFunctionUnitTest.scala b/public/scala/test/org/broadinstitute/sting/queue/function/CommandLineFunctionUnitTest.scala new file mode 100644 index 000000000..eb50c3a2e --- /dev/null +++ b/public/scala/test/org/broadinstitute/sting/queue/function/CommandLineFunctionUnitTest.scala @@ -0,0 +1,171 @@ +package org.broadinstitute.sting.queue.function + +import org.testng.Assert +import org.testng.annotations.{DataProvider, Test} + +// Since "protected" in Scala is subclass-only and doesn't allow package-level access, we need to +// extend a class if we want to test it +class CommandLineFunctionUnitTest extends CommandLineFunction { + def commandLine = "" + + @DataProvider( name="formatArgumentTestData" ) + def formatArgumentDataProvider = { + Array(Array("", "argvalue", "", true, true, "'argvalue'"), + Array("", "argvalue", "", true, false, "argvalue"), + Array("", "argvalue", "", false, true, "'argvalue'"), + Array("", "argvalue", "", false, false, "argvalue"), + Array("-arg", "argvalue", "", true, true, "'-arg' 'argvalue'"), + Array("-arg", "argvalue", "", true, false, "-arg argvalue"), + Array("ARGNAME=", "ARGVALUE", "", false, true, "'ARGNAME=ARGVALUE'"), + Array("ARGNAME=", "ARGVALUE", "", false, false, "ARGNAME=ARGVALUE"), + Array("-Xmx", "4", "G", true, true, "'-Xmx' '4' 'G'"), + Array("-Xmx", "4", "G", true, false, "-Xmx 4 G"), + Array("-Xmx", "4", "G", false, true, "'-Xmx4G'"), + Array("-Xmx", "4", "G", false, false, "-Xmx4G"), + Array("", "", "", true, true, "''"), + Array("", "", "", true, false, ""), + Array("", "", "", false, true, "''"), + Array("", "", "", false, false, ""), + Array("", null, "", true, true, ""), + Array("", Nil, "", true, true, ""), + Array("", None, "", true, true, ""), + Array(null, null, null, true, true, ""), + Array("", Some("argvalue"), "", true, true, "'argvalue'") + ) + } + + @Test( dataProvider="formatArgumentTestData" ) + def testFormatArgument( prefix: String, param: Any, suffix: String, spaceSeparated: Boolean, escape: Boolean, expectedReturnValue: String ) { + Assert.assertEquals(formatArgument(prefix, param, suffix, spaceSeparated, escape, "%s"), + expectedReturnValue) + } + + @Test + def testFormatArgumentCustomFormatString() { + Assert.assertEquals(formatArgument("", "argvalue", "", true, true, "%.3s"), "'arg'") + } + + @DataProvider( name = "requiredTestData" ) + def requiredDataProvider = { + Array(Array("", "argvalue", "", true, true, " 'argvalue' "), + Array("", "argvalue", "", true, false, " argvalue "), + Array("", "argvalue", "", false, true, " 'argvalue' "), + Array("", "argvalue", "", false, false, " argvalue "), + Array("-arg", "argvalue", "", true, true, " '-arg' 'argvalue' "), + Array("-arg", "argvalue", "", true, false, " -arg argvalue "), + Array("ARGNAME=", "ARGVALUE", "", false, true, " 'ARGNAME=ARGVALUE' "), + Array("ARGNAME=", "ARGVALUE", "", false, false, " ARGNAME=ARGVALUE "), + Array("-Xmx", "4", "G", true, true, " '-Xmx' '4' 'G' "), + Array("-Xmx", "4", "G", true, false, " -Xmx 4 G "), + Array("-Xmx", "4", "G", false, true, " '-Xmx4G' "), + Array("-Xmx", "4", "G", false, false, " -Xmx4G "), + Array("", "", "", true, true, " '' "), + Array("", "", "", true, false, " "), + Array("", "", "", false, true, " '' "), + Array("", "", "", false, false, " "), + Array("", null, "", true, true, " "), + Array("", Nil, "", true, true, " "), + Array("", None, "", true, true, " ") + ) + } + + @Test( dataProvider="requiredTestData" ) + def testRequired( prefix: String, param: Any, suffix: String, spaceSeparated: Boolean, escape: Boolean, expectedReturnValue: String ) { + Assert.assertEquals(required(prefix, param, suffix, spaceSeparated, escape), + expectedReturnValue) + } + + @DataProvider( name = "optionalTestData" ) + def optionalDataProvider = { + Array(Array("-arg", "argvalue", "", true, true, " '-arg' 'argvalue' "), + Array("-arg", null, "", true, true, ""), + Array("-arg", Nil, "", true, true, ""), + Array("-arg", None, "", true, true, ""), + Array("-arg", "", "", true, true, " '-arg' ") + ) + } + + @Test( dataProvider="optionalTestData" ) + def testOptional( prefix: String, param: Any, suffix: String, spaceSeparated: Boolean, escape: Boolean, expectedReturnValue: String ) { + Assert.assertEquals(optional(prefix, param, suffix, spaceSeparated, escape), + expectedReturnValue) + } + + @DataProvider( name = "conditionalTestData" ) + def conditionalDataProvider = { + Array(Array(true, "-FLAG", true, " '-FLAG' "), + Array(true, "-FLAG", false, " -FLAG "), + Array(false, "-FLAG", true, ""), + Array(false, "-FLAG", false, ""), + Array(true, null, true, " "), + Array(true, Nil, true, " "), + Array(true, None, true, " "), + Array(false, null, true, ""), + Array(false, Nil, true, ""), + Array(false, None, true, "") + ) + } + + @Test( dataProvider="conditionalTestData" ) + def testConditional( condition: Boolean, param: Any, escape: Boolean, expectedReturnValue: String ) { + Assert.assertEquals(conditional(condition, param, escape), + expectedReturnValue) + } + + @DataProvider( name = "repeatTestData" ) + def repeatDataProvider = { + Array(Array("", List("a", "bc", "d"), "", " ", true, true, " 'a' 'bc' 'd' "), + Array("", List("a", "bc", "d"), "", " ", true, false, " a bc d "), + Array("", List("a", "bc", "d"), "", "", true, true, " 'a''bc''d' "), + Array("", List("a", "bc", "d"), "", "", true, false, " abcd "), + Array("-f", List("file1", "file2", "file3"), "", " ", true, true, " '-f' 'file1' '-f' 'file2' '-f' 'file3' "), + Array("-f", List("file1", "file2", "file3"), "", " ", true, false, " -f file1 -f file2 -f file3 "), + Array("-f", List("file1", "file2", "file3"), "", " ", false, true, " '-ffile1' '-ffile2' '-ffile3' "), + Array("-f", List("file1", "file2", "file3"), "", " ", false, false, " -ffile1 -ffile2 -ffile3 "), + Array("-f", List("file1", "file2", "file3"), "", "", false, true, " '-ffile1''-ffile2''-ffile3' "), + Array("-f", List("file1", "file2", "file3"), "", "", false, false, " -ffile1-ffile2-ffile3 "), + Array("-f", List("file1", "file2", "file3"), "suffix", " ", true, true, " '-f' 'file1' 'suffix' '-f' 'file2' 'suffix' '-f' 'file3' 'suffix' "), + Array("-f", List("file1", "file2", "file3"), "suffix", " ", true, false, " -f file1 suffix -f file2 suffix -f file3 suffix "), + Array("-f", List("file1", "file2", "file3"), "suffix", " ", false, true, " '-ffile1suffix' '-ffile2suffix' '-ffile3suffix' "), + Array("-f", List("file1", "file2", "file3"), "suffix", " ", false, false, " -ffile1suffix -ffile2suffix -ffile3suffix "), + Array("-f", null, "", " ", true, true, ""), + Array("-f", Nil, "", " ", true, true, "") + ) + } + + @Test( dataProvider="repeatTestData" ) + def testRepeat( prefix: String, params: Traversable[_], suffix: String, separator: String, + spaceSeparated: Boolean, escape: Boolean, expectedReturnValue: String ) { + Assert.assertEquals(repeat(prefix, params, suffix, separator, spaceSeparated, escape), + expectedReturnValue) + } + + // Need to test None separately due to implicit conversion issues when using None in a TestNG data provider + @Test + def testRepeatNone() { + testRepeat("", None, "", " ", true, true, "") + } + + @DataProvider( name = "repeatWithPrefixFormattingTestData" ) + def repeatWithPrefixFormattingDataProvider = { + Array(Array("-f", List("file1", "file2", "file3"), "", " ", true, true, (prefix: String, value: Any) => "%s:tag%s".format(prefix, value), + " '-f:tagfile1' 'file1' '-f:tagfile2' 'file2' '-f:tagfile3' 'file3' "), + Array("-f", List("file1", "file2", "file3"), "", " ", true, false, (prefix: String, value: Any) => "%s:tag%s".format(prefix, value), + " -f:tagfile1 file1 -f:tagfile2 file2 -f:tagfile3 file3 "), + Array("", List("file1", "file2", "file3"), "", " ", true, true, (prefix: String, value: Any) => "-%s".format(value), + " '-file1' 'file1' '-file2' 'file2' '-file3' 'file3' "), + Array("-f", null, "", " ", true, true, (prefix: String, value: Any) => "%s:tag%s".format(prefix, value), + ""), + Array("-f", Nil, "", " ", true, true, (prefix: String, value: Any) => "%s:tag%s".format(prefix, value), + "") + ) + } + + @Test( dataProvider = "repeatWithPrefixFormattingTestData" ) + def testRepeatWithPrefixFormatting( prefix: String, params: Traversable[_], suffix: String, separator: String, + spaceSeparated: Boolean, escape: Boolean, formatPrefix: (String, Any) => String, + expectedReturnValue: String ) { + Assert.assertEquals(repeat(prefix, params, suffix, separator, spaceSeparated, escape, "%s", formatPrefix), + expectedReturnValue) + } +} \ No newline at end of file diff --git a/public/scala/test/org/broadinstitute/sting/queue/util/ShellUtilsUnitTest.scala b/public/scala/test/org/broadinstitute/sting/queue/util/ShellUtilsUnitTest.scala new file mode 100644 index 000000000..76585d207 --- /dev/null +++ b/public/scala/test/org/broadinstitute/sting/queue/util/ShellUtilsUnitTest.scala @@ -0,0 +1,57 @@ +package org.broadinstitute.sting.queue.util + +import org.testng.annotations.Test +import org.testng.Assert +import java.io.{InputStreamReader, BufferedReader} + +class ShellUtilsUnitTest { + + @Test + def testEscapeShellArgumentOneCharSequences() { + // Test all ASCII characters except \0, \n, and \r, which we do not support escaping + for ( asciiCode <- 1 to 127 if asciiCode != 10 && asciiCode != 13 ) { + val originalString: String = "%c".format(asciiCode.toChar) + val quotedString: String = ShellUtils.escapeShellArgument(originalString) + + val child : Process = new ProcessBuilder("/bin/sh", "-c", "printf \"%s\" " + quotedString).start() + val childReader : BufferedReader = new BufferedReader(new InputStreamReader(child.getInputStream)) + val childOutputBuffer : StringBuilder = new StringBuilder + + val childReaderThread : Thread = new Thread(new Runnable() { + def run() { + var line : String = childReader.readLine() + + while ( line != null ) { + childOutputBuffer.append(line) + line = childReader.readLine() + } + } + }) + childReaderThread.start() + + val childReturnValue = child.waitFor() + childReaderThread.join() + + childReader.close() + val childOutput = childOutputBuffer.toString() + + if ( childReturnValue != 0 ) { + Assert.fail("With character ASCII %d, sh child process returned: %d".format(asciiCode, childReturnValue)) + } + else if ( ! originalString.equals(childOutput) ) { + Assert.fail("With character ASCII %d, sh child process output \"%s\" instead of the expected \"%s\"".format( + asciiCode, childOutput, originalString)) + } + } + } + + @Test(expectedExceptions = Array(classOf[IllegalArgumentException])) + def testEscapeShellArgumentNullString() { + ShellUtils.escapeShellArgument(null) + } + + @Test + def testEscapeShellArgumentEmptyString() { + Assert.assertEquals(ShellUtils.escapeShellArgument(""), "''") + } +} \ No newline at end of file