Issue 52886: SourcesAuditEvent update event newRecordMap diff missing value for an updated field with a long name

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

@@ -204,7 +204,7 @@ public Map<Integer, Map<String, Object>> getExistingRows(User user, Container co
             Map<String, Object> keyValues = key.getValue();
             Map<String, Object> row = getRow(user, container, keyValues, verifyNoCrossFolderData);
             boolean hasValidExisting = false;
-            if (row != null)
+            if (row != null && !row.isEmpty())
             {
                 result.put(key.getKey(), row);
                 if (verifyNoCrossFolderData)

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

@@ -234,7 +234,7 @@ describe('Import with update / merge', () => {
        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: ';
-       const CROSS_FOLDER_UPDATE_NOT_SUPPORTED_ERROR = "Data doesn't belong to folder ";
+       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);
@@ -577,4 +577,4 @@ describe('Duplicate IDs', () => {
 
     });
 
-});
+});

experiment/src/org/labkey/experiment/ExpDataIterators.java

@@ -805,6 +805,29 @@ public boolean next() throws BatchValidationException
         }
     }
 
+    /**
+     * Issue 52504 (sort of): Chooses a container filter that is appropriate for import, merge or update actions in the face of product folders.
+     * Note that this is slightly different from our treatment of lookups:
+     *   - when in a project, we allow import or update to all subfolders,
+     *   - when in a folder, we only allow references to data up the folder tree
+     * @param qDef The QueryDefinition in use for the import action
+     * @param container The container that is the target of the import or update
+     * @param user The user doing the action
+     */
+    public static void setContainerFilterForImport(QueryDefinition qDef, Container container, User user)
+    {
+        if (container.isProductFoldersEnabled())
+        {
+            ContainerFilter cf;
+
+            if (container.isProject())
+                cf = new ContainerFilter.AllInProjectPlusShared(container, user);
+            else
+                cf = new ContainerFilter.CurrentPlusProjectAndShared(container, user);
+            qDef.setContainerFilter(cf);
+        }
+    }
+
     /* setup mini dataiterator pipeline to process lineage */
     public static void derive(User user, Container container, DataIterator di, boolean isSample, ExpObject dataType, boolean skipAliquot) throws BatchValidationException
     {
@@ -2487,8 +2510,7 @@ private int _importPartition(TypeData typeData)
                         Container splitContainer = ContainerManager.getForRowId(containerSplitFile.getKey());
                         AbstractExpSchema schema = _isSamples ? new SamplesSchema(_user, splitContainer) : new DataClassUserSchema(splitContainer, _user);
                         QueryDefinition qDef = schema.getQueryDefForTable(typeData.dataType.getName());
-                        // Issue 52504: For lookup validation, we need to use the proper lookup container filter on the table
-                        qDef.setContainerFilter(QueryService.get().getContainerFilterForLookups(splitContainer, _user));
+                        setContainerFilterForImport(qDef, splitContainer, _user);
                         TableInfo dataTable = qDef.getTable(schema, new ArrayList<>(), true);
 
                         if (dataTable == null)
@@ -2743,8 +2765,7 @@ private TypeData createDataClassHeaderRow(ExpDataClass dataClass, Container cont
             List<QueryException> qpe = new ArrayList<>();
             DataClassUserSchema schema = new DataClassUserSchema(container, _user);
             QueryDefinition qDef = schema.getQueryDefForTable(dataClass.getName());
-            // Issue 52504: For lookup validation, we need to use the proper lookup container filter on the table
-            qDef.setContainerFilter(QueryService.get().getContainerFilterForLookups(container, _user));
+            setContainerFilterForImport(qDef, container, _user);
             TableInfo dataTable = qDef.getTable(schema, qpe, true);
             if (dataTable == null)
             {
@@ -2774,8 +2795,7 @@ private TypeData createSampleHeaderRow(ExpSampleTypeImpl sampleType, Container c
             List<QueryException> qpe = new ArrayList<>();
             SamplesSchema schema = new SamplesSchema(_user, container);
             QueryDefinition qDef = schema.getQueryDefForTable(sampleType.getName());
-            // Issue 52504: For lookup validation, we need to use the proper lookup container filter on the table
-            qDef.setContainerFilter(QueryService.get().getContainerFilterForLookups(container, _user));
+            setContainerFilterForImport(qDef, container, _user);
             TableInfo samplesTable = qDef.getTable(schema, qpe, true);
             if (samplesTable == null)
             {

experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java

@@ -40,6 +40,7 @@
 import org.labkey.api.data.ContainerFilter;
 import org.labkey.api.data.ContainerManager;
 import org.labkey.api.data.DbScope;
+import org.labkey.api.data.FieldKeyRowMap;
 import org.labkey.api.data.JdbcType;
 import org.labkey.api.data.MutableColumnInfo;
 import org.labkey.api.data.PHI;
@@ -1241,14 +1242,14 @@ public List<Map<String, Object>> insertRows(User user, Container container, List
         }
 
         @Override
-        protected Map<String, Object> getRow(User user, Container container, Map<String, Object> keys) throws InvalidKeyException
+        protected Map<String, Object> getRow(User user, Container container, Map<String, Object> keys) throws InvalidKeyException, SQLException
         {
             return getRow(user, container, keys, false);
         }
 
         /* This class overrides getRow() in order to support getRow() using "rowid" or "lsid" */
         @Override
-        protected Map<String, Object> getRow(User user, Container container, Map<String, Object> keys, boolean allowCrossContainer) throws InvalidKeyException
+        protected Map<String, Object> getRow(User user, Container container, Map<String, Object> keys, boolean allowCrossContainer) throws InvalidKeyException, SQLException
         {
             aliasColumns(_columnMapping, keys);
 
@@ -1263,19 +1264,7 @@ protected Map<String, Object> getRow(User user, Container container, Map<String,
             if (null == rowId && null == lsid && null == name)
                 throw new InvalidKeyException("Value must be supplied for key field 'rowid' or 'lsid' or 'name'", keys);
 
-            Map<String,Object> row = _select(container, rowId, lsid, name, classId, allowCrossContainer);
-
-            //PostgreSQL includes a column named _row for the row index, but since this is selecting by
-            //primary key, it will always be 1, which is not only unnecessary, but confusing, so strip it
-            if (null != row)
-            {
-                if (row instanceof ArrayListMap arrayListMap)
-                    arrayListMap.getFindMap().remove("_row");
-                else
-                    row.remove("_row");
-            }
-
-            return row;
+            return _select(container, rowId, lsid, name, classId, allowCrossContainer);
         }
 
         @Override
@@ -1284,32 +1273,34 @@ protected Map<String, Object> _select(Container container, Object[] keys) throws
             throw new IllegalStateException();
         }
 
-        protected Map<String, Object> _select(Container container, Integer rowid, String lsid, String name, Integer classId, boolean allowCrossContainer) throws ConversionException
+        protected Map<String, Object> _select(Container container, Integer rowId, String lsid, String name, Integer classId, boolean allowCrossContainer) throws SQLException
         {
-            if (null == rowid && null == lsid && (null == name || null == classId))
+            if (null == rowId && null == lsid && (null == name || null == classId))
                 return null;
 
-            // FIXME Issue 52886: This retrieves raw db column names, which doesn't work well for comparing existing and new audit records if the name doesn't match the field key
-            TableInfo d = getDbTable();
-            TableInfo t = _dataClassDataTableSupplier.get();
-
-            SQLFragment sql = new SQLFragment()
-                    .append("SELECT t.*, d.RowId, d.Name, d.ClassId, d.Container, d.Description, d.CreatedBy, d.Created, d.ModifiedBy, d.Modified")
-                    .append(" FROM ").append(d, "d")
-                    .append(" LEFT OUTER JOIN ").append(t, "t")
-                    .append(" ON d.lsid = t.lsid WHERE ");
-
-            if (null != rowid)
-                sql.append("d.rowid=?").add(rowid);
+            // Issue 52886: Use queryTable here, not raw database table, so the rows are from the user schema with names
+            // as expected to match row inserts and other querySchema data
+            SimpleFilter filter = new SimpleFilter();
+            if (null != rowId)
+                filter.addCondition(Column.RowId.fieldKey(), rowId);
             else if (null != lsid)
-                sql.append("d.lsid=?").add(lsid);
+                filter.addCondition(Column.LSID.fieldKey(), lsid);
             else
-                sql.append("d.classid=? AND d.name=?").add(classId).add(name);
-
+                filter.addCondition(Column.ClassId.fieldKey(), classId)
+                        .addCondition(Column.Name.fieldKey(), name);
             if (!allowCrossContainer)
-                sql.append(" AND d.Container=?").add(container.getEntityId());
+                filter.addCondition(Column.Folder.fieldKey(), container.getEntityId());
+
+            TableInfo queryTable = getQueryTable();
+            TableSelector selector = new TableSelector(queryTable, filter, null);
 
-            return new SqlSelector(getDbTable().getSchema(), sql).getMap();
+            try (var results = selector.getResults()) {
+                if (results.next())
+                {
+                    return FieldKeyRowMap.toNameMap(results.getFieldKeyRowMap());
+                }
+            }
+            return null;
         }
 
         @Override

experiment/src/org/labkey/experiment/api/ExpDataClassDataTestCase.jsp

@@ -1181,7 +1181,7 @@ private @NotNull TableInfo getDataClassTable(String dataClassName)
     return schema.getTableOrThrow(dataClassName);
 }
 
-// @Test // Issue 52886
+@Test // Issue 52886
 public void testUpdateAuditForLongField() throws Exception
 {
     User user = TestContext.get().getUser();

experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java

@@ -971,9 +971,11 @@ private void addSampleTypeColumns(ExpSampleType st, List<FieldKey> visibleColumn
                 continue;
             }
 
+            var wrapped = wrapColumnFromJoinedTable(dbColumn.getName(), dbColumn);
+
             // TODO missing values? comments? flags?
             DomainProperty dp = domain.getPropertyByURI(dbColumn.getPropertyURI());
-            var propColumn = copyColumnFromJoinedTable(null==dp?dbColumn.getName():dp.getName(), dbColumn);
+            var propColumn = copyColumnFromJoinedTable(null==dp ? dbColumn.getName() : dp.getName(), wrapped);
             if (propColumn.getName().equalsIgnoreCase("genid"))
             {
                 propColumn.setHidden(true);
@@ -1020,6 +1022,7 @@ private void addSampleTypeColumns(ExpSampleType st, List<FieldKey> visibleColumn
 
             if (!mvColumns.contains(propColumn.getFieldKey()))
                 addColumn(propColumn);
+
         }
 
         setDefaultVisibleColumns(visibleColumns);

experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java

@@ -366,6 +366,7 @@
 import static org.labkey.api.util.DOM.UL;
 import static org.labkey.api.util.DOM.at;
 import static org.labkey.api.util.DOM.cl;
+import static org.labkey.experiment.ExpDataIterators.setContainerFilterForImport;
 import static org.labkey.experiment.api.SampleTypeServiceImpl.SampleChangeType.update;
 
 public class ExperimentController extends SpringActionController
@@ -4530,8 +4531,7 @@ public void validateForm(QueryForm form, Errors errors)
         protected void initRequest(QueryForm form) throws ServletException
         {
             QueryDefinition query = form.getQueryDef();
-            // Issue 52504: For lookup validation, we need to use the proper lookup container filter on the table
-            query.setContainerFilter(QueryService.get().getContainerFilterForLookups(getContainer(), getUser()));
+            setContainerFilterForImport(query, getContainer(), getUser());
             List<QueryException> qpe = new ArrayList<>();
             TableInfo t = query.getTable(form.getSchema(), qpe, true);
 

query/src/org/labkey/query/controllers/QueryController.java

@@ -4122,8 +4122,6 @@ protected void initRequest(QueryForm form) throws ServletException
 
             _insertOption = form.getInsertOption();
             QueryDefinition query = form.getQueryDef();
-            // Issue 52504: For lookup validation, we need to use the proper lookup container filter on the table
-            query.setContainerFilter(QueryService.get().getContainerFilterForLookups(getContainer(), getUser()));
             List<QueryException> qpe = new ArrayList<>();
             TableInfo t = query.getTable(form.getSchema(), qpe, true);
             if (!qpe.isEmpty())