From 98e5b3fbae8871ef0fecbd0550ad8fefb00e2b22 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 21 Jun 2023 12:34:37 +0200 Subject: [PATCH 01/19] fix(ct): enable sane default for upload storage location in containers The default from microprofile-config.properties does NOT work, as the location must already be resolvable while the servlet is being initialized - the app shipped defaults file is not yet read at this point. This is similar to the database options, which must be set using one of the other Payara included config sources. (Non-easily resolvable timing issue). The solution for containers is to add an env var to the docker file, which can be overriden by any env var from compose or K8s etc. (Problem is the high ordinal of the env source though) --- src/main/docker/Dockerfile | 4 +++- src/main/resources/META-INF/microprofile-config.properties | 1 - 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/main/docker/Dockerfile b/src/main/docker/Dockerfile index 88020a118b5..f64e88cb414 100644 --- a/src/main/docker/Dockerfile +++ b/src/main/docker/Dockerfile @@ -27,7 +27,9 @@ FROM $BASE_IMAGE # Make Payara use the "ct" profile for MicroProfile Config. Will switch various defaults for the application # setup in META-INF/microprofile-config.properties. # See also https://download.eclipse.org/microprofile/microprofile-config-3.0/microprofile-config-spec-3.0.html#configprofile -ENV MP_CONFIG_PROFILE=ct +ENV MP_CONFIG_PROFILE=ct \ + # NOTE: this cannot be provided as default from microprofile-config.properties as not yet avail when servlet starts + DATAVERSE_FILES_UPLOADS="${STORAGE_DIR}/uploads" # Copy app and deps from assembly in proper layers COPY --chown=payara:payara maven/deps ${DEPLOY_DIR}/dataverse/WEB-INF/lib/ diff --git a/src/main/resources/META-INF/microprofile-config.properties b/src/main/resources/META-INF/microprofile-config.properties index 7c16495f870..748ed6de55a 100644 --- a/src/main/resources/META-INF/microprofile-config.properties +++ b/src/main/resources/META-INF/microprofile-config.properties @@ -12,7 +12,6 @@ dataverse.build= dataverse.files.directory=/tmp/dataverse # The variables are replaced with the environment variables from our base image, but still easy to override %ct.dataverse.files.directory=${STORAGE_DIR} -%ct.dataverse.files.uploads=${STORAGE_DIR}/uploads # SEARCH INDEX dataverse.solr.host=localhost From d71cdf2d427011fc660794bb12afbab9db1c2bc7 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 21 Jun 2023 16:07:03 +0200 Subject: [PATCH 02/19] fix(ct,conf): switch to different approach to default upload location Instead of trying to provide a default using STORAGE_DIR env var from microprofile-config.properties as before, using this env var reference in glassfish-web.xml directly now. By defaulting to "." if not present (as in classic installations), it is fully equivalent to the former hardcoded default value. Providing a synced variant of it in microprofile-config.properties and leaving a hint about the pitfalls, we can reuse the setting for other purposes within the codebase as well (and expect the same behaviour because same defaults). --- src/main/docker/Dockerfile | 4 +--- src/main/resources/META-INF/microprofile-config.properties | 6 ++++++ src/main/webapp/WEB-INF/glassfish-web.xml | 2 +- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/main/docker/Dockerfile b/src/main/docker/Dockerfile index f64e88cb414..88020a118b5 100644 --- a/src/main/docker/Dockerfile +++ b/src/main/docker/Dockerfile @@ -27,9 +27,7 @@ FROM $BASE_IMAGE # Make Payara use the "ct" profile for MicroProfile Config. Will switch various defaults for the application # setup in META-INF/microprofile-config.properties. # See also https://download.eclipse.org/microprofile/microprofile-config-3.0/microprofile-config-spec-3.0.html#configprofile -ENV MP_CONFIG_PROFILE=ct \ - # NOTE: this cannot be provided as default from microprofile-config.properties as not yet avail when servlet starts - DATAVERSE_FILES_UPLOADS="${STORAGE_DIR}/uploads" +ENV MP_CONFIG_PROFILE=ct # Copy app and deps from assembly in proper layers COPY --chown=payara:payara maven/deps ${DEPLOY_DIR}/dataverse/WEB-INF/lib/ diff --git a/src/main/resources/META-INF/microprofile-config.properties b/src/main/resources/META-INF/microprofile-config.properties index 748ed6de55a..f3745126cb2 100644 --- a/src/main/resources/META-INF/microprofile-config.properties +++ b/src/main/resources/META-INF/microprofile-config.properties @@ -12,6 +12,12 @@ dataverse.build= dataverse.files.directory=/tmp/dataverse # The variables are replaced with the environment variables from our base image, but still easy to override %ct.dataverse.files.directory=${STORAGE_DIR} +# NOTE: the following uses STORAGE_DIR for both containers and classic installations. By defaulting to "." if not +# present, it equals the hardcoded default from before again. Also, be aware that this props file cannot provide +# any value for lookups in glassfish-web.xml during servlet initialization, as this file will not have +# been read yet! The names and their values are in sync here and over there to ensure the config checker +# is able to check for the directories (exist + writeable). +dataverse.files.uploads=${STORAGE_DIR:.}/uploads # SEARCH INDEX dataverse.solr.host=localhost diff --git a/src/main/webapp/WEB-INF/glassfish-web.xml b/src/main/webapp/WEB-INF/glassfish-web.xml index e56d7013abf..8041ebd4447 100644 --- a/src/main/webapp/WEB-INF/glassfish-web.xml +++ b/src/main/webapp/WEB-INF/glassfish-web.xml @@ -18,5 +18,5 @@ This folder is not only holding compiled JSP pages but also the place where file streams are stored during uploads. As Dataverse does not use any JSP, there will only be uploads stored here. --> - + From a4ec3a66e76aa1559aea0c05cedc2da2b38d7b03 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 21 Jun 2023 16:44:08 +0200 Subject: [PATCH 03/19] feat(conf): introduce ConfigCheckService to validate config on startup #9572 Starting with important local storage locations for the Dataverse application, this service uses EJB startup mechanisms to verify configuration bits on startup. Checks for the temp storage location and JSF upload location as crucial parts of the app, which, if not exist or write protected, while only cause errors and failures on the first data upload attempt. This is not desirable as it might cause users to be blocked. --- .../settings/ConfigCheckService.java | 65 +++++++++++++++++++ .../iq/dataverse/settings/JvmSettings.java | 1 + .../harvard/iq/dataverse/util/FileUtil.java | 29 ++++----- 3 files changed, 77 insertions(+), 18 deletions(-) create mode 100644 src/main/java/edu/harvard/iq/dataverse/settings/ConfigCheckService.java diff --git a/src/main/java/edu/harvard/iq/dataverse/settings/ConfigCheckService.java b/src/main/java/edu/harvard/iq/dataverse/settings/ConfigCheckService.java new file mode 100644 index 00000000000..4ba028903b0 --- /dev/null +++ b/src/main/java/edu/harvard/iq/dataverse/settings/ConfigCheckService.java @@ -0,0 +1,65 @@ +package edu.harvard.iq.dataverse.settings; + +import edu.harvard.iq.dataverse.util.FileUtil; + +import javax.annotation.PostConstruct; +import javax.ejb.DependsOn; +import javax.ejb.Singleton; +import javax.ejb.Startup; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Map; +import java.util.logging.Level; +import java.util.logging.Logger; + +@Startup +@Singleton +@DependsOn("StartupFlywayMigrator") +public class ConfigCheckService { + + private static final Logger logger = Logger.getLogger(ConfigCheckService.class.getCanonicalName()); + + public static class ConfigurationError extends RuntimeException { + public ConfigurationError(String message) { + super(message); + } + } + + @PostConstruct + public void startup() { + if (!checkSystemDirectories()) { + throw new ConfigurationError("Not all configuration checks passed successfully. See logs above."); + } + } + + /** + * In this method, we check the existence and write-ability of all important directories we use during + * normal operations. It does not include checks for the storage system. If directories are not available, + * try to create them (and fail when not allowed to). + * + * @return True if all checks successful, false otherwise. + */ + public boolean checkSystemDirectories() { + Map paths = Map.of( + Path.of(JvmSettings.UPLOADS_DIRECTORY.lookup()), "temporary JSF upload space (see " + JvmSettings.UPLOADS_DIRECTORY.getScopedKey() + ")", + Path.of(FileUtil.getFilesTempDirectory()), "temporary processing space (see " + JvmSettings.FILES_DIRECTORY.getScopedKey() + ")"); + + boolean success = true; + for (Path path : paths.keySet()) { + if (Files.notExists(path)) { + try { + Files.createDirectories(path); + } catch (IOException e) { + logger.log(Level.SEVERE, () -> "Could not create directory " + path + " for " + paths.get(path)); + success = false; + } + } else if (!Files.isWritable(path)) { + logger.log(Level.SEVERE, () -> "Directory " + path + " for " + paths.get(path) + " exists, but is not writeable"); + success = false; + } + } + return success; + } + +} diff --git a/src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java b/src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java index ff04a633ea7..c5c5682821a 100644 --- a/src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java +++ b/src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java @@ -49,6 +49,7 @@ public enum JvmSettings { // FILES SETTINGS SCOPE_FILES(PREFIX, "files"), FILES_DIRECTORY(SCOPE_FILES, "directory"), + UPLOADS_DIRECTORY(SCOPE_FILES, "uploads"), // SOLR INDEX SETTINGS SCOPE_SOLR(PREFIX, "solr"), diff --git a/src/main/java/edu/harvard/iq/dataverse/util/FileUtil.java b/src/main/java/edu/harvard/iq/dataverse/util/FileUtil.java index 6bb7e1d583b..ee1ee5a6a1c 100644 --- a/src/main/java/edu/harvard/iq/dataverse/util/FileUtil.java +++ b/src/main/java/edu/harvard/iq/dataverse/util/FileUtil.java @@ -40,6 +40,7 @@ import edu.harvard.iq.dataverse.ingest.IngestServiceShapefileHelper; import edu.harvard.iq.dataverse.ingest.IngestableDataChecker; import edu.harvard.iq.dataverse.license.License; +import edu.harvard.iq.dataverse.settings.ConfigCheckService; import edu.harvard.iq.dataverse.settings.JvmSettings; import edu.harvard.iq.dataverse.util.file.BagItFileHandler; import edu.harvard.iq.dataverse.util.file.CreateDataFileResult; @@ -1478,25 +1479,17 @@ public static boolean canIngestAsTabular(String mimeType) { } } + /** + * Return the location where data should be stored temporarily after uploading (UI or API) + * for local processing (ingest, unzip, ...) and transfer to final destination (see storage subsystem). + * + * This location is checked to be configured, does exist, and is writeable via + * {@link ConfigCheckService#checkSystemDirectories()}. + * + * @return String with a path to the temporary location. Will not be null (former versions did to indicate failure) + */ public static String getFilesTempDirectory() { - - String filesRootDirectory = JvmSettings.FILES_DIRECTORY.lookup(); - String filesTempDirectory = filesRootDirectory + "/temp"; - - if (!Files.exists(Paths.get(filesTempDirectory))) { - /* Note that "createDirectories()" must be used - not - * "createDirectory()", to make sure all the parent - * directories that may not yet exist are created as well. - */ - try { - Files.createDirectories(Paths.get(filesTempDirectory)); - } catch (IOException ex) { - logger.severe("Failed to create filesTempDirectory: " + filesTempDirectory ); - return null; - } - } - - return filesTempDirectory; + return JvmSettings.FILES_DIRECTORY.lookup() + File.separator + "temp"; } public static void generateS3PackageStorageIdentifier(DataFile dataFile) { From 6999093dcea8e889a24aafbe84dd6035e8a4b5db Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 21 Jun 2023 17:37:40 +0200 Subject: [PATCH 04/19] feat(conf): make docroot location configurable #9662 Add JVM Setting and add to config checker on startup to ensure target location is in good shape. --- .../harvard/iq/dataverse/settings/ConfigCheckService.java | 3 ++- .../edu/harvard/iq/dataverse/settings/JvmSettings.java | 1 + .../resources/META-INF/microprofile-config.properties | 1 + src/main/webapp/WEB-INF/glassfish-web.xml | 8 ++++---- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/main/java/edu/harvard/iq/dataverse/settings/ConfigCheckService.java b/src/main/java/edu/harvard/iq/dataverse/settings/ConfigCheckService.java index 4ba028903b0..443d12fc17a 100644 --- a/src/main/java/edu/harvard/iq/dataverse/settings/ConfigCheckService.java +++ b/src/main/java/edu/harvard/iq/dataverse/settings/ConfigCheckService.java @@ -43,7 +43,8 @@ public void startup() { public boolean checkSystemDirectories() { Map paths = Map.of( Path.of(JvmSettings.UPLOADS_DIRECTORY.lookup()), "temporary JSF upload space (see " + JvmSettings.UPLOADS_DIRECTORY.getScopedKey() + ")", - Path.of(FileUtil.getFilesTempDirectory()), "temporary processing space (see " + JvmSettings.FILES_DIRECTORY.getScopedKey() + ")"); + Path.of(FileUtil.getFilesTempDirectory()), "temporary processing space (see " + JvmSettings.FILES_DIRECTORY.getScopedKey() + ")", + Path.of(JvmSettings.DOCROOT_DIRECTORY.lookup()), "docroot space (see " + JvmSettings.DOCROOT_DIRECTORY.getScopedKey() + ")"); boolean success = true; for (Path path : paths.keySet()) { diff --git a/src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java b/src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java index c5c5682821a..540dc8201a0 100644 --- a/src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java +++ b/src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java @@ -50,6 +50,7 @@ public enum JvmSettings { SCOPE_FILES(PREFIX, "files"), FILES_DIRECTORY(SCOPE_FILES, "directory"), UPLOADS_DIRECTORY(SCOPE_FILES, "uploads"), + DOCROOT_DIRECTORY(SCOPE_FILES, "docroot"), // SOLR INDEX SETTINGS SCOPE_SOLR(PREFIX, "solr"), diff --git a/src/main/resources/META-INF/microprofile-config.properties b/src/main/resources/META-INF/microprofile-config.properties index f3745126cb2..597d50b2e0c 100644 --- a/src/main/resources/META-INF/microprofile-config.properties +++ b/src/main/resources/META-INF/microprofile-config.properties @@ -18,6 +18,7 @@ dataverse.files.directory=/tmp/dataverse # been read yet! The names and their values are in sync here and over there to ensure the config checker # is able to check for the directories (exist + writeable). dataverse.files.uploads=${STORAGE_DIR:.}/uploads +dataverse.files.docroot=${STORAGE_DIR:.}/docroot # SEARCH INDEX dataverse.solr.host=localhost diff --git a/src/main/webapp/WEB-INF/glassfish-web.xml b/src/main/webapp/WEB-INF/glassfish-web.xml index 8041ebd4447..5088e5a7fba 100644 --- a/src/main/webapp/WEB-INF/glassfish-web.xml +++ b/src/main/webapp/WEB-INF/glassfish-web.xml @@ -10,10 +10,10 @@ - - - - + + + + + From 2913a52f35645621bace35c93a9c0b2707004da1 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 21 Jun 2023 18:32:55 +0200 Subject: [PATCH 08/19] refactor(conf): simplify sitemap output location lookup using new docroot setting --- .../iq/dataverse/sitemap/SiteMapUtil.java | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/main/java/edu/harvard/iq/dataverse/sitemap/SiteMapUtil.java b/src/main/java/edu/harvard/iq/dataverse/sitemap/SiteMapUtil.java index e32b811ee2c..86ae697f771 100644 --- a/src/main/java/edu/harvard/iq/dataverse/sitemap/SiteMapUtil.java +++ b/src/main/java/edu/harvard/iq/dataverse/sitemap/SiteMapUtil.java @@ -3,6 +3,8 @@ import edu.harvard.iq.dataverse.Dataset; import edu.harvard.iq.dataverse.Dataverse; import edu.harvard.iq.dataverse.DvObjectContainer; +import edu.harvard.iq.dataverse.settings.ConfigCheckService; +import edu.harvard.iq.dataverse.settings.JvmSettings; import edu.harvard.iq.dataverse.util.SystemConfig; import edu.harvard.iq.dataverse.util.xml.XmlValidator; import java.io.File; @@ -210,16 +212,17 @@ public static boolean stageFileExists() { } return false; } - + + /** + * Lookup the location where to generate the sitemap. + * + * Note: the location is checked to be configured, does exist and is writeable in + * {@link ConfigCheckService#checkSystemDirectories()} + * + * @return Sitemap storage location ([docroot]/sitemap) + */ private static String getSitemapPathString() { - String sitemapPathString = "/tmp"; - // i.e. /usr/local/glassfish4/glassfish/domains/domain1 - String domainRoot = System.getProperty("com.sun.aas.instanceRoot"); - if (domainRoot != null) { - // Note that we write to a directory called "sitemap" but we serve just "/sitemap.xml" using PrettyFaces. - sitemapPathString = domainRoot + File.separator + "docroot" + File.separator + "sitemap"; - } - return sitemapPathString; + return JvmSettings.DOCROOT_DIRECTORY.lookup() + File.separator + "sitemap"; } } From 1db004245edd2f62f8d76290f4a183f095af36cc Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Tue, 22 Aug 2023 14:24:55 +0200 Subject: [PATCH 09/19] build(settings): migrate ConfigCheckService to Jakarta EE 10 --- .../harvard/iq/dataverse/settings/ConfigCheckService.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main/java/edu/harvard/iq/dataverse/settings/ConfigCheckService.java b/src/main/java/edu/harvard/iq/dataverse/settings/ConfigCheckService.java index 9e2a82d6b58..b175eafc3e0 100644 --- a/src/main/java/edu/harvard/iq/dataverse/settings/ConfigCheckService.java +++ b/src/main/java/edu/harvard/iq/dataverse/settings/ConfigCheckService.java @@ -2,10 +2,10 @@ import edu.harvard.iq.dataverse.util.FileUtil; -import javax.annotation.PostConstruct; -import javax.ejb.DependsOn; -import javax.ejb.Singleton; -import javax.ejb.Startup; +import jakarta.annotation.PostConstruct; +import jakarta.ejb.DependsOn; +import jakarta.ejb.Singleton; +import jakarta.ejb.Startup; import java.io.IOException; import java.nio.file.FileSystemException; import java.nio.file.Files; From 72722e77afb848131dc38042d23b7cf44156a6e8 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Tue, 22 Aug 2023 15:01:05 +0200 Subject: [PATCH 10/19] refactor(collection): make logo location non-static #9662 By introducing a new static method ThemeWidgetFragment.getLogoDir all other places (api.Access, api.Dataverse, UpdateDataverseThemeCommand, DataverseServiceBean) can use a lookup function from one central place instead of building the path on their own. Reducing code duplication also means we can more easily get the location from a setting, enabling relocation of the data. That is especially important for container usage. Also, we can now use ConfigCheckService to detect if the folders we configured are read/write accessible to us. --- .../iq/dataverse/DataverseServiceBean.java | 11 +---------- .../iq/dataverse/ThemeWidgetFragment.java | 18 ++++++++++++++---- .../edu/harvard/iq/dataverse/api/Access.java | 12 ++---------- .../harvard/iq/dataverse/api/Dataverses.java | 2 ++ .../impl/UpdateDataverseThemeCommand.java | 4 ++-- 5 files changed, 21 insertions(+), 26 deletions(-) diff --git a/src/main/java/edu/harvard/iq/dataverse/DataverseServiceBean.java b/src/main/java/edu/harvard/iq/dataverse/DataverseServiceBean.java index 7194a1ef31e..549b8310122 100644 --- a/src/main/java/edu/harvard/iq/dataverse/DataverseServiceBean.java +++ b/src/main/java/edu/harvard/iq/dataverse/DataverseServiceBean.java @@ -399,16 +399,7 @@ private File getLogo(Dataverse dataverse) { DataverseTheme theme = dataverse.getDataverseTheme(); if (theme != null && theme.getLogo() != null && !theme.getLogo().isEmpty()) { - Properties p = System.getProperties(); - String domainRoot = p.getProperty("com.sun.aas.instanceRoot"); - - if (domainRoot != null && !"".equals(domainRoot)) { - return new File (domainRoot + File.separator + - "docroot" + File.separator + - "logos" + File.separator + - dataverse.getLogoOwnerId() + File.separator + - theme.getLogo()); - } + return ThemeWidgetFragment.getLogoDir(dataverse.getLogoOwnerId()).resolve(theme.getLogo()).toFile(); } return null; diff --git a/src/main/java/edu/harvard/iq/dataverse/ThemeWidgetFragment.java b/src/main/java/edu/harvard/iq/dataverse/ThemeWidgetFragment.java index 9a62a99722a..f30051e26ae 100644 --- a/src/main/java/edu/harvard/iq/dataverse/ThemeWidgetFragment.java +++ b/src/main/java/edu/harvard/iq/dataverse/ThemeWidgetFragment.java @@ -7,6 +7,7 @@ import edu.harvard.iq.dataverse.engine.command.Command; import edu.harvard.iq.dataverse.engine.command.impl.UpdateDataverseThemeCommand; +import edu.harvard.iq.dataverse.settings.JvmSettings; import edu.harvard.iq.dataverse.util.BundleUtil; import edu.harvard.iq.dataverse.util.JsfHelper; import java.io.File; @@ -14,6 +15,7 @@ import java.net.MalformedURLException; import java.net.URL; import java.nio.file.Files; +import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.StandardCopyOption; import java.util.logging.Level; @@ -49,6 +51,8 @@ public class ThemeWidgetFragment implements java.io.Serializable { static final String DEFAULT_TEXT_COLOR = "888888"; private static final Logger logger = Logger.getLogger(ThemeWidgetFragment.class.getCanonicalName()); + public static final String LOGOS_SUBDIR = "logos"; + public static final String LOGOS_TEMP_SUBDIR = LOGOS_SUBDIR + File.separator + "temp"; private File tempDir; private File uploadedFile; @@ -86,12 +90,18 @@ public void setTaglineInput(HtmlInputText taglineInput) { } - + public static Path getLogoDir(String ownerId) { + return Path.of(JvmSettings.DOCROOT_DIRECTORY.lookup(), LOGOS_SUBDIR, ownerId); + } - private void createTempDir() { + private void createTempDir() { try { - File tempRoot = Files.createDirectories(Paths.get("../docroot/logos/temp")).toFile(); - tempDir = Files.createTempDirectory(tempRoot.toPath(),editDv.getId().toString()).toFile(); + // Create the temporary space if not yet existing (will silently ignore preexisting) + // Note that the docroot directory is checked within ConfigCheckService for presence and write access. + Path tempRoot = Path.of(JvmSettings.DOCROOT_DIRECTORY.lookup(), LOGOS_TEMP_SUBDIR); + Files.createDirectories(tempRoot); + + this.tempDir = Files.createTempDirectory(tempRoot, editDv.getId().toString()).toFile(); } catch (IOException e) { throw new RuntimeException("Error creating temp directory", e); // improve error handling } diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Access.java b/src/main/java/edu/harvard/iq/dataverse/api/Access.java index 0341f8c1127..ce7cfb6b254 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Access.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/Access.java @@ -31,6 +31,7 @@ import edu.harvard.iq.dataverse.RoleAssignment; import edu.harvard.iq.dataverse.UserNotification; import edu.harvard.iq.dataverse.UserNotificationServiceBean; +import edu.harvard.iq.dataverse.ThemeWidgetFragment; import static edu.harvard.iq.dataverse.api.Datasets.handleVersion; @@ -1196,16 +1197,7 @@ private File getLogo(Dataverse dataverse) { DataverseTheme theme = dataverse.getDataverseTheme(); if (theme != null && theme.getLogo() != null && !theme.getLogo().equals("")) { - Properties p = System.getProperties(); - String domainRoot = p.getProperty("com.sun.aas.instanceRoot"); - - if (domainRoot != null && !"".equals(domainRoot)) { - return new File (domainRoot + File.separator + - "docroot" + File.separator + - "logos" + File.separator + - dataverse.getLogoOwnerId() + File.separator + - theme.getLogo()); - } + return ThemeWidgetFragment.getLogoDir(dataverse.getLogoOwnerId()).resolve(theme.getLogo()).toFile(); } return null; diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java b/src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java index a60775cbd38..30c14535251 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java @@ -976,6 +976,8 @@ public Response listAssignments(@Context ContainerRequestContext crc, @PathParam */ // File tempDir; // +// TODO: Code duplicate in ThemeWidgetFragment. Maybe extract, make static and put some place else? +// Important: at least use JvmSettings.DOCROOT_DIRECTORY and not the hardcoded location! // private void createTempDir(Dataverse editDv) { // try { // File tempRoot = java.nio.file.Files.createDirectories(Paths.get("../docroot/logos/temp")).toFile(); diff --git a/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/UpdateDataverseThemeCommand.java b/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/UpdateDataverseThemeCommand.java index add7b825659..9ef9fed4b1b 100644 --- a/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/UpdateDataverseThemeCommand.java +++ b/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/UpdateDataverseThemeCommand.java @@ -1,6 +1,7 @@ package edu.harvard.iq.dataverse.engine.command.impl; import edu.harvard.iq.dataverse.Dataverse; +import edu.harvard.iq.dataverse.ThemeWidgetFragment; import edu.harvard.iq.dataverse.authorization.Permission; import edu.harvard.iq.dataverse.engine.command.AbstractCommand; import edu.harvard.iq.dataverse.engine.command.CommandContext; @@ -22,7 +23,6 @@ public class UpdateDataverseThemeCommand extends AbstractCommand { private final Dataverse editedDv; private final File uploadedFile; - private final Path logoPath = Paths.get("../docroot/logos"); private String locate; public UpdateDataverseThemeCommand(Dataverse editedDv, File uploadedFile, DataverseRequest aRequest, String location) { @@ -44,7 +44,7 @@ public UpdateDataverseThemeCommand(Dataverse editedDv, File uploadedFile, Datave public Dataverse execute(CommandContext ctxt) throws CommandException { // Get current dataverse, so we can delete current logo file if necessary Dataverse currentDv = ctxt.dataverses().find(editedDv.getId()); - File logoFileDir = new File(logoPath.toFile(), editedDv.getId().toString()); + File logoFileDir = ThemeWidgetFragment.getLogoDir(editedDv.getId().toString()).toFile(); File currentFile=null; if (locate.equals("FOOTER")){ From 431afed3d78fd514151e8e25c9389fcbfea79f22 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Tue, 22 Aug 2023 16:12:22 +0200 Subject: [PATCH 11/19] fix(webapp): revert to hardcoded default for dirs in glassfish-web.xml #9662 Payara does not support looking up variables in default values of a lookup. As a consequence, we must return to the hardcoded "./docroot" and "./uploads" and instead supply default values using two environment variables in the applications' Dockerfile. This way they stay configurable from cmdline or other sources of env vars. --- src/main/docker/Dockerfile | 5 +++++ .../META-INF/microprofile-config.properties | 4 ++-- src/main/webapp/WEB-INF/glassfish-web.xml | 14 +++++++++----- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/main/docker/Dockerfile b/src/main/docker/Dockerfile index 88020a118b5..201f164d961 100644 --- a/src/main/docker/Dockerfile +++ b/src/main/docker/Dockerfile @@ -29,6 +29,11 @@ FROM $BASE_IMAGE # See also https://download.eclipse.org/microprofile/microprofile-config-3.0/microprofile-config-spec-3.0.html#configprofile ENV MP_CONFIG_PROFILE=ct +# Workaround to configure upload directories by default to useful place until we can have variable lookups in +# defaults for glassfish-web.xml and other places. +ENV DATAVERSE_FILES_UPLOADS="${STORAGE_DIR}/uploads" +ENV DATAVERSE_FILES_DOCROOT="${STORAGE_DIR}/docroot" + # Copy app and deps from assembly in proper layers COPY --chown=payara:payara maven/deps ${DEPLOY_DIR}/dataverse/WEB-INF/lib/ COPY --chown=payara:payara maven/app ${DEPLOY_DIR}/dataverse/ diff --git a/src/main/resources/META-INF/microprofile-config.properties b/src/main/resources/META-INF/microprofile-config.properties index b58c728316b..11471663fc3 100644 --- a/src/main/resources/META-INF/microprofile-config.properties +++ b/src/main/resources/META-INF/microprofile-config.properties @@ -12,8 +12,8 @@ dataverse.build= dataverse.files.directory=/tmp/dataverse # The variables are replaced with the environment variables from our base image, but still easy to override %ct.dataverse.files.directory=${STORAGE_DIR} -# NOTE: the following uses STORAGE_DIR for both containers and classic installations. By defaulting to -# "com.sun.aas.instanceRoot" if not present, it equals the hardcoded former default "." in glassfish-web.xml +# NOTE: The following uses STORAGE_DIR for both containers and classic installations. By defaulting to +# "com.sun.aas.instanceRoot" if not present, it equals the hardcoded default "." in glassfish-web.xml # (which is relative to the domain root folder). # Also, be aware that this props file cannot provide any value for lookups in glassfish-web.xml during servlet # initialization, as this file will not have been read yet! The names and their values are in sync here and over diff --git a/src/main/webapp/WEB-INF/glassfish-web.xml b/src/main/webapp/WEB-INF/glassfish-web.xml index 9677bf089e2..015a309fd6b 100644 --- a/src/main/webapp/WEB-INF/glassfish-web.xml +++ b/src/main/webapp/WEB-INF/glassfish-web.xml @@ -11,13 +11,17 @@ - - - - + + + + + - + + From ec131f8b93463047811ff3b0ed626f61ee831041 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Tue, 22 Aug 2023 16:15:28 +0200 Subject: [PATCH 12/19] fix(settings): make ConfigCheckService use Files.exist #9662 With Files.notExist if some folder does not have the "execute" attribute, it cannot detect a folder does not exist. Inverting the Files.exists call solves the problem. Also adding tests for the business logic. --- .../settings/ConfigCheckService.java | 2 +- .../settings/ConfigCheckServiceTest.java | 92 +++++++++++++++++++ 2 files changed, 93 insertions(+), 1 deletion(-) create mode 100644 src/test/java/edu/harvard/iq/dataverse/settings/ConfigCheckServiceTest.java diff --git a/src/main/java/edu/harvard/iq/dataverse/settings/ConfigCheckService.java b/src/main/java/edu/harvard/iq/dataverse/settings/ConfigCheckService.java index b175eafc3e0..83c3f6ac90d 100644 --- a/src/main/java/edu/harvard/iq/dataverse/settings/ConfigCheckService.java +++ b/src/main/java/edu/harvard/iq/dataverse/settings/ConfigCheckService.java @@ -49,7 +49,7 @@ public boolean checkSystemDirectories() { boolean success = true; for (Path path : paths.keySet()) { - if (Files.notExists(path)) { + if (! Files.exists(path)) { try { Files.createDirectories(path); } catch (IOException e) { diff --git a/src/test/java/edu/harvard/iq/dataverse/settings/ConfigCheckServiceTest.java b/src/test/java/edu/harvard/iq/dataverse/settings/ConfigCheckServiceTest.java new file mode 100644 index 00000000000..796448e579a --- /dev/null +++ b/src/test/java/edu/harvard/iq/dataverse/settings/ConfigCheckServiceTest.java @@ -0,0 +1,92 @@ +package edu.harvard.iq.dataverse.settings; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Assumptions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Set; + +import static java.nio.file.attribute.PosixFilePermission.GROUP_READ; +import static java.nio.file.attribute.PosixFilePermission.OWNER_READ; + +class ConfigCheckServiceTest { + + @Nested + class TestDirNotWritable { + @TempDir + Path testDir; + + private String oldUploadDirSetting; + + @BeforeEach + void setUp() throws IOException { + Files.setPosixFilePermissions(this.testDir, Set.of(OWNER_READ, GROUP_READ)); + + // TODO: This is a workaround until PR #9273 is merged, providing the ability to lookup values for + // @JvmSetting from static methods. Should be deleted. + this.oldUploadDirSetting = System.getProperty(JvmSettings.UPLOADS_DIRECTORY.getScopedKey()); + System.setProperty(JvmSettings.UPLOADS_DIRECTORY.getScopedKey(), this.testDir.toString()); + } + + @AfterEach + void tearDown() { + // TODO: This is a workaround until PR #9273 is merged, providing the ability to lookup values for + // @JvmSetting from static methods. Should be deleted. + if (this.oldUploadDirSetting != null) + System.setProperty(JvmSettings.UPLOADS_DIRECTORY.getScopedKey(), this.oldUploadDirSetting); + } + + @Test + void writeCheckFails() { + Assumptions.assumeTrue(Files.exists(this.testDir)); + + ConfigCheckService sut = new ConfigCheckService(); + Assertions.assertFalse(sut.checkSystemDirectories()); + } + } + + @Nested + class TestDirNotExistent { + @TempDir + Path testDir; + String subFolder = "foobar"; + + String oldUploadDirSetting; + + @BeforeEach + void setUp() throws IOException { + // Make test dir not writeable, so the subfolder cannot be created + Files.setPosixFilePermissions(this.testDir, Set.of(OWNER_READ, GROUP_READ)); + + // TODO: This is a workaround until PR #9273 is merged, providing the ability to lookup values for + // @JvmSetting from static methods. Should be deleted. + oldUploadDirSetting = System.getProperty(JvmSettings.UPLOADS_DIRECTORY.getScopedKey()); + System.setProperty(JvmSettings.UPLOADS_DIRECTORY.getScopedKey(), this.testDir.resolve(this.subFolder).toString()); + } + + @AfterEach + void tearDown() { + // TODO: This is a workaround until PR #9273 is merged, providing the ability to lookup values for + // @JvmSetting from static methods. Should be deleted. + if (this.oldUploadDirSetting != null) + System.setProperty(JvmSettings.UPLOADS_DIRECTORY.getScopedKey(), this.oldUploadDirSetting); + } + + @Test + void mkdirFails() { + Assumptions.assumeTrue(Files.exists(this.testDir)); + Assumptions.assumeFalse(Files.exists(this.testDir.resolve(this.subFolder))); + + ConfigCheckService sut = new ConfigCheckService(); + Assertions.assertFalse(sut.checkSystemDirectories()); + } + } + +} \ No newline at end of file From 848f564a1c63656381352a6a0f52443bd05f9cb7 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Tue, 22 Aug 2023 23:41:42 +0200 Subject: [PATCH 13/19] test(settings): make ConfigCheckService actually testable #9662 --- .../settings/ConfigCheckServiceTest.java | 82 ++++++++++--------- .../META-INF/microprofile-config.properties | 7 +- 2 files changed, 50 insertions(+), 39 deletions(-) diff --git a/src/test/java/edu/harvard/iq/dataverse/settings/ConfigCheckServiceTest.java b/src/test/java/edu/harvard/iq/dataverse/settings/ConfigCheckServiceTest.java index 796448e579a..1018ad8d47b 100644 --- a/src/test/java/edu/harvard/iq/dataverse/settings/ConfigCheckServiceTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/settings/ConfigCheckServiceTest.java @@ -1,6 +1,6 @@ package edu.harvard.iq.dataverse.settings; -import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.BeforeEach; @@ -17,35 +17,32 @@ import static java.nio.file.attribute.PosixFilePermission.OWNER_READ; class ConfigCheckServiceTest { + + @TempDir + static Path testDir; + + private static final String testDirProp = "test.filesDir"; + + @AfterAll + static void tearDown() { + System.clearProperty(testDirProp); + } @Nested class TestDirNotWritable { - @TempDir - Path testDir; - private String oldUploadDirSetting; + Path notWriteableSubfolder = testDir.resolve("readonly"); @BeforeEach void setUp() throws IOException { - Files.setPosixFilePermissions(this.testDir, Set.of(OWNER_READ, GROUP_READ)); - - // TODO: This is a workaround until PR #9273 is merged, providing the ability to lookup values for - // @JvmSetting from static methods. Should be deleted. - this.oldUploadDirSetting = System.getProperty(JvmSettings.UPLOADS_DIRECTORY.getScopedKey()); - System.setProperty(JvmSettings.UPLOADS_DIRECTORY.getScopedKey(), this.testDir.toString()); - } - - @AfterEach - void tearDown() { - // TODO: This is a workaround until PR #9273 is merged, providing the ability to lookup values for - // @JvmSetting from static methods. Should be deleted. - if (this.oldUploadDirSetting != null) - System.setProperty(JvmSettings.UPLOADS_DIRECTORY.getScopedKey(), this.oldUploadDirSetting); + Files.createDirectory(notWriteableSubfolder); + Files.setPosixFilePermissions(notWriteableSubfolder, Set.of(OWNER_READ, GROUP_READ)); + System.setProperty(testDirProp, notWriteableSubfolder.toString()); } @Test void writeCheckFails() { - Assumptions.assumeTrue(Files.exists(this.testDir)); + Assumptions.assumeTrue(Files.exists(notWriteableSubfolder)); ConfigCheckService sut = new ConfigCheckService(); Assertions.assertFalse(sut.checkSystemDirectories()); @@ -54,38 +51,47 @@ void writeCheckFails() { @Nested class TestDirNotExistent { - @TempDir - Path testDir; - String subFolder = "foobar"; - String oldUploadDirSetting; + Path notExistTestfolder = testDir.resolve("parent-readonly"); + Path notExistConfigSubfolder = notExistTestfolder.resolve("foobar"); @BeforeEach void setUp() throws IOException { + Files.createDirectory(notExistTestfolder); // Make test dir not writeable, so the subfolder cannot be created - Files.setPosixFilePermissions(this.testDir, Set.of(OWNER_READ, GROUP_READ)); + Files.setPosixFilePermissions(notExistTestfolder, Set.of(OWNER_READ, GROUP_READ)); + System.setProperty(testDirProp, notExistConfigSubfolder.toString()); + } + + @Test + void mkdirFails() { + Assumptions.assumeTrue(Files.exists(notExistTestfolder)); + Assumptions.assumeFalse(Files.exists(notExistConfigSubfolder)); - // TODO: This is a workaround until PR #9273 is merged, providing the ability to lookup values for - // @JvmSetting from static methods. Should be deleted. - oldUploadDirSetting = System.getProperty(JvmSettings.UPLOADS_DIRECTORY.getScopedKey()); - System.setProperty(JvmSettings.UPLOADS_DIRECTORY.getScopedKey(), this.testDir.resolve(this.subFolder).toString()); + ConfigCheckService sut = new ConfigCheckService(); + Assertions.assertFalse(sut.checkSystemDirectories()); } + } + + @Nested + class TestDirCreated { + + Path missingToBeCreatedTestfolder = testDir.resolve("create-me"); + Path missingToBeCreatedSubfolder = missingToBeCreatedTestfolder.resolve("foobar"); - @AfterEach - void tearDown() { - // TODO: This is a workaround until PR #9273 is merged, providing the ability to lookup values for - // @JvmSetting from static methods. Should be deleted. - if (this.oldUploadDirSetting != null) - System.setProperty(JvmSettings.UPLOADS_DIRECTORY.getScopedKey(), this.oldUploadDirSetting); + @BeforeEach + void setUp() throws IOException { + Files.createDirectory(missingToBeCreatedTestfolder); + System.setProperty(testDirProp, missingToBeCreatedSubfolder.toString()); } @Test - void mkdirFails() { - Assumptions.assumeTrue(Files.exists(this.testDir)); - Assumptions.assumeFalse(Files.exists(this.testDir.resolve(this.subFolder))); + void mkdirSucceeds() { + Assumptions.assumeTrue(Files.exists(missingToBeCreatedTestfolder)); + Assumptions.assumeFalse(Files.exists(missingToBeCreatedSubfolder)); ConfigCheckService sut = new ConfigCheckService(); - Assertions.assertFalse(sut.checkSystemDirectories()); + Assertions.assertTrue(sut.checkSystemDirectories()); } } diff --git a/src/test/resources/META-INF/microprofile-config.properties b/src/test/resources/META-INF/microprofile-config.properties index 21f70b53896..8e5521f8287 100644 --- a/src/test/resources/META-INF/microprofile-config.properties +++ b/src/test/resources/META-INF/microprofile-config.properties @@ -8,4 +8,9 @@ dataverse.pid.ezid.api-url=http://example.org # Also requires the username and the password to be present when used in production, use a default for unit testing. dataverse.pid.ezid.username=Dataverse Unit Test -dataverse.pid.ezid.password=supersecret \ No newline at end of file +dataverse.pid.ezid.password=supersecret + +# To test ConfigCheckService, point our files directories to a common test dir +dataverse.files.directory=${test.filesDir} +dataverse.files.uploads=${test.filesDir}/uploads +dataverse.files.docroot=${test.filesDir}/docroot From c49ee0ab86891521801969a73367fd1cb50817ee Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Tue, 22 Aug 2023 23:47:45 +0200 Subject: [PATCH 14/19] test(settings): make ConfigCheckService require absolute paths #9662 --- .../iq/dataverse/settings/ConfigCheckService.java | 7 +++++++ .../iq/dataverse/settings/ConfigCheckServiceTest.java | 10 ++++++++++ 2 files changed, 17 insertions(+) diff --git a/src/main/java/edu/harvard/iq/dataverse/settings/ConfigCheckService.java b/src/main/java/edu/harvard/iq/dataverse/settings/ConfigCheckService.java index 83c3f6ac90d..a2c3f53d59d 100644 --- a/src/main/java/edu/harvard/iq/dataverse/settings/ConfigCheckService.java +++ b/src/main/java/edu/harvard/iq/dataverse/settings/ConfigCheckService.java @@ -49,6 +49,13 @@ public boolean checkSystemDirectories() { boolean success = true; for (Path path : paths.keySet()) { + // Check if the configured path is absolute - avoid potential problems with relative paths this way + if (! path.isAbsolute()) { + logger.log(Level.SEVERE, () -> "Configured directory " + path + " for " + paths.get(path) + " is not absolute"); + success = false; + continue; + } + if (! Files.exists(path)) { try { Files.createDirectories(path); diff --git a/src/test/java/edu/harvard/iq/dataverse/settings/ConfigCheckServiceTest.java b/src/test/java/edu/harvard/iq/dataverse/settings/ConfigCheckServiceTest.java index 1018ad8d47b..b031b9429c6 100644 --- a/src/test/java/edu/harvard/iq/dataverse/settings/ConfigCheckServiceTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/settings/ConfigCheckServiceTest.java @@ -27,6 +27,16 @@ class ConfigCheckServiceTest { static void tearDown() { System.clearProperty(testDirProp); } + + @Nested + class TestDirNotAbsolute { + @Test + void nonAbsolutePathForTestDir() { + System.setProperty(testDirProp, "foobar"); + ConfigCheckService sut = new ConfigCheckService(); + Assertions.assertFalse(sut.checkSystemDirectories()); + } + } @Nested class TestDirNotWritable { From 28ddccc6d0f0baf4a3e48243b81c1ed001428b13 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 23 Aug 2023 10:18:27 +0200 Subject: [PATCH 15/19] fix(test,settings): make SiteMapUtilTest use test.filesDir property This is also used in ConfigCheckServiceTest to verify the checks are working. This property is picked up when sitemap util looks up the storage location via MPCONFIG, parsing the default values during testing from src/test/resources/META-INF/microprofile-config.properties --- .../edu/harvard/iq/dataverse/sitemap/SiteMapUtilTest.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/test/java/edu/harvard/iq/dataverse/sitemap/SiteMapUtilTest.java b/src/test/java/edu/harvard/iq/dataverse/sitemap/SiteMapUtilTest.java index 4f2b00bbea4..2ded6cb6a33 100644 --- a/src/test/java/edu/harvard/iq/dataverse/sitemap/SiteMapUtilTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/sitemap/SiteMapUtilTest.java @@ -82,7 +82,8 @@ public void testUpdateSiteMap() throws IOException, ParseException { String tmpDir = tmpDirPath.toString(); File docroot = new File(tmpDir + File.separator + "docroot"); docroot.mkdirs(); - System.setProperty("com.sun.aas.instanceRoot", tmpDir); + // TODO: this and the above should be replaced with JUnit 5 @TestDir + System.setProperty("test.filesDir", tmpDir); SiteMapUtil.updateSiteMap(dataverses, datasets); @@ -117,7 +118,7 @@ public void testUpdateSiteMap() throws IOException, ParseException { assertFalse(sitemapString.contains(harvestedPid)); assertFalse(sitemapString.contains(deaccessionedPid)); - System.clearProperty("com.sun.aas.instanceRoot"); + System.clearProperty("test.filesDir"); } From ba8e6d221ad9e0ce5de4391e96757d120e0313b4 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 23 Aug 2023 10:20:29 +0200 Subject: [PATCH 16/19] fix(test,settings): provide default for test.filesDir By providing a sane default unter /tmp, we enable a few tests that do not use a custom testclass scoped directory to run --- src/test/resources/META-INF/microprofile-config.properties | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/test/resources/META-INF/microprofile-config.properties b/src/test/resources/META-INF/microprofile-config.properties index 8e5521f8287..113a098a1fe 100644 --- a/src/test/resources/META-INF/microprofile-config.properties +++ b/src/test/resources/META-INF/microprofile-config.properties @@ -10,7 +10,9 @@ dataverse.pid.ezid.api-url=http://example.org dataverse.pid.ezid.username=Dataverse Unit Test dataverse.pid.ezid.password=supersecret -# To test ConfigCheckService, point our files directories to a common test dir +# To test ConfigCheckService, point our files directories to a common test dir by overriding the +# property test.filesDir via system properties +test.filesDir=/tmp/dataverse dataverse.files.directory=${test.filesDir} dataverse.files.uploads=${test.filesDir}/uploads dataverse.files.docroot=${test.filesDir}/docroot From 396ffff1c5dc2b38435c140ba5860fe5fbee9fd6 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 23 Aug 2023 11:45:49 +0200 Subject: [PATCH 17/19] style(settings): unify default dataverse.files dir options - No more profile to work around Payaras bug with overriding profiled values - Group together because every item using $STORAGE_DIR and a default to match classic installs now --- src/main/resources/META-INF/microprofile-config.properties | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/main/resources/META-INF/microprofile-config.properties b/src/main/resources/META-INF/microprofile-config.properties index 11471663fc3..f51a728332e 100644 --- a/src/main/resources/META-INF/microprofile-config.properties +++ b/src/main/resources/META-INF/microprofile-config.properties @@ -9,15 +9,13 @@ dataverse.build= %ct.dataverse.siteUrl=http://${dataverse.fqdn}:8080 # FILES -dataverse.files.directory=/tmp/dataverse -# The variables are replaced with the environment variables from our base image, but still easy to override -%ct.dataverse.files.directory=${STORAGE_DIR} -# NOTE: The following uses STORAGE_DIR for both containers and classic installations. By defaulting to +# NOTE: The following uses STORAGE_DIR for both containers and classic installations. When defaulting to # "com.sun.aas.instanceRoot" if not present, it equals the hardcoded default "." in glassfish-web.xml # (which is relative to the domain root folder). # Also, be aware that this props file cannot provide any value for lookups in glassfish-web.xml during servlet # initialization, as this file will not have been read yet! The names and their values are in sync here and over # there to ensure the config checker is able to check for the directories (exist + writeable). +dataverse.files.directory=${STORAGE_DIR:/tmp/dataverse} dataverse.files.uploads=${STORAGE_DIR:${com.sun.aas.instanceRoot}}/uploads dataverse.files.docroot=${STORAGE_DIR:${com.sun.aas.instanceRoot}}/docroot From d37eedfd7898eecb9fecb8bbe564f8efd76d5e8b Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 23 Aug 2023 11:48:26 +0200 Subject: [PATCH 18/19] docs(settings): refactor docs on the important directories and how to configure them #9662 --- .../source/installation/config.rst | 49 ++++++++++++++----- 1 file changed, 38 insertions(+), 11 deletions(-) diff --git a/doc/sphinx-guides/source/installation/config.rst b/doc/sphinx-guides/source/installation/config.rst index f9fe74afc7c..dc31b1afae8 100644 --- a/doc/sphinx-guides/source/installation/config.rst +++ b/doc/sphinx-guides/source/installation/config.rst @@ -1759,8 +1759,8 @@ protocol, host, and port number and should not include a trailing slash. dataverse.files.directory +++++++++++++++++++++++++ -Please provide an absolute path to a directory backed by some mounted file system. This directory is used for a number -of purposes: +Providing an explicit location here makes it easier to reuse some mounted filesystem and we recommend doing so +to avoid filled up disks, aid in performance, etc. This directory is used for a number of purposes: 1. ``/temp`` after uploading, data is temporarily stored here for ingest and/or before shipping to the final storage destination. @@ -1773,24 +1773,51 @@ of purposes: under certain conditions. This directory may also be used by file stores for :ref:`permanent file storage `, but this is controlled by other, store-specific settings. -Defaults to ``/tmp/dataverse``. Can also be set via *MicroProfile Config API* sources, e.g. the environment variable -``DATAVERSE_FILES_DIRECTORY``. Defaults to ``${STORAGE_DIR}`` for profile ``ct``, important for the -:ref:`Dataverse Application Image `. +Notes: + +- Please provide an absolute path to a directory backed by some mounted file system. +- Can also be set via *MicroProfile Config API* sources, e.g. the environment variable ``DATAVERSE_FILES_DIRECTORY``. +- Defaults to ``/tmp/dataverse`` in a :doc:`default installation `. +- Defaults to ``${STORAGE_DIR}`` using our :ref:`Dataverse container ` (resolving to ``/dv``). +- During startup, this directory will be checked for existence and write access. It will be created for you + if missing. If it cannot be created or does not have proper write access, application deployment will fail. .. _dataverse.files.uploads: dataverse.files.uploads +++++++++++++++++++++++ -Configure a folder to store the incoming file stream during uploads (before transfering to `${dataverse.files.directory}/temp`). +Configure a folder to store the incoming file stream during uploads (before transfering to ``${dataverse.files.directory}/temp``). +Providing an explicit location here makes it easier to reuse some mounted filesystem. Please also see :ref:`temporary-file-storage` for more details. -You can use an absolute path or a relative, which is relative to the application server domain directory. -Defaults to ``./uploads``, which resolves to ``/usr/local/payara6/glassfish/domains/domain1/uploads`` in a default -installation. +Notes: + +- Please provide an absolute path to a directory backed by some mounted file system. +- Defaults to ``${com.sun.aas.instanceRoot}/uploads`` in a :doc:`default installation ` + (resolving to ``/usr/local/payara6/glassfish/domains/domain1/uploads``). +- Defaults to ``${STORAGE_DIR}/uploads`` using our :ref:`Dataverse container ` (resolving to ``/dv/uploads``). +- Can also be set via *MicroProfile Config API* sources, e.g. the environment variable ``DATAVERSE_FILES_UPLOADS``. +- During startup, this directory will be checked for existence and write access. It will be created for you + if missing. If it cannot be created or does not have proper write access, application deployment will fail. + +.. _dataverse.files.docroot: + +dataverse.files.docroot ++++++++++++++++++++++++ + +Configure a folder to store and retrieve additional materials like user uploaded collection logos, generated sitemaps, +and so on. Providing an explicit location here makes it easier to reuse some mounted filesystem. +See also logo customization above. + +Notes: -Can also be set via *MicroProfile Config API* sources, e.g. the environment variable ``DATAVERSE_FILES_UPLOADS``. -Defaults to ``${STORAGE_DIR}/uploads`` for profile ``ct``, important for the :ref:`Dataverse Application Image `. +- Defaults to ``${com.sun.aas.instanceRoot}/docroot`` in a :doc:`default installation ` + (resolves to ``/usr/local/payara6/glassfish/domains/domain1/docroot``). +- Defaults to ``${STORAGE_DIR}/docroot`` using our :ref:`Dataverse container ` (resolving to ``/dv/docroot``). +- Can also be set via *MicroProfile Config API* sources, e.g. the environment variable ``DATAVERSE_FILES_DOCROOT``. +- During startup, this directory will be checked for existence and write access. It will be created for you + if missing. If it cannot be created or does not have proper write access, application deployment will fail. dataverse.auth.password-reset-timeout-in-minutes ++++++++++++++++++++++++++++++++++++++++++++++++ From efaf5d558b34705f8f6998c56a53a8a3d62050ad Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Fri, 15 Sep 2023 14:19:33 +0200 Subject: [PATCH 19/19] refactor(test,sitemap): make SiteMapUtilTest use better JUnit5 checks --- .../iq/dataverse/sitemap/SiteMapUtilTest.java | 73 +++++++++---------- 1 file changed, 35 insertions(+), 38 deletions(-) diff --git a/src/test/java/edu/harvard/iq/dataverse/sitemap/SiteMapUtilTest.java b/src/test/java/edu/harvard/iq/dataverse/sitemap/SiteMapUtilTest.java index ac6fa1e5166..41032ffa811 100644 --- a/src/test/java/edu/harvard/iq/dataverse/sitemap/SiteMapUtilTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/sitemap/SiteMapUtilTest.java @@ -10,7 +10,6 @@ import edu.harvard.iq.dataverse.util.xml.XmlValidator; import java.io.File; import java.io.IOException; -import java.net.MalformedURLException; import java.net.URL; import java.nio.file.Files; import java.nio.file.Path; @@ -21,17 +20,39 @@ import java.util.ArrayList; import java.util.Date; import java.util.List; -import static org.junit.jupiter.api.Assertions.*; + import static org.junit.jupiter.api.Assertions.*; import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; import org.xml.sax.SAXException; -public class SiteMapUtilTest { - +class SiteMapUtilTest { + + @TempDir + Path tempDir; + Path tempDocroot; + + @BeforeEach + void setup() throws IOException { + // NOTE: This might be unsafe for parallel tests, but our @SystemProperty helper does not yet support + // lookups from vars or methods. + System.setProperty("test.filesDir", tempDir.toString()); + this.tempDocroot = tempDir.resolve("docroot"); + Files.createDirectory(tempDocroot); + } + + @AfterEach + void teardown() { + System.clearProperty("test.filesDir"); + } + @Test - public void testUpdateSiteMap() throws IOException, ParseException { - + void testUpdateSiteMap() throws IOException, ParseException, SAXException { + // given List dataverses = new ArrayList<>(); String publishedDvString = "publishedDv1"; Dataverse publishedDataverse = new Dataverse(); @@ -77,40 +98,18 @@ public void testUpdateSiteMap() throws IOException, ParseException { datasetVersions.add(datasetVersion); deaccessioned.setVersions(datasetVersions); datasets.add(deaccessioned); - - Path tmpDirPath = Files.createTempDirectory(null); - String tmpDir = tmpDirPath.toString(); - File docroot = new File(tmpDir + File.separator + "docroot"); - docroot.mkdirs(); - // TODO: this and the above should be replaced with JUnit 5 @TestDir - System.setProperty("test.filesDir", tmpDir); - + + // when SiteMapUtil.updateSiteMap(dataverses, datasets); - - String pathToTest = tmpDirPath + File.separator + "docroot" + File.separator + "sitemap"; - String pathToSiteMap = pathToTest + File.separator + "sitemap.xml"; - - Exception wellFormedXmlException = null; - try { - assertTrue(XmlValidator.validateXmlWellFormed(pathToSiteMap)); - } catch (Exception ex) { - System.out.println("Exception caught checking that XML is well formed: " + ex); - wellFormedXmlException = ex; - } - assertNull(wellFormedXmlException); - - Exception notValidAgainstSchemaException = null; - try { - assertTrue(XmlValidator.validateXmlSchema(pathToSiteMap, new URL("https://www.sitemaps.org/schemas/sitemap/0.9/sitemap.xsd"))); - } catch (MalformedURLException | SAXException ex) { - System.out.println("Exception caught validating XML against the sitemap schema: " + ex); - notValidAgainstSchemaException = ex; - } - assertNull(notValidAgainstSchemaException); + + // then + String pathToSiteMap = tempDocroot.resolve("sitemap").resolve("sitemap.xml").toString(); + assertDoesNotThrow(() -> XmlValidator.validateXmlWellFormed(pathToSiteMap)); + assertTrue(XmlValidator.validateXmlSchema(pathToSiteMap, new URL("https://www.sitemaps.org/schemas/sitemap/0.9/sitemap.xsd"))); File sitemapFile = new File(pathToSiteMap); String sitemapString = XmlPrinter.prettyPrintXml(new String(Files.readAllBytes(Paths.get(sitemapFile.getAbsolutePath())))); - System.out.println("sitemap: " + sitemapString); + //System.out.println("sitemap: " + sitemapString); assertTrue(sitemapString.contains("1955-11-12")); assertTrue(sitemapString.contains(publishedPid)); @@ -118,8 +117,6 @@ public void testUpdateSiteMap() throws IOException, ParseException { assertFalse(sitemapString.contains(harvestedPid)); assertFalse(sitemapString.contains(deaccessionedPid)); - System.clearProperty("test.filesDir"); - } }