Merge pull request #8207 from atmire/w2p-88141_scripts-and-processes-bug-7992

Fix Scripts and Processes bug #7992

dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataDeletionScriptConfiguration.java

@@ -41,10 +41,8 @@ public class MetadataDeletionScriptConfiguration<T extends MetadataDeletion> ext
             Options options = new Options();
 
             options.addOption("m", "metadata", true, "metadata field name");
-            options.getOption("m").setType(String.class);
 
             options.addOption("l", "list", false, "lists the metadata fields that can be deleted");
-            options.getOption("l").setType(boolean.class);
 
             super.options = options;
         }

dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataExportScriptConfiguration.java

@@ -54,12 +54,9 @@ public class MetadataExportScriptConfiguration<T extends MetadataExport> extends
             Options options = new Options();
 
             options.addOption("i", "id", true, "ID or handle of thing to export (item, collection, or community)");
-            options.getOption("i").setType(String.class);
             options.addOption("a", "all", false,
                               "include all metadata fields that are not normally changed (e.g. provenance)");
-            options.getOption("a").setType(boolean.class);
             options.addOption("h", "help", false, "help");
-            options.getOption("h").setType(boolean.class);
 
 
             super.options = options;

dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataImportCliScriptConfiguration.java

@@ -19,7 +19,6 @@ public class MetadataImportCliScriptConfiguration extends MetadataImportScriptCo
     public Options getOptions() {
         Options options = super.getOptions();
         options.addOption("e", "email", true, "email address or user id of user (required if adding new items)");
-        options.getOption("e").setType(String.class);
         options.getOption("e").setRequired(true);
         super.options = options;
         return options;

dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataImportScriptConfiguration.java

@@ -59,20 +59,14 @@ public class MetadataImportScriptConfiguration<T extends MetadataImport> extends
             options.getOption("f").setRequired(true);
             options.addOption("s", "silent", false,
                               "silent operation - doesn't request confirmation of changes USE WITH CAUTION");
-            options.getOption("s").setType(boolean.class);
             options.addOption("w", "workflow", false, "workflow - when adding new items, use collection workflow");
-            options.getOption("w").setType(boolean.class);
             options.addOption("n", "notify", false,
                               "notify - when adding new items using a workflow, send notification emails");
-            options.getOption("n").setType(boolean.class);
             options.addOption("v", "validate-only", false,
                               "validate - just validate the csv, don't run the import");
-            options.getOption("v").setType(boolean.class);
             options.addOption("t", "template", false,
                               "template - when adding new items, use the collection template (if it exists)");
-            options.getOption("t").setType(boolean.class);
             options.addOption("h", "help", false, "help");
-            options.getOption("h").setType(boolean.class);
 
             super.options = options;
         }

dspace-api/src/main/java/org/dspace/app/harvest/HarvestScriptConfiguration.java

@@ -43,22 +43,14 @@ public class HarvestScriptConfiguration<T extends Harvest> extends ScriptConfigu
     public Options getOptions() {
         Options options = new Options();
         options.addOption("p", "purge", false, "delete all items in the collection");
-        options.getOption("p").setType(boolean.class);
         options.addOption("r", "run", false, "run the standard harvest procedure");
-        options.getOption("r").setType(boolean.class);
         options.addOption("g", "ping", false, "test the OAI server and set");
-        options.getOption("g").setType(boolean.class);
         options.addOption("s", "setup", false, "Set the collection up for harvesting");
-        options.getOption("s").setType(boolean.class);
         options.addOption("S", "start", false, "start the harvest loop");
-        options.getOption("S").setType(boolean.class);
         options.addOption("R", "reset", false, "reset harvest status on all collections");
-        options.getOption("R").setType(boolean.class);
         options.addOption("P", "purgeCollections", false, "purge all harvestable collections");
-        options.getOption("P").setType(boolean.class);
         options.addOption("o", "reimport", false, "reimport all items in the collection, " +
             "this is equivalent to -p -r, purging all items in a collection and reimporting them");
-        options.getOption("o").setType(boolean.class);
         options.addOption("c", "collection", true,
                           "harvesting collection (handle or id)");
         options.addOption("t", "type", true,
@@ -72,7 +64,6 @@ public class HarvestScriptConfiguration<T extends Harvest> extends ScriptConfigu
                                   "crosswalk in dspace.cfg");
 
         options.addOption("h", "help", false, "help");
-        options.getOption("h").setType(boolean.class);
 
         return options;
     }

dspace-api/src/main/java/org/dspace/app/mediafilter/MediaFilterScriptConfiguration.java

@@ -50,15 +50,11 @@ public class MediaFilterScriptConfiguration<T extends MediaFilterScript> extends
     public Options getOptions() {
         Options options = new Options();
         options.addOption("v", "verbose", false, "print all extracted text and other details to STDOUT");
-        options.getOption("v").setType(boolean.class);
         options.addOption("q", "quiet", false, "do not print anything except in the event of errors.");
-        options.getOption("q").setType(boolean.class);
         options.addOption("f", "force", false, "force all bitstreams to be processed");
-        options.getOption("f").setType(boolean.class);
         options.addOption("i", "identifier", true, "ONLY process bitstreams belonging to identifier");
         options.addOption("m", "maximum", true, "process no more than maximum items");
         options.addOption("h", "help", false, "help");
-        options.getOption("h").setType(boolean.class);
 
         Option pluginOption = Option.builder("p")
                                     .longOpt("plugins")

dspace-api/src/main/java/org/dspace/curate/CurationCliScriptConfiguration.java

@@ -19,7 +19,6 @@ public class CurationCliScriptConfiguration extends CurationScriptConfiguration<
     public Options getOptions() {
         options = super.getOptions();
         options.addOption("e", "eperson", true, "email address of curating eperson");
-        options.getOption("e").setType(String.class);
         options.getOption("e").setRequired(true);
         return options;
     }

dspace-api/src/main/java/org/dspace/discovery/IndexClientOptions.java

@@ -74,25 +74,17 @@ public enum IndexClientOptions {
 
         options
             .addOption("r", "remove", true, "remove an Item, Collection or Community from index based on its handle");
-        options.getOption("r").setType(String.class);
         options.addOption("i", "index", true,
                           "add or update an Item, Collection or Community based on its handle or uuid");
-        options.getOption("i").setType(boolean.class);
         options.addOption("c", "clean", false,
                           "clean existing index removing any documents that no longer exist in the db");
-        options.getOption("c").setType(boolean.class);
         options.addOption("d", "delete", false,
                 "delete all records from existing index");
-        options.getOption("d").setType(boolean.class);
         options.addOption("b", "build", false, "(re)build index, wiping out current one if it exists");
-        options.getOption("b").setType(boolean.class);
         options.addOption("s", "spellchecker", false, "Rebuild the spellchecker, can be combined with -b and -f.");
-        options.getOption("s").setType(boolean.class);
         options.addOption("f", "force", false,
                           "if updating existing index, force each handle to be reindexed even if uptodate");
-        options.getOption("f").setType(boolean.class);
         options.addOption("h", "help", false, "print this help message");
-        options.getOption("h").setType(boolean.class);
         return options;
     }
 }

dspace-api/src/main/java/org/dspace/statistics/export/RetryFailedOpenUrlTrackerScriptConfiguration.java

@@ -56,15 +56,12 @@ public class RetryFailedOpenUrlTrackerScriptConfiguration<T extends RetryFailedO
             Options options = new Options();
 
             options.addOption("a", true, "Add a new \"failed\" row to the table with a url (test purposes only)");
-            options.getOption("a").setType(String.class);
 
             options.addOption("r", false,
                               "Retry sending requests to all urls stored in the table with failed requests. " +
                                       "This includes the url that can be added through the -a option.");
-            options.getOption("r").setType(boolean.class);
 
             options.addOption("h", "help", false, "print this help message");
-            options.getOption("h").setType(boolean.class);
 
             super.options = options;
         }

dspace-api/src/main/java/org/dspace/submit/migration/SubmissionFormsMigrationCliScriptConfiguration.java

@@ -53,11 +53,8 @@ public class SubmissionFormsMigrationCliScriptConfiguration<T extends Submission
             Options options = new Options();
 
             options.addOption("f", "input-forms", true, "Path to source input-forms.xml file location");
-            options.getOption("f").setType(String.class);
             options.addOption("s", "item-submission", true, "Path to source item-submission.xml file location");
-            options.getOption("s").setType(String.class);
             options.addOption("h", "help", false, "help");
-            options.getOption("h").setType(boolean.class);
 
             super.options = options;
         }

dspace-api/src/test/java/org/dspace/scripts/MockDSpaceRunnableScriptConfiguration.java

@@ -54,9 +54,7 @@ public class MockDSpaceRunnableScriptConfiguration<T extends MockDSpaceRunnableS
             Options options = new Options();
 
             options.addOption("r", "remove", true, "description r");
-            options.getOption("r").setType(String.class);
             options.addOption("i", "index", false, "description i");
-            options.getOption("i").setType(boolean.class);
             options.getOption("i").setRequired(true);
             options.addOption("f", "file", true, "source file");
             options.getOption("f").setType(InputStream.class);

dspace-server-webapp/src/main/java/org/dspace/app/rest/converter/ScriptConverter.java

@@ -12,6 +12,7 @@ import java.util.List;
 
 import org.apache.commons.cli.Option;
 import org.apache.commons.collections4.CollectionUtils;
+import org.apache.commons.lang.StringUtils;
 import org.dspace.app.rest.model.ParameterRest;
 import org.dspace.app.rest.model.ScriptRest;
 import org.dspace.app.rest.projection.Projection;
@@ -39,7 +40,7 @@ public class ScriptConverter implements DSpaceConverter<ScriptConfiguration, Scr
             parameterRest.setDescription(option.getDescription());
             parameterRest.setName((option.getOpt() != null ? "-" + option.getOpt() : "--" + option.getLongOpt()));
             parameterRest.setNameLong(option.getLongOpt() != null ? "--" + option.getLongOpt() : null);
-            parameterRest.setType(((Class) option.getType()).getSimpleName());
+            parameterRest.setType(getType(option));
             parameterRest.setMandatory(option.isRequired());
             parameterRestList.add(parameterRest);
         }
@@ -48,6 +49,29 @@ public class ScriptConverter implements DSpaceConverter<ScriptConfiguration, Scr
         return scriptRest;
     }
 
+    /**
+     * Retrieve the type for this Option. In Apache Commons CLI, Option objects default to a "String" type.
+     *
+     * In most situations, the Option type will correspond to the Java Class name (e.g. "String", "boolean", etc.).
+     * However, when the Option type is "String" (the default) and the Option is configured to not allow for
+     * additional arguments (hasArg=false), we switch the type to be "boolean". This behavior is necessary to ensure
+     * our REST API differentiates between an Option that requires an additional argument (which our REST API returns
+     * as a "String" type), and an Option which does not accept additional arguments (which is just a boolean flag
+     * used to trigger different behavior, hence a "boolean" type is more appropriate).
+     *
+     * @param option Option to retrieve the type for
+     * @return the type of the option based on the aforementioned logic
+     */
+    private String getType(Option option) {
+        String simpleName = ((Class) option.getType()).getSimpleName();
+        if (StringUtils.equalsIgnoreCase(simpleName, "string")) {
+            if (!option.hasArg()) {
+                return boolean.class.getSimpleName();
+            }
+        }
+        return simpleName;
+    }
+
     @Override
     public Class<ScriptConfiguration> getModelClass() {
         return ScriptConfiguration.class;

dspace-server-webapp/src/test/data/dspaceFolder/config/spring/rest/scripts.xml

@@ -27,4 +27,10 @@
         <property name="description" value="Delete all the values of the specified metadata field"/>
         <property name="dspaceRunnableClass" value="org.dspace.app.bulkedit.MetadataDeletion"/>
     </bean>
-</beans>
+
+     <bean id="type-conversion-test" class="org.dspace.app.scripts.TypeConversionTestScriptConfiguration" primary="true">
+        <property name="description" value="Test the type conversion different option types"/>
+        <property name="dspaceRunnableClass" value="org.dspace.app.scripts.TypeConversionTestScript"/>
+    </bean>
+
+</beans>

dspace-server-webapp/src/test/java/org/dspace/app/rest/ScriptRestRepositoryIT.java

@@ -8,6 +8,8 @@
 package org.dspace.app.rest;
 
 import static com.jayway.jsonpath.JsonPath.read;
+import static com.jayway.jsonpath.matchers.JsonPathMatchers.hasJsonPath;
+import static org.hamcrest.Matchers.allOf;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.hasItem;
 import static org.hamcrest.Matchers.is;
@@ -478,6 +480,47 @@ public class ScriptRestRepositoryIT extends AbstractControllerIntegrationTest {
         }
     }
 
+    @Test
+    public void scriptTypeConversionTest() throws Exception {
+        String token = getAuthToken(admin.getEmail(), password);
+
+        getClient(token).perform(get("/api/system/scripts/type-conversion-test"))
+                        .andExpect(status().isOk())
+                        .andExpect(jsonPath("$", ScriptMatcher
+                                .matchScript("type-conversion-test",
+                                             "Test the type conversion different option types")))
+                        .andExpect(jsonPath("$.parameters", containsInAnyOrder(
+                                allOf(
+                                        hasJsonPath("$.name", is("-b")),
+                                        hasJsonPath("$.description", is("option set to the boolean class")),
+                                        hasJsonPath("$.type", is("boolean")),
+                                        hasJsonPath("$.mandatory", is(false)),
+                                        hasJsonPath("$.nameLong", is("--boolean"))
+                                ),
+                                allOf(
+                                        hasJsonPath("$.name", is("-s")),
+                                        hasJsonPath("$.description", is("string option with an argument")),
+                                        hasJsonPath("$.type", is("String")),
+                                        hasJsonPath("$.mandatory", is(false)),
+                                        hasJsonPath("$.nameLong", is("--string"))
+                                ),
+                                allOf(
+                                        hasJsonPath("$.name", is("-n")),
+                                        hasJsonPath("$.description", is("string option without an argument")),
+                                        hasJsonPath("$.type", is("boolean")),
+                                        hasJsonPath("$.mandatory", is(false)),
+                                        hasJsonPath("$.nameLong", is("--noargument"))
+                                ),
+                                allOf(
+                                        hasJsonPath("$.name", is("-f")),
+                                        hasJsonPath("$.description", is("file option with an argument")),
+                                        hasJsonPath("$.type", is("InputStream")),
+                                        hasJsonPath("$.mandatory", is(false)),
+                                        hasJsonPath("$.nameLong", is("--file"))
+                                )
+                        ) ));
+    }
+
 
     @After
     public void destroy() throws Exception {

dspace-server-webapp/src/test/java/org/dspace/app/rest/matcher/ScriptMatcher.java

@@ -11,6 +11,7 @@ import static com.jayway.jsonpath.matchers.JsonPathMatchers.hasJsonPath;
 import static org.hamcrest.Matchers.allOf;
 import static org.hamcrest.Matchers.is;
 
+import org.apache.commons.cli.Option;
 import org.apache.commons.cli.Options;
 import org.hamcrest.Matcher;
 import org.hamcrest.Matchers;
@@ -28,11 +29,15 @@ public class ScriptMatcher {
     }
 
     public static Matcher<? super Object> matchMockScript(Options options) {
+        // Set the to be compared option to the expected boolean type
+        Option i = options.getOption("i");
+        i.setType(boolean.class);
+
         return allOf(
             matchScript("mock-script", "Mocking a script for testing purposes"),
             hasJsonPath("$.parameters", Matchers.containsInAnyOrder(
                 ParameterMatcher.matchParameter(options.getOption("r")),
-                ParameterMatcher.matchParameter(options.getOption("i")),
+                ParameterMatcher.matchParameter(i),
                 ParameterMatcher.matchParameter(options.getOption("f"))
             ))
         );

dspace-server-webapp/src/test/java/org/dspace/app/scripts/TypeConversionTestScript.java

@@ -0,0 +1,33 @@
+/**
+ * The contents of this file are subject to the license and copyright
+ * detailed in the LICENSE and NOTICE files at the root of the source
+ * tree and available online at
+ *
+ * http://www.dspace.org/license/
+ */
+package org.dspace.app.scripts;
+
+import org.apache.commons.cli.ParseException;
+import org.dspace.app.rest.converter.ScriptConverter;
+import org.dspace.scripts.DSpaceRunnable;
+import org.dspace.utils.DSpace;
+
+/**
+ * Script used to test the type conversion in the {@link ScriptConverter}
+ */
+public class TypeConversionTestScript extends DSpaceRunnable<TypeConversionTestScriptConfiguration> {
+
+
+    public TypeConversionTestScriptConfiguration getScriptConfiguration() {
+        return new DSpace().getServiceManager()
+                           .getServiceByName("type-conversion-test", TypeConversionTestScriptConfiguration.class);
+    }
+
+    public void setup() throws ParseException {
+        // This script is only used to test rest exposure, no setup is required.
+    }
+
+    public void internalRun() throws Exception {
+        // This script is only used to test rest exposure, no internal run is required.
+    }
+}

dspace-server-webapp/src/test/java/org/dspace/app/scripts/TypeConversionTestScriptConfiguration.java

@@ -0,0 +1,47 @@
+/**
+ * The contents of this file are subject to the license and copyright
+ * detailed in the LICENSE and NOTICE files at the root of the source
+ * tree and available online at
+ *
+ * http://www.dspace.org/license/
+ */
+package org.dspace.app.scripts;
+
+import java.io.InputStream;
+
+import org.apache.commons.cli.Options;
+import org.dspace.app.rest.converter.ScriptConverter;
+import org.dspace.core.Context;
+import org.dspace.scripts.configuration.ScriptConfiguration;
+
+/**
+ * Script configuration used to test the type conversion in the {@link ScriptConverter}
+ */
+public class TypeConversionTestScriptConfiguration<T extends TypeConversionTestScript> extends ScriptConfiguration<T> {
+
+
+    public Class<T> getDspaceRunnableClass() {
+        return null;
+    }
+
+    public void setDspaceRunnableClass(final Class<T> dspaceRunnableClass) {
+
+    }
+
+    public boolean isAllowedToExecute(final Context context) {
+        return true;
+    }
+
+    public Options getOptions() {
+
+        Options options = new Options();
+
+        options.addOption("b", "boolean", false, "option set to the boolean class");
+        options.addOption("s", "string", true, "string option with an argument");
+        options.addOption("n", "noargument", false, "string option without an argument");
+        options.addOption("f", "file", true, "file option with an argument");
+        options.getOption("f").setType(InputStream.class);
+
+        return options;
+    }
+}

dspace-server-webapp/src/test/java/org/dspace/scripts/MockDSpaceRunnableScriptConfiguration.java

@@ -54,9 +54,7 @@ public class MockDSpaceRunnableScriptConfiguration<T extends MockDSpaceRunnableS
             Options options = new Options();
 
             options.addOption("r", "remove", true, "description r");
-            options.getOption("r").setType(String.class);
             options.addOption("i", "index", false, "description i");
-            options.getOption("i").setType(boolean.class);
             options.getOption("i").setRequired(true);
             options.addOption("f", "file", true, "source file");
             options.getOption("f").setType(InputStream.class);