Skip to content

Make DatasetUpdateService respect isBulkLoad() for audit purposes #6791

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 14, 2025
+209 −52
Merged

Make DatasetUpdateService respect isBulkLoad() for audit purposes #6791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e307e92
Make DatasetUpdateService respect isBulkLoad() for audit purposes
bbimber Jun 24, 2025
ee0db60
Add test case
bbimber Jul 11, 2025
ba5c3ab
Use constants
labkey-jeckels Jul 11, 2025
3983821
No need to register handler for bulk loads
labkey-jeckels Jul 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion study/src/org/labkey/study/model/DatasetDefinition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2785,6 +2785,11 @@ private void deleteProvenance(Container c, User u, Collection<String> lsids)

@Override
public void deleteDatasetRows(User u, Collection<String> lsids)
{
deleteDatasetRows(u, lsids, false);
}

public void deleteDatasetRows(User u, Collection<String> lsids, boolean isBulkLoad)
{
// Need to fetch the old item in order to log the deletion
List<Map<String, Object>> oldDatas = getDatasetRows(u, lsids);
Expand All @@ -2794,7 +2799,10 @@ public void deleteDatasetRows(User u, Collection<String> lsids)
deleteProvenance(getContainer(), u, lsids);
deleteRows(lsids);

new DatasetAuditHandler(this).addAuditEvent(u, getContainer(), getTableInfo(u), AuditBehaviorType.DETAILED, null, QueryService.AuditAction.DELETE, oldDatas, null);
if (!isBulkLoad)
{
new DatasetAuditHandler(this).addAuditEvent(u, getContainer(), getTableInfo(u), AuditBehaviorType.DETAILED, null, QueryService.AuditAction.DELETE, oldDatas, null);
}

transaction.commit();
}
Expand Down
251 changes: 200 additions & 51 deletions study/src/org/labkey/study/query/DatasetUpdateService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
import org.labkey.api.audit.AbstractAuditTypeProvider;
import org.labkey.api.collections.CaseInsensitiveHashMap;
import org.labkey.api.collections.CaseInsensitiveHashSet;
import org.labkey.api.collections.ResultSetRowMapFactory;
Expand All @@ -32,11 +33,11 @@
import org.labkey.api.data.ContainerManager;
import org.labkey.api.data.DbScope;
import org.labkey.api.data.JdbcType;
import org.labkey.api.data.MVDisplayColumn;
import org.labkey.api.data.MvUtil;
import org.labkey.api.data.RuntimeSQLException;
import org.labkey.api.data.SQLFragment;
import org.labkey.api.data.SimpleFilter;
import org.labkey.api.data.Sort;
import org.labkey.api.data.SqlExecutor;
import org.labkey.api.data.TableInfo;
import org.labkey.api.data.TableSelector;
Expand Down Expand Up @@ -78,7 +79,9 @@
import org.labkey.api.util.DateUtil;
import org.labkey.api.util.GUID;
import org.labkey.api.util.JunitUtil;
import org.labkey.api.util.PageFlowUtil;
import org.labkey.api.util.TestContext;
import org.labkey.study.dataset.DatasetAuditProvider;
import org.labkey.study.model.DatasetDefinition;
import org.labkey.study.model.DatasetDomainKind;
import org.labkey.study.model.SecurityType;
Expand All @@ -88,6 +91,7 @@

import java.sql.SQLException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Date;
import java.util.HashMap;
Expand Down Expand Up @@ -675,8 +679,11 @@ protected Map<String, Object> updateRow(User user, Container container, Map<Stri
resetCreatedColumnsSQL.add(newLsid);
new SqlExecutor(_dataset.getStorageTableInfo().getSchema()).execute(resetCreatedColumnsSQL);

new DatasetDefinition.DatasetAuditHandler(_dataset).addAuditEvent(user, container, target, AuditBehaviorType.DETAILED, null, QueryService.AuditAction.UPDATE,
List.of(mergeData), List.of(oldData));
if (!isBulkLoad())
{
new DatasetDefinition.DatasetAuditHandler(_dataset).addAuditEvent(user, container, target, AuditBehaviorType.DETAILED, null, QueryService.AuditAction.UPDATE,
List.of(mergeData), List.of(oldData));
}

// Successfully updated
transaction.commit();
Expand Down Expand Up @@ -734,7 +741,7 @@ protected Map<String, Object> deleteRow(User user, Container container, Map<Stri
{
// Make sure we've found the original participant before doing the delete
String participant = getParticipant(oldRow, user, container);
_dataset.deleteDatasetRows(user, Collections.singleton(keyFromMap(oldRow)));
_dataset.deleteDatasetRows(user, Collections.singleton(keyFromMap(oldRow)), isBulkLoad());
_potentiallyDeletedParticipants.add(participant);
_participantVisitResyncRequired = true;
return oldRow;
Expand Down Expand Up @@ -812,61 +819,203 @@ else if (lsids.length > 1)
@TestWhen(TestWhen.When.BVT)
public static class TestCase extends Assert
{
private static final String SUBJECT_COLUMN_NAME = "SubjectID";
private static final String DATASET_NAME = "DS1";
TestContext _context = null;
User _user = null;
Container _container = null;
StudyImpl _junitStudy = null;
StudyManager _manager = StudyManager.getInstance();
String longName = "this is a very long name (with punctuation) that raises many questions \"?\" about your database design choices";

@Test
public void updateRowTest() throws Exception
private void createDataset() throws Exception
{
var dsd = new DatasetDefinition(_junitStudy, 1001, "DS1", "DS1", null, null, null);
if (DefaultSchema.get(_user, _container).getSchema(StudyQuerySchema.SCHEMA_NAME).getTable(DATASET_NAME) != null)
{
return;
}

var dsd = new DatasetDefinition(_junitStudy, 1001, DATASET_NAME, DATASET_NAME, null, null, null);
_manager.createDatasetDefinition(_user, dsd);
dsd = _manager.getDatasetDefinition(_junitStudy, 1001);
dsd.refreshDomain();
{
var domain = dsd.getDomain();
DomainProperty p;

p = domain.addProperty();
p.setName("Field1");
p.setPropertyURI(domain.getTypeURI() + "." + Lsid.encodePart(p.getName()));
p.setRangeURI(PropertyType.getFromJdbcType(JdbcType.VARCHAR).getTypeUri());

p = domain.addProperty();
p.setName("SELECT");
p.setPropertyURI(domain.getTypeURI() + "." + Lsid.encodePart(p.getName()));
p.setRangeURI(PropertyType.getFromJdbcType(JdbcType.VARCHAR).getTypeUri());

p = domain.addProperty();
p.setName(longName);
p.setPropertyURI(domain.getTypeURI() + "." + Lsid.encodePart(p.getName()));
p.setRangeURI(PropertyType.getFromJdbcType(JdbcType.VARCHAR).getTypeUri());

p = domain.addProperty();
p.setName("Value1");
p.setPropertyURI(domain.getTypeURI() + "." + Lsid.encodePart(p.getName()));
p.setRangeURI(PropertyType.getFromJdbcType(JdbcType.DOUBLE).getTypeUri());
p.setMvEnabled(true);

p = domain.addProperty();
p.setName("Value2");
p.setPropertyURI(domain.getTypeURI() + "." + Lsid.encodePart(p.getName()));
p.setRangeURI(PropertyType.getFromJdbcType(JdbcType.DOUBLE).getTypeUri());
p.setMvEnabled(true);

p = domain.addProperty();
p.setName("Value3");
p.setPropertyURI(domain.getTypeURI() + "." + Lsid.encodePart(p.getName()));
p.setRangeURI(PropertyType.getFromJdbcType(JdbcType.DOUBLE).getTypeUri());
p.setMvEnabled(true);

domain.save(_user);
var domain = dsd.getDomain();
DomainProperty p;

p = domain.addProperty();
p.setName("Field1");
p.setPropertyURI(domain.getTypeURI() + "." + Lsid.encodePart(p.getName()));
p.setRangeURI(PropertyType.getFromJdbcType(JdbcType.VARCHAR).getTypeUri());

p = domain.addProperty();
p.setName("SELECT");
p.setPropertyURI(domain.getTypeURI() + "." + Lsid.encodePart(p.getName()));
p.setRangeURI(PropertyType.getFromJdbcType(JdbcType.VARCHAR).getTypeUri());

p = domain.addProperty();
p.setName(longName);
p.setPropertyURI(domain.getTypeURI() + "." + Lsid.encodePart(p.getName()));
p.setRangeURI(PropertyType.getFromJdbcType(JdbcType.VARCHAR).getTypeUri());

p = domain.addProperty();
p.setName("Value1");
p.setPropertyURI(domain.getTypeURI() + "." + Lsid.encodePart(p.getName()));
p.setRangeURI(PropertyType.getFromJdbcType(JdbcType.DOUBLE).getTypeUri());
p.setMvEnabled(true);

p = domain.addProperty();
p.setName("Value2");
p.setPropertyURI(domain.getTypeURI() + "." + Lsid.encodePart(p.getName()));
p.setRangeURI(PropertyType.getFromJdbcType(JdbcType.DOUBLE).getTypeUri());
p.setMvEnabled(true);

p = domain.addProperty();
p.setName("Value3");
p.setPropertyURI(domain.getTypeURI() + "." + Lsid.encodePart(p.getName()));
p.setRangeURI(PropertyType.getFromJdbcType(JdbcType.DOUBLE).getTypeUri());
p.setMvEnabled(true);

domain.save(_user);
}
}

private long getDatasetAuditRowCount()
{
return new TableSelector(QueryService.get().getUserSchema(_user, _container, AbstractAuditTypeProvider.QUERY_SCHEMA_NAME).getTable(DatasetAuditProvider.DATASET_AUDIT_EVENT)).getRowCount();
}

private String getLatestAuditMessage()
{
return new TableSelector(QueryService.get().getUserSchema(_user, _container, AbstractAuditTypeProvider.QUERY_SCHEMA_NAME).getTable(DatasetAuditProvider.DATASET_AUDIT_EVENT), PageFlowUtil.set("Comment"), null, new Sort("-created")).setMaxRows(1).getObject(String.class);
}

@Test
public void testAuditing() throws Exception
{
createDataset();
TableInfo t = DefaultSchema.get(_user, _container).getSchema(StudyQuerySchema.SCHEMA_NAME).getTable(DATASET_NAME);
t.getUpdateService().truncateRows(_user, _container, null, null);

final QueryUpdateService qus = t.getUpdateService();
BatchValidationException errors = new BatchValidationException();

long actualAuditRows = getDatasetAuditRowCount();
long expectedAuditRows;

List<Map<String, Object>> insertedRows = qus.insertRows(_user, _container,
List.of(Map.of(
SUBJECT_COLUMN_NAME, "S1",
"SequenceNum", "1.2345",
longName, "NA"),
Map.of(
SUBJECT_COLUMN_NAME, "S2",
"SequenceNum", "1.2345",
longName, "WithoutBulkLoad")),
errors, null, null);

if (errors.hasErrors())
{
fail(errors.getMessage());
}

TableInfo t = DefaultSchema.get(_user, _container).getSchema("study").getTable("DS1");
expectedAuditRows = actualAuditRows + 2;
actualAuditRows = getDatasetAuditRowCount();
Assert.assertEquals("Incorrect number of audit records", expectedAuditRows, actualAuditRows);
Assert.assertEquals("Incorrect comment", "A new dataset record was inserted", getLatestAuditMessage());

qus.insertRows(_user, _container,
List.of(Map.of(
SUBJECT_COLUMN_NAME, "S3",
"SequenceNum", "1.2345",
longName, "WithoutBulkLoad")),
errors, null, null);

if (errors.hasErrors())
{
fail(errors.getMessage());
}

expectedAuditRows = actualAuditRows + 1;
actualAuditRows = getDatasetAuditRowCount();
Assert.assertEquals("Incorrect number of audit records", expectedAuditRows, actualAuditRows);

// Now update:
insertedRows.get(0).put(longName, "NewValue");
insertedRows.get(1).put(longName, "NewValue");
List<Map<String, Object>> oldKeys = Arrays.asList(
Map.of("lsid", insertedRows.get(0).get("lsid")),
Map.of("lsid", insertedRows.get(1).get("lsid"))
);
qus.updateRows(_user, _container, insertedRows, oldKeys, errors, null, null);
if (errors.hasErrors())
{
fail(errors.getMessage());
}

expectedAuditRows = actualAuditRows + 2;
actualAuditRows = getDatasetAuditRowCount();
Assert.assertEquals("Incorrect number of audit records", expectedAuditRows, actualAuditRows);
Assert.assertEquals("Incorrect comment", "A dataset record was modified", getLatestAuditMessage());

qus.deleteRows(_user, _container, oldKeys, null, null);
expectedAuditRows = actualAuditRows + 2;
actualAuditRows = getDatasetAuditRowCount();
Assert.assertEquals("Incorrect number of audit records", expectedAuditRows, actualAuditRows);
Assert.assertEquals("Incorrect comment", "A dataset record was deleted", getLatestAuditMessage());

// Repeat using bulkLoad=true:
qus.setBulkLoad(true);

insertedRows = qus.insertRows(_user, _container,
List.of(Map.of(
SUBJECT_COLUMN_NAME, "S4",
"SequenceNum", "1.2345",
longName, "WithBulkLoad"),
Map.of(
SUBJECT_COLUMN_NAME, "S5",
"SequenceNum", "1.2345",
longName, "WithBulkLoad")),
errors, null, null);

if (errors.hasErrors())
{
fail(errors.getMessage());
}

expectedAuditRows = actualAuditRows;
actualAuditRows = getDatasetAuditRowCount();
Assert.assertEquals("Incorrect number of audit records", expectedAuditRows, actualAuditRows);

// Now update:
insertedRows.get(0).put(longName, "NewValue");
insertedRows.get(1).put(longName, "NewValue");
oldKeys = Arrays.asList(
Map.of("lsid", insertedRows.get(0).get("lsid")),
Map.of("lsid", insertedRows.get(1).get("lsid"))
);
qus.updateRows(_user, _container, insertedRows, oldKeys, errors, null, null);
if (errors.hasErrors())
{
fail(errors.getMessage());
}

expectedAuditRows = actualAuditRows;
actualAuditRows = getDatasetAuditRowCount();
Assert.assertEquals("Incorrect number of audit records", expectedAuditRows, actualAuditRows);

qus.deleteRows(_user, _container, oldKeys, null, null);
expectedAuditRows = actualAuditRows;
actualAuditRows = getDatasetAuditRowCount();
Assert.assertEquals("Incorrect number of audit records", expectedAuditRows, actualAuditRows);
}

@Test
public void updateRowTest() throws Exception
{
createDataset();

TableInfo t = DefaultSchema.get(_user, _container).getSchema(StudyQuerySchema.SCHEMA_NAME).getTable(DATASET_NAME);
assertNotNull(t);
assertTrue("Field1".equalsIgnoreCase(t.getColumn("Field1").getAlias()));
assertFalse("SELECT".equalsIgnoreCase(t.getColumn("SELECT").getAlias()));
Expand All @@ -877,7 +1026,7 @@ public void updateRowTest() throws Exception

var result = up.insertRows(_user, _container,
List.of(Map.of(
"subjectid", "S1",
SUBJECT_COLUMN_NAME, "S1",
"SequenceNum", "1.2345",
"Field1", "f",
"SELECT", "s",
Expand All @@ -892,7 +1041,7 @@ public void updateRowTest() throws Exception
assertNotNull(result);
assertEquals(1, result.size());
var map = result.get(0);
assertEquals("S1", map.get("SubjectId"));
assertEquals("S1", map.get(SUBJECT_COLUMN_NAME));
assertEquals("f", map.get("Field1"));
assertEquals("s", map.get("SELECT"));
assertEquals("l", map.get(longName));
Expand All @@ -909,15 +1058,15 @@ public void updateRowTest() throws Exception

// update subjectid column
result = up.updateRows(_user, _container,
List.of(Map.of("subjectid", "S2")),
List.of(Map.of(SUBJECT_COLUMN_NAME, "S2")),
List.of(Map.of("lsid", lsid)),
errors, null, null);
if (errors.hasErrors())
fail(errors.getMessage());
assertNotNull(result);
assertEquals(1, result.size());
map = result.get(0);
assertEquals("S2", map.get("SubjectId"));
assertEquals("S2", map.get(SUBJECT_COLUMN_NAME));
// All other columns are preserved
assertEquals("f", map.get("Field1"));
assertEquals("s", map.get("SELECT"));
Expand Down Expand Up @@ -950,7 +1099,7 @@ public void updateRowTest() throws Exception
assertNotNull(result);
assertEquals(1, result.size());
map = result.get(0);
assertEquals("S2", map.get("SubjectId"));
assertEquals("S2", map.get(SUBJECT_COLUMN_NAME));
assertEquals("fUpdated", map.get("Field1"));
assertEquals("sUpdated", map.get("SELECT"));
assertEquals("lUpdated", map.get(longName));
Expand All @@ -977,7 +1126,7 @@ public void createStudy()
StudyImpl s = new StudyImpl(c, "Junit Study");
s.setTimepointType(TimepointType.VISIT);
s.setStartDate(new Date(DateUtil.parseDateTime(c, "2014-01-01")));
s.setSubjectColumnName("SubjectID");
s.setSubjectColumnName(SUBJECT_COLUMN_NAME);
s.setSubjectNounPlural("Subjects");
s.setSubjectNounSingular("Subject");
s.setSecurityType(SecurityType.BASIC_WRITE);
Expand Down