From e307e92e5ed0e303c885cded1be1bdeeca62e447 Mon Sep 17 00:00:00 2001 From: bbimber Date: Tue, 24 Jun 2025 15:30:22 -0700 Subject: [PATCH 1/4] Make DatasetUpdateService respect isBulkLoad() for audit purposes --- study/src/org/labkey/study/model/DatasetDefinition.java | 7 ++++++- .../src/org/labkey/study/query/DatasetUpdateService.java | 9 ++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/study/src/org/labkey/study/model/DatasetDefinition.java b/study/src/org/labkey/study/model/DatasetDefinition.java index 906b36a4fc1..73f1f530330 100644 --- a/study/src/org/labkey/study/model/DatasetDefinition.java +++ b/study/src/org/labkey/study/model/DatasetDefinition.java @@ -2785,6 +2785,11 @@ private void deleteProvenance(Container c, User u, Collection lsids) @Override public void deleteDatasetRows(User u, Collection lsids) + { + deleteDatasetRows(u, lsids, false); + } + + public void deleteDatasetRows(User u, Collection lsids, boolean isBulkLoad) { // Need to fetch the old item in order to log the deletion List> oldDatas = getDatasetRows(u, lsids); @@ -2794,7 +2799,7 @@ public void deleteDatasetRows(User u, Collection lsids) deleteProvenance(getContainer(), u, lsids); deleteRows(lsids); - new DatasetAuditHandler(this).addAuditEvent(u, getContainer(), getTableInfo(u), AuditBehaviorType.DETAILED, null, QueryService.AuditAction.DELETE, oldDatas, null); + new DatasetAuditHandler(this).addAuditEvent(u, getContainer(), getTableInfo(u), isBulkLoad ? AuditBehaviorType.SUMMARY : AuditBehaviorType.DETAILED, null, QueryService.AuditAction.DELETE, oldDatas, null); transaction.commit(); } diff --git a/study/src/org/labkey/study/query/DatasetUpdateService.java b/study/src/org/labkey/study/query/DatasetUpdateService.java index e9af4a66b50..7d5ac33bad4 100644 --- a/study/src/org/labkey/study/query/DatasetUpdateService.java +++ b/study/src/org/labkey/study/query/DatasetUpdateService.java @@ -675,8 +675,11 @@ protected Map updateRow(User user, Container container, Map deleteRow(User user, Container container, Map Date: Thu, 10 Jul 2025 19:33:07 -0700 Subject: [PATCH 2/4] Add test case --- .../study/query/DatasetUpdateService.java | 222 ++++++++++++++---- 1 file changed, 182 insertions(+), 40 deletions(-) diff --git a/study/src/org/labkey/study/query/DatasetUpdateService.java b/study/src/org/labkey/study/query/DatasetUpdateService.java index 7d5ac33bad4..809f55d98ac 100644 --- a/study/src/org/labkey/study/query/DatasetUpdateService.java +++ b/study/src/org/labkey/study/query/DatasetUpdateService.java @@ -32,11 +32,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; @@ -78,6 +78,7 @@ 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.model.DatasetDefinition; import org.labkey.study.model.DatasetDomainKind; @@ -88,6 +89,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; @@ -822,53 +824,193 @@ public static class TestCase extends Assert 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 { + if (DefaultSchema.get(_user, _container).getSchema("study").getTable("DS1") != null) + { + return; + } + var dsd = new DatasetDefinition(_junitStudy, 1001, "DS1", "DS1", 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, "auditLog").getTable("DatasetAuditEvent")).getRowCount(); + } + + private String getLatestAuditMessage() + { + return new TableSelector(QueryService.get().getUserSchema(_user, _container, "auditLog").getTable("DatasetAuditEvent"), 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("study").getTable("DS1"); + t.getUpdateService().truncateRows(_user, _container, null, null); + + final QueryUpdateService qus = t.getUpdateService(); + BatchValidationException errors = new BatchValidationException(); + + long actualAuditRows = getDatasetAuditRowCount(); + long expectedAuditRows; + + List> insertedRows = qus.insertRows(_user, _container, + List.of(Map.of( + "subjectid", "S1", + "SequenceNum", "1.2345", + longName, "NA"), + Map.of( + "subjectid", "S2", + "SequenceNum", "1.2345", + longName, "WithoutBulkLoad")), + 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 new dataset record was inserted", getLatestAuditMessage()); + + qus.insertRows(_user, _container, + List.of(Map.of( + "subjectid", "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> 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( + "subjectid", "S4", + "SequenceNum", "1.2345", + longName, "WithBulkLoad"), + Map.of( + "subjectid", "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("study").getTable("DS1"); assertNotNull(t); assertTrue("Field1".equalsIgnoreCase(t.getColumn("Field1").getAlias())); From ba5c3ab17ea513a2a4f89d937178e056dc119967 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Fri, 11 Jul 2025 11:33:13 -0700 Subject: [PATCH 3/4] Use constants --- .../study/query/DatasetUpdateService.java | 38 ++++++++++--------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/study/src/org/labkey/study/query/DatasetUpdateService.java b/study/src/org/labkey/study/query/DatasetUpdateService.java index 809f55d98ac..6838e8f30f1 100644 --- a/study/src/org/labkey/study/query/DatasetUpdateService.java +++ b/study/src/org/labkey/study/query/DatasetUpdateService.java @@ -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; @@ -80,6 +81,7 @@ 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; @@ -817,6 +819,8 @@ 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; @@ -826,12 +830,12 @@ public static class TestCase extends Assert private void createDataset() throws Exception { - if (DefaultSchema.get(_user, _container).getSchema("study").getTable("DS1") != null) + if (DefaultSchema.get(_user, _container).getSchema(StudyQuerySchema.SCHEMA_NAME).getTable(DATASET_NAME) != null) { return; } - var dsd = new DatasetDefinition(_junitStudy, 1001, "DS1", "DS1", null, null, null); + var dsd = new DatasetDefinition(_junitStudy, 1001, DATASET_NAME, DATASET_NAME, null, null, null); _manager.createDatasetDefinition(_user, dsd); dsd = _manager.getDatasetDefinition(_junitStudy, 1001); dsd.refreshDomain(); @@ -878,19 +882,19 @@ private void createDataset() throws Exception private long getDatasetAuditRowCount() { - return new TableSelector(QueryService.get().getUserSchema(_user, _container, "auditLog").getTable("DatasetAuditEvent")).getRowCount(); + 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, "auditLog").getTable("DatasetAuditEvent"), PageFlowUtil.set("Comment"), null, new Sort("-created")).setMaxRows(1).getObject(String.class); + 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("study").getTable("DS1"); + 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(); @@ -901,11 +905,11 @@ public void testAuditing() throws Exception List> insertedRows = qus.insertRows(_user, _container, List.of(Map.of( - "subjectid", "S1", + SUBJECT_COLUMN_NAME, "S1", "SequenceNum", "1.2345", longName, "NA"), Map.of( - "subjectid", "S2", + SUBJECT_COLUMN_NAME, "S2", "SequenceNum", "1.2345", longName, "WithoutBulkLoad")), errors, null, null); @@ -922,7 +926,7 @@ public void testAuditing() throws Exception qus.insertRows(_user, _container, List.of(Map.of( - "subjectid", "S3", + SUBJECT_COLUMN_NAME, "S3", "SequenceNum", "1.2345", longName, "WithoutBulkLoad")), errors, null, null); @@ -965,11 +969,11 @@ public void testAuditing() throws Exception insertedRows = qus.insertRows(_user, _container, List.of(Map.of( - "subjectid", "S4", + SUBJECT_COLUMN_NAME, "S4", "SequenceNum", "1.2345", longName, "WithBulkLoad"), Map.of( - "subjectid", "S5", + SUBJECT_COLUMN_NAME, "S5", "SequenceNum", "1.2345", longName, "WithBulkLoad")), errors, null, null); @@ -1011,7 +1015,7 @@ public void updateRowTest() throws Exception { createDataset(); - TableInfo t = DefaultSchema.get(_user, _container).getSchema("study").getTable("DS1"); + 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())); @@ -1022,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", @@ -1037,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)); @@ -1054,7 +1058,7 @@ 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()) @@ -1062,7 +1066,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)); // All other columns are preserved assertEquals("f", map.get("Field1")); assertEquals("s", map.get("SELECT")); @@ -1095,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)); @@ -1122,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); From 398382113b441f30a664efd743168c3b2bb95476 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Fri, 11 Jul 2025 11:48:15 -0700 Subject: [PATCH 4/4] No need to register handler for bulk loads --- study/src/org/labkey/study/model/DatasetDefinition.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/study/src/org/labkey/study/model/DatasetDefinition.java b/study/src/org/labkey/study/model/DatasetDefinition.java index 73f1f530330..175cfd40ec9 100644 --- a/study/src/org/labkey/study/model/DatasetDefinition.java +++ b/study/src/org/labkey/study/model/DatasetDefinition.java @@ -2799,7 +2799,10 @@ public void deleteDatasetRows(User u, Collection lsids, boolean isBulkLo deleteProvenance(getContainer(), u, lsids); deleteRows(lsids); - new DatasetAuditHandler(this).addAuditEvent(u, getContainer(), getTableInfo(u), isBulkLoad ? AuditBehaviorType.SUMMARY : 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(); }