diff --git a/api/src/org/labkey/api/attachments/AttachmentService.java b/api/src/org/labkey/api/attachments/AttachmentService.java index 5524e0aa545..5b10805aa3d 100644 --- a/api/src/org/labkey/api/attachments/AttachmentService.java +++ b/api/src/org/labkey/api/attachments/AttachmentService.java @@ -140,6 +140,8 @@ static AttachmentService get() HttpView getFindAttachmentParentsView(); + void detectOrphans(); + class DuplicateFilenameException extends IOException implements SkipMothershipLogging { private final List _errors = new ArrayList<>(); diff --git a/api/src/org/labkey/api/data/ContainerManager.java b/api/src/org/labkey/api/data/ContainerManager.java index 8e7f4765bb9..74507dfb6ac 100644 --- a/api/src/org/labkey/api/data/ContainerManager.java +++ b/api/src/org/labkey/api/data/ContainerManager.java @@ -38,6 +38,7 @@ import org.labkey.api.admin.FolderWriterImpl; import org.labkey.api.admin.StaticLoggerGetter; import org.labkey.api.attachments.AttachmentParent; +import org.labkey.api.attachments.AttachmentService; import org.labkey.api.audit.AuditLogService; import org.labkey.api.audit.AuditTypeEvent; import org.labkey.api.audit.provider.ContainerAuditProvider; @@ -422,6 +423,7 @@ public static Container createContainerFromTemplate(Container parent, String nam // import objects into the target folder XmlObject folderXml = vf.getXmlBean("folder.xml"); + if (folderXml instanceof FolderDocument folderDoc) { FolderImportContext importCtx = new FolderImportContext(user, c, folderDoc, null, new StaticLoggerGetter(LogManager.getLogger(FolderImporterImpl.class)), vf); @@ -1539,7 +1541,7 @@ else if (hasAncestryRead) } if (!addFolder) - LOG.debug("isNavAccessOpen restriction: \"" + f.getPath() + "\""); + LOG.debug("isNavAccessOpen restriction: \"{}\"", f.getPath()); } if (addFolder) @@ -1915,7 +1917,7 @@ private static boolean delete(final Container c, User user, @Nullable String com throw new IllegalStateException("Container not flagged as being deleted: " + c.getPath()); } - LOG.debug("Starting container delete for " + c.getContainerNoun(true) + " " + c.getPath()); + LOG.debug("Starting container delete for {} {}", c.getContainerNoun(true), c.getPath()); // Tell the search indexer to drop work for the container that's about to be deleted SearchService.get().purgeForContainer(c); @@ -1942,6 +1944,8 @@ private static boolean delete(final Container c, User user, @Nullable String com setContainerTabDeleted(c.getParent(), c.getName(), c.getParent().getFolderType().getName()); } + AttachmentService.get().detectOrphans(); + fireDeleteContainer(c, user); SqlExecutor sqlExecutor = new SqlExecutor(CORE.getSchema()); @@ -1981,11 +1985,11 @@ private static boolean delete(final Container c, User user, @Nullable String com boolean success = CORE.getSchema().getScope().executeWithRetry(tryDeleteContainer); if (success) { - LOG.debug("Completed container delete for " + c.getContainerNoun(true) + " " + c.getPath()); + LOG.debug("Completed container delete for {} {}", c.getContainerNoun(true), c.getPath()); } else { - LOG.warn("Failed to delete container: " + c.getPath()); + LOG.warn("Failed to delete container: {}", c.getPath()); } return success; } @@ -2025,13 +2029,13 @@ public static void deleteAll(Container root, User user, @Nullable String comment if (!hasTreePermission(root, user, DeletePermission.class)) throw new UnauthorizedException("You don't have delete permissions to all folders"); - LOG.debug("Starting container (and children) delete for " + root.getContainerNoun(true) + " " + root.getPath()); + LOG.debug("Starting container (and children) delete for {} {}", root.getContainerNoun(true), root.getPath()); Set depthFirst = getAllChildrenDepthFirst(root); depthFirst.add(root); delete(depthFirst, user, comment); - LOG.debug("Completed container (and children) delete for " + root.getContainerNoun(true) + " " + root.getPath()); + LOG.debug("Completed container (and children) delete for {} {}", root.getContainerNoun(true), root.getPath()); } public static void deleteAll(Container root, User user) throws UnauthorizedException @@ -2426,7 +2430,7 @@ protected static void fireCreateContainer(Container c, User user, @Nullable Stri } catch (Throwable t) { - LOG.error("fireCreateContainer for " + cl.getClass().getName(), t); + LOG.error("fireCreateContainer for {}", cl.getClass().getName(), t); } } } @@ -2437,14 +2441,14 @@ protected static void fireDeleteContainer(Container c, User user) for (ContainerListener l : list) { - LOG.debug("Deleting " + c.getPath() + ": fireDeleteContainer for " + l.getClass().getName()); + LOG.debug("Deleting {}: fireDeleteContainer for {}", c.getPath(), l.getClass().getName()); try { l.containerDeleted(c, user); } catch (RuntimeException e) { - LOG.error("fireDeleteContainer for " + l.getClass().getName(), e); + LOG.error("fireDeleteContainer for {}", l.getClass().getName(), e); // Issue 17560: Fail fast (first Throwable aborts iteration) throw e; @@ -2490,7 +2494,7 @@ public static void firePropertyChangeEvent(ContainerPropertyChangeEvent evt) } catch (Throwable t) { - LOG.error("firePropertyChangeEvent for " + l.getClass().getName(), t); + LOG.error("firePropertyChangeEvent for {}", l.getClass().getName(), t); } } } @@ -2523,8 +2527,8 @@ public static Container createDefaultSupportContainer() // create a "support" container. Admins can do anything, // Users can read/write, Guests can read. return bootstrapContainer(DEFAULT_SUPPORT_PROJECT_PATH, - RoleManager.getRole(AuthorRole.class), - RoleManager.getRole(ReaderRole.class) + RoleManager.getRole(AuthorRole.class), + RoleManager.getRole(ReaderRole.class) ); } @@ -2693,7 +2697,7 @@ public static Container bootstrapContainer(String path, @NotNull Role userRole, if (c == null) { - LOG.debug("Creating new container for path '" + path + "'"); + LOG.debug("Creating new container for path '{}'", path); newContainer = true; c = ensureContainer(path, user); } @@ -2710,7 +2714,7 @@ public static Container bootstrapContainer(String path, @NotNull Role userRole, if (newContainer || 0 == policyCount.intValue()) { - LOG.debug("Setting permissions for '" + path + "'"); + LOG.debug("Setting permissions for '{}'", path); MutableSecurityPolicy policy = new MutableSecurityPolicy(c); policy.addRoleAssignment(SecurityManager.getGroup(Group.groupUsers), userRole); if (guestRole != null) @@ -2881,7 +2885,7 @@ public void testFolderType() private void testOneFolderType(FolderType folderType) { - LOG.info("testOneFolderType(" + folderType.getName() + "): creating container"); + LOG.info("testOneFolderType({}): creating container", folderType.getName()); Container newFolder = createContainer(_testRoot, "folderTypeTest", TestContext.get().getUser()); FolderType ft = newFolder.getFolderType(); assertEquals(FolderType.NONE, ft); @@ -2889,7 +2893,7 @@ private void testOneFolderType(FolderType folderType) Container newFolderFromCache = getForId(newFolder.getId()); assertNotNull(newFolderFromCache); assertEquals(FolderType.NONE, newFolderFromCache.getFolderType()); - LOG.info("testOneFolderType(" + folderType.getName() + "): setting folder type"); + LOG.info("testOneFolderType({}): setting folder type", folderType.getName()); newFolder.setFolderType(folderType, TestContext.get().getUser()); newFolderFromCache = getForId(newFolder.getId()); @@ -2897,9 +2901,9 @@ private void testOneFolderType(FolderType folderType) assertEquals(newFolderFromCache.getFolderType().getName(), folderType.getName()); assertEquals(newFolderFromCache.getFolderType().getDescription(), folderType.getDescription()); - LOG.info("testOneFolderType(" + folderType.getName() + "): deleteAll"); + LOG.info("testOneFolderType({}): deleteAll", folderType.getName()); deleteAll(newFolder, TestContext.get().getUser()); // There might be subfolders because of container tabs - LOG.info("testOneFolderType(" + folderType.getName() + "): deleteAll complete"); + LOG.info("testOneFolderType({}): deleteAll complete", folderType.getName()); Container deletedContainer = getForId(newFolder.getId()); if (deletedContainer != null) @@ -2946,7 +2950,7 @@ private static void logNode(MultiValuedMap mm, String name, int for (String childName : nodes) { - LOG.debug(StringUtils.repeat(" ", offset) + childName); + LOG.debug("{}{}", StringUtils.repeat(" ", offset), childName); logNode(mm, childName, offset + 1); } } diff --git a/api/src/org/labkey/api/data/SqlScriptManager.java b/api/src/org/labkey/api/data/SqlScriptManager.java index 3aba217fb2b..45cbf08fb20 100644 --- a/api/src/org/labkey/api/data/SqlScriptManager.java +++ b/api/src/org/labkey/api/data/SqlScriptManager.java @@ -41,6 +41,8 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; /** @@ -172,7 +174,27 @@ public Set getPreviouslyRunScripts() return runScripts; } + @NotNull + public Collection getPreviouslyRunSqlScriptNames() + { + TableInfo tinfo = getTableInfoSqlScripts(); + + // Skip if the table hasn't been created yet (bootstrap case) + if (getTableInfoSqlScripts().getTableType() == DatabaseTableType.NOT_IN_DB) + return Collections.emptySet(); + + SimpleFilter filter = new SimpleFilter(); + ColumnInfo fileNameColumn = tinfo.getColumn("FileName"); + filter.addCondition(tinfo.getColumn("ModuleName"), _provider.getProviderName()); + filter.addCondition(tinfo.getColumn("FileName"), _schema.getResourcePrefix() + "-", CompareType.STARTS_WITH); + + return new TableSelector(tinfo, Collections.singleton(fileNameColumn), filter, null).getCollection(String.class); + } + private static final String SKIP_SCRIPT_ANNOTATION = "@SkipScriptIfSchemaExists"; + // Use to annotate a script that may take a long time to execute. A message will be logged, including a reason and + // strongly discouraging server shutdown or restart during upgrade. + private static final Pattern LONG_RUNNING_SCRIPT_ANNOTATION_PATTERN = Pattern.compile("@LongRunningScript\\('(?.+)'\\)"); public void runScript(@Nullable User user, SqlScript script, ModuleContext moduleContext, @Nullable Connection conn) throws SqlScriptException { @@ -204,74 +226,49 @@ public void runScript(@Nullable User user, SqlScript script, ModuleContext modul } else { + Matcher matcher = LONG_RUNNING_SCRIPT_ANNOTATION_PATTERN.matcher(contents); + if (matcher.find()) + { + // Reason is expected to be a gerund phrase that summarizes the time-consuming action(s) that the + // script is taking. It should start with a lowercase letter and should not end with punctuation. + // Examples: + // - updating all ObjectId columns to BIGINT + // - restructuring the way workflow jobs are stored + String reason = matcher.group("reason"); + LOG.info( + """ + This script could take a long time to execute because it is {}. + Do NOT shut down or restart the server until this script and the rest of the upgrade is complete. + Any interruption will likely corrupt the schemas, requiring a database restore and a restart of the upgrade process.""", + reason + ); + } dialect.runSql(description, schema, contents, moduleContext, conn); LOG.info("Finished running script: {}", description); } } - catch(Throwable t) + catch (Throwable t) { throw new SqlScriptException(t, description); } if (script.isValidName()) - { - // Should never be true, unless getNewScripts() isn't doing its job. TODO: Remove update() branch below. - assert !hasBeenRun(script); - if (hasBeenRun(script)) - update(user, script); - else - insert(user, script); - } + insert(user, script); } - @NotNull - public Collection getPreviouslyRunSqlScriptNames() + private void insert(@Nullable User user, SqlScript script) { TableInfo tinfo = getTableInfoSqlScripts(); - // Skip if the table hasn't been created yet (bootstrap case) - if (getTableInfoSqlScripts().getTableType() == DatabaseTableType.NOT_IN_DB) - return Collections.emptySet(); - - SimpleFilter filter = new SimpleFilter(); - ColumnInfo fileNameColumn = tinfo.getColumn("FileName"); - filter.addCondition(tinfo.getColumn("ModuleName"), _provider.getProviderName()); - filter.addCondition(tinfo.getColumn("FileName"), _schema.getResourcePrefix() + "-", CompareType.STARTS_WITH); - - return new TableSelector(tinfo, Collections.singleton(fileNameColumn), filter, null).getCollection(String.class); - } - - public boolean hasBeenRun(SqlScript script) - { - TableInfo tinfo = getTableInfoSqlScripts(); - - // Make sure DbSchema thinks SqlScript table is in the database. If not, we're bootstrapping and it's either just before or just after the first - // script is run. In either case, invalidate to force reloading schema from database meta data. + // Make sure DbSchema thinks SqlScripts table is in the database. If not, we're bootstrapping, and it's just + // after the first script has run. Invalidate to force reloading the schema from database metadata. if (tinfo.getTableType() == DatabaseTableType.NOT_IN_DB) { CacheManager.clearAllKnownCaches(); - return false; + tinfo = getTableInfoSqlScripts(); // Reload to update table type } - PkFilter filter = new PkFilter(tinfo, new String[]{_provider.getProviderName(), script.getDescription()}); - - return new TableSelector(getTableInfoSqlScripts(), filter, null).exists(); - } - - - public void insert(@Nullable User user, SqlScript script) - { - SqlScriptBean ss = new SqlScriptBean(script.getProvider().getProviderName(), script.getDescription()); - - Table.insert(user, getTableInfoSqlScripts(), ss); - } - - - public void update(@Nullable User user, SqlScript script) - { - Object[] pk = new Object[]{script.getProvider().getProviderName(), script.getDescription()}; - - Table.update(user, getTableInfoSqlScripts(), new HashMap<>(), pk); // Update user and modified date + Table.insert(user, tinfo, new SqlScriptBean(script.getProvider().getProviderName(), script.getDescription())); } // Allow null version for oddball cases like gel_reports, which claims to have schemas but no schema version. That @@ -301,7 +298,7 @@ public void updateSchemaVersion(Double version) } - public @NotNull SchemaBean ensureSchemaBean() + protected @NotNull SchemaBean ensureSchemaBean() { SchemaBean bean = getSchemaBean(); @@ -309,7 +306,7 @@ public void updateSchemaVersion(Double version) } - protected @Nullable SchemaBean getSchemaBean() + private @Nullable SchemaBean getSchemaBean() { TableInfo tinfo = getTableInfoSchemas(); diff --git a/api/src/org/labkey/api/reports/model/ReportPropsManager.java b/api/src/org/labkey/api/reports/model/ReportPropsManager.java index d6a63835079..5425c068355 100644 --- a/api/src/org/labkey/api/reports/model/ReportPropsManager.java +++ b/api/src/org/labkey/api/reports/model/ReportPropsManager.java @@ -47,7 +47,6 @@ import java.util.ArrayList; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; diff --git a/api/src/org/labkey/api/search/NoopSearchService.java b/api/src/org/labkey/api/search/NoopSearchService.java index 3b91b203d22..d9461ed8af8 100644 --- a/api/src/org/labkey/api/search/NoopSearchService.java +++ b/api/src/org/labkey/api/search/NoopSearchService.java @@ -29,7 +29,6 @@ import org.labkey.api.webdav.WebdavResource; import java.io.Reader; -import java.time.Duration; import java.util.Collection; import java.util.Collections; import java.util.Date; @@ -292,7 +291,7 @@ public void deleteIndex(String reason) } @Override - public void clearLastIndexed() + public void clearLastIndexed(String reason) { } @@ -379,7 +378,7 @@ public boolean isRunning() } @Override - public void updateIndex() + public void updateIndex(String reason) { } diff --git a/api/src/org/labkey/api/search/SearchService.java b/api/src/org/labkey/api/search/SearchService.java index 9a1fc3687f6..48a2632c59a 100644 --- a/api/src/org/labkey/api/search/SearchService.java +++ b/api/src/org/labkey/api/search/SearchService.java @@ -54,7 +54,6 @@ import java.io.IOException; import java.io.InputStream; import java.io.Reader; -import java.net.URISyntaxException; import java.sql.SQLException; import java.sql.Timestamp; import java.util.ArrayList; @@ -438,7 +437,7 @@ public String normalizeHref(Path contextPath, Container c) void resetIndex(); void startCrawler(); void pauseCrawler(); - void updateIndex(); + void updateIndex(String reason); void refreshNow(); @Nullable Throwable getConfigurationError(); @@ -472,7 +471,7 @@ public String normalizeHref(Path contextPath, Container c) void deleteContainer(String id); void deleteIndex(String reason); // close the index if it's been initialized, then delete the index directory and reset lastIndexed values - void clearLastIndexed(); // reset lastIndexed values and initiate aggressive crawl. must be callable before (and after) start() has been called. + void clearLastIndexed(String reason); // reset lastIndexed values and initiate aggressive crawl. must be callable before (and after) start() has been called. void maintenance(); // diff --git a/api/src/org/labkey/api/settings/OptionalFeatureFlag.java b/api/src/org/labkey/api/settings/OptionalFeatureFlag.java index baf0c447ed5..5cabb76c0a1 100644 --- a/api/src/org/labkey/api/settings/OptionalFeatureFlag.java +++ b/api/src/org/labkey/api/settings/OptionalFeatureFlag.java @@ -86,7 +86,9 @@ public String getPropertyName() String name = getFlag(); if (!StringUtilsLabKey.isValidJavaIdentifier(name)) { - LOG.warn("Feature flag name doesn't conform to the property name rules so it won't be available as a startup property: {}", name); + // This is a developer thing... no need to log a warning on production deployments + if (AppProps.getInstance().isDevMode()) + LOG.warn("Feature flag name doesn't conform to the property name rules so it won't be available as a startup property: {}", name); name = null; } return name; diff --git a/api/src/org/labkey/api/study/DataspaceContainerFilter.java b/api/src/org/labkey/api/study/DataspaceContainerFilter.java index 9f5ba049207..f99c5e222cd 100644 --- a/api/src/org/labkey/api/study/DataspaceContainerFilter.java +++ b/api/src/org/labkey/api/study/DataspaceContainerFilter.java @@ -15,7 +15,6 @@ */ package org.labkey.api.study; -import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.labkey.api.cache.Cache; import org.labkey.api.cache.CacheManager; diff --git a/api/src/org/labkey/api/util/MailHelper.java b/api/src/org/labkey/api/util/MailHelper.java index 3f3e7017439..fc6e31d4786 100644 --- a/api/src/org/labkey/api/util/MailHelper.java +++ b/api/src/org/labkey/api/util/MailHelper.java @@ -29,7 +29,6 @@ import jakarta.mail.internet.MimeMessage; import jakarta.mail.internet.MimeMultipart; import org.apache.commons.lang3.StringUtils; -import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -38,11 +37,9 @@ import org.labkey.api.data.Container; import org.labkey.api.data.ContainerManager; import org.labkey.api.security.User; -import org.labkey.api.security.permissions.TroubleshooterPermission; import org.labkey.api.settings.AppProps; -import org.labkey.api.util.HtmlString; import org.labkey.api.util.emailTemplate.EmailTemplate; -import org.labkey.api.view.ViewContext; +import org.labkey.api.util.logging.LogHelper; import org.labkey.api.view.template.WarningProvider; import org.labkey.api.view.template.WarningService; import org.labkey.api.view.template.Warnings; @@ -59,14 +56,13 @@ import java.util.StringTokenizer; /** - * Provides static functions for help with sending email. - * Supports SMTP and Microsoft Graph transport providers. + * Provides static functions for help with sending email. Supports SMTP and Microsoft Graph transport providers. */ public class MailHelper { public static final String MESSAGE_AUDIT_EVENT = "MessageAuditEvent"; - private static final Logger _log = LogManager.getLogger(MailHelper.class); + private static final Logger _log = LogHelper.getLogger(MailHelper.class, "Errors sending and configuring email"); // Transport providers private static final SmtpTransportProvider _smtpProvider = new SmtpTransportProvider(); @@ -109,16 +105,15 @@ private static EmailTransportProvider loadActiveProvider() WarningService.get().register(new WarningProvider() { @Override - public void addDynamicWarnings(@NotNull Warnings warnings, @Nullable ViewContext context, boolean showAllWarnings) + public void addStaticWarnings(@NotNull Warnings warnings, boolean showAllWarnings) { - if (context == null || context.getUser().hasRootPermission(TroubleshooterPermission.class)) - warnings.add(HtmlString.of(message)); + warnings.add(HtmlString.of(message)); } }); return null; } - return configured.isEmpty() ? null : configured.get(0); + return configured.isEmpty() ? null : configured.getFirst(); } public static EmailTransportProvider getActiveProvider() @@ -196,12 +191,12 @@ private static ViewMessage _createMessage(ViewMessage m, String from, @Nullable */ public static Address[] createAddressArray(String s) throws AddressException { - List addrs = new ArrayList<>(); + List addresses = new ArrayList<>(); StringTokenizer st = new StringTokenizer(s, ";"); while (st.hasMoreTokens()) - addrs.add(new InternetAddress(st.nextToken())); + addresses.add(new InternetAddress(st.nextToken())); - return addrs.toArray(new Address[0]); + return addresses.toArray(new Address[0]); } /** @@ -221,8 +216,7 @@ public static void send(Message m, @Nullable User user, Container c) // Check for configuration conflict if (_configurationConflict) { - throw new ConfigurationException( - "Multiple email transport methods are configured. Please configure only one email transport method."); + throw new ConfigurationException("Multiple email transport methods are configured. Please configure only one email transport method."); } // Check if any provider is configured @@ -289,10 +283,14 @@ private static void logMessagingException(Message m, Exception e) { try { - _log.error(ERROR_MESSAGE + - "\nfrom: " + StringUtils.join(m.getFrom(), "; ") + "\n" + - "to: " + StringUtils.join(m.getRecipients(RecipientType.TO), "; ") + "\n" + - "subject: " + m.getSubject(), e); + _log.error( + "{}\nfrom: {}\nto: {}\nsubject: {}", + ERROR_MESSAGE, + StringUtils.join(m.getFrom(), "; "), + StringUtils.join(m.getRecipients(RecipientType.TO), "; "), + m.getSubject(), + e + ); } catch (MessagingException ex) { @@ -545,11 +543,11 @@ public void run() } catch (MessagingException e) { - _log.error("Failed to send message to " + email, e); + _log.error("Failed to send message to {}", email, e); } catch (ConfigurationException e) { - _log.error("Error sending email: " + e.getMessage(), e); + _log.error("Error sending email: {}", e.getMessage(), e); } } } diff --git a/api/src/org/labkey/api/view/template/WarningProvider.java b/api/src/org/labkey/api/view/template/WarningProvider.java index 427cc3956f3..eb7b4c12834 100644 --- a/api/src/org/labkey/api/view/template/WarningProvider.java +++ b/api/src/org/labkey/api/view/template/WarningProvider.java @@ -22,8 +22,10 @@ public interface WarningProvider { /** - * Add warnings for conditions that will never change while the server is running (e.g., size of JVM heap or Tomcat - * version). These warnings are displayed to site administrators, application administrators, and troubleshooters. + * Add warnings for conditions that don't change after this method is first called, unless the showAllWarnings + * flag is toggled. Examples of static conditions: size of JVM heap or the email settings in application.properties. + * These warnings are displayed to site administrators, application admins, and troubleshooters. The method could + * be called multiple times, for example, if the showAllWarnings flag is toggled or a new provider is registered. * @param warnings A @NotNull Warnings collector * @param showAllWarnings A flag for testing that indicates the provider should unconditionally add all warnings */ @@ -37,7 +39,8 @@ default void addStaticWarnings(@NotNull Warnings warnings, boolean showAllWarnin * @param warnings A @NotNull Warnings collector * @param context optionally, a ViewContext for which the warnings can be customized. Null when checking for * warnings that are scoped to the whole server's health, which typically correlate with site-wide - * messages shown to site admins. If a ViewContext is provided, it will have a user, container, and request + * messages shown to site admins. If a ViewContext is provided, it will have a user, container, and + * request * @param showAllWarnings A flag for testing that indicates the provider should add all warnings if its standard * permissions check passes. */ diff --git a/core/src/org/labkey/core/CoreContainerListener.java b/core/src/org/labkey/core/CoreContainerListener.java index d2f66ef7c78..11e4740ac19 100644 --- a/core/src/org/labkey/core/CoreContainerListener.java +++ b/core/src/org/labkey/core/CoreContainerListener.java @@ -67,8 +67,9 @@ public void containerDeleted(Container c, User user) // report engine folder mapping Table.delete(CoreSchema.getInstance().getTableInfoReportEngineMap(), containerFilter); - // Let containerManager delete ACLs, we want that to happen last Portal.containerDeleted(c); + + // Note: ContainerManager deletes security policies and DB sequences after it deletes the container } @Override diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index 0698a8e0f08..e8b00fb4851 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -88,6 +88,7 @@ import org.labkey.api.files.FileBrowserConfigWriter; import org.labkey.api.files.FileContentService; import org.labkey.api.markdown.MarkdownService; +import org.labkey.api.mcp.McpService; import org.labkey.api.message.settings.MessageConfigService; import org.labkey.api.migration.DatabaseMigrationService; import org.labkey.api.module.FolderType; @@ -98,7 +99,6 @@ import org.labkey.api.module.SchemaUpdateType; import org.labkey.api.module.SpringModule; import org.labkey.api.module.Summary; -import org.labkey.api.mcp.McpService; import org.labkey.api.notification.EmailMessage; import org.labkey.api.notification.EmailService; import org.labkey.api.notification.NotificationMenuView; @@ -149,7 +149,7 @@ import org.labkey.api.settings.AppProps; import org.labkey.api.settings.CustomLabelService; import org.labkey.api.settings.CustomLabelService.CustomLabelServiceImpl; -import org.labkey.api.settings.FolderSettingsCache; +import org.labkey.api.settings.FolderSettingsCache.FolderSettingsCacheListener; import org.labkey.api.settings.LookAndFeelProperties; import org.labkey.api.settings.LookAndFeelPropertiesManager; import org.labkey.api.settings.LookAndFeelPropertiesManager.ResourceType; @@ -246,6 +246,7 @@ import org.labkey.core.admin.writer.SecurityGroupWriterFactory; import org.labkey.core.analytics.AnalyticsController; import org.labkey.core.analytics.AnalyticsServiceImpl; +import org.labkey.core.attachment.AttachmentContainerListener; import org.labkey.core.attachment.AttachmentServiceImpl; import org.labkey.core.dialect.PostgreSqlDialectFactory; import org.labkey.core.dialect.PostgreSqlVersion; @@ -253,9 +254,9 @@ import org.labkey.core.login.DbLoginAuthenticationProvider; import org.labkey.core.login.DbLoginManager; import org.labkey.core.login.LoginController; +import org.labkey.core.mcp.McpServiceImpl; import org.labkey.core.metrics.SimpleMetricsServiceImpl; import org.labkey.core.metrics.WebSocketConnectionManager; -import org.labkey.core.mcp.McpServiceImpl; import org.labkey.core.notification.EmailPreferenceConfigServiceImpl; import org.labkey.core.notification.EmailPreferenceContainerListener; import org.labkey.core.notification.EmailPreferenceUserListener; @@ -542,7 +543,7 @@ public QuerySchema createSchema(DefaultSchema schema, Module module) false); OptionalFeatureService.get().addExperimentalFeatureFlag(AppProps.REJECT_CONTROLLER_FIRST_URLS, "Reject controller-first URLs", - "Require standard path-first URLs. Note: This option will be ignored if the deprecated feature for generating controller-first URLs is enabled.", + "Require standard path-first URLs.", false ); @@ -961,9 +962,12 @@ public void startupAfterSpringConfig(ModuleContext moduleContext) checkForMissingDbViews(); ProductConfiguration.handleStartupProperties(); + // This listener deletes all properties; make sure it executes after most of the other listeners ContainerManager.addContainerListener(new CoreContainerListener(), ContainerManager.ContainerListener.Order.Last); - ContainerManager.addContainerListener(new FolderSettingsCache.FolderSettingsCacheListener()); + // This listener deletes all attachments in the container; execute it just before CoreContainerListener + ContainerManager.addContainerListener(new AttachmentContainerListener(), ContainerManager.ContainerListener.Order.Last); + ContainerManager.addContainerListener(new FolderSettingsCacheListener()); SecurityManager.init(); FolderTypeManager.get().registerFolderType(this, FolderType.NONE); FolderTypeManager.get().registerFolderType(this, new CollaborationFolderType()); diff --git a/core/src/org/labkey/core/attachment/AttachmentContainerListener.java b/core/src/org/labkey/core/attachment/AttachmentContainerListener.java new file mode 100644 index 00000000000..9c183c91510 --- /dev/null +++ b/core/src/org/labkey/core/attachment/AttachmentContainerListener.java @@ -0,0 +1,17 @@ +package org.labkey.core.attachment; + +import org.labkey.api.data.Container; +import org.labkey.api.data.ContainerManager.ContainerListener; +import org.labkey.api.data.CoreSchema; +import org.labkey.api.security.User; +import org.labkey.api.util.ContainerUtil; + +public class AttachmentContainerListener implements ContainerListener +{ + @Override + public void containerDeleted(Container c, User user) + { + ContainerUtil.purgeTable(CoreSchema.getInstance().getTableInfoDocuments(), c, null); + AttachmentCache.removeAttachments(c); + } +} diff --git a/core/src/org/labkey/core/attachment/AttachmentServiceImpl.java b/core/src/org/labkey/core/attachment/AttachmentServiceImpl.java index 6a1012da65a..ed266908800 100644 --- a/core/src/org/labkey/core/attachment/AttachmentServiceImpl.java +++ b/core/src/org/labkey/core/attachment/AttachmentServiceImpl.java @@ -21,6 +21,7 @@ import org.apache.commons.io.IOUtils; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.Strings; +import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.junit.Assert; @@ -37,12 +38,14 @@ import org.labkey.api.audit.AuditLogService; import org.labkey.api.audit.provider.FileSystemAuditProvider; import org.labkey.api.collections.CaseInsensitiveHashSet; +import org.labkey.api.collections.CsvSet; import org.labkey.api.collections.LabKeyCollectors; import org.labkey.api.collections.Sets; import org.labkey.api.data.ColumnInfo; import org.labkey.api.data.ColumnRenderProperties; import org.labkey.api.data.CompareType; import org.labkey.api.data.Container; +import org.labkey.api.data.ContainerFilter.AllFolders; import org.labkey.api.data.ContainerManager; import org.labkey.api.data.CoreSchema; import org.labkey.api.data.DatabaseTableType; @@ -66,20 +69,22 @@ import org.labkey.api.exp.Lsid; import org.labkey.api.files.FileContentService; import org.labkey.api.files.MissingRootDirectoryException; +import org.labkey.api.query.DefaultSchema; import org.labkey.api.query.FieldKey; import org.labkey.api.query.QuerySettings; import org.labkey.api.query.QueryView; import org.labkey.api.query.UserSchema; import org.labkey.api.search.SearchService; import org.labkey.api.security.AuthenticationLogoAttachmentParent; +import org.labkey.api.security.ElevatedUser; import org.labkey.api.security.SecurableResource; import org.labkey.api.security.SecurityManager; import org.labkey.api.security.User; import org.labkey.api.security.UserManager; import org.labkey.api.security.permissions.Permission; +import org.labkey.api.security.roles.TroubleshooterRole; import org.labkey.api.settings.AppProps; import org.labkey.api.test.TestWhen; -import org.labkey.api.util.ContainerUtil; import org.labkey.api.util.FileStream; import org.labkey.api.util.FileUtil; import org.labkey.api.util.GUID; @@ -91,8 +96,10 @@ import org.labkey.api.util.Path; import org.labkey.api.util.ResponseHelper; import org.labkey.api.util.ResultSetUtil; +import org.labkey.api.util.StringUtilsLabKey; import org.labkey.api.util.TestContext; import org.labkey.api.util.URLHelper; +import org.labkey.api.util.logging.LogHelper; import org.labkey.api.view.ActionURL; import org.labkey.api.view.HttpView; import org.labkey.api.view.JspView; @@ -106,6 +113,7 @@ import org.labkey.api.webdav.WebdavResolver; import org.labkey.api.webdav.WebdavResource; import org.labkey.core.query.AttachmentAuditProvider; +import org.labkey.core.query.CoreQuerySchema; import org.springframework.http.ContentDisposition; import org.springframework.mock.web.MockMultipartFile; import org.springframework.validation.BindException; @@ -136,18 +144,15 @@ import java.util.Objects; import java.util.Set; import java.util.TreeSet; +import java.util.stream.Collectors; -public class AttachmentServiceImpl implements AttachmentService, ContainerManager.ContainerListener +public class AttachmentServiceImpl implements AttachmentService { + private static final Logger LOG = LogHelper.getLogger(AttachmentServiceImpl.class, "Orphaned attachments"); private static final String UPLOAD_LOG = ".upload.log"; private static final Map ATTACHMENT_TYPE_MAP = new HashMap<>(); private static final Set ATTACHMENT_COLUMNS = Set.of("Parent", "Container", "DocumentName", "DocumentSize", "DocumentType", "Created", "CreatedBy", "LastIndexed"); - public AttachmentServiceImpl() - { - ContainerManager.addContainerListener(this); - } - @Override public void download(HttpServletResponse response, AttachmentParent parent, String filename, @Nullable String alias, boolean inlineIfPossible) throws ServletException, IOException { @@ -182,7 +187,6 @@ public void download(HttpServletResponse response, AttachmentParent parent, Stri } } - @Override public void download(HttpServletResponse response, AttachmentParent parent, String filename, boolean inlineIfPossible) throws ServletException, IOException { @@ -906,14 +910,6 @@ public Map getAttachments(AttachmentParent parent, Collectio } } - @Override - public void containerDeleted(Container c, User user) - { - // TODO: do we need to get each document and remove its security policy? - ContainerUtil.purgeTable(coreTables().getTableInfoDocuments(), c, null); - AttachmentCache.removeAttachments(c); - } - private void writeDocument(DocumentWriter writer, AttachmentParent parent, String name, @Nullable String alias, boolean asAttachment) throws ServletException, IOException { checkSecurityPolicy(parent); @@ -1007,7 +1003,6 @@ public void writeDocument(DocumentWriter writer, AttachmentParent parent, String writeDocument(writer, parent, name, null, asAttachment); } - @Override @NotNull public InputStream getInputStream(AttachmentParent parent, String name) throws FileNotFoundException @@ -1098,6 +1093,47 @@ public int available() } } + private static final int MAX_ORPHANS_TO_LOG = 20; + + private record Orphan(String documentName, String parentType){} + + @Override + public void detectOrphans() + { + // Log orphaned attachments in this server, but in dev mode only, since this is for our testing. Also, we + // don't yet offer a way to delete orphaned attachments via the UI, so it's not helpful to inform admins. + if (AppProps.getInstance().isDevMode()) + { + User user = ElevatedUser.getElevatedUser(User.getSearchUser(), TroubleshooterRole.class); + UserSchema core = DefaultSchema.get(user, ContainerManager.getRoot()).getUserSchema(CoreQuerySchema.NAME); + if (core != null) + { + TableInfo documents = core.getTable(CoreQuerySchema.DOCUMENTS_TABLE_NAME, new AllFolders(user)); + if (null != documents) + { + SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("Orphaned"), true); + List orphans = new TableSelector(documents, new CsvSet("DocumentName, ParentType"), filter, null).getArrayList(Orphan.class); + if (!orphans.isEmpty()) + { + LOG.error("Found {}, which likely indicates a problem with a delete method or a container listener.", StringUtilsLabKey.pluralize(orphans.size(), "orphaned attachment")); + + final String message; + if (orphans.size() > MAX_ORPHANS_TO_LOG) + { + orphans = orphans.subList(0, MAX_ORPHANS_TO_LOG); + message = "The first " + MAX_ORPHANS_TO_LOG; + } + else + { + message = "All"; + } + + LOG.error("{} detected orphans are listed below:\n{}", message, orphans.stream().map(Record::toString).collect(Collectors.joining("\n"))); + } + } + } + } + } private CoreSchema coreTables() { diff --git a/core/src/org/labkey/core/view/template/bootstrap/WarningServiceImpl.java b/core/src/org/labkey/core/view/template/bootstrap/WarningServiceImpl.java index c005543005d..3537243d157 100644 --- a/core/src/org/labkey/core/view/template/bootstrap/WarningServiceImpl.java +++ b/core/src/org/labkey/core/view/template/bootstrap/WarningServiceImpl.java @@ -42,6 +42,8 @@ public class WarningServiceImpl implements WarningService { + private static final Object STATIC_WARNING_LOCK = new Object(); + private static volatile Collection STATIC_ADMIN_WARNINGS = null; private static volatile boolean SHOW_ALL_WARNINGS = false; @@ -81,6 +83,7 @@ public boolean showAllWarnings() public void register(WarningProvider provider) { _providers.add(provider); + STATIC_ADMIN_WARNINGS = null; } @Override @@ -90,27 +93,22 @@ public void forEachProvider(Consumer consumer) } // Check warning conditions that don't change after the server has started up. We'll collect and stash these during - // the first request after startup is complete. They are also re-collected if the "show all warnings" experimental - // feature flag is used. + // the first request after startup is complete. They are also re-collected if a new provider is registered or if + // the "show all warnings" experimental feature flag is used. private static Collection getStaticAdminWarnings() { - if (STATIC_ADMIN_WARNINGS != null) + if (null == STATIC_ADMIN_WARNINGS) { - return STATIC_ADMIN_WARNINGS; - } - - LazyInitializer.init(); // Invoke no-op method to ensure static initializer is executed - Warnings warnings = Warnings.of(new LinkedList<>()); - WarningService.get().forEachProvider(p -> p.addStaticWarnings(warnings, WarningService.get().showAllWarnings())); - List messages = Collections.unmodifiableList(warnings.getMessages()); - - if (ModuleLoader.getInstance().isStartupComplete()) - { - // We should have our full list of warnings at this point, so safe to stash them - STATIC_ADMIN_WARNINGS = messages; + synchronized (STATIC_WARNING_LOCK) + { + LazyInitializer.init(); // Invoke no-op method to ensure static initializer is executed + Warnings warnings = Warnings.of(new LinkedList<>()); + WarningService.get().forEachProvider(p -> p.addStaticWarnings(warnings, WarningService.get().showAllWarnings())); + STATIC_ADMIN_WARNINGS = Collections.unmodifiableList(warnings.getMessages());; + } } - return messages; + return STATIC_ADMIN_WARNINGS; } private static final String DISMISSAL_SCRIPT_FORMAT = diff --git a/experiment/resources/schemas/dbscripts/postgresql/exp-25.004-25.005.sql b/experiment/resources/schemas/dbscripts/postgresql/exp-25.004-25.005.sql index 795e2a18a80..c942d8f139c 100644 --- a/experiment/resources/schemas/dbscripts/postgresql/exp-25.004-25.005.sql +++ b/experiment/resources/schemas/dbscripts/postgresql/exp-25.004-25.005.sql @@ -1,3 +1,5 @@ +-- @LongRunningScript('updating all ObjectId columns to BIGINT') + -- Change all ObjectId columns to BIGINT ALTER TABLE exp.Object ALTER COLUMN ObjectId TYPE BIGINT; ALTER TABLE exp.Object ALTER COLUMN OwnerObjectId TYPE BIGINT; diff --git a/experiment/resources/schemas/dbscripts/sqlserver/exp-25.004-25.005.sql b/experiment/resources/schemas/dbscripts/sqlserver/exp-25.004-25.005.sql index ebac9019797..1c4e21f90af 100644 --- a/experiment/resources/schemas/dbscripts/sqlserver/exp-25.004-25.005.sql +++ b/experiment/resources/schemas/dbscripts/sqlserver/exp-25.004-25.005.sql @@ -1,3 +1,5 @@ +-- @LongRunningScript('updating all ObjectId columns to BIGINT') + -- Viability no longer supports SQL Server, but its old schema may have been left behind. Ensure it and its FK to ObjectId are dropped. EXEC core.fn_dropifexists '*', 'viability', 'SCHEMA'; diff --git a/search/src/org/labkey/search/SearchController.java b/search/src/org/labkey/search/SearchController.java index 74f4da262b4..68b6ebdc79d 100644 --- a/search/src/org/labkey/search/SearchController.java +++ b/search/src/org/labkey/search/SearchController.java @@ -345,7 +345,7 @@ else if (form.isDelete()) else if (form.isPath()) { SearchPropertyManager.setIndexPath(getUser(), form.getIndexPath()); - ss.updateIndex(); + ss.updateIndex("a site admin changed the full-text search index path"); _msgid = 2; } else if (form.isDirectory()) diff --git a/search/src/org/labkey/search/SearchModule.java b/search/src/org/labkey/search/SearchModule.java index 679d8bbe5c9..7851bca0178 100644 --- a/search/src/org/labkey/search/SearchModule.java +++ b/search/src/org/labkey/search/SearchModule.java @@ -212,7 +212,7 @@ public List getTablesToCopy() public void afterMigration(DatabaseMigrationConfiguration configuration) { // Clear index and all last indexed tracking - SearchService.get().deleteIndex("Database was just migrated"); + SearchService.get().deleteIndex("the database was just migrated"); } }); } diff --git a/search/src/org/labkey/search/model/AbstractSearchService.java b/search/src/org/labkey/search/model/AbstractSearchService.java index 683b37437a1..3fae3b2f7a7 100644 --- a/search/src/org/labkey/search/model/AbstractSearchService.java +++ b/search/src/org/labkey/search/model/AbstractSearchService.java @@ -488,15 +488,15 @@ public final void deleteContainer(final String id) } @Override - public void clearLastIndexed() + public void clearLastIndexed(String reason) { - _log.info("Clearing last indexed for all providers"); + _log.info("Clearing last indexed for all providers because: {}", reason); for (DocumentProvider p : _documentProviders) { try { - _log.info("Clearing last indexed for provider : " + p.getClass().getName()); + _log.info("Clearing last indexed for provider : {}", p.getClass().getName()); p.indexDeleted(); } catch (Throwable t) @@ -960,7 +960,7 @@ public void pauseCrawler() @Override - public void updateIndex() + public void updateIndex(String reason) { // Subclasses should switch out the index at this point. } @@ -1155,7 +1155,7 @@ public Container getContainer() { /* */ } - _log.error("Error running " + (null != i ? i._id : ""), x); + _log.error("Error running {}", null != i ? i._id : "", x); } finally { @@ -1284,7 +1284,7 @@ private void _indexLoop() WebdavResource r = i.getResource(); if (null == r || !r.exists()) { - _log.info("Document no longer exist, skipping: " + i._id); + _log.info("Document no longer exist, skipping: {}", i._id); // This is a strange case. If this resource doesn't exist anymore, it is not really an error. // see 34102: Search indexing is unreliable for wiki attachments i.complete(true); @@ -1296,7 +1296,7 @@ private void _indexLoop() i._modified = r.getLastModified(); MemTracker.getInstance().put(r); - _log.debug("processAndIndex(" + i._id + ")"); + _log.debug("processAndIndex({})", i._id); Throwable[] out = new Throwable[] {null}; @@ -1334,12 +1334,12 @@ private void _indexLoop() } } else - _log.debug("skipping " + i._id); + _log.debug("skipping {}", i._id); } catch (InterruptedException ignored) {} catch (Throwable x) { - _log.error("Error indexing " + (null != i ? i._id : ""), x); + _log.error("Error indexing {}", null != i ? i._id : "", x); } finally { @@ -1462,7 +1462,7 @@ public void addDocumentParser(DocumentParser parser) @Override public IndexTask indexContainer(IndexTask in, final Container c, final Date since) { - _log.debug("Indexing container \"" + c + "\", since: " + since); + _log.debug("Indexing container \"{}\", since: {}", c, since); final IndexTask task = null==in ? createTask("Index folder " + c.getPath()) : in; task.getQueue(c, PRIORITY.crawl).addRunnable((q) -> { @@ -1485,7 +1485,7 @@ public IndexTask indexContainer(IndexTask in, final Container c, final Date sinc @Override public void indexFull(final boolean force, String reason) { - _log.info("Initiating an aggressive full-text search reindex because: " + reason); + _log.info("Initiating an aggressive full-text search reindex because: {}", reason); // crank crawler into high gear! DavCrawler.getInstance().startFull(WebdavService.getPath(), force); } diff --git a/search/src/org/labkey/search/model/LuceneSearchServiceImpl.java b/search/src/org/labkey/search/model/LuceneSearchServiceImpl.java index f3cea527fdf..3c086c45513 100644 --- a/search/src/org/labkey/search/model/LuceneSearchServiceImpl.java +++ b/search/src/org/labkey/search/model/LuceneSearchServiceImpl.java @@ -447,9 +447,9 @@ static LuceneDirectoryType getDirectoryType() } @Override - public void updateIndex() + public void updateIndex(String reason) { - super.updateIndex(); + super.updateIndex(reason); // Commit and close current index commit(); @@ -464,7 +464,7 @@ public void updateIndex() // Initialize new index and clear last indexed initializeIndex(); - clearLastIndexed(); + clearLastIndexed(reason); } @Override @@ -547,7 +547,7 @@ private void handleEmptyOrMismatchedIndex() try { if (getNumDocs() == 0) - clearLastIndexed(); + clearLastIndexed("the index is empty or missing"); Map map = getProperties(); @NotNull String serverGuid = AppProps.getInstance().getServerGUID(); @@ -619,7 +619,7 @@ public void deleteIndex(String reason) { // Queue the work to run on the indexing thread to avoid deadlocks with concurrent // database operations (e.g., truncating lastIndexed while indexing threads are querying) - _log.info("Queuing deletion of full-text search index because: " + reason); + _log.info("Queuing deletion of full-text search index because: {}", reason); final CountDownLatch latch = new CountDownLatch(1); _IndexTask task = createTask("DeleteIndex", new TaskListener() { @@ -654,7 +654,7 @@ public void indexError(Resource r, Throwable t) private void deleteIndexImpl(String reason) { - _log.info("Deleting full-text search index and clearing last indexed because: " + reason); + _log.info("Deleting full-text search index because: {}", reason); if (isIndexManagerReady()) closeIndex(); @@ -663,16 +663,16 @@ private void deleteIndexImpl(String reason) if (indexDir != null && indexDir.exists()) FileUtil.deleteDir(indexDir); - clearLastIndexed(); + clearLastIndexed(reason); } @Override - public void clearLastIndexed() + public void clearLastIndexed(String reason) { // Short circuit if nothing has been indexed since clearLastIndexed() was last called if (_countIndexedSinceClearLastIndexed.get() > 0) { - super.clearLastIndexed(); + super.clearLastIndexed(reason); _countIndexedSinceClearLastIndexed.set(0); } }