Merge branch 'develop' into fb_revertSampleNameCase

# Conflicts:
#	api/src/org/labkey/api/query/AbstractQueryUpdateService.java
#	experiment/src/client/test/integration/DataClassCrud.ispec.ts

Author: XingY

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

@@ -4,6 +4,7 @@
 import org.jetbrains.annotations.Nullable;
 import org.labkey.api.data.ColumnInfo;
 import org.labkey.api.data.Container;
+import org.labkey.api.data.MultiValuedForeignKey;
 import org.labkey.api.data.TableInfo;
 import org.labkey.api.dataiterator.DataIterator;
 import org.labkey.api.dataiterator.ExistingRecordDataIterator;
@@ -64,16 +65,27 @@ static Pair<Map<String, Object>, Map<String, Object>> getOldAndNewRecordForMerge
         // and we won't convert sample type and data class names into lower case.
         for (Map.Entry<String, Object> entry : existingRow.entrySet())
         {
+            boolean isMultiValued = false;
             String key = entry.getKey();
             // getDatasetRows() (at least) should return key==column.getName(), expect getColumn(name) to work
             ColumnInfo col = null==table ? null : table.getColumn(key);
-            String nameFromAlias = null != col
-                    ? col.getName()
-                    : columns.stream()
-                        .filter(column -> column.getAlias().getId().equalsIgnoreCase(key))
-                        .map((ColumnInfo::getName))
-                        .findFirst()
-                        .orElse(key);
+            if (col != null && col.getFk() instanceof MultiValuedForeignKey)
+                isMultiValued = true;
+
+            String nameFromAlias = key;
+            if (null != col)
+                nameFromAlias = col.getName();
+            else
+            {
+                ColumnInfo aliasColumn = columns.stream().filter(c -> c.getAlias().getId().equalsIgnoreCase(key)).findFirst().orElse(null);
+
+                if (aliasColumn != null)
+                {
+                    if (aliasColumn.getFk() != null && aliasColumn.getFk() instanceof MultiValuedForeignKey)
+                        isMultiValued = true;
+                    nameFromAlias = aliasColumn.getName();
+                }
+            }
             String lcName = nameFromAlias.toLowerCase();
             // Preserve casing of inputs so we can show the names properly
             boolean isExpInput = false;
@@ -131,8 +143,22 @@ else if (newValue instanceof Number && oldValue != null)
                 }
                 else if (!Objects.equals(oldValue, newValue) || isExtraAuditField)
                 {
-                    originalRow.put(nameFromAlias, oldValue);
-                    modifiedRow.put(nameFromAlias, newValue);
+                    // If multivalued columns change, the value in this table will remain the key to the junction table
+                    // but at this point newValue will look like the newly chosen values not that key. So we skip
+                    // this in the diff unless the value changes from non-null to null or vice versa.
+                    if (isMultiValued)
+                    {
+                        if ((oldValue == null && newValue != null) || (newValue == null && oldValue != null))
+                        {
+                            originalRow.put(nameFromAlias, oldValue);
+                            modifiedRow.put(nameFromAlias, newValue);
+                        }
+                    }
+                    else
+                    {
+                        originalRow.put(nameFromAlias, oldValue);
+                        modifiedRow.put(nameFromAlias, newValue);
+                    }
                 }
             }
             else if (isExtraAuditField)

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

@@ -27,7 +27,7 @@
 import java.util.Map;
 import java.util.Set;
 
-class FieldKeyRowMap implements Map<FieldKey, Object>
+public class FieldKeyRowMap implements Map<FieldKey, Object>
 {
     private final Results _results;
 
@@ -131,8 +131,6 @@ public static Map<String, Object> toNameMap(Map<FieldKey, Object> rowMap)
     {
         Map<String, Object> map = new CaseInsensitiveHashMap<>();
         rowMap.forEach((key, value) -> {
-            if (key.getParent() != null)
-                throw new IllegalArgumentException("Multi-part field key '" + key + "' cannot be used as key in string map since it may not be unique.");
             if (map.containsKey(key.getName()))
                 throw new IllegalArgumentException("Duplicate key '" + key + "' found in fieldKey map.");
             map.put(key.getName(), value);

api/src/org/labkey/api/exp/PropertyColumn.java

@@ -35,6 +35,7 @@
 import org.labkey.api.exp.property.PropertyService;
 import org.labkey.api.query.FieldKey;
 import org.labkey.api.query.PdLookupForeignKey;
+import org.labkey.api.query.QueryService;
 import org.labkey.api.query.SchemaKey;
 import org.labkey.api.security.User;
 import org.labkey.api.study.assay.FileLinkDisplayColumn;
@@ -182,7 +183,12 @@ public static void copyAttributes(
         }
 
         if (user != null && ((pd.getLookupSchema() != null && pd.getLookupQuery() != null) || pd.getConceptURI() != null))
-            to.setFk(PdLookupForeignKey.create(to.getParentTable().getUserSchema(), user, container, pd, cf));
+        {
+            // Issue 52504: Use proper container filter for lookups
+            var _cf = pd.isLookup() ? QueryService.get().getContainerFilterForLookups(container, user) : cf;
+
+            to.setFk(PdLookupForeignKey.create(to.getParentTable().getUserSchema(), user, container, pd, _cf));
+        }
 
         to.setDefaultValueType(pd.getDefaultValueTypeEnum());
         to.setConditionalFormats(PropertyService.get().getConditionalFormats(pd));

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

@@ -202,7 +202,7 @@ public Map<Integer, Map<String, Object>> getExistingRows(User user, Container co
         for (Map.Entry<Integer, Map<String, Object>> key : keys.entrySet())
         {
             Map<String, Object> row = getRow(user, container, key.getValue(), verifyNoCrossFolderData);
-            if (row != null)
+            if (row != null && !row.isEmpty())
             {
                 result.put(key.getKey(), row);
                 if (verifyNoCrossFolderData)

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

@@ -21,6 +21,7 @@
 import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.LogManager;
 import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
 import org.labkey.api.collections.CaseInsensitiveHashMap;
 import org.labkey.api.data.Aggregate;
 import org.labkey.api.data.AnalyticsProviderItem;
@@ -52,8 +53,6 @@
 import java.util.List;
 import java.util.Map;
 
-import static org.apache.commons.lang3.StringUtils.isNotBlank;
-
 public class QuerySettings
 {
     public static final String URL_PARAMETER_PREFIX = "param.";
@@ -98,7 +97,6 @@ public class QuerySettings
 
     private final Map<String, Object> _queryParameters = new CaseInsensitiveHashMap<>();
 
-
     protected QuerySettings(String dataRegionName)
     {
         _dataRegionName = dataRegionName;
@@ -118,7 +116,6 @@ public QuerySettings(ViewContext context, String dataRegionName)
         assert MemTracker.getInstance().put(this);
     }
 
-
     /**
      * Init the QuerySettings using all the request parameters, from context.getPropertyValues().
      * @see UserSchema#getSettings(org.labkey.api.view.ViewContext, String, String)
@@ -132,7 +129,6 @@ public QuerySettings(ViewContext context, String dataRegionName, String queryNam
         assert MemTracker.getInstance().put(this);
     }
 
-
     /**
      * @param params    all parameters from URL or POST, including dataregion.filter parameters
      * @param dataRegionName    prefix for filter params etc
@@ -146,7 +142,6 @@ public QuerySettings(PropertyValues params, String dataRegionName)
         assert MemTracker.getInstance().put(this);
     }
 
-
     protected PropertyValues getPropertyValues(ViewContext context)
     {
         PropertyValues pvs = context.getBindPropertyValues();
@@ -158,7 +153,6 @@ protected PropertyValues getPropertyValues(ViewContext context)
         return pvs;
     }
 
-
     /**
      * @param url parameters for filter/sort
      */
@@ -167,7 +161,6 @@ public void setSortFilterURL(ActionURL url)
         setSortFilter(url.getPropertyValues());
     }
 
-
     public void setSortFilter(PropertyValues pvs)
     {
         _filterSort = pvs;
@@ -185,7 +178,7 @@ public void setSortFilter(PropertyValues pvs)
         }
     }
 
-    protected String _getParameter(String param)
+    protected @Nullable String _getParameter(String param)
     {
         PropertyValue pv = _filterSort.getPropertyValue(param);
         if (pv == null)
@@ -198,18 +191,17 @@ protected String _getParameter(String param)
             Object[] a = (Object[])v;
             v = a.length == 0 ? null : a[0];
         }
-        return v == null ? null : String.valueOf(v);
+        return v == null ? null : StringUtils.trimToNull(String.valueOf(v));
     }
 
     public void init(ViewContext context)
     {
         init(getPropertyValues(context));    
     }
 
-
     /**
      * Initialize QuerySettings from the PropertyValues, binds all fields that are supported on the URL
-     *. such as viewName.  Use setSortFilter() to provide sort filter parameters w/o affecting the other
+     * such as viewName.  Use setSortFilter() to provide sort filter parameters w/o affecting the other
      * properties.
      */
     public void init(PropertyValues pvs)
@@ -220,33 +212,35 @@ public void init(PropertyValues pvs)
         setAnalyticsProviders(pvs);
 
         // Let URL parameter control which query we show, even if we don't show the Query drop-down menu to let the user choose
-        String param = param(QueryParam.queryName);
-        String queryName = StringUtils.trimToNull(_getParameter(param));
+        String queryName = _getParameter(param(QueryParam.queryName));
         if (queryName != null)
         {
             setQueryName(queryName);
         }
 
         if (getAllowChooseView())
         {
-            String viewName = StringUtils.trimToNull(_getParameter(param(QueryParam.viewName)));
+            String viewName = _getParameter(param(QueryParam.viewName));
             if (viewName != null)
             {
                 setViewName(viewName);
             }
+
             String ignoreFilter = _getParameter(param(QueryParam.ignoreFilter));
-            try
+            if (ignoreFilter != null)
             {
-                if (isNotBlank(ignoreFilter))
-                    _ignoreUserFilter = (Boolean) ConvertUtils.convert(ignoreFilter, Boolean.class);
-            }
-            catch (ConversionException e)
-            {
-                throwParameterParseException(QueryParam.ignoreFilter);
+                try
+                {
+                    _ignoreViewFilter = (Boolean) ConvertUtils.convert(ignoreFilter, Boolean.class);
+                }
+                catch (ConversionException e)
+                {
+                    throwParameterParseException(QueryParam.ignoreFilter);
+                }
             }
 
             String reportId = _getParameter(param(QueryParam.reportId));
-            if (isNotBlank(reportId))
+            if (reportId != null)
             {
                 var identifier = ReportService.get().getReportIdentifier(reportId, null, null);
                 if (null == identifier)
@@ -259,7 +253,7 @@ public void init(PropertyValues pvs)
         if (_showRows == ShowRows.PAGINATED)
         {
             String offsetParam = _getParameter(param(QueryParam.offset));
-            if (isNotBlank(offsetParam))
+            if (offsetParam != null)
             {
                 try
                 {
@@ -274,7 +268,7 @@ public void init(PropertyValues pvs)
             }
 
             String maxRowsParam = _getParameter(param(QueryParam.maxRows));
-            if (isNotBlank(maxRowsParam))
+            if (maxRowsParam != null)
             {
                 try
                 {
@@ -295,7 +289,7 @@ public void init(PropertyValues pvs)
         }
 
         String containerFilterNameParam = _getParameter(param(QueryParam.containerFilterName));
-        if (isNotBlank(containerFilterNameParam))
+        if (containerFilterNameParam != null)
         {
             // fail fast
             if (null == ContainerFilter.getType(containerFilterNameParam))
@@ -319,7 +313,7 @@ public void init(PropertyValues pvs)
             }
         }
 
-        String columns = StringUtils.trimToNull(_getParameter(param(QueryParam.columns)));
+        String columns = _getParameter(param(QueryParam.columns));
         if (null != columns)
         {
             String[] colArray = columns.split(",");
@@ -333,7 +327,7 @@ public void init(PropertyValues pvs)
             }
         }
 
-        String extraColumns = StringUtils.trimToNull(_getParameter(param(QueryParam.extraColumns)));
+        String extraColumns = _getParameter(param(QueryParam.extraColumns));
         if (null != extraColumns)
         {
             String[] colArray = extraColumns.split(",");
@@ -347,13 +341,13 @@ public void init(PropertyValues pvs)
             }
         }
 
-        String selectionKey = StringUtils.trimToNull(_getParameter(param(QueryParam.selectionKey)));
+        String selectionKey = _getParameter(param(QueryParam.selectionKey));
         if (null != selectionKey)
             setSelectionKey(selectionKey);
 
         _parseQueryParameters(_filterSort);
 
-        String allowHeaderLock = StringUtils.trimToNull(_getParameter(param(QueryParam.allowHeaderLock)));
+        String allowHeaderLock = _getParameter(param(QueryParam.allowHeaderLock));
         if (null != allowHeaderLock)
         {
             try

experiment/src/client/test/integration/DataClassCrud.ispec.ts

@@ -233,8 +233,8 @@ describe('Import with update / merge', () => {
    it ("Issue 52922: Blank sample id in the file are getting ignored in update from file", async () => {
        const BLANK_KEY_UPDATE_ERROR_NO_EXPRESSION = 'Missing value for required property: Name';
        const BLANK_KEY_UPDATE_ERROR_WITH_EXPRESSION = 'Name value not provided on row ';
-       const BOGUS_KEY_UPDATE_ERROR = 'Data not found for ';
-       const CROSS_FOLDER_UPDATE_NOT_SUPPORTED_ERROR = "Data doesn't belong to folder ";
+       const BOGUS_KEY_UPDATE_ERROR = 'Data not found: ';
+       const DUPLICATE_KEY_ERROR = 'duplicate key value';
 
        const dataType = "NoExpressionNameRequired52922";
        const createPayload = {
@@ -306,9 +306,9 @@ describe('Import with update / merge', () => {
 
        // cross folder update not supported when folder type is "Collaboration"
        let crossFolderErrorResp = await ExperimentCRUDUtils.importData(server, "Name\tDescription\nData1\tNotblank\n\tisBlank", dataTypeWithExpression, "MERGE", subfolder1Options, editorUserOptions);
-       expect(crossFolderErrorResp.text.indexOf(CROSS_FOLDER_UPDATE_NOT_SUPPORTED_ERROR) > -1).toBeTruthy();
+       expect(crossFolderErrorResp.text.indexOf(DUPLICATE_KEY_ERROR) > -1).toBeTruthy();
        crossFolderErrorResp = await ExperimentCRUDUtils.importData(server, "Name\tDescription\nData1\tNotblank", dataTypeWithExpression, "UPDATE", subfolder1Options, editorUserOptions);
-       expect(crossFolderErrorResp.text.indexOf(CROSS_FOLDER_UPDATE_NOT_SUPPORTED_ERROR) > -1).toBeTruthy();
+       expect(crossFolderErrorResp.text.indexOf(BOGUS_KEY_UPDATE_ERROR) > -1).toBeTruthy();
 
        // bogus name
        bogusKeyProvidedError = await ExperimentCRUDUtils.importData(server, "Name\tDescription\nbogus\tisBogus", dataTypeWithExpression, "UPDATE", topFolderOptions, editorUserOptions);
@@ -433,4 +433,4 @@ describe('Duplicate IDs', () => {
         expect(caseInsensitive(dataResults[1], 'description')).toBe('created');
 
     });
-});
+});