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

Author: labkey-susanh

.github/pull_request_template.md

@@ -6,3 +6,10 @@
 
 #### Changes
 - <!-- list of descriptions of changes that are worth noting (replace this comment) -->
+
+<!-- list of standard tasks (remove this comment to enable)
+#### Tasks 📍
+- [ ] Manual Testing
+- [ ] Needs Automation
+- [ ] Verify Fix
+-->

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

@@ -29,7 +29,6 @@
 import org.labkey.api.pipeline.PipeRoot;
 import org.labkey.api.pipeline.PipelineService;
 import org.labkey.api.query.AbstractQueryUpdateService;
-import org.labkey.api.util.DateUtil;
 import org.labkey.api.util.FileUtil;
 import org.labkey.api.util.NetworkDrive;
 import org.labkey.api.view.ViewContext;
@@ -44,7 +43,6 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.ArrayDeque;
-import java.util.Date;
 import java.util.Deque;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -138,33 +136,29 @@ public static FileLike ensureUploadDirectoryPath(FileLike dir) throws Experiment
         }
     }
 
+    public static String generateFileName(ExpProtocol protocol, boolean shouldEncode)
+    {
+        String protocolName = protocol.getName();
+        if (shouldEncode)
+            return FileUtil.makeFileNameWithTimestamp(protocolName);
+
+        return protocolName + "_" + FileUtil.getTimestamp(); // Issue 52075
+    }
+
     /**
      * Create file name based upon the assay protocol's name and the current time.
      * e.g., <code>assayname-2020-04-14-1602345</code>
      */
     public static FileLike createFile(ExpProtocol protocol, FileLike dir, String extension)
     {
-        Date dateCreated = new Date();
-        String dateString = DateUtil.formatDateTime(dateCreated, "yyy-MM-dd-HHmmss-SSS");
+        String fileNamePrefix = generateFileName(protocol, true);
         int id = 0;
 
-        String protocolName = protocol.getName();
-        char[] characters = protocolName.toCharArray();
-
-        for (int i = 0; i < characters.length; i++)
-        {
-            char character = characters[i];
-            boolean isAtoZchar = character >= 'A' && character <= 'z';
-            if (!Character.isDigit(character) && !isAtoZchar)
-                characters[i] = '_';
-        }
-        protocolName = new String(characters);
-
         FileLike file;
         do
         {
             String extra = id++ == 0 ? "" : String.valueOf(id);
-            String fileName = protocolName + "-" + dateString + extra + "." + extension;
+            String fileName = fileNamePrefix + extra + "." + extension;
             fileName = fileName.replace('\\', '_').replace('/', '_').replace(':', '_');
             file = dir.resolveChild(fileName);
         }

api/src/org/labkey/api/cache/CacheManager.java

@@ -60,7 +60,7 @@ public class CacheManager
     public static final int DEFAULT_CACHE_SIZE = 5000;
 
     // Set useCache = false to completely disable all caching... and slow your server to a near halt. Possibly useful for
-    // reproducing CacheLoader re-entrancy problems, but not much else.
+    // reproducing CacheLoader reentrancy problems, but not much else.
     private static final boolean useCache = true;
     private static final CacheProvider PROVIDER = useCache ? EhCacheProvider.getInstance() : new NoopCacheProvider();
 
@@ -176,7 +176,7 @@ public static void shutdown()
 
     private static final Set<Class<?>> CLASSES = new HashSet<>();
 
-    // Validate a cached value. For now, just log warnings for mutable collections.
+    // Validate a cached value. Log errors for mutable collections/arrays and for values holding Container or User objects.
     public static <V> void validate(String debugName, @Nullable V value)
     {
         if (value instanceof Wrapper<?>)
@@ -186,14 +186,11 @@ public static <V> void validate(String debugName, @Nullable V value)
 
         if (null != description)
         {
-            LOG.warn("{} attempted to cache {}, which could be mutated by callers!", debugName, description);
+            LOG.error("{} attempted to cache {}, which could be mutated by callers!", debugName, description);
         }
 
-        // Log questionable members, but don't do the work if we're not going to log it
-        if (LOG.isDebugEnabled())
-        {
-            analyzeValue(value, debugName, null, 1);
-        }
+        // Flag values that hold a Container or User object
+        analyzeValue(value, debugName, null, 1);
     }
 
     private static final int MAX_DEPTH = 4;
@@ -236,9 +233,7 @@ else if (CLASSES.add(clazz))
 
                     if (Container.class.isAssignableFrom(type) || User.class.isAssignableFrom(type) || Project.class.isAssignableFrom(type))
                     {
-//                        String message = cacheName + ": " + clazz.getName() + " field " + newFieldPath + " (" + field.getType().getName() + ")";
-//                        throw new IllegalStateException(message);
-                        LOG.debug("{}: {} field {} ({})", cacheName, clazz.getName(), newFieldPath, field.getType().getName());
+                        LOG.error("Cached value holds an unsafe object - {}: {} field {} ({})", cacheName, clazz.getName(), newFieldPath, field.getType().getName());
                     }
                     else
                     {

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

@@ -107,7 +107,7 @@ public void renderDetailsCellContents(RenderContext ctx, HtmlWriter out)
     {
         Object value = getValue(ctx);
         if (value != null)
-            out.write(value.toString());
+            out.write(value);
     }
 
     @Override

api/src/org/labkey/api/data/dialect/MutatingSqlDetector.java

@@ -129,6 +129,7 @@ State getNextState(char c, StringBuilder firstWord, String sql)
         // Needed for PostgreSQL
         WORD_MUTATING_MAP.putAll(Map.of(
             "ANALYZE", true,   // Typically executed after UPDATE, CREATE INDEX, et al
+            "LOCK", true,      // Not technically mutating. However, it is currently only used in mutating TX.
             "VACUUM", true,    // VACUUM is mutating
             "{call", true      // Execute a stored procedure, which is likely to be mutating
         ));

api/src/org/labkey/api/exp/property/DomainUtil.java

@@ -982,6 +982,14 @@ public static ValidationException updateDomainDescriptor(GWTDomain<? extends GWT
         return validationException;
     }
 
+    // Issue 51321: check reserved domain name: First, All
+    public static @Nullable String validateReservedName(@NotNull String domainName, @NotNull String kindName)
+    {
+        if ("First".equalsIgnoreCase(domainName) || "All".equalsIgnoreCase(domainName))
+            return kindName + " name '" + domainName + "' is a reserved name.";
+        return null;
+    }
+
     public static @Nullable String validateDomainName(@NotNull String domainName, String kindName, boolean supportsNamingPattern)
     {
         String prefix = "Invalid " + kindName + " name '" + domainName + "'. ";

api/src/org/labkey/api/query/AbstractQueryImportAction.java

@@ -604,6 +604,7 @@ else if (!dataFileDir.exists())
                             QueryImportPipelineJob job = new QueryImportPipelineJob(getQueryImportProviderName(), info, root, importContextBuilder);
                             PipelineService.get().queueJob(job, getQueryImportJobNotificationProviderName());
 
+                            SimpleMetricsService.get().increment("query", "fileBackgroundImports", getMetricPrefix(_target));
                             JSONObject response = new JSONObject();
                             response.put("success", true);
                             response.put("jobId", PipelineService.get().getJobId(user, getContainer(), job.getJobGUID()));
@@ -635,7 +636,10 @@ else if (!dataFileDir.exists())
             int rowCount = importData(loader, file, originalName, ve, behaviorType, auditEvent, _auditUserComment);
 
             if (ve.hasErrors())
+            {
+                addImportValidationErrorMetric(_insertOption, _target, (form instanceof QueryForm qf) ? qf.getQueryName() : null);
                 throw ve;
+            }
 
             JSONObject response = createSuccessResponse(rowCount);
             if (auditEvent != null)
@@ -879,9 +883,7 @@ public static int importData(DataLoader dl, TableInfo target, QueryUpdateService
                 if (auditEvent != null)
                     auditEvent.addComment(auditAction, count);
 
-                String metricPrefix = target.getUserSchema() == null ? target.getSchema().getName() : target.getUserSchema().getSchemaName();
-                metricPrefix = metricPrefix.replace("exp.", "");
-                incrementRowCountMetric(count, context.getInsertOption(), metricPrefix);
+                incrementRowCountMetric(count, context.getInsertOption(), getMetricPrefix(target));
                 transaction.commit();
 
                 return count;
@@ -903,6 +905,24 @@ else if (!context.isCrossTypeImport())
         return 0;
     }
 
+    public static void addImportValidationErrorMetric(QueryUpdateService.InsertOption insertOption, @Nullable TableInfo target, @Nullable String queryName)
+    {
+        String featureArea =  insertOption.toString().toLowerCase() + "FailWithValidationError";
+        String targetType = null;
+        if (target != null)
+            targetType = getMetricPrefix(target);
+        else if (queryName != null)
+            targetType = queryName.equalsIgnoreCase("materials") ? "samples" : queryName;
+        if (targetType != null)
+            SimpleMetricsService.get().increment("query", featureArea, targetType);
+    }
+
+    public static String getMetricPrefix(TableInfo target)
+    {
+        String metricPrefix = target.getUserSchema() == null ? target.getSchema().getName() : target.getUserSchema().getSchemaName();
+        return metricPrefix.replace("exp.", "");
+    }
+
     public static void incrementRowCountMetric(int count, QueryUpdateService.InsertOption insertOption, String prefix)
     {
         String featureArea = "file" + StringUtils.capitalize(insertOption.toString().toLowerCase()) + "Counts";

api/src/org/labkey/api/query/QueryImportPipelineJob.java

@@ -305,7 +305,10 @@ public void run()
             int importedCount = AbstractQueryImportAction.importData(loader, target, updateService, diContext, auditEvent, getInfo().getUser(), getInfo().getContainer());
 
             if (ve.hasErrors())
+            {
+                AbstractQueryImportAction.addImportValidationErrorMetric(_importContextBuilder.getInsertOption(), target, null);
                 throw ve;
+            }
 
             if (auditEvent != null)
                 _transactionAuditId = auditEvent.getRowId();

api/src/org/labkey/api/security/ElevatedUser.java

@@ -2,6 +2,7 @@
 
 import org.labkey.api.audit.permissions.CanSeeAuditLogPermission;
 import org.labkey.api.data.Container;
+import org.labkey.api.security.impersonation.ImpersonationContext;
 import org.labkey.api.security.impersonation.WrappedImpersonationContext;
 import org.labkey.api.security.permissions.Permission;
 import org.labkey.api.security.roles.CanSeeAuditLogRole;
@@ -26,6 +27,11 @@ private ElevatedUser(User user, Set<Role> rolesToAdd)
         super(user, new WrappedImpersonationContext(user.getImpersonationContext(), rolesToAdd));
     }
 
+    private ElevatedUser(User user, ImpersonationContext ctx)
+    {
+        super(user, ctx);
+    }
+
     /**
      * Wrap the supplied user and unconditionally add the supplied role(s). Always returns an ElevatedUser.
      */
@@ -43,6 +49,14 @@ public static ElevatedUser getElevatedUser(User user, Collection<Class<? extends
         return new ElevatedUser(user, getRoles(roleClassesToAdd));
     }
 
+    /**
+     * Used to reconstitute an ElevatedUser from its component parts
+     */
+    public static ElevatedUser getElevatedUser(User user, ImpersonationContext ctx)
+    {
+        return new ElevatedUser(user, ctx);
+    }
+
     /**
      * Ensure the supplied user has the supplied permissions. If so, return that user. If not, wrap the user with
      * ElevatedUser and, for each pair of permission + role, add the role if the user doesn't have the corresponding

api/src/org/labkey/api/security/permissions/AdminOperationsPermission.java

@@ -16,7 +16,9 @@
 package org.labkey.api.security.permissions;
 
 /**
- * Describes the ability to manage operational site administration settings.
+ * Describes the ability to manage operational site administration settings, such as arbitrary paths on the server's
+ * underlying file system. Use {@see org.labkey.api.security.permissions.ApplicationAdminPermission} for gating
+ * access to server-wide settings that don't have the potential to access these underlying server resources.
  */
 public class AdminOperationsPermission extends AdminPermission
 {