Merge remote-tracking branch 'origin/develop' into fb_issue1058

Author: XingY

announcements/src/org/labkey/announcements/announcementThread.jsp

@@ -127,7 +127,7 @@ if (!announcementModel.getAttachments().isEmpty())
         {
             ActionURL downloadURL = AnnouncementsController.getDownloadURL(announcementModel, d.getName());
         %>
-        <a href="<%=h(downloadURL)%>"><img alt="" src="<%=getWebappURL(d.getFileIcon())%>">&nbsp;<%=h(d.getName())%></a>&nbsp;<%
+        <%=d.renderDownloadLink(downloadURL)%>&nbsp;<%
         } %>
     </div></td>
 </tr><%
@@ -210,7 +210,7 @@ if (!announcementModel.getResponses().isEmpty())
                 {
                     ActionURL downloadURL = AnnouncementsController.getDownloadURL(r, rd.getName());
                 %>
-                    <a href="<%=h(downloadURL)%>"><img alt="" src="<%=getWebappURL(rd.getFileIcon())%>">&nbsp;<%=h(rd.getName())%></a>&nbsp;<%
+                    <%=rd.renderDownloadLink(downloadURL)%>&nbsp;<%
                 }
                 %>
                 </div></td>

announcements/src/org/labkey/announcements/announcementWebPartSimple.jsp

@@ -195,7 +195,7 @@ for (AnnouncementModel a : bean.announcementModels)
         for (Attachment d : a.getAttachments())
         {
             ActionURL downloadURL = AnnouncementsController.getDownloadURL(a, d.getName());
-            %><a href="<%=h(downloadURL)%>"><img src="<%=getWebappURL(d.getFileIcon())%>">&nbsp;<%=h(d.getName())%></a>&nbsp;<%
+            %><%=d.renderDownloadLink(downloadURL)%>&nbsp;<%
         }
         %></td></tr><%
     }

announcements/src/org/labkey/announcements/announcementWebPartWithExpandos.jsp

@@ -217,7 +217,7 @@ for (AnnouncementModel a : bean.announcementModels)
         for (Attachment d : a.getAttachments())
         {
             ActionURL downloadURL = AnnouncementsController.getDownloadURL(a, d.getName());
-            %><a href="<%=h(downloadURL)%>"><img src="<%=getWebappURL(d.getFileIcon())%>">&nbsp;<%=h(d.getName())%></a>&nbsp;<%
+            %><%=d.renderDownloadLink(downloadURL)%>&nbsp;<%
         }
         %></td></tr><%
     }

announcements/src/org/labkey/announcements/update.jsp

@@ -140,7 +140,6 @@ if (settings.hasExpires())
             <tbody>
                 <%
                     int x = -1;
-                    String id;
                     for (Attachment att : ann.getAttachments())
                 {
                     x++;

api/src/org/labkey/api/assay/AbstractAssayProvider.java

@@ -124,6 +124,7 @@
 import javax.script.ScriptEngine;
 import java.io.File;
 import java.io.IOException;
+import java.io.InputStream;
 import java.net.URI;
 import java.net.URL;
 import java.sql.ResultSet;
@@ -1274,9 +1275,9 @@ public Pair<ValidationException, Pair<String, String>> setValidationAndAnalysisS
                     if (!(engine instanceof ExternalScriptEngine && ((ExternalScriptEngine) engine).isBinary(scriptFile)))
                     {
                         String scriptText;
-                        try
+                        try (InputStream is = scriptFile.openInputStream())
                         {
-                            scriptText = IOUtils.toString(scriptFile.openInputStream(), StringUtilsLabKey.DEFAULT_CHARSET);
+                            scriptText = IOUtils.toString(is, StringUtilsLabKey.DEFAULT_CHARSET);
                         }
                         catch (IOException e)
                         {

api/src/org/labkey/api/attachments/Attachment.java

@@ -21,9 +21,13 @@
 import org.labkey.api.security.User;
 import org.labkey.api.security.UserManager;
 import org.labkey.api.services.ServiceRegistry;
+import org.labkey.api.util.DOM;
+import org.labkey.api.util.HtmlString;
 import org.labkey.api.util.MemTracker;
 import org.labkey.api.util.MimeMap;
+import org.labkey.api.util.PageFlowUtil;
 import org.labkey.api.util.Path;
+import org.labkey.api.view.ActionURL;
 import org.labkey.api.view.ViewServlet;
 import org.labkey.api.webdav.WebdavResolver;
 
@@ -350,4 +354,29 @@ public void setDocumentSize(int documentSize)
     {
         _documentSize = documentSize;
     }
+
+    /**
+     * Returns an HtmlString rendering a download link: an anchor containing a file type icon and the filename.
+     * The icon is marked aria-hidden since it is decorative; the link text serves as the accessible name.
+     */
+    public HtmlString renderDownloadLink(ActionURL downloadURL)
+    {
+        return renderDownloadLink(downloadURL, getName());
+    }
+
+    /**
+     * Returns an HtmlString rendering a download link: an anchor containing a file type icon and custom link text.
+     * Use this overload when the visible link label differs from the filename (e.g. "Study Protocol Document").
+     * The icon is marked aria-hidden since it is decorative; linkText serves as the accessible name.
+     */
+    public HtmlString renderDownloadLink(ActionURL downloadURL, String linkText)
+    {
+        return DOM.createHtmlFragment(
+            DOM.A(DOM.at(DOM.Attribute.href, downloadURL.toString()),
+                DOM.IMG(DOM.at(DOM.Attribute.alt, "").at(DOM.Attribute.src, PageFlowUtil.staticResourceUrl(getFileIcon()))),
+                HtmlString.NBSP,
+                linkText
+            )
+        );
+    }
 }

api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java

@@ -29,6 +29,7 @@
 import org.labkey.api.data.DbSchema;
 import org.labkey.api.data.DbSchemaType;
 import org.labkey.api.data.DbScope;
+import org.labkey.api.data.MultiChoice;
 import org.labkey.api.data.MutableColumnInfo;
 import org.labkey.api.data.Table;
 import org.labkey.api.data.TableInfo;
@@ -377,6 +378,12 @@ else if (value instanceof Date date)
                 String formatted = DateUtil.toISO(date);
                 stringMap.put(entry.getKey(), formatted);
             }
+            else if (value instanceof java.sql.Array arr)
+            {
+                // GitHub Issue 1073: Updating a List MVTC field shows array in audit for values with quotes
+                var arrayVal = MultiChoice.Converter.getInstance().convert(MultiChoice.Array.class, arr);
+                stringMap.put(entry.getKey(), PageFlowUtil.joinValuesToStringForExport(arrayVal));
+            }
             else
                 stringMap.put(entry.getKey(), value == null ? null : value.toString());
         }

api/src/org/labkey/api/data/DbScope.java

@@ -19,7 +19,7 @@
 import jakarta.servlet.ServletContext;
 import jakarta.servlet.ServletException;
 import org.apache.commons.collections4.IteratorUtils;
-import org.apache.commons.lang3.StringUtils;
+import org.apache.commons.lang3.Strings;
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.Logger;
 import org.jetbrains.annotations.NotNull;
@@ -110,7 +110,7 @@
 /**
  * Class that wraps a data source and is shared amongst that data source's DbSchemas. Allows "nested" transactions,
  * implemented via a reference-counting style approach. Each (potentially nested) set of code should call
- * {@code ensureTransaction()}. This will either start a new transaction, or join an existing one. Once the outermost
+ * {@code ensureTransaction()}. This will either start a new transaction or join an existing one. Once the outermost
  * caller calls commit(), the WHOLE transaction will be committed at once. The most common usage scenario looks
  * something like:
  * <pre>{@code
@@ -121,8 +121,27 @@
  *     transaction.commit();
  * }
  *
- * The DbScope.Transaction class implements AutoCloseable, so it will be cleaned up automatically by JDK 7's try {}
- * resource handling.
+ * }
+ * </pre>
+ * The DbScope.Transaction class implements AutoCloseable, so it will be cleaned up automatically via try-with-resources.
+ * <p>
+ * All return pathways inside the transaction must commit, or the transaction will be considered abandoned to be rolled
+ * back. There should be no further database work prior to exiting the try block after the commit. Example:
+ * <pre>{@code
+ * DbScope scope = dbSchemaInstance.getScope();
+ * try (DbScope.Transaction transaction = scope.ensureTransaction())
+ * {
+ *     // Do some work
+ *     if (condition) {
+ *         transaction.commit();
+ *         return true;
+ *     }
+ *     // Do some other work
+ *     boolean result = determineResult();
+ *     transaction.commit();
+ *     return result;
+ * }
+ *
  * }
  * </pre>
  */
@@ -869,7 +888,7 @@ public Transaction beginTransaction(TransactionKind transactionKind, Lock... loc
                         int stackDepth;
                         synchronized (_transaction)
                         {
-                            List<TransactionImpl> transactions = _transaction.computeIfAbsent(getEffectiveThread(), k -> new ArrayList<>());
+                            List<TransactionImpl> transactions = _transaction.computeIfAbsent(getEffectiveThread(), _ -> new ArrayList<>());
                             transactions.add(result);
                             stackDepth = transactions.size();
                         }
@@ -1093,7 +1112,7 @@ public boolean isTransactionActive()
         synchronized (_transaction)
         {
             List<TransactionImpl> transactions = _transaction.get(thread);
-            return transactions == null ? null : transactions.get(transactions.size() - 1);
+            return transactions == null ? null : transactions.getLast();
         }
     }
 
@@ -1242,7 +1261,7 @@ private ConnectionHolder getConnectionHolder()
         // Synchronize just long enough to get a ConnectionHolder into the map
         synchronized (_threadConnections)
         {
-            return _threadConnections.computeIfAbsent(thread, t -> new ConnectionHolder());
+            return _threadConnections.computeIfAbsent(thread, _ -> new ConnectionHolder());
         }
     }
 
@@ -1301,7 +1320,7 @@ private static void ensureDelegation(Connection conn)
                     try
                     {
                         // Test the method to make sure we can access it
-                        Connection test = (Connection) methodGetInnermostDelegate.invoke(conn);
+                        var _ = (Connection) methodGetInnermostDelegate.invoke(conn);
                         isDelegating = true;
                         return;
                     }
@@ -2000,7 +2019,7 @@ private static void createDataBase(SqlDialect dialect, LabKeyDataSource ds) thro
 
         LOG.info("Attempting to create database \"{}\"", dbName);
 
-        String defaultUrl = StringUtils.replace(url, dbName, dialect.getDefaultDatabaseName());
+        String defaultUrl = Strings.CS.replace(url, dbName, dialect.getDefaultDatabaseName());
         String createSql = "(undefined)";
 
         try (Connection conn = getRawConnection(defaultUrl, ds))
@@ -2415,8 +2434,8 @@ public <T extends Runnable> T add(TransactionImpl transaction, T task)
 
     public interface Transaction extends AutoCloseable
     {
-        /*
-         * @return  the task that was inserted or the existing object (equal to the runnable passed in) that will be run instead
+        /**
+         * @return the task that was inserted or the existing object (equal to the runnable passed in) that will be run instead
          */
         @NotNull
         <T extends Runnable> T addCommitTask(@NotNull T runnable, @NotNull CommitTaskOption firstOption, CommitTaskOption... additionalOptions);
@@ -2796,7 +2815,7 @@ private boolean isOutermostTransaction()
         /** Remove current transaction nesting and unlock any locks */
         private void decrement()
         {
-            List<Lock> locks = _locks.remove(_locks.size() - 1);
+            List<Lock> locks = _locks.removeLast();
             for (Lock lock : locks)
             {
                 // Release all the locks
@@ -2853,7 +2872,7 @@ public void increment(boolean releaseOnFinalCommit, List<Lock> extraLocks)
                 if (!_locks.isEmpty() && releaseOnFinalCommit)
                 {
                     // Add the new locks to the outermost set of locks
-                    _locks.get(0).addAll(extraLocks);
+                    _locks.getFirst().addAll(extraLocks);
                     // Add an empty list to this layer of the transaction
                     _locks.add(new ArrayList<>());
                 }
@@ -3328,7 +3347,7 @@ public void testNestedMissingCommit()
                     //noinspection EmptyTryBlock
                     try (Transaction ignored = getLabKeyScope().ensureTransaction())
                     {
-                        // Intentionally don't call t2.commit();
+                        // Intentionally, don't call t2.commit();
                     }
                     try
                     {
@@ -3359,14 +3378,8 @@ public void testMultipleTransactionKinds()
                 try (Transaction t2 = getLabKeyScope().ensureTransaction())
                 {
                     assertSame(connection, t2.getConnection());
-                    try (Transaction t3 = getLabKeyScope().ensureTransaction(new TransactionKind()
-                    {
-                        @NotNull
-                        @Override
-                        public String getKind()
-                        {
-                            return "PIPELINESTATUS";  // We can't really see PipelineStatus here, but just need something non-normal to test
-                        }
+                    try (Transaction t3 = getLabKeyScope().ensureTransaction(() -> {
+                        return "PIPELINESTATUS";  // We can't really see PipelineStatus here, but just need something non-normal to test
                     }))
                     {
                         assertTrue(getLabKeyScope().isTransactionActive());
@@ -3392,7 +3405,7 @@ public void testServerRowLock()
             Lock lockUser = new ServerPrimaryKeyLock(true, CoreSchema.getInstance().getTableInfoUsersData(), user.getUserId());
             Lock lockHome = new ServerPrimaryKeyLock(true, CoreSchema.getInstance().getTableInfoContainers(), ContainerManager.getHomeContainer().getId());
 
-            attemptToDeadlock(lockUser, lockHome, (x) -> {}, PessimisticLockingFailureException.class);
+            attemptToDeadlock(lockUser, lockHome, (_) -> {}, PessimisticLockingFailureException.class);
         }
 
         private void attemptToDeadlock(Lock lock1, Lock lock2, @NotNull Consumer<Transaction> transactionModifier, Class<? extends Throwable> expectedException)

api/src/org/labkey/api/mcp/McpService.java

@@ -29,11 +29,11 @@
 /// ### MCP Development Guide
 /// `McpService` lets you expose functionality over the MCP protocol (only simple http for now). This allows external
 /// chat sessions to pull information from LabKey Server. Exposed functionality is also made available to chat sessions
-/// hosted by LabKey (see `AbstractAgentAction``).
+/// hosted by LabKey (see `AbstractAgentAction`).
 ///
 /// ### Adding a new MCP class
 /// 1. Create a new class that implements `McpImpl` (see below) in the appropriate module
-/// 2. Register that class in your module `init()` method: `McpService.get().register(new MyMcp())`
+/// 2. Register that class in your module's `startup()` method: `McpService.get().register(new MyMcp())`
 /// 3. Add tools and resources
 ///
 /// ### Adding a new MCP tool
@@ -44,13 +44,13 @@
 ///     permission annotation is required, otherwise your tool will not be registered.**
 /// 4.  Add `ToolContext` as the first parameter to the method
 /// 5.  Add additional required or optional parameters to the method signature, as needed. Note that "required" is the
-///     default. Again here, the parameter descriptions are very important. Provide examples.
+///     default. Again here, the parameter descriptions are very important. Provide examples of parameter values.
 /// 6.  Use the helper method `getContext(ToolContext)` to retrieve the current `Container` and `User`
 /// 7.  Use the helper method `getUser(ToolContext)` in the rare cases where you need just a `User`
 /// 8.  Perform additional permissions checking (beyond what the annotations offer), where appropriate
 /// 9.  Filter all results to the current container, of course
 /// 10. For any error conditions, throw exceptions with detailed information. These will get translated into appropriate
-///     failure responses and the LLM client will attempt to correct the problem.
+///     failure responses and the LLM client will attempt to correct any problems (hopefully).
 /// 11. For success cases, return a String with a message or JSON content, for example, `JSONObject.toString()`. Spring
 ///     has some limited ability to convert other objects into JSON strings, but we haven't experimented with that. See
 ///     `DefaultToolCallResultConverter` and the ability to provide a custom result converter via the `@Tool` annotation.
@@ -126,6 +126,7 @@ static void setInstance(McpService service)
 
     boolean isReady();
 
+    // Register MCPs in Module.startup()
     default void register(McpImpl mcp)
     {
         try

api/src/org/labkey/api/module/ModuleLoader.java

@@ -580,19 +580,6 @@ private void doInit(Execution execution) throws ServletException
             throw new IllegalStateException("Core module was not first or could not find the Core module. Ensure that Tomcat user can create directories under the <LABKEY_HOME>/modules directory.");
         setProjectRoot(coreModule);
 
-        for (Module module : modules)
-        {
-            module.registerFilters(_servletContext);
-        }
-        for (Module module : modules)
-        {
-            module.registerServlets(_servletContext);
-        }
-        for (Module module : modules)
-        {
-            module.registerFinalServlets(_servletContext);
-        }
-
         // Do this after we've checked to see if we can find the core module. See issue 22797.
         verifyProductionModeMatchesBuild();
 
@@ -790,12 +777,34 @@ public void addStaticWarnings(@NotNull Warnings warnings, boolean showAllWarning
         if (!modulesRequiringUpgrade.isEmpty() || !additionalSchemasRequiringUpgrade.isEmpty())
             setUpgradeState(UpgradeState.UpgradeRequired);
 
-        // Don't accept any requests if we're bootstrapping empty schemas or migrating from SQL Server
+        // Don't accept any requests if we're bootstrapping empty schemas or doing a database migration
         if (!shouldInsertData())
             execution = Execution.Synchronous;
 
         startNonCoreUpgradeAndStartup(execution, lockFile);
 
+        // Register filters and servlets at the last minute, just before Tomcat starts. At this point, the list of
+        // modules is final. We've had one case where the CSP filter was getting initialized before the core module
+        // was initialized, GitHub Issue 1008. We have no idea how that happened, but registering late won't hurt.
+
+        _log.info("Registering filters");
+
+        for (Module module : _modules)
+        {
+            module.registerFilters(_servletContext);
+        }
+
+        _log.info("Registering servlets");
+
+        for (Module module : _modules)
+        {
+            module.registerServlets(_servletContext);
+        }
+        for (Module module : _modules)
+        {
+            module.registerFinalServlets(_servletContext);
+        }
+
         _log.info("LabKey Server startup is complete; {}", execution.getLogMessage());
     }