From 2d7a52dc005323df0d41cc723c5063a72a44469d Mon Sep 17 00:00:00 2001 From: Yana De Pauw Date: Tue, 8 Mar 2022 13:24:53 +0100 Subject: [PATCH 1/4] 88141: Scripts and processes bug #7992 --- .../app/rest/converter/ScriptConverter.java | 23 ++++++++- .../config/spring/rest/scripts.xml | 8 +++- .../app/rest/ScriptRestRepositoryIT.java | 43 +++++++++++++++++ .../app/scripts/TypeConversionTestScript.java | 33 +++++++++++++ ...TypeConversionTestScriptConfiguration.java | 48 +++++++++++++++++++ 5 files changed, 153 insertions(+), 2 deletions(-) create mode 100644 dspace-server-webapp/src/test/java/org/dspace/app/scripts/TypeConversionTestScript.java create mode 100644 dspace-server-webapp/src/test/java/org/dspace/app/scripts/TypeConversionTestScriptConfiguration.java diff --git a/dspace-server-webapp/src/main/java/org/dspace/app/rest/converter/ScriptConverter.java b/dspace-server-webapp/src/main/java/org/dspace/app/rest/converter/ScriptConverter.java index 105975355b..9c9957a55b 100644 --- a/dspace-server-webapp/src/main/java/org/dspace/app/rest/converter/ScriptConverter.java +++ b/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 getModelClass() { return ScriptConfiguration.class; diff --git a/dspace-server-webapp/src/test/data/dspaceFolder/config/spring/rest/scripts.xml b/dspace-server-webapp/src/test/data/dspaceFolder/config/spring/rest/scripts.xml index ab9826b84d..4e88ef2763 100644 --- a/dspace-server-webapp/src/test/data/dspaceFolder/config/spring/rest/scripts.xml +++ b/dspace-server-webapp/src/test/data/dspaceFolder/config/spring/rest/scripts.xml @@ -27,4 +27,10 @@ - \ No newline at end of file + + + + + + + diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/ScriptRestRepositoryIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/ScriptRestRepositoryIT.java index 9976fb4be1..48cd26227c 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/ScriptRestRepositoryIT.java +++ b/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; @@ -484,6 +486,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 { diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/scripts/TypeConversionTestScript.java b/dspace-server-webapp/src/test/java/org/dspace/app/scripts/TypeConversionTestScript.java new file mode 100644 index 0000000000..824e5dda9f --- /dev/null +++ b/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 { + + + 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. + } +} diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/scripts/TypeConversionTestScriptConfiguration.java b/dspace-server-webapp/src/test/java/org/dspace/app/scripts/TypeConversionTestScriptConfiguration.java new file mode 100644 index 0000000000..5738748e0b --- /dev/null +++ b/dspace-server-webapp/src/test/java/org/dspace/app/scripts/TypeConversionTestScriptConfiguration.java @@ -0,0 +1,48 @@ +/** + * 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 extends ScriptConfiguration { + + + public Class getDspaceRunnableClass() { + return null; + } + + public void setDspaceRunnableClass(final Class 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.getOption("b").setType(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; + } +} From e7cc945f92fc12839008715e53719c306a474865 Mon Sep 17 00:00:00 2001 From: Yana De Pauw Date: Fri, 8 Apr 2022 11:46:07 +0300 Subject: [PATCH 2/4] 88141: Update script converter comment and clean up setType(boolean) --- .../MetadataDeletionScriptConfiguration.java | 2 -- .../bulkedit/MetadataExportScriptConfiguration.java | 3 --- .../MetadataImportCliScriptConfiguration.java | 1 - .../bulkedit/MetadataImportScriptConfiguration.java | 6 ------ .../app/harvest/HarvestScriptConfiguration.java | 9 --------- .../mediafilter/MediaFilterScriptConfiguration.java | 4 ---- .../curate/CurationCliScriptConfiguration.java | 1 - .../org/dspace/discovery/IndexClientOptions.java | 8 -------- ...etryFailedOpenUrlTrackerScriptConfiguration.java | 3 --- ...missionFormsMigrationCliScriptConfiguration.java | 3 --- .../MockDSpaceRunnableScriptConfiguration.java | 2 -- .../dspace/app/rest/converter/ScriptConverter.java | 13 ++++++++----- .../TypeConversionTestScriptConfiguration.java | 1 - .../MockDSpaceRunnableScriptConfiguration.java | 2 -- 14 files changed, 8 insertions(+), 50 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataDeletionScriptConfiguration.java b/dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataDeletionScriptConfiguration.java index b8d41318db..9ccd53944a 100644 --- a/dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataDeletionScriptConfiguration.java +++ b/dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataDeletionScriptConfiguration.java @@ -41,10 +41,8 @@ public class MetadataDeletionScriptConfiguration 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; } diff --git a/dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataExportScriptConfiguration.java b/dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataExportScriptConfiguration.java index 0c513c4667..31556afc8d 100644 --- a/dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataExportScriptConfiguration.java +++ b/dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataExportScriptConfiguration.java @@ -54,12 +54,9 @@ public class MetadataExportScriptConfiguration 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; diff --git a/dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataImportCliScriptConfiguration.java b/dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataImportCliScriptConfiguration.java index 038df616ca..7e1537fe9d 100644 --- a/dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataImportCliScriptConfiguration.java +++ b/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; diff --git a/dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataImportScriptConfiguration.java b/dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataImportScriptConfiguration.java index 07e6a9aec9..65994040ba 100644 --- a/dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataImportScriptConfiguration.java +++ b/dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataImportScriptConfiguration.java @@ -59,20 +59,14 @@ public class MetadataImportScriptConfiguration 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; } diff --git a/dspace-api/src/main/java/org/dspace/app/harvest/HarvestScriptConfiguration.java b/dspace-api/src/main/java/org/dspace/app/harvest/HarvestScriptConfiguration.java index 6290562143..982973e47c 100644 --- a/dspace-api/src/main/java/org/dspace/app/harvest/HarvestScriptConfiguration.java +++ b/dspace-api/src/main/java/org/dspace/app/harvest/HarvestScriptConfiguration.java @@ -43,22 +43,14 @@ public class HarvestScriptConfiguration 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 extends ScriptConfigu "crosswalk in dspace.cfg"); options.addOption("h", "help", false, "help"); - options.getOption("h").setType(boolean.class); return options; } diff --git a/dspace-api/src/main/java/org/dspace/app/mediafilter/MediaFilterScriptConfiguration.java b/dspace-api/src/main/java/org/dspace/app/mediafilter/MediaFilterScriptConfiguration.java index 49ee23b924..26347c56ee 100644 --- a/dspace-api/src/main/java/org/dspace/app/mediafilter/MediaFilterScriptConfiguration.java +++ b/dspace-api/src/main/java/org/dspace/app/mediafilter/MediaFilterScriptConfiguration.java @@ -50,15 +50,11 @@ public class MediaFilterScriptConfiguration 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") diff --git a/dspace-api/src/main/java/org/dspace/curate/CurationCliScriptConfiguration.java b/dspace-api/src/main/java/org/dspace/curate/CurationCliScriptConfiguration.java index 5e1d014873..eaa04f4778 100644 --- a/dspace-api/src/main/java/org/dspace/curate/CurationCliScriptConfiguration.java +++ b/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; } diff --git a/dspace-api/src/main/java/org/dspace/discovery/IndexClientOptions.java b/dspace-api/src/main/java/org/dspace/discovery/IndexClientOptions.java index 62357bd95f..74d9ba0c3a 100644 --- a/dspace-api/src/main/java/org/dspace/discovery/IndexClientOptions.java +++ b/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; } } diff --git a/dspace-api/src/main/java/org/dspace/statistics/export/RetryFailedOpenUrlTrackerScriptConfiguration.java b/dspace-api/src/main/java/org/dspace/statistics/export/RetryFailedOpenUrlTrackerScriptConfiguration.java index b5d65aa4e5..dcae4aa4cb 100644 --- a/dspace-api/src/main/java/org/dspace/statistics/export/RetryFailedOpenUrlTrackerScriptConfiguration.java +++ b/dspace-api/src/main/java/org/dspace/statistics/export/RetryFailedOpenUrlTrackerScriptConfiguration.java @@ -56,15 +56,12 @@ public class RetryFailedOpenUrlTrackerScriptConfiguration Date: Fri, 8 Apr 2022 14:24:01 +0300 Subject: [PATCH 3/4] 88141: Fix issue with the mock script test --- .../java/org/dspace/app/rest/matcher/ScriptMatcher.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/matcher/ScriptMatcher.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/matcher/ScriptMatcher.java index c919aadd86..05e637e89f 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/matcher/ScriptMatcher.java +++ b/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 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")) )) ); From aa62178f95e11197bd59efa232c11ee17f026250 Mon Sep 17 00:00:00 2001 From: Yana De Pauw Date: Fri, 8 Apr 2022 15:10:31 +0300 Subject: [PATCH 4/4] Fix checkstyle issue --- .../test/java/org/dspace/app/rest/matcher/ScriptMatcher.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/matcher/ScriptMatcher.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/matcher/ScriptMatcher.java index 05e637e89f..5fe3635c2b 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/matcher/ScriptMatcher.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/matcher/ScriptMatcher.java @@ -32,7 +32,7 @@ public class ScriptMatcher { // 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(