Skip to content

Commit 4efc813

Browse files
labkey-susanhlabkey-susanh
authored
Issue 52886: SourcesAuditEvent update event newRecordMap diff missing value for an updated field with a long name (#6858)
1 parent 2ccc0ae commit 4efc813

11 files changed

Lines changed: 106 additions & 64 deletions

File tree

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

Lines changed: 35 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import org.jetbrains.annotations.Nullable;
55
import org.labkey.api.data.ColumnInfo;
66
import org.labkey.api.data.Container;
7+
import org.labkey.api.data.MultiValuedForeignKey;
78
import org.labkey.api.data.TableInfo;
89
import org.labkey.api.dataiterator.DataIterator;
910
import org.labkey.api.dataiterator.ExistingRecordDataIterator;
@@ -64,16 +65,27 @@ static Pair<Map<String, Object>, Map<String, Object>> getOldAndNewRecordForMerge
6465
// and we won't convert sample type and data class names into lower case.
6566
for (Map.Entry<String, Object> entry : existingRow.entrySet())
6667
{
68+
boolean isMultiValued = false;
6769
String key = entry.getKey();
6870
// getDatasetRows() (at least) should return key==column.getName(), expect getColumn(name) to work
6971
ColumnInfo col = null==table ? null : table.getColumn(key);
70-
String nameFromAlias = null != col
71-
? col.getName()
72-
: columns.stream()
73-
.filter(column -> column.getAlias().getId().equalsIgnoreCase(key))
74-
.map((ColumnInfo::getName))
75-
.findFirst()
76-
.orElse(key);
72+
if (col != null && col.getFk() instanceof MultiValuedForeignKey)
73+
isMultiValued = true;
74+
75+
String nameFromAlias = key;
76+
if (null != col)
77+
nameFromAlias = col.getName();
78+
else
79+
{
80+
ColumnInfo aliasColumn = columns.stream().filter(c -> c.getAlias().getId().equalsIgnoreCase(key)).findFirst().orElse(null);
81+
82+
if (aliasColumn != null)
83+
{
84+
if (aliasColumn.getFk() != null && aliasColumn.getFk() instanceof MultiValuedForeignKey)
85+
isMultiValued = true;
86+
nameFromAlias = aliasColumn.getName();
87+
}
88+
}
7789
String lcName = nameFromAlias.toLowerCase();
7890
// Preserve casing of inputs so we can show the names properly
7991
boolean isExpInput = false;
@@ -131,8 +143,22 @@ else if (newValue instanceof Number && oldValue != null)
131143
}
132144
else if (!Objects.equals(oldValue, newValue) || isExtraAuditField)
133145
{
134-
originalRow.put(nameFromAlias, oldValue);
135-
modifiedRow.put(nameFromAlias, newValue);
146+
// If multivalued columns change, the value in this table will remain the key to the junction table
147+
// but at this point newValue will look like the newly chosen values not that key. So we skip
148+
// this in the diff unless the value changes from non-null to null or vice versa.
149+
if (isMultiValued)
150+
{
151+
if ((oldValue == null && newValue != null) || (newValue == null && oldValue != null))
152+
{
153+
originalRow.put(nameFromAlias, oldValue);
154+
modifiedRow.put(nameFromAlias, newValue);
155+
}
156+
}
157+
else
158+
{
159+
originalRow.put(nameFromAlias, oldValue);
160+
modifiedRow.put(nameFromAlias, newValue);
161+
}
136162
}
137163
}
138164
else if (isExtraAuditField)

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
import java.util.Map;
2828
import java.util.Set;
2929

30-
class FieldKeyRowMap implements Map<FieldKey, Object>
30+
public class FieldKeyRowMap implements Map<FieldKey, Object>
3131
{
3232
private final Results _results;
3333

@@ -131,8 +131,6 @@ public static Map<String, Object> toNameMap(Map<FieldKey, Object> rowMap)
131131
{
132132
Map<String, Object> map = new CaseInsensitiveHashMap<>();
133133
rowMap.forEach((key, value) -> {
134-
if (key.getParent() != null)
135-
throw new IllegalArgumentException("Multi-part field key '" + key + "' cannot be used as key in string map since it may not be unique.");
136134
if (map.containsKey(key.getName()))
137135
throw new IllegalArgumentException("Duplicate key '" + key + "' found in fieldKey map.");
138136
map.put(key.getName(), value);

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import org.labkey.api.exp.property.PropertyService;
3636
import org.labkey.api.query.FieldKey;
3737
import org.labkey.api.query.PdLookupForeignKey;
38+
import org.labkey.api.query.QueryService;
3839
import org.labkey.api.query.SchemaKey;
3940
import org.labkey.api.security.User;
4041
import org.labkey.api.study.assay.FileLinkDisplayColumn;
@@ -182,7 +183,12 @@ public static void copyAttributes(
182183
}
183184

184185
if (user != null && ((pd.getLookupSchema() != null && pd.getLookupQuery() != null) || pd.getConceptURI() != null))
185-
to.setFk(PdLookupForeignKey.create(to.getParentTable().getUserSchema(), user, container, pd, cf));
186+
{
187+
// Issue 52504: Use proper container filter for lookups
188+
var _cf = pd.isLookup() ? QueryService.get().getContainerFilterForLookups(container, user) : cf;
189+
190+
to.setFk(PdLookupForeignKey.create(to.getParentTable().getUserSchema(), user, container, pd, _cf));
191+
}
186192

187193
to.setDefaultValueType(pd.getDefaultValueTypeEnum());
188194
to.setConditionalFormats(PropertyService.get().getConditionalFormats(pd));

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ public Map<Integer, Map<String, Object>> getExistingRows(User user, Container co
204204
Map<String, Object> keyValues = key.getValue();
205205
Map<String, Object> row = getRow(user, container, keyValues, verifyNoCrossFolderData);
206206
boolean hasValidExisting = false;
207-
if (row != null)
207+
if (row != null && !row.isEmpty())
208208
{
209209
result.put(key.getKey(), row);
210210
if (verifyNoCrossFolderData)

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,7 @@ describe('Import with update / merge', () => {
234234
const BLANK_KEY_UPDATE_ERROR_NO_EXPRESSION = 'Missing value for required property: Name';
235235
const BLANK_KEY_UPDATE_ERROR_WITH_EXPRESSION = 'Name value not provided on row ';
236236
const BOGUS_KEY_UPDATE_ERROR = 'Data not found: ';
237-
const CROSS_FOLDER_UPDATE_NOT_SUPPORTED_ERROR = "Data doesn't belong to folder ";
237+
const DUPLICATE_KEY_ERROR = 'duplicate key value';
238238

239239
const dataType = "NoExpressionNameRequired52922";
240240
const createPayload = {
@@ -306,9 +306,9 @@ describe('Import with update / merge', () => {
306306

307307
// cross folder update not supported when folder type is "Collaboration"
308308
let crossFolderErrorResp = await ExperimentCRUDUtils.importData(server, "Name\tDescription\nData1\tNotblank\n\tisBlank", dataTypeWithExpression, "MERGE", subfolder1Options, editorUserOptions);
309-
expect(crossFolderErrorResp.text.indexOf(CROSS_FOLDER_UPDATE_NOT_SUPPORTED_ERROR) > -1).toBeTruthy();
309+
expect(crossFolderErrorResp.text.indexOf(DUPLICATE_KEY_ERROR) > -1).toBeTruthy();
310310
crossFolderErrorResp = await ExperimentCRUDUtils.importData(server, "Name\tDescription\nData1\tNotblank", dataTypeWithExpression, "UPDATE", subfolder1Options, editorUserOptions);
311-
expect(crossFolderErrorResp.text.indexOf(CROSS_FOLDER_UPDATE_NOT_SUPPORTED_ERROR) > -1).toBeTruthy();
311+
expect(crossFolderErrorResp.text.indexOf(BOGUS_KEY_UPDATE_ERROR) > -1).toBeTruthy();
312312

313313
// bogus name
314314
bogusKeyProvidedError = await ExperimentCRUDUtils.importData(server, "Name\tDescription\nbogus\tisBogus", dataTypeWithExpression, "UPDATE", topFolderOptions, editorUserOptions);
@@ -577,4 +577,4 @@ describe('Duplicate IDs', () => {
577577

578578
});
579579

580-
});
580+
});

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

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -805,6 +805,29 @@ public boolean next() throws BatchValidationException
805805
}
806806
}
807807

808+
/**
809+
* Issue 52504 (sort of): Chooses a container filter that is appropriate for import, merge or update actions in the face of product folders.
810+
* Note that this is slightly different from our treatment of lookups:
811+
* - when in a project, we allow import or update to all subfolders,
812+
* - when in a folder, we only allow references to data up the folder tree
813+
* @param qDef The QueryDefinition in use for the import action
814+
* @param container The container that is the target of the import or update
815+
* @param user The user doing the action
816+
*/
817+
public static void setContainerFilterForImport(QueryDefinition qDef, Container container, User user)
818+
{
819+
if (container.isProductFoldersEnabled())
820+
{
821+
ContainerFilter cf;
822+
823+
if (container.isProject())
824+
cf = new ContainerFilter.AllInProjectPlusShared(container, user);
825+
else
826+
cf = new ContainerFilter.CurrentPlusProjectAndShared(container, user);
827+
qDef.setContainerFilter(cf);
828+
}
829+
}
830+
808831
/* setup mini dataiterator pipeline to process lineage */
809832
public static void derive(User user, Container container, DataIterator di, boolean isSample, ExpObject dataType, boolean skipAliquot) throws BatchValidationException
810833
{
@@ -2487,8 +2510,7 @@ private int _importPartition(TypeData typeData)
24872510
Container splitContainer = ContainerManager.getForRowId(containerSplitFile.getKey());
24882511
AbstractExpSchema schema = _isSamples ? new SamplesSchema(_user, splitContainer) : new DataClassUserSchema(splitContainer, _user);
24892512
QueryDefinition qDef = schema.getQueryDefForTable(typeData.dataType.getName());
2490-
// Issue 52504: For lookup validation, we need to use the proper lookup container filter on the table
2491-
qDef.setContainerFilter(QueryService.get().getContainerFilterForLookups(splitContainer, _user));
2513+
setContainerFilterForImport(qDef, splitContainer, _user);
24922514
TableInfo dataTable = qDef.getTable(schema, new ArrayList<>(), true);
24932515

24942516
if (dataTable == null)
@@ -2743,8 +2765,7 @@ private TypeData createDataClassHeaderRow(ExpDataClass dataClass, Container cont
27432765
List<QueryException> qpe = new ArrayList<>();
27442766
DataClassUserSchema schema = new DataClassUserSchema(container, _user);
27452767
QueryDefinition qDef = schema.getQueryDefForTable(dataClass.getName());
2746-
// Issue 52504: For lookup validation, we need to use the proper lookup container filter on the table
2747-
qDef.setContainerFilter(QueryService.get().getContainerFilterForLookups(container, _user));
2768+
setContainerFilterForImport(qDef, container, _user);
27482769
TableInfo dataTable = qDef.getTable(schema, qpe, true);
27492770
if (dataTable == null)
27502771
{
@@ -2774,8 +2795,7 @@ private TypeData createSampleHeaderRow(ExpSampleTypeImpl sampleType, Container c
27742795
List<QueryException> qpe = new ArrayList<>();
27752796
SamplesSchema schema = new SamplesSchema(_user, container);
27762797
QueryDefinition qDef = schema.getQueryDefForTable(sampleType.getName());
2777-
// Issue 52504: For lookup validation, we need to use the proper lookup container filter on the table
2778-
qDef.setContainerFilter(QueryService.get().getContainerFilterForLookups(container, _user));
2798+
setContainerFilterForImport(qDef, container, _user);
27792799
TableInfo samplesTable = qDef.getTable(schema, qpe, true);
27802800
if (samplesTable == null)
27812801
{

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

Lines changed: 25 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
import org.labkey.api.data.ContainerFilter;
4141
import org.labkey.api.data.ContainerManager;
4242
import org.labkey.api.data.DbScope;
43+
import org.labkey.api.data.FieldKeyRowMap;
4344
import org.labkey.api.data.JdbcType;
4445
import org.labkey.api.data.MutableColumnInfo;
4546
import org.labkey.api.data.PHI;
@@ -1241,14 +1242,14 @@ public List<Map<String, Object>> insertRows(User user, Container container, List
12411242
}
12421243

12431244
@Override
1244-
protected Map<String, Object> getRow(User user, Container container, Map<String, Object> keys) throws InvalidKeyException
1245+
protected Map<String, Object> getRow(User user, Container container, Map<String, Object> keys) throws InvalidKeyException, SQLException
12451246
{
12461247
return getRow(user, container, keys, false);
12471248
}
12481249

12491250
/* This class overrides getRow() in order to support getRow() using "rowid" or "lsid" */
12501251
@Override
1251-
protected Map<String, Object> getRow(User user, Container container, Map<String, Object> keys, boolean allowCrossContainer) throws InvalidKeyException
1252+
protected Map<String, Object> getRow(User user, Container container, Map<String, Object> keys, boolean allowCrossContainer) throws InvalidKeyException, SQLException
12521253
{
12531254
aliasColumns(_columnMapping, keys);
12541255

@@ -1263,19 +1264,7 @@ protected Map<String, Object> getRow(User user, Container container, Map<String,
12631264
if (null == rowId && null == lsid && null == name)
12641265
throw new InvalidKeyException("Value must be supplied for key field 'rowid' or 'lsid' or 'name'", keys);
12651266

1266-
Map<String,Object> row = _select(container, rowId, lsid, name, classId, allowCrossContainer);
1267-
1268-
//PostgreSQL includes a column named _row for the row index, but since this is selecting by
1269-
//primary key, it will always be 1, which is not only unnecessary, but confusing, so strip it
1270-
if (null != row)
1271-
{
1272-
if (row instanceof ArrayListMap arrayListMap)
1273-
arrayListMap.getFindMap().remove("_row");
1274-
else
1275-
row.remove("_row");
1276-
}
1277-
1278-
return row;
1267+
return _select(container, rowId, lsid, name, classId, allowCrossContainer);
12791268
}
12801269

12811270
@Override
@@ -1284,32 +1273,34 @@ protected Map<String, Object> _select(Container container, Object[] keys) throws
12841273
throw new IllegalStateException();
12851274
}
12861275

1287-
protected Map<String, Object> _select(Container container, Integer rowid, String lsid, String name, Integer classId, boolean allowCrossContainer) throws ConversionException
1276+
protected Map<String, Object> _select(Container container, Integer rowId, String lsid, String name, Integer classId, boolean allowCrossContainer) throws SQLException
12881277
{
1289-
if (null == rowid && null == lsid && (null == name || null == classId))
1278+
if (null == rowId && null == lsid && (null == name || null == classId))
12901279
return null;
12911280

1292-
// 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
1293-
TableInfo d = getDbTable();
1294-
TableInfo t = _dataClassDataTableSupplier.get();
1295-
1296-
SQLFragment sql = new SQLFragment()
1297-
.append("SELECT t.*, d.RowId, d.Name, d.ClassId, d.Container, d.Description, d.CreatedBy, d.Created, d.ModifiedBy, d.Modified")
1298-
.append(" FROM ").append(d, "d")
1299-
.append(" LEFT OUTER JOIN ").append(t, "t")
1300-
.append(" ON d.lsid = t.lsid WHERE ");
1301-
1302-
if (null != rowid)
1303-
sql.append("d.rowid=?").add(rowid);
1281+
// Issue 52886: Use queryTable here, not raw database table, so the rows are from the user schema with names
1282+
// as expected to match row inserts and other querySchema data
1283+
SimpleFilter filter = new SimpleFilter();
1284+
if (null != rowId)
1285+
filter.addCondition(Column.RowId.fieldKey(), rowId);
13041286
else if (null != lsid)
1305-
sql.append("d.lsid=?").add(lsid);
1287+
filter.addCondition(Column.LSID.fieldKey(), lsid);
13061288
else
1307-
sql.append("d.classid=? AND d.name=?").add(classId).add(name);
1308-
1289+
filter.addCondition(Column.ClassId.fieldKey(), classId)
1290+
.addCondition(Column.Name.fieldKey(), name);
13091291
if (!allowCrossContainer)
1310-
sql.append(" AND d.Container=?").add(container.getEntityId());
1292+
filter.addCondition(Column.Folder.fieldKey(), container.getEntityId());
1293+
1294+
TableInfo queryTable = getQueryTable();
1295+
TableSelector selector = new TableSelector(queryTable, filter, null);
13111296

1312-
return new SqlSelector(getDbTable().getSchema(), sql).getMap();
1297+
try (var results = selector.getResults()) {
1298+
if (results.next())
1299+
{
1300+
return FieldKeyRowMap.toNameMap(results.getFieldKeyRowMap());
1301+
}
1302+
}
1303+
return null;
13131304
}
13141305

13151306
@Override

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1181,7 +1181,7 @@ private @NotNull TableInfo getDataClassTable(String dataClassName)
11811181
return schema.getTableOrThrow(dataClassName);
11821182
}
11831183
1184-
// @Test // Issue 52886
1184+
@Test // Issue 52886
11851185
public void testUpdateAuditForLongField() throws Exception
11861186
{
11871187
User user = TestContext.get().getUser();

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -971,9 +971,11 @@ private void addSampleTypeColumns(ExpSampleType st, List<FieldKey> visibleColumn
971971
continue;
972972
}
973973

974+
var wrapped = wrapColumnFromJoinedTable(dbColumn.getName(), dbColumn);
975+
974976
// TODO missing values? comments? flags?
975977
DomainProperty dp = domain.getPropertyByURI(dbColumn.getPropertyURI());
976-
var propColumn = copyColumnFromJoinedTable(null==dp?dbColumn.getName():dp.getName(), dbColumn);
978+
var propColumn = copyColumnFromJoinedTable(null==dp ? dbColumn.getName() : dp.getName(), wrapped);
977979
if (propColumn.getName().equalsIgnoreCase("genid"))
978980
{
979981
propColumn.setHidden(true);
@@ -1020,6 +1022,7 @@ private void addSampleTypeColumns(ExpSampleType st, List<FieldKey> visibleColumn
10201022

10211023
if (!mvColumns.contains(propColumn.getFieldKey()))
10221024
addColumn(propColumn);
1025+
10231026
}
10241027

10251028
setDefaultVisibleColumns(visibleColumns);

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,7 @@
366366
import static org.labkey.api.util.DOM.UL;
367367
import static org.labkey.api.util.DOM.at;
368368
import static org.labkey.api.util.DOM.cl;
369+
import static org.labkey.experiment.ExpDataIterators.setContainerFilterForImport;
369370
import static org.labkey.experiment.api.SampleTypeServiceImpl.SampleChangeType.update;
370371

371372
public class ExperimentController extends SpringActionController
@@ -4530,8 +4531,7 @@ public void validateForm(QueryForm form, Errors errors)
45304531
protected void initRequest(QueryForm form) throws ServletException
45314532
{
45324533
QueryDefinition query = form.getQueryDef();
4533-
// Issue 52504: For lookup validation, we need to use the proper lookup container filter on the table
4534-
query.setContainerFilter(QueryService.get().getContainerFilterForLookups(getContainer(), getUser()));
4534+
setContainerFilterForImport(query, getContainer(), getUser());
45354535
List<QueryException> qpe = new ArrayList<>();
45364536
TableInfo t = query.getTable(form.getSchema(), qpe, true);
45374537

0 commit comments

Comments
 (0)