diff --git a/api/src/org/labkey/api/audit/AbstractAuditHandler.java b/api/src/org/labkey/api/audit/AbstractAuditHandler.java index 801ba46957b..cc9211bb257 100644 --- a/api/src/org/labkey/api/audit/AbstractAuditHandler.java +++ b/api/src/org/labkey/api/audit/AbstractAuditHandler.java @@ -26,14 +26,19 @@ import java.util.ArrayList; import java.util.Collections; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; import static org.labkey.api.gwt.client.AuditBehaviorType.SUMMARY; public abstract class AbstractAuditHandler implements AuditHandler { - protected abstract AuditTypeEvent createSummaryAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, int rowCount, @Nullable Map row); + /** Bounds both the JDBC batch and the events held in memory while a batch accumulates. */ + public static final int AUDIT_BATCH_SIZE = 2500; + + protected abstract AuditTypeEvent createSummaryAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, int rowCount, @Nullable Map row, List sideEffectEvents); @Override public void addSummaryAuditEvent(User user, Container c, TableInfo table, QueryService.AuditAction action, Integer dataRowCount, @Nullable AuditBehaviorType auditBehaviorType, @Nullable String userComment) @@ -51,9 +56,11 @@ public void addSummaryAuditEvent(User user, Container c, TableInfo table, QueryS if (auditType == SUMMARY || skipAuditLevelCheck) { - AuditTypeEvent event = createSummaryAuditRecord(user, c, auditConfigurable, action, userComment, dataRowCount, null); + List sideEffectEvents = new ArrayList<>(); + AuditTypeEvent event = createSummaryAuditRecord(user, c, auditConfigurable, action, userComment, dataRowCount, null, sideEffectEvents); AuditLogService.get().addEvent(user, event); + addSideEffectEvents(AuditLogService.get(), user, sideEffectEvents, false); } } } @@ -68,9 +75,10 @@ public void addSummaryAuditEvent(User user, Container c, TableInfo table, QueryS * @param row map of new data values * @param existingRow map of data values * @param providedValues map of values provided by the user before conversion (e.g., for quantity values) + * @param sideEffectEvents collects audit events raised as a side effect of building this record, for the caller to flush through {@link #addSideEffectEvents} * @return DetailedAuditTypeEvent object describing audit record (NOTE: not committed to DB yet) */ - protected abstract DetailedAuditTypeEvent createDetailedAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, @Nullable Map row, Map existingRow, Map providedValues); + protected abstract DetailedAuditTypeEvent createDetailedAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, @Nullable Map row, Map existingRow, Map providedValues, List sideEffectEvents); /** * Allow for adding fields that may be present in the updated row but not represented in the original row @@ -103,8 +111,10 @@ public void addAuditEvent(User user, Container c, TableInfo table, @Nullable Aud case SUMMARY: case DETAILED: - AuditTypeEvent event = createSummaryAuditRecord(user, c, auditConfigurable, action, userComment, 0, null); + List truncateSideEffects = new ArrayList<>(); + AuditTypeEvent event = createSummaryAuditRecord(user, c, auditConfigurable, action, userComment, 0, null, truncateSideEffects); AuditLogService.get().addEvent(user, event); + addSideEffectEvents(AuditLogService.get(), user, truncateSideEffects, useTransactionAuditCache); return; } } @@ -118,9 +128,11 @@ public void addAuditEvent(User user, Container c, TableInfo table, @Nullable Aud { assert null != rows; - AuditTypeEvent event = createSummaryAuditRecord(user, c, auditConfigurable, action, userComment, rows.size(), rows.getFirst()); + List sideEffectEvents = new ArrayList<>(); + AuditTypeEvent event = createSummaryAuditRecord(user, c, auditConfigurable, action, userComment, rows.size(), rows.getFirst(), sideEffectEvents); AuditLogService.get().addEvent(user, event); + addSideEffectEvents(AuditLogService.get(), user, sideEffectEvents, useTransactionAuditCache); return; } @@ -130,13 +142,14 @@ public void addAuditEvent(User user, Container c, TableInfo table, @Nullable Aud AuditLogService auditLog = AuditLogService.get(); List batch = new ArrayList<>(); + List sideEffectEvents = new ArrayList<>(); for (int i=0; i < rows.size(); i++) { Map row = rows.get(i); Map existingRow = null == existingRows ? Collections.emptyMap() : existingRows.get(i); Map providedValueRow = null == providedValues || providedValues.size() <= i ? null : providedValues.get(i); - DetailedAuditTypeEvent event = createDetailedAuditRecord(user, c, auditConfigurable, action, userComment, row, existingRow, providedValueRow); + DetailedAuditTypeEvent event = createDetailedAuditRecord(user, c, auditConfigurable, action, userComment, row, existingRow, providedValueRow, sideEffectEvents); switch (action) { @@ -175,23 +188,43 @@ public void addAuditEvent(User user, Container c, TableInfo table, @Nullable Aud } } batch.add(event); - if (batch.size() > 1000) + if (batch.size() >= AUDIT_BATCH_SIZE) { auditLog.addEvents(user, batch, useTransactionAuditCache); batch.clear(); } + // a row can contribute more than one side effect, so bound these separately from the row batch + if (sideEffectEvents.size() >= AUDIT_BATCH_SIZE) + { + addSideEffectEvents(auditLog, user, sideEffectEvents, useTransactionAuditCache); + sideEffectEvents.clear(); + } } if (!batch.isEmpty()) { auditLog.addEvents(user, batch, useTransactionAuditCache); batch.clear(); } + addSideEffectEvents(auditLog, user, sideEffectEvents, useTransactionAuditCache); break; } } } } + /** + * The one place side-effect events are written, so they can't pick up different batching or transaction-cache + * behavior depending on which call path produced them. insertEvents() batches only a fully homogeneous list, so + * group by event type and container -- each type is stored in its own provisioned table. + */ + public static void addSideEffectEvents(AuditLogService auditLog, User user, List events, boolean useTransactionAuditCache) + { + events.stream() + .collect(Collectors.groupingBy(event -> Pair.of(event.getEventType(), event.getContainer()), LinkedHashMap::new, Collectors.toList())) + .values() + .forEach(group -> auditLog.addEvents(user, group, useTransactionAuditCache)); + } + private void setOldAndNewMapsForUpdate(DetailedAuditTypeEvent event, Container c, Map row, Map existingRow, TableInfo table) { Pair, Map> rowPair = AuditHandler.getOldAndNewRecordForMerge(row, existingRow, table.getExtraDetailedUpdateAuditFields(), table.getExcludedDetailedUpdateAuditFields(), table); diff --git a/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java b/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java index d749b2c1b5f..acefaa116c3 100644 --- a/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java +++ b/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java @@ -31,6 +31,7 @@ import org.labkey.api.data.DbScope; import org.labkey.api.data.MultiChoice; import org.labkey.api.data.MutableColumnInfo; +import org.labkey.api.data.SchemaTableInfo; import org.labkey.api.data.Table; import org.labkey.api.data.TableInfo; import org.labkey.api.dataiterator.DataIterator; @@ -85,6 +86,9 @@ public abstract class AbstractAuditTypeProvider implements AuditTypeProvider private final AbstractAuditDomainKind _domainKind; + private record CachedStorageTable(SchemaTableInfo schemaTableInfo, TableInfo storageTableInfo) {} + private volatile CachedStorageTable _cachedStorageTable; + public AbstractAuditTypeProvider(@NotNull AbstractAuditDomainKind domainKind) { // TODO: consolidate domain kind initialization to this constructor and stop overriding getDomainKind() @@ -266,6 +270,27 @@ public TableInfo createStorageTableInfo() return StorageProvisioner.createTableInfo(domain); } + @Override @NotNull + public TableInfo getStorageTableInfoForInsert() + { + Domain domain = getDomain(); + if (null == domain) + throw new IllegalStateException("Could not find domain for audit event type " + getEventName()); + + // We want to reuse a cached provisioned TableInfo to avoid construction costs. Getting the SchemaTableInfo is + // cheap so use that as a guide for when the table has changed (primarily during startup as its shape may + // need to be updated based on current code expectations) and when it's safe to reuse the previous copy. + SchemaTableInfo schemaTableInfo = StorageProvisioner.get().getSchemaTableInfo(domain); + CachedStorageTable cached = _cachedStorageTable; + if (null != cached && cached.schemaTableInfo() == schemaTableInfo) + return cached.storageTableInfo(); + + TableInfo storageTableInfo = StorageProvisioner.createSharedTableInfo(domain); + _cachedStorageTable = new CachedStorageTable(schemaTableInfo, storageTableInfo); + + return storageTableInfo; + } + @Override public TableInfo createTableInfo(UserSchema userSchema, ContainerFilter cf) { diff --git a/api/src/org/labkey/api/audit/AuditTypeProvider.java b/api/src/org/labkey/api/audit/AuditTypeProvider.java index 8cf1711c363..26df2504583 100644 --- a/api/src/org/labkey/api/audit/AuditTypeProvider.java +++ b/api/src/org/labkey/api/audit/AuditTypeProvider.java @@ -15,6 +15,7 @@ */ package org.labkey.api.audit; +import org.jetbrains.annotations.NotNull; import org.labkey.api.data.ContainerFilter; import org.labkey.api.data.TableInfo; import org.labkey.api.exp.property.Domain; @@ -43,6 +44,14 @@ public interface AuditTypeProvider TableInfo createTableInfo(UserSchema schema, ContainerFilter cf); + /** + * Provisioned storage TableInfo for the insert path. Implementations should cache - creating TableInfos is expensive. + * Never use this to read audit rows: it has no ContainerFilter and bypasses the CanSeeAuditLog check that + * {@link org.labkey.api.audit.query.DefaultAuditTypeTable} applies. Go through {@link #createTableInfo} for reads. + */ + @NotNull + TableInfo getStorageTableInfoForInsert(); + Class getEventClass(); ActionURL getAuditUrl(); diff --git a/api/src/org/labkey/api/data/MaterializedQueryHelper.java b/api/src/org/labkey/api/data/MaterializedQueryHelper.java index 39262aba186..a88240a71ba 100644 --- a/api/src/org/labkey/api/data/MaterializedQueryHelper.java +++ b/api/src/org/labkey/api/data/MaterializedQueryHelper.java @@ -183,15 +183,18 @@ boolean load(SQLFragment selectQuery, boolean isSelectInto) try (var ignored = SpringActionController.ignoreSqlUpdates()) { - for (String index : _mqh._indexes) - { - new SqlExecutor(_mqh._scope).execute(StringUtils.replace(index, "${NAME}", _tableName)); - } + createIndexes(_mqh._indexes, false); analyze(); } }); + // Published here, not after the deferred indexes: those only make the table faster, and building them + // first keeps every reader on the unmaterialized query for the length of the slowest index. _loadingState.set(LoadingState.LOADED); + + if (!_mqh._deferredIndexes.isEmpty()) + traced("full.deferredIndexes", _mqh.getMaterializationName(), () -> createIndexes(_mqh._deferredIndexes, true)); + return true; } catch (RuntimeException rex) @@ -206,6 +209,28 @@ boolean load(SQLFragment selectQuery, boolean isSelectInto) } } + /** + * @param tolerateFailure true once the table is serving reads, where a missing index costs speed but nothing else, + * so the failure is logged and the remaining indexes are still attempted + */ + private void createIndexes(List indexes, boolean tolerateFailure) + { + for (String index : indexes) + { + String sql = StringUtils.replace(index, "${NAME}", _tableName); + try + { + new SqlExecutor(_mqh._scope).execute(sql); + } + catch (RuntimeException x) + { + if (!tolerateFailure) + throw x; + LOG.error("Failed to create index on materialized table {}. The table is serving queries without it. DDL: {}", _tableName, sql, x); + } + } + } + /** SELECT INTO leaves the table with no statistics, so the planner guesses until autovacuum eventually analyzes it. */ private void analyze() { @@ -389,6 +414,7 @@ private String makeKey(DbScope.Transaction t) protected final SQLFragment _uptodateQuery; protected final Supplier _supplier; private final List _indexes = new ArrayList<>(); + private final List _deferredIndexes = new ArrayList<>(); protected final long _maxTimeToCache; private final Map _map = Collections.synchronizedMap(new LinkedHashMap<>() { @@ -410,7 +436,7 @@ protected boolean removeEldestEntry(Map.Entry eldest) private boolean _closed = false; - protected MaterializedQueryHelper(String prefix, DbScope scope, SQLFragment select, @Nullable SQLFragment uptodate, Supplier supplier, @Nullable Collection indexes, long maxTimeToCache, + protected MaterializedQueryHelper(String prefix, DbScope scope, SQLFragment select, @Nullable SQLFragment uptodate, Supplier supplier, @Nullable Collection indexes, @Nullable Collection deferredIndexes, long maxTimeToCache, boolean isSelectIntoSql, boolean unlogged) { _prefix = Objects.toString(prefix,"mat"); @@ -421,6 +447,8 @@ protected MaterializedQueryHelper(String prefix, DbScope scope, SQLFragment sele _maxTimeToCache = maxTimeToCache; if (null != indexes) _indexes.addAll(indexes); + if (null != deferredIndexes) + _deferredIndexes.addAll(deferredIndexes); _isSelectIntoSql = isSelectIntoSql; _unlogged = unlogged; assert MemTracker.get().put(this); @@ -769,14 +797,14 @@ protected void initMaterialized(Materialized materialized) @Deprecated // use Builder public static MaterializedQueryHelper create(String prefix, DbScope scope, SQLFragment select, @Nullable SQLFragment uptodate, Collection indexes, long maxTimeToCache) { - return new MaterializedQueryHelper(prefix, scope, select, uptodate, null, indexes, maxTimeToCache, false, false); + return new MaterializedQueryHelper(prefix, scope, select, uptodate, null, indexes, null, maxTimeToCache, false, false); } @Deprecated // use Builder public static MaterializedQueryHelper create(String prefix, DbScope scope, SQLFragment select, Supplier uptodate, Collection indexes, long maxTimeToCache) { - return new MaterializedQueryHelper(prefix, scope, select, null, uptodate, indexes, maxTimeToCache, false, false); + return new MaterializedQueryHelper(prefix, scope, select, null, uptodate, indexes, null, maxTimeToCache, false, false); } public static class Builder implements org.labkey.api.data.Builder @@ -791,6 +819,7 @@ public static class Builder implements org.labkey.api.data.Builder _supplier = null; protected Collection _indexes = new ArrayList<>(); + protected Collection _deferredIndexes = new ArrayList<>(); public Builder(String prefix, DbScope scope, SQLFragment select) { @@ -834,16 +863,24 @@ public Builder addInvalidCheck(Supplier supplier) return this; } + /** Index built before the table is published, for anything a reader or the incremental maintenance SQL cannot go without. */ public Builder addIndex(String index) { _indexes.add(index); return this; } + /** Index built after the table is published to readers; a failure is logged and the table serves without it. */ + public Builder addDeferredIndex(String index) + { + _deferredIndexes.add(index); + return this; + } + @Override public MaterializedQueryHelper build() { - return new MaterializedQueryHelper(_prefix, _scope, _select, _uptodate, _supplier, _indexes, _max, _isSelectInto, _unlogged); + return new MaterializedQueryHelper(_prefix, _scope, _select, _uptodate, _supplier, _indexes, _deferredIndexes, _max, _isSelectInto, _unlogged); } } diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index 3d3fe1528bf..b17fc536299 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -220,7 +220,6 @@ public RemapConverter(@NotNull TableInfo targetTable, boolean includeTitleColumn public void setIncludePkLookup(boolean includePkLookup) { _includePkLookup = includePkLookup; - _maps = null; } public ColumnInfo getPkColumn() @@ -228,6 +227,17 @@ public ColumnInfo getPkColumn() return _targetTable.getPkColumns().getFirst(); } + private Pair> pkLookupMap() + { + if (!_includePkLookup) + return null; + + if (_pkColumnLookupMap == null) + _pkColumnLookupMap = Pair.of(getPkColumn(), new HashMap<>()); + + return _pkColumnLookupMap; + } + private List>> getMaps() { if (_maps == null) @@ -271,11 +281,6 @@ public ColumnInfo getPkColumn() _titleColumnLookupMap = Triple.of(pkCol, titleColumn, new ArrayListValuedHashMap()); } } - - if (_includePkLookup) - { - _pkColumnLookupMap = Pair.of(pkCol, new HashMap<>()); - } } return _maps; } @@ -291,9 +296,10 @@ public Object mappedValue(Object k) List>> maps = getMaps(); - if (_pkColumnLookupMap != null) + Pair> pkLookupMap = pkLookupMap(); + if (pkLookupMap != null) { - Object v = fetch(_pkColumnLookupMap, k); + Object v = fetch(pkLookupMap, k); if (v != null) return v; } @@ -2233,6 +2239,97 @@ public void convertRemapTest() throws Exception } + /** Lookup fixture with one text alternate key (Value) over an integer pk (RowId); Ordinal is unique but not text, so it yields no map. */ + private EnumTableInfo remapLookupTable() + { + var core = QueryService.get().getUserSchema(TestContext.get().getUser(), JunitUtil.getTestContainer(), "core"); + return new EnumTableInfo<>(LookupValues.class, core, "fake enum", true); + } + + @Test + public void remapCacheSurvivesPkLookupToggle() + { + RemapConverter converter = new RemapConverter(remapLookupTable(), true, false, true); + + // RemappingConvertColumn flips this before every row, so it must not discard what earlier rows resolved + converter.setIncludePkLookup(false); + + List>> maps = converter.getMaps(); + assertEquals("expected one alternate-key map, on the Value column", 1, maps.size()); + + // Seed keys no enum value can supply, so anything but a cache hit resolves to null + MultiValuedMap cache = maps.getFirst().getRight(); + Integer seeded = 42; + cache.put("seeded-hit", seeded); + cache.put("seeded-miss", converter.MISS); + + assertEquals(seeded, converter.mappedValue("seeded-hit")); + assertNull(converter.mappedValue("seeded-miss")); + + for (int i = 0; i < 3; i++) + { + converter.setIncludePkLookup(true); + converter.setIncludePkLookup(false); + } + + assertSame("toggling includePkLookup discarded the cached lookups", maps, converter.getMaps()); + assertEquals("resolved value was discarded, so every row re-queries it", seeded, converter.mappedValue("seeded-hit")); + assertNull("MISS marker was discarded, so every row re-queries the absent value", converter.mappedValue("seeded-miss")); + } + + @Test + public void remapResolutionIsStableAcrossPkLookupToggle() + { + RemapConverter converter = new RemapConverter(remapLookupTable(), true, false, true); + converter.setIncludePkLookup(false); + + Object resolved = converter.mappedValue(LookupValues.Two.name()); + assertNotNull("expected " + LookupValues.Two + " to resolve by alternate key", resolved); + + converter.setIncludePkLookup(true); + converter.setIncludePkLookup(false); + + assertEquals(resolved, converter.mappedValue(LookupValues.Two.name())); + } + + @Test + public void remapAlternateKeyWinsWhenPkLookupIsOff() + { + RemapConverter converter = new RemapConverter(remapLookupTable(), true, false, true); + + // Seed the two maps to disagree on one key, so the resolved value says which map was consulted + Integer key = 7; + Integer pkResolution = 7; + Integer akResolution = 99; + Map pkCache = converter.pkLookupMap().getValue(); + MultiValuedMap akCache = converter.getMaps().getFirst().getRight(); + pkCache.put(key, pkResolution); + akCache.put(key, akResolution); + + assertEquals("pk lookup should take precedence while includePkLookup is on", pkResolution, converter.mappedValue(key)); + + // The pk map survives the toggle, so this also pins that it is not consulted while the flag is off + converter.setIncludePkLookup(false); + assertEquals("alternate key should resolve while includePkLookup is off", akResolution, converter.mappedValue(key)); + } + + @Test + public void remapPkLookupMapIsRetained() + { + RemapConverter converter = new RemapConverter(remapLookupTable(), true, false, false); + assertNull("pk lookup map should not exist while includePkLookup is off", converter.pkLookupMap()); + + converter.setIncludePkLookup(true); + Pair> pkMap = converter.pkLookupMap(); + assertNotNull(pkMap); + + converter.setIncludePkLookup(false); + assertNull(converter.pkLookupMap()); + + converter.setIncludePkLookup(true); + assertSame("pk lookup map was rebuilt rather than retained", pkMap, converter.pkLookupMap()); + } + @Test public void getFileRootSubstitutedFilePathTest() { diff --git a/api/src/org/labkey/api/exp/api/SampleTypeService.java b/api/src/org/labkey/api/exp/api/SampleTypeService.java index cd12c102669..e2ddd82f266 100644 --- a/api/src/org/labkey/api/exp/api/SampleTypeService.java +++ b/api/src/org/labkey/api/exp/api/SampleTypeService.java @@ -18,6 +18,7 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.labkey.api.audit.SampleTimelineAuditEvent; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerFilter; import org.labkey.api.data.DbSequence; @@ -249,6 +250,9 @@ default Map incrementSampleCounts(@Nullable Date counterDate) void addAuditEvents(User user, Container container, String comment, String userComment, Collection samples, Map metadata); + /** Builds the same event addAuditEvent() would write, for callers on row-scaling paths that need to batch the inserts themselves. */ + SampleTimelineAuditEvent createTimelineAuditRecord(Container container, String comment, String userComment, ExpMaterial sample, Map metadata, String updateType); + // find the max sequence number with '${sampleName}-' prefix long getMaxAliquotId(@NotNull String sampleName, @NotNull String sampleTypeLsid, Container container); diff --git a/api/src/org/labkey/api/exp/api/StorageProvisioner.java b/api/src/org/labkey/api/exp/api/StorageProvisioner.java index 1c0494ef7b7..c59be93bea4 100644 --- a/api/src/org/labkey/api/exp/api/StorageProvisioner.java +++ b/api/src/org/labkey/api/exp/api/StorageProvisioner.java @@ -71,6 +71,20 @@ static TableInfo createTableInfo(@NotNull Domain domain) /* NOTE: static createTable/createTableImpl is a very minor hack to avoid having to update a zillions repos at once */ TableInfo createTableInfoImpl(@NotNull Domain domain); + /** + * Variant of {@link #createTableInfo} for callers that cache the result for the process lifetime. The table is + * locked for cross-thread sharing, and it plus the DomainDescriptor it retains are dropped from MemTracker, which + * would otherwise report both as leaks. + */ + @NotNull + static TableInfo createSharedTableInfo(@NotNull Domain domain) + { + return get().createSharedTableInfoImpl(domain); + } + + @NotNull + TableInfo createSharedTableInfoImpl(@NotNull Domain domain); + /** * This is really an internal method, use createTableInfo() in most scenarios * This is public to support upgrade scenarios only. @@ -86,6 +100,7 @@ static TableInfo createTableInfo(@NotNull Domain domain) void ensureTableIndices(@NotNull Domain domain); void ensureTableIndices(@NotNull Domain domain, Supplier afterAddSupplier); + @NotNull SchemaTableInfo getSchemaTableInfo(Domain domain); /** diff --git a/audit/src/org/labkey/audit/AuditLogImpl.java b/audit/src/org/labkey/audit/AuditLogImpl.java index ac0364a9be8..6b87feda48a 100644 --- a/audit/src/org/labkey/audit/AuditLogImpl.java +++ b/audit/src/org/labkey/audit/AuditLogImpl.java @@ -20,6 +20,11 @@ import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.Nullable; +import org.junit.AfterClass; +import org.junit.Assert; +import org.junit.Assume; +import org.junit.BeforeClass; +import org.junit.Test; import org.labkey.api.action.SpringActionController; import org.labkey.api.audit.AbstractAuditTypeProvider; import org.labkey.api.audit.AuditLogService; @@ -27,6 +32,7 @@ import org.labkey.api.audit.AuditTypeProvider; import org.labkey.api.audit.DetailedAuditTypeEvent; import org.labkey.api.audit.SampleTimelineAuditEvent; +import org.labkey.api.audit.permissions.CanSeeAuditLogPermission; import org.labkey.api.cache.Cache; import org.labkey.api.cache.CacheManager; import org.labkey.api.collections.CaseInsensitiveHashMap; @@ -37,16 +43,27 @@ import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.Sort; import org.labkey.api.data.TableSelector; +import org.labkey.api.data.WorkbookContainerType; import org.labkey.api.exp.api.ExperimentService; import org.labkey.api.module.ModuleLoader; +import org.labkey.api.query.AbstractQueryUpdateService; import org.labkey.api.query.FieldKey; import org.labkey.api.query.QueryService; import org.labkey.api.query.UserSchema; +import org.labkey.api.security.LimitedUser; +import org.labkey.api.security.SecurityManager; import org.labkey.api.security.User; import org.labkey.api.security.UserManager; +import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.roles.CanSeeAuditLogRole; +import org.labkey.api.security.roles.ReaderRole; +import org.labkey.api.security.roles.Role; +import org.labkey.api.security.roles.RoleManager; import org.labkey.api.util.ContextListener; +import org.labkey.api.util.GUID; import org.labkey.api.util.Pair; import org.labkey.api.util.StartupListener; +import org.labkey.api.util.TestContext; import org.labkey.api.util.logging.LogHelper; import org.labkey.api.view.ActionURL; import org.labkey.api.view.HttpView; @@ -54,13 +71,17 @@ import org.labkey.audit.query.AuditQuerySchema; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Queue; +import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Predicate; import java.util.stream.Collectors; public class AuditLogImpl implements AuditLogService, StartupListener @@ -248,6 +269,48 @@ public ActionURL getAuditUrl() return new ActionURL(AuditController.ShowAuditLogAction.class, ContainerManager.getRoot()); } + /** + * Mirrors {@link ContainerFilter}'s SQL: an event matches on its own container, or on being a child of an in-scope + * container whose type the filter includes. Cached events need this applied by hand -- reading them back from the + * database is what would otherwise apply it. + */ + private static Predicate inScope(User user, Container container, @Nullable ContainerFilter containerFilter) + { + ContainerFilter cf = null == containerFilter ? ContainerFilter.current(container, user) : containerFilter; + Collection ids = scopeIds(user, container, cf); + if (null == ids) + return _ -> true; + + // Ensure an O(1) lookup + Set scope = new HashSet<>(ids); + Set childTypes = cf.getIncludedChildTypes(); + + return event -> { + Container c = event.getContainer(); + if (null == c) + return false; + if (scope.contains(c.getEntityId())) + return true; + Container parent = c.getParent(); + return null != parent && childTypes.contains(c.getType()) && scope.contains(parent.getEntityId()); + }; + } + + /** + * Scopes by CanSeeAuditLogPermission the way {@link org.labkey.api.audit.query.DefaultAuditTypeTable} does, not by + * the ReadPermission that {@link ContainerFilter#getIds()} applies, so a folder the user can read but whose audit + * log they cannot see stays out of scope. + */ + private static @Nullable Collection scopeIds(User user, Container container, ContainerFilter cf) + { + if (!(cf instanceof ContainerFilter.ContainerFilterWithPermission cfp)) + return cf.getIds(); + + Set roles = SecurityManager.canSeeAuditLog(user) ? RoleManager.roleSet(CanSeeAuditLogRole.class) : null; + + return cfp.generateIds(container, CanSeeAuditLogPermission.class, roles); + } + public record TransactionRowIds(List rowIds, Map dataTypeRowCounts) {} public TransactionRowIds getTransactionSampleIds(long transactionAuditId, User user, Container container, @Nullable ContainerFilter containerFilter) @@ -265,6 +328,7 @@ public TransactionRowIds getTransactionSampleIds(long transactionAuditId, User u events = transactionEvents.stream() .filter(SampleTimelineAuditEvent.class::isInstance) .map(SampleTimelineAuditEvent.class::cast) + .filter(inScope(user, container, containerFilter)) .toList(); } Map dataTypeRowCounts = new HashMap<>(); @@ -287,6 +351,7 @@ public TransactionRowIds getTransactionSourceIds(long transactionAuditId, User u : transactionEvents.stream() .filter(DetailedAuditTypeEvent.class::isInstance) .map(DetailedAuditTypeEvent.class::cast) + .filter(inScope(user, container, containerFilter)) .toList(); detailedEvents.forEach(event -> { @@ -314,4 +379,134 @@ else if (newRecord.containsKey("LSID") && !StringUtils.isEmpty(newRecord.get("LS } return new TransactionRowIds(sourceIds, dataTypeRowCounts); } + + /** + * {@link #inScope} reproduces {@link org.labkey.api.audit.query.DefaultAuditTypeTable}'s container filtering in + * memory, so the branch that reads cached transaction events has to agree with the branch that reads them back from + * the database. Two cases separate the two: a workbook, which is in scope only through its parent via + * {@link ContainerFilter#getIncludedChildTypes()}, and a user who can read a folder but not its audit log. + */ + public static class TransactionScopeTestCase extends Assert + { + private static final String PROJECT_NAME = "AuditTransactionScopeTest Project"; + private static final long SAMPLE_TYPE_ID = 4242; + + private static User _user; + private static Container _project; + private static Container _workbook; + private static Container _subfolder; + + @BeforeClass + public static void setup() + { + Assume.assumeTrue("The SampleTimelineEvent provider is registered by the experiment module", + null != AuditLogService.get().getAuditProvider(SampleTimelineAuditEvent.EVENT_TYPE)); + + _user = TestContext.get().getUser(); + + deleteTestContainer(); + _project = ContainerManager.createContainer(ContainerManager.getRoot(), PROJECT_NAME, _user); + _workbook = ContainerManager.createContainer(_project, null, "Workbook", null, WorkbookContainerType.NAME, _user); + _subfolder = ContainerManager.createContainer(_project, "Subfolder", _user); + } + + @AfterClass + public static void cleanup() + { + _subfolder = null; + _workbook = null; + _project = null; + _user = null; + + deleteTestContainer(); + } + + private static void deleteTestContainer() + { + Container project = ContainerManager.getForPath(PROJECT_NAME); + + if (null != project) + ContainerManager.deleteAll(project, TestContext.get().getUser()); + } + + /** Unique per call, and from the sequence production draws transaction ids from. */ + private static long newTransactionId() + { + return AbstractQueryUpdateService.createTransactionAuditEvent(_project, QueryService.AuditAction.INSERT).getRowId(); + } + + private static SampleTimelineAuditEvent timelineEvent(Container c, long transactionId, long sampleId) + { + SampleTimelineAuditEvent event = new SampleTimelineAuditEvent(c, "Sample inserted"); + event.setTransactionId(transactionId); + event.setSampleId(sampleId); + event.setSampleTypeId(SAMPLE_TYPE_ID); + + return event; + } + + /** One event per container, all in one transaction. Written with the cache on, so both branches see them. */ + private static long writeEvents(User user, long projectSampleId, long workbookSampleId, long subfolderSampleId) + { + long transactionId = newTransactionId(); + + AuditLogService.get().addEvents(user, List.of( + timelineEvent(_project, transactionId, projectSampleId), + timelineEvent(_workbook, transactionId, workbookSampleId), + timelineEvent(_subfolder, transactionId, subfolderSampleId)), true); + + assertFalse("expected addEvents() to populate the transaction event cache", + TRANSACTION_EVENT_CACHE.get(transactionId).second.isEmpty()); + + return transactionId; + } + + /** Emptying the cache entry is what sends getTransactionSampleIds() to the database. */ + private static TransactionRowIds fromDatabase(long transactionId, User user) + { + TRANSACTION_EVENT_CACHE.get(transactionId).second.clear(); + + return get().getTransactionSampleIds(transactionId, user, _project, null); + } + + @Test + public void workbookIsInScopeForBothBranches() + { + long transactionId = writeEvents(_user, 101, 102, 103); + + TransactionRowIds cached = get().getTransactionSampleIds(transactionId, _user, _project, null); + TransactionRowIds database = fromDatabase(transactionId, _user); + + assertEquals("the workbook is in scope through its parent, the subfolder is not in scope at all", + Set.of(101L, 102L), Set.copyOf(cached.rowIds())); + assertEquals("cached and database branches disagreed on sample ids", + Set.copyOf(cached.rowIds()), Set.copyOf(database.rowIds())); + assertEquals("cached and database branches disagreed on row counts", + Map.of(SAMPLE_TYPE_ID, 2L), cached.dataTypeRowCounts()); + assertEquals("cached and database branches disagreed on row counts", + cached.dataTypeRowCounts(), database.dataTypeRowCounts()); + } + + @Test + public void auditLogPermissionIsAppliedByBothBranches() + { + // Read alone is not enough to see audit events; the production caller elevates with CanSeeAuditLogRole first + User reader = new LimitedUser(_user, ReaderRole.class); + assertTrue("expected the reader to be able to read the project", _project.hasPermission(reader, ReadPermission.class)); + assertFalse("expected the reader to lack the audit log permission", _project.hasPermission(reader, CanSeeAuditLogPermission.class)); + + long transactionId = writeEvents(_user, 201, 202, 203); + + TransactionRowIds cached = get().getTransactionSampleIds(transactionId, reader, _project, null); + TransactionRowIds database = fromDatabase(transactionId, reader); + + assertEquals("a reader without the audit log permission should see no events", List.of(), cached.rowIds()); + assertEquals("cached and database branches disagreed on sample ids", cached.rowIds(), database.rowIds()); + + // The same events are visible to a user who can see the audit log, so the empty results above are the + // permission check and not a fixture that failed to write + assertEquals("expected the admin to see the events the reader could not", + Set.of(201L, 202L), Set.copyOf(get().getTransactionSampleIds(transactionId, _user, _project, null).rowIds())); + } + } } diff --git a/audit/src/org/labkey/audit/AuditModule.java b/audit/src/org/labkey/audit/AuditModule.java index 99f7572b177..15885d1f5e3 100644 --- a/audit/src/org/labkey/audit/AuditModule.java +++ b/audit/src/org/labkey/audit/AuditModule.java @@ -84,6 +84,14 @@ public void doStartup(ModuleContext moduleContext) AuditController.registerAdminConsoleLinks(); } + @Override + public @NotNull Set> getIntegrationTests() + { + return Set.of( + AuditLogImpl.TransactionScopeTestCase.class + ); + } + @Override @NotNull public Set getSchemaNames() diff --git a/audit/src/org/labkey/audit/model/LogManager.java b/audit/src/org/labkey/audit/model/LogManager.java index 5349ade7eb8..7ba1c0f64ab 100644 --- a/audit/src/org/labkey/audit/model/LogManager.java +++ b/audit/src/org/labkey/audit/model/LogManager.java @@ -22,11 +22,9 @@ import org.labkey.api.audit.AuditLogService; import org.labkey.api.audit.AuditTypeEvent; import org.labkey.api.audit.AuditTypeProvider; -import org.labkey.api.audit.query.DefaultAuditTypeTable; import org.labkey.api.collections.CaseInsensitiveHashMap; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerFilter; -import org.labkey.api.data.ContainerManager; import org.labkey.api.data.DbSchema; import org.labkey.api.data.ObjectFactory; import org.labkey.api.data.ParameterMapStatement; @@ -81,23 +79,9 @@ public K insertEvent(User user, K type) if (provider != null) { - Container c = type.getContainer(); - - UserSchema schema = AuditLogService.getAuditLogSchema(user, c != null ? c : ContainerManager.getRoot()); - - if (schema != null) - { - TableInfo table = schema.getTable(provider.getEventName(), false); - - if (table instanceof DefaultAuditTypeTable auditTypeTable) - { - // consider using etl data iterator for inserts - type = validateFields(provider, type); - TableInfo dbTable = auditTypeTable.getRealTable(); - K ret = Table.insert(user, dbTable, type); - return ret; - } - } + // consider using etl data iterator for inserts + type = validateFields(provider, type); + return Table.insert(user, provider.getStorageTableInfoForInsert(), type); } return null; } @@ -142,33 +126,28 @@ public void insertEvents(User user, List events) if (null == provider) return; Container c = type.getContainer(); - UserSchema schema = AuditLogService.getAuditLogSchema(user, c != null ? c : ContainerManager.getRoot()); - TableInfo table = null==schema ? null : schema.getTable(provider.getEventName(), false); - TableInfo dbTable = table instanceof DefaultAuditTypeTable auditTypeTable ? auditTypeTable.getRealTable() : null; + TableInfo dbTable = provider.getStorageTableInfoForInsert(); Logger auditLogger = getAuditLogger(type); SQLException sqlx = null; - if (null != dbTable) + try (Connection conn = dbTable.getSchema().getScope().getConnection(); + ParameterMapStatement stmt = StatementUtils.insertStatement(conn, dbTable, c, user, false, true)) { - try (Connection conn = dbTable.getSchema().getScope().getConnection()) - { - ParameterMapStatement stmt = StatementUtils.insertStatement(conn, dbTable, c, user, false, true); - for (var event : events) - { - event = validateFields(provider, event); - Map map = ObjectFactory.Registry.getFactory((Class)event.getClass()).toMap(event, null); - stmt.clearParameters(); - stmt.putAll(map); - stmt.addBatch(); - } - stmt.executeBatch(); - } - catch (SQLException x) + for (var event : events) { - auditLogger.warn("Error occurred saving audit entries to database"); - sqlx = x; + event = validateFields(provider, event); + Map map = ObjectFactory.Registry.getFactory((Class)event.getClass()).toMap(event, null); + stmt.clearParameters(); + stmt.putAll(map); + stmt.addBatch(); } + stmt.executeBatch(); + } + catch (SQLException x) + { + auditLogger.warn("Error occurred saving audit entries to database"); + sqlx = x; } if (auditLogger.isInfoEnabled()) diff --git a/experiment/src/org/labkey/experiment/CustomPropertiesView.java b/experiment/src/org/labkey/experiment/CustomPropertiesView.java index 7c010a71e36..a8846b76098 100644 --- a/experiment/src/org/labkey/experiment/CustomPropertiesView.java +++ b/experiment/src/org/labkey/experiment/CustomPropertiesView.java @@ -27,8 +27,8 @@ import org.labkey.api.exp.PropertyType; import org.labkey.api.exp.property.Domain; import org.labkey.api.exp.property.DomainProperty; +import org.labkey.api.exp.query.ExpMaterialTable; import org.labkey.api.exp.query.SamplesSchema; -import org.labkey.api.query.FieldKey; import org.labkey.api.query.QueryService; import org.labkey.api.query.UserSchema; import org.labkey.api.security.User; @@ -155,7 +155,7 @@ public CustomPropertiesView(ExpMaterialImpl m, Container c, User u) propertyUris.add(property.getPropertyURI()); } - SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("lsid"), parentLSID); + SimpleFilter filter = new SimpleFilter(ExpMaterialTable.Column.RowId.fieldKey(), m.getRowId()); Map tableProps = new TableSelector(queryTable, filter, null).getMap(); // include calculated fields from the domain / query as well List cols = queryTable.getColumns().stream() diff --git a/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java b/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java index 29b7b3768ca..6ff9ce9f4f0 100644 --- a/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java @@ -25,6 +25,7 @@ import org.jetbrains.annotations.Nullable; import org.junit.AfterClass; import org.junit.Assert; +import org.junit.Assume; import org.junit.BeforeClass; import org.junit.Test; import org.labkey.api.assay.plate.AssayPlateMetadataService; @@ -52,6 +53,7 @@ import org.labkey.api.data.MaterializedQueryHelper; import org.labkey.api.data.MutableColumnInfo; import org.labkey.api.data.PHI; +import org.labkey.api.data.PropertyStorageSpec; import org.labkey.api.data.RenderContext; import org.labkey.api.data.SQLFragment; import org.labkey.api.data.Sort; @@ -78,6 +80,7 @@ import org.labkey.api.exp.api.StorageProvisioner; import org.labkey.api.exp.property.DefaultPropertyValidator; import org.labkey.api.exp.property.Domain; +import org.labkey.api.exp.property.DomainKind; import org.labkey.api.exp.property.DomainProperty; import org.labkey.api.exp.property.DomainUtil; import org.labkey.api.exp.property.IPropertyValidator; @@ -88,6 +91,7 @@ import org.labkey.api.exp.query.ExpSchema; import org.labkey.api.exp.query.SamplesSchema; import org.labkey.api.gwt.client.AuditBehaviorType; +import org.labkey.api.gwt.client.model.GWTIndex; import org.labkey.api.gwt.client.model.GWTPropertyDescriptor; import org.labkey.api.gwt.client.model.PropertyValidatorType; import org.labkey.api.inventory.InventoryService; @@ -146,6 +150,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Set; import java.util.TreeSet; @@ -1260,7 +1265,7 @@ void recordPendingUpdate(@NotNull Timestamp changedSince) } } - static final BlockingCache _materializedQueries = CacheManager.getBlockingStringKeyCache(CacheManager.UNLIMITED, 12 * CacheManager.HOUR, "materialized sample types", null); + static final BlockingCache _materializedQueries = CacheManager.getBlockingStringKeyCache(CacheManager.UNLIMITED, 8 * CacheManager.DAY, "materialized sample types", null); static final Map _invalidationCounters = Collections.synchronizedMap(new HashMap<>()); static final AtomicBoolean initializedListeners = new AtomicBoolean(false); @@ -1364,10 +1369,15 @@ private _MaterializedQueryHelper getOrCreateMQH() MaterializedQueryHelper.Builder builder = new _MaterializedQueryHelper.Builder(_ss.getLSID(), "", getExpSchema().getDbSchema().getScope(), viewSql) .updateColumns(updateColumns) .unlogged(true) + // RowId and Container are used in many places so ensure they're created before use. Other indices can + // be added as a followup step .addIndex("CREATE UNIQUE INDEX uq_${NAME}_rowid ON temp.${NAME} (rowid)") - .addIndex("CREATE UNIQUE INDEX uq_${NAME}_lsid ON temp.${NAME} (lsid)") .addIndex("CREATE INDEX idx_${NAME}_container ON temp.${NAME} (container)") - .addIndex("CREATE INDEX idx_${NAME}_root ON temp.${NAME} (rootmaterialrowid)"); + .addDeferredIndex("CREATE INDEX idx_${NAME}_root ON temp.${NAME} (rootmaterialrowid)") + // Deferred despite being UNIQUE. Source data guarantees uniqueness, and this is very expensive to build + .addDeferredIndex("CREATE UNIQUE INDEX uq_${NAME}_lsid ON temp.${NAME} (lsid)"); + + getDomainIndexDdl().forEach(builder::addDeferredIndex); if (isIncrementalUpdateDisabled()) builder.addInvalidCheck(() -> String.valueOf(getInvalidateCounters(_ss.getLSID()).update.get())); @@ -1376,6 +1386,75 @@ private _MaterializedQueryHelper getOrCreateMQH() }); } + /** + * DDL mirroring the sample type domain's admin-defined indices onto the materialized table. Read from the + * provisioned table's own indices, not {@link Domain#getPropertyIndices()} -- that set is only ever populated by + * the caller that is saving a domain, so it is empty on a domain read back from the database. Never unique: + * {@link #getJoinSQL} selects root-scoped columns from the root sample's row, so a value that is unique in the + * provisioned table repeats across every aliquot sharing that root. PostgreSQL only: SQL Server refuses an index + * over a column as wide as a default string property, so a mirrored index there would fail the materialization. + */ + private List getDomainIndexDdl() + { + if (!getExpSchema().getDbSchema().getSqlDialect().isPostgreSQL()) + return List.of(); + + TableInfo provisioned = _ss.getTinfo(); + if (null == provisioned) + return List.of(); + + Domain domain = _ss.getDomain(); + DomainKind kind = domain.getDomainKind(); + if (null == kind) + return List.of(); + + List ddl = new ArrayList<>(); + Set distinctColumnLists = new HashSet<>(); + + // Seeded with the provisioned columns the builder already indexes above: the kind's own indices (rowid, name) + // and lsid. Its other indices are over exp.material columns, which cannot appear in a provisioned index. + for (PropertyStorageSpec.Index index : kind.getPropertyIndices(domain)) + distinctColumnLists.add(indexKey(Arrays.asList(index.translateToStorageNames(domain).columnNames))); + distinctColumnLists.add(indexKey(List.of("lsid"))); + + for (TableInfo.IndexDefinition index : StorageProvisioner.get().getSchemaTableInfo(domain).getAllIndices()) + { + if (TableInfo.IndexType.Primary == index.indexType()) + continue; + + List names = new ArrayList<>(); + List identifiers = new ArrayList<>(); + + for (ColumnInfo indexColumn : index.columns()) + { + ColumnInfo column = provisioned.getColumn(indexColumn.getName()); + + if (null == column) + { + _log.warn("Sample type {} is indexed on {}, which is not in its provisioned table; that index is not mirrored onto the materialized table.", _ss.getName(), indexColumn.getName()); + names.clear(); + break; + } + + names.add(column.getName()); + identifiers.add(column.getSelectIdentifier().getSql().getSQL()); + } + + if (names.isEmpty() || !distinctColumnLists.add(indexKey(names))) + continue; + + // Ordinal names because ${NAME} is already 33 of the 63 chars Postgres allows before it silently truncates and collides. + ddl.add("CREATE INDEX idx_${NAME}_d" + ddl.size() + " ON temp.${NAME} (" + String.join(", ", identifiers) + ")"); + } + + return ddl; + } + + private static String indexKey(List columnNames) + { + return columnNames.stream().map(name -> name.toLowerCase(Locale.ROOT)).collect(Collectors.joining(",")); + } + /* SELECT and JOIN, does not include WHERE, same as getJoinSQL() */ private @Nullable SQLFragment getMaterializedSQL() { @@ -1448,7 +1527,7 @@ public Builder updateColumns(List updateColumns) @Override public _MaterializedQueryHelper build() { - return new _MaterializedQueryHelper(_lsid, _updateColumns, _prefix, _scope, _select, _uptodate, _supplier, _indexes, _max, _isSelectInto, _unlogged); + return new _MaterializedQueryHelper(_lsid, _updateColumns, _prefix, _scope, _select, _uptodate, _supplier, _indexes, _deferredIndexes, _max, _isSelectInto, _unlogged); } } @@ -1461,12 +1540,13 @@ public _MaterializedQueryHelper build() @Nullable SQLFragment uptodate, Supplier supplier, @Nullable Collection indexes, + @Nullable Collection deferredIndexes, long maxTimeToCache, boolean isSelectIntoSql, boolean unlogged ) { - super(prefix, scope, select, uptodate, supplier, indexes, maxTimeToCache, isSelectIntoSql, unlogged); + super(prefix, scope, select, uptodate, supplier, indexes, deferredIndexes, maxTimeToCache, isSelectIntoSql, unlogged); this._lsid = lsid; this._updateColumns = updateColumns; } @@ -1493,6 +1573,14 @@ protected void incrementalUpdateBeforeSelect(Materialized m) if (Materialized.LoadingState.ERROR == materialized._loadingState.get()) throw materialized._loadException; + // The lock is held by a full rebuild (including its deferred indexes). Leave the counters untouched so + // readers stay on the live query and the next materializeAsync retries, rather than racing that rebuild. + if (!lockAcquired) + { + _log.info("Skipping incremental update of {}; a rebuild still holds the lock.", getMaterializationName()); + return; + } + runIncremental("delete", materialized.incrementalDeleteCheck, this::executeIncrementalDelete); runIncremental("update", materialized.incrementalUpdateCheck, this::executeIncrementalUpdate); runIncremental("rollup", materialized.incrementalRollupCheck, this::executeIncrementalRollup); @@ -2259,7 +2347,30 @@ public void testMergeInsertAndUpdate() throws Exception assertCacheMatchesFreshDerivation(table, st.getLSID()); } + @Test + public void testDomainIndexMirroredNonUnique() throws Exception + { + Assume.assumeTrue("Domain indices are only mirrored on PostgreSQL", ExperimentService.get().getSchema().getSqlDialect().isPostgreSQL()); + + ExpSampleType st = createSampleType("IncrUpdIndexed", List.of(new GWTIndex(List.of("rootProp"), true))); + insertRoots(st, "R1"); + insertAliquots(st, "R1", 3); + + ExpMaterialTableImpl table = getSamplesTable(st); + List ddl = table.getDomainIndexDdl(); + assertEquals("Expected the domain's one index to be mirrored: " + ddl, 1, ddl.size()); + assertFalse("Mirrored index must not be unique: " + ddl.getFirst(), ddl.getFirst().contains("UNIQUE")); + + // All four rows share the root's rootProp, so mirroring the domain's unique index as-is would fail the build. + assertCacheMatchesFreshDerivation(table, st.getLSID()); + } + private ExpSampleType createSampleType(String name) throws Exception + { + return createSampleType(name, Collections.emptyList()); + } + + private ExpSampleType createSampleType(String name, List indices) throws Exception { List props = new ArrayList<>(); props.add(new GWTPropertyDescriptor("name", "string")); @@ -2269,7 +2380,7 @@ private ExpSampleType createSampleType(String name) throws Exception GWTPropertyDescriptor aliquotProp = new GWTPropertyDescriptor("aliquotProp", "string"); aliquotProp.setDerivationDataScope(ExpSchema.DerivationDataScopeType.ChildOnly.name()); // -> m_aliquot join props.add(aliquotProp); - return SampleTypeService.get().createSampleType(_c, _user, name, null, props, Collections.emptyList(), -1, -1, -1, -1, null); + return SampleTypeService.get().createSampleType(_c, _user, name, null, props, indices, -1, -1, -1, -1, null); } private ExpMaterialTableImpl getSamplesTable(ExpSampleType st) diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java b/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java index ed105bbe393..58b6cea2ba6 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java @@ -1231,13 +1231,13 @@ public String getCommentDetailed(QueryService.AuditAction action, boolean isUpda } @Override - public DetailedAuditTypeEvent createDetailedAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, @Nullable Map row, Map existingRow, Map providedValues) + public DetailedAuditTypeEvent createDetailedAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, @Nullable Map row, Map existingRow, Map providedValues, List sideEffectEvents) { return createAuditRecord(c, tInfo, getCommentDetailed(action, !existingRow.isEmpty()), userComment, action, row, existingRow, providedValues); } @Override - protected AuditTypeEvent createSummaryAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, int rowCount, @Nullable Map row) + protected AuditTypeEvent createSummaryAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, int rowCount, @Nullable Map row, List sideEffectEvents) { return createAuditRecord(c, tInfo, String.format(action.getCommentSummary(), rowCount), userComment, row); } @@ -1367,20 +1367,38 @@ public void addAuditEvent(User user, Container container, String comment, String @Override public void addAuditEvent(User user, Container container, String comment, String userComment, ExpMaterial sample, Map metadata, String updateType) + { + AuditLogService.get().addEvent(user, createTimelineAuditRecord(container, comment, userComment, sample, metadata, updateType)); + } + + @Override + public SampleTimelineAuditEvent createTimelineAuditRecord(Container container, String comment, String userComment, ExpMaterial sample, Map metadata, String updateType) { SampleTimelineAuditEvent event = createAuditRecord(container, comment, userComment, sample, metadata); event.setInventoryUpdateType(updateType); event.setUserComment(userComment); - AuditLogService.get().addEvent(user, event); + return event; } @Override public void addAuditEvents(User user, Container container, String comment, String userComment, Collection samples, Map metadata) { - List events = samples.stream() - .map(sample -> createAuditRecord(container, comment, userComment, sample, metadata)) - .collect(Collectors.toList()); - AuditLogService.get().addEvents(user, events); + AuditLogService auditLog = AuditLogService.get(); + List events = new ArrayList<>(Math.min(samples.size(), AUDIT_BATCH_SIZE)); + + for (ExpMaterial sample : samples) + { + events.add(createAuditRecord(container, comment, userComment, sample, metadata)); + + if (events.size() >= AUDIT_BATCH_SIZE) + { + auditLog.addEvents(user, events); + events.clear(); + } + } + + if (!events.isEmpty()) + auditLog.addEvents(user, events); } @Override diff --git a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java index 21e4632f511..0ffb757bd08 100644 --- a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java +++ b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java @@ -87,6 +87,7 @@ import org.labkey.api.util.ExceptionUtil; import org.labkey.api.util.GUID; import org.labkey.api.util.JunitUtil; +import org.labkey.api.util.MemTracker; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.Path; import org.labkey.api.util.TestContext; @@ -650,6 +651,19 @@ public TableInfo createTableInfoImpl(@NotNull Domain domain) return wrapper; } + @Override + @NotNull + public TableInfo createSharedTableInfoImpl(@NotNull Domain domain) + { + TableInfo table = createTableInfoImpl(domain); + table.setLocked(true); + MemTracker.getInstance().remove(table); + // The table holds the Domain, which holds a DomainDescriptor that MemTracker tracks separately + if (domain instanceof DomainImpl di) + MemTracker.getInstance().remove(di._dd); + return table; + } + @NotNull private SchemaTableInfo getSchemaTableInfo(@NotNull Domain domain, String schemaName, String tableName, DbSchema schema) { @@ -1049,7 +1063,7 @@ private static SqlDialect getSqlDialect(Domain domain) return CoreSchema.getInstance().getSqlDialect(); } - @Override + @Override @NotNull public SchemaTableInfo getSchemaTableInfo(Domain domain) { DomainKind kind = getDomainKind(domain); diff --git a/list/src/org/labkey/list/ListModule.java b/list/src/org/labkey/list/ListModule.java index b06f55ef246..bb7fe12597b 100644 --- a/list/src/org/labkey/list/ListModule.java +++ b/list/src/org/labkey/list/ListModule.java @@ -58,6 +58,7 @@ import org.labkey.list.model.ListManager; import org.labkey.list.model.ListManagerSchema; import org.labkey.list.model.ListQuerySchema; +import org.labkey.list.model.ListQueryUpdateService; import org.labkey.list.model.ListSchema; import org.labkey.list.model.ListServiceImpl; import org.labkey.list.model.ListWriter; @@ -236,4 +237,12 @@ public Collection getProvisionedSchemaNames() ListAuditProvider.TestCase.class ); } + + @Override + public @NotNull Set> getIntegrationTests() + { + return Set.of( + ListQueryUpdateService.AuditBatchTestCase.class + ); + } } diff --git a/list/src/org/labkey/list/model/ListManager.java b/list/src/org/labkey/list/model/ListManager.java index 68fa5058360..876ab565239 100644 --- a/list/src/org/labkey/list/model/ListManager.java +++ b/list/src/org/labkey/list/model/ListManager.java @@ -33,6 +33,7 @@ import org.labkey.api.cache.CacheLoader; import org.labkey.api.cache.CacheManager; import org.labkey.api.collections.CaseInsensitiveHashMap; +import org.labkey.api.collections.CaseInsensitiveHashSet; import org.labkey.api.collections.LabKeyCollectors; import org.labkey.api.data.*; import org.labkey.api.data.Selector.ForEachBlock; @@ -1202,6 +1203,11 @@ void addAuditEvent(ListDefinitionImpl list, User user, String comment) } void addAuditEvent(ListDefinitionImpl list, User user, Container c, String comment, String entityId, @Nullable String oldRecord, @Nullable String newRecord) + { + AuditLogService.get().addEvent(user, createAuditEvent(list, c, comment, entityId, oldRecord, newRecord)); + } + + ListAuditProvider.ListAuditEvent createAuditEvent(ListDefinitionImpl list, Container c, String comment, String entityId, @Nullable String oldRecord, @Nullable String newRecord) { ListAuditProvider.ListAuditEvent event = new ListAuditProvider.ListAuditEvent(c, comment, list); @@ -1209,72 +1215,100 @@ void addAuditEvent(ListDefinitionImpl list, User user, Container c, String comme if (oldRecord != null) event.setOldRecordMap(oldRecord); if (newRecord != null) event.setNewRecordMap(newRecord); - AuditLogService.get().addEvent(user, event); + return event; } String formatAuditItem(ListDefinitionImpl list, User user, Map props) { - String itemRecord = ""; - TableInfo ti = list.getTable(user); + return getAuditItemFormatter(list, user).format(props); + } - if (null != ti) + AuditItemFormatter getAuditItemFormatter(ListDefinitionImpl list, User user) + { + return new AuditItemFormatter(list, user); + } + + /** + * Builds the audit record map for list rows. Resolve one instance per operation, not per row: the table, the + * reserved property names, and the name resolution for a given key are identical for every row of a list. + */ + static class AuditItemFormatter + { + private final TableInfo _table; + private final Domain _domain; + private final Set _reserved; + private final Map _resolvedNames = new CaseInsensitiveHashMap<>(); + + AuditItemFormatter(ListDefinitionImpl list, User user) { + _table = list.getTable(user); + _domain = list.getDomain(); + _reserved = null == _table ? Set.of() : new CaseInsensitiveHashSet(_domain.getDomainKind().getReservedPropertyNames(_domain, user)); + } + + String format(Map props) + { + if (null == _table) + return ""; + Map recordChangedMap = new CaseInsensitiveHashMap<>(); - Set reserved = list.getDomain().getDomainKind().getReservedPropertyNames(list.getDomain(), user); - // Match props to columns for (Map.Entry entry : props.entrySet()) { - String baseKey = entry.getKey(); - - boolean isReserved = false; - for (String res : reserved) - { - if (res.equalsIgnoreCase(baseKey)) - { - isReserved = true; - break; - } - } - - if (isReserved) - continue; - - ColumnInfo col = ti.getColumn(FieldKey.fromParts(baseKey)); Object value = entry.getValue(); - String key = null; - if (null != col) - { - // Found the column - key = col.getName(); // best good - } - else - { - // See if there is a match in the domain properties - for (DomainProperty dp : list.getDomain().getProperties()) - { - if (dp.getName().equalsIgnoreCase(baseKey)) - { - key = dp.getName(); // middle good - } - } + if (null == value) + continue; - // Try by name - DomainProperty dp = list.getDomain().getPropertyByName(baseKey); - if (null != dp) - key = dp.getName(); - } + String key = resolveName(entry.getKey()); - if (null != key && null != value) + if (null != key) recordChangedMap.put(key, value); } - if (!recordChangedMap.isEmpty()) - itemRecord = ListAuditProvider.encodeForDataMap(recordChangedMap); + return recordChangedMap.isEmpty() ? "" : ListAuditProvider.encodeForDataMap(recordChangedMap); + } + + /** @return the canonical property name to audit under, or null if the key is reserved or unknown */ + private String resolveName(String baseKey) + { + if (_resolvedNames.containsKey(baseKey)) + return _resolvedNames.get(baseKey); + + String key = computeName(baseKey); + _resolvedNames.put(baseKey, key); + + return key; } - return itemRecord; + private String computeName(String baseKey) + { + if (_reserved.contains(baseKey)) + return null; + + ColumnInfo col = _table.getColumn(FieldKey.fromParts(baseKey)); + + if (null != col) + return col.getName(); // best good + + String key = null; + + // See if there is a match in the domain properties + for (DomainProperty dp : _domain.getProperties()) + { + if (dp.getName().equalsIgnoreCase(baseKey)) + { + key = dp.getName(); // middle good + } + } + + // Try by name + DomainProperty dp = _domain.getPropertyByName(baseKey); + if (null != dp) + key = dp.getName(); + + return key; + } } boolean importListSchema( diff --git a/list/src/org/labkey/list/model/ListQueryUpdateService.java b/list/src/org/labkey/list/model/ListQueryUpdateService.java index 7b3426148f4..1a2e35ab321 100644 --- a/list/src/org/labkey/list/model/ListQueryUpdateService.java +++ b/list/src/org/labkey/list/model/ListQueryUpdateService.java @@ -18,10 +18,15 @@ import org.apache.commons.lang3.StringUtils; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.junit.AfterClass; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; import org.labkey.api.attachments.AttachmentFile; import org.labkey.api.attachments.AttachmentParent; import org.labkey.api.attachments.AttachmentParentFactory; import org.labkey.api.attachments.AttachmentService; +import org.labkey.api.audit.AbstractAuditHandler; import org.labkey.api.audit.AbstractAuditTypeProvider; import org.labkey.api.audit.AuditLogService; import org.labkey.api.audit.TransactionAuditProvider; @@ -52,6 +57,7 @@ import org.labkey.api.exp.property.Domain; import org.labkey.api.exp.property.DomainProperty; import org.labkey.api.exp.property.IPropertyValidator; +import org.labkey.api.exp.property.PropertyService; import org.labkey.api.exp.property.ValidatorContext; import org.labkey.api.gwt.client.AuditBehaviorType; import org.labkey.api.lists.permissions.ManagePicklistsPermission; @@ -73,11 +79,13 @@ import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.UpdatePermission; import org.labkey.api.security.roles.EditorRole; +import org.labkey.api.test.TestTimeout; import org.labkey.api.usageMetrics.SimpleMetricsService; import org.labkey.api.util.GUID; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.Pair; import org.labkey.api.util.StringUtilsLabKey; +import org.labkey.api.util.TestContext; import org.labkey.api.util.UnexpectedException; import org.labkey.api.view.UnauthorizedException; import org.labkey.api.writer.VirtualFile; @@ -102,6 +110,12 @@ public class ListQueryUpdateService extends DefaultQueryUpdateService { private final ListDefinitionImpl _list; private static final String ID = "entityId"; + private static final String AUDIT_COMMENT_INSERT = "A new list record was inserted"; + private static final String AUDIT_COMMENT_UPDATE = "An existing list record was modified"; + private static final String AUDIT_COMMENT_DELETE = "An existing list record was deleted"; + + /** Non-null while a multi-row operation is in flight; see {@link RowAuditBatch}. */ + private RowAuditBatch _auditBatch; public ListQueryUpdateService(ListTable queryTable, TableInfo dbTable, @NotNull ListDefinition list) { @@ -179,27 +193,112 @@ public List> insertRows(User user, Container container, List if (null != result) { - ListManager mgr = ListManager.get(); - - for (Map row : result) + try (RowAuditBatch ignored = beginAuditBatch(user)) { - if (null != row.get(ID)) + for (Map row : result) { - // Audit each row - String entityId = (String) row.get(ID); - String newRecord = mgr.formatAuditItem(_list, user, row); + if (null != row.get(ID)) + { + // Audit each row + String entityId = (String) row.get(ID); - mgr.addAuditEvent(_list, user, container, "A new list record was inserted", entityId, null, newRecord); + auditRowChange(user, container, AUDIT_COMMENT_INSERT, entityId, null, row); + } } } if (!result.isEmpty() && !errors.hasErrors()) - mgr.indexList(_list); + ListManager.get().indexList(_list); } return result; } + /** + * Accumulates row audit events so one operation writes them with a single batched statement. + */ + private class RowAuditBatch implements AutoCloseable + { + private final User _user; + private final ListManager.AuditItemFormatter _formatter; + private final List _events = new ArrayList<>(); + + RowAuditBatch(User user) + { + _user = user; + _formatter = ListManager.get().getAuditItemFormatter(_list, user); + } + + String format(Map props) + { + return _formatter.format(props); + } + + void add(Container c, String comment, String entityId, @Nullable String oldRecord, @Nullable String newRecord) + { + _events.add(ListManager.get().createAuditEvent(_list, c, comment, entityId, oldRecord, newRecord)); + + if (_events.size() >= AbstractAuditHandler.AUDIT_BATCH_SIZE) + flush(); + } + + void flush() + { + if (_events.isEmpty()) + return; + + // close() flushes again on the way out of a failed operation, so hand off before writing to avoid re-inserting + List toWrite = new ArrayList<>(_events); + _events.clear(); + + AuditLogService.get().addEvents(_user, toWrite); + } + + @Override + public void close() + { + try + { + flush(); + } + finally + { + if (this == _auditBatch) + _auditBatch = null; + } + } + } + + /** @return a batch to close when the operation completes, or null if an enclosing operation already owns one */ + private @Nullable RowAuditBatch beginAuditBatch(User user) + { + if (null != _auditBatch) + return null; + + _auditBatch = new RowAuditBatch(user); + + return _auditBatch; + } + + private void auditRowChange(User user, Container container, String comment, String entityId, @Nullable Map oldRow, @Nullable Map newRow) + { + RowAuditBatch batch = _auditBatch; + + if (null == batch) + { + ListManager mgr = ListManager.get(); + mgr.addAuditEvent(_list, user, container, comment, entityId, + null == oldRow ? null : mgr.formatAuditItem(_list, user, oldRow), + null == newRow ? null : mgr.formatAuditItem(_list, user, newRow)); + } + else + { + batch.add(container, comment, entityId, + null == oldRow ? null : batch.format(oldRow), + null == newRow ? null : batch.format(newRow)); + } + } + private User getListUser(User user, Container container) { if (_list.isPicklist() && container.hasPermission(user, ManagePicklistsPermission.class)) @@ -306,7 +405,15 @@ public List> updateRows(User user, Container container, List if (!_list.isVisible(user)) throw new UnauthorizedException("You do not have permission to update data into this table."); - List> result = super.updateRows(getListUser(user, container), container, rows, oldKeys, errors, configParameters, extraScriptContext); + List> result; + User listUser = getListUser(user, container); + + // updateRow() audits as the list user, so the batch must format and log as that user too + try (RowAuditBatch ignored = beginAuditBatch(listUser)) + { + result = super.updateRows(listUser, container, rows, oldKeys, errors, configParameters, extraScriptContext); + } + if (!result.isEmpty()) ListManager.get().indexList(_list); return result; @@ -394,7 +501,6 @@ else if (r.getValue() != null && !StringUtils.isEmpty(String.valueOf(r.getValue( if (null != result && null != result.get(ID)) { - ListManager mgr = ListManager.get(); String entityId = (String) result.get(ID); try @@ -430,11 +536,8 @@ else if (r.getValue() != null && !StringUtils.isEmpty(String.valueOf(r.getValue( } } - String oldRecord = mgr.formatAuditItem(_list, user, oldRow); - String newRecord = mgr.formatAuditItem(_list, user, result); - // Audit - mgr.addAuditEvent(_list, user, container, "An existing list record was modified", entityId, oldRecord, newRecord); + auditRowChange(user, container, AUDIT_COMMENT_UPDATE, entityId, oldRow, result); } } @@ -728,6 +831,16 @@ private int addDetailedMoveAuditEvents(User user, Container sourceContainer, Con return auditEvents.size(); } + @Override + public List> deleteRows(User user, Container container, List> keys, @Nullable Map configParameters, @Nullable Map extraScriptContext) + throws InvalidKeyException, BatchValidationException, QueryUpdateServiceException, SQLException + { + try (RowAuditBatch ignored = beginAuditBatch(user)) + { + return super.deleteRows(user, container, keys, configParameters, extraScriptContext); + } + } + @Override protected Map deleteRow(User user, Container container, Map oldRowMap) throws InvalidKeyException, QueryUpdateServiceException, SQLException { @@ -743,10 +856,9 @@ protected Map deleteRow(User user, Container container, Map> rows = new ArrayList<>(ROW_COUNT); + for (int i = 0; i < ROW_COUNT; i++) + rows.add(CaseInsensitiveHashMap.of(VALUE_FIELD, "row-" + i)); + + BatchValidationException errors = new BatchValidationException(); + List> inserted; + + // Without a transaction LogManager declines to batch, so the batched insert path only runs inside one + try (DbScope.Transaction tx = table.getSchema().getScope().ensureTransaction()) + { + inserted = table.getUpdateService().insertRows(_user, _container, rows, errors, null, null); + if (errors.hasErrors()) + throw errors.getLastRowError(); + tx.commit(); + } + + assertEquals("expected every row to be inserted", ROW_COUNT, inserted.size()); + assertEquals("one insert audit event per row", ROW_COUNT, auditCount(AUDIT_COMMENT_INSERT)); + + List> keys = new ArrayList<>(ROW_COUNT); + for (Map row : inserted) + keys.add(CaseInsensitiveHashMap.of(KEY_NAME, row.get(KEY_NAME))); + + List> deleted; + + try (DbScope.Transaction tx = table.getSchema().getScope().ensureTransaction()) + { + deleted = table.getUpdateService().deleteRows(_user, _container, keys, null, null); + tx.commit(); + } + + assertEquals("expected every row to be deleted", ROW_COUNT, deleted.size()); + assertEquals("one delete audit event per row", ROW_COUNT, auditCount(AUDIT_COMMENT_DELETE)); + assertEquals("deleting rows must not emit insert events", ROW_COUNT, auditCount(AUDIT_COMMENT_INSERT)); + } + } } diff --git a/query/src/org/labkey/query/QueryServiceImpl.java b/query/src/org/labkey/query/QueryServiceImpl.java index 6db625d39af..1e1927aea41 100644 --- a/query/src/org/labkey/query/QueryServiceImpl.java +++ b/query/src/org/labkey/query/QueryServiceImpl.java @@ -3083,7 +3083,7 @@ public AuditHandler getDefaultAuditHandler() return new AbstractAuditHandler() { @Override - protected AuditTypeEvent createSummaryAuditRecord(User user, Container c, AuditConfigurable tinfo, AuditAction action, @Nullable String userComment, int rowCount, @Nullable Map row) + protected AuditTypeEvent createSummaryAuditRecord(User user, Container c, AuditConfigurable tinfo, AuditAction action, @Nullable String userComment, int rowCount, @Nullable Map row, List sideEffectEvents) { DetailedAuditTypeEvent event = createAuditRecord(c, tinfo, String.format(action.getCommentSummary(), rowCount), row, null); event.setUserComment(userComment); @@ -3091,7 +3091,7 @@ protected AuditTypeEvent createSummaryAuditRecord(User user, Container c, AuditC } @Override - protected DetailedAuditTypeEvent createDetailedAuditRecord(User user, Container c, AuditConfigurable tinfo, AuditAction action, @Nullable String userComment, @Nullable Map updatedRow, Map existingRow, @Nullable Map providedValues) + protected DetailedAuditTypeEvent createDetailedAuditRecord(User user, Container c, AuditConfigurable tinfo, AuditAction action, @Nullable String userComment, @Nullable Map updatedRow, Map existingRow, @Nullable Map providedValues, List sideEffectEvents) { DetailedAuditTypeEvent event = createAuditRecord(c, tinfo, action.getCommentDetailed(), updatedRow, existingRow); event.setUserComment(userComment); diff --git a/study/src/org/labkey/study/model/DatasetDefinition.java b/study/src/org/labkey/study/model/DatasetDefinition.java index fd9c1cebc1a..7f9c0d629a8 100644 --- a/study/src/org/labkey/study/model/DatasetDefinition.java +++ b/study/src/org/labkey/study/model/DatasetDefinition.java @@ -1790,9 +1790,9 @@ public void addAuditEvent(User user, Container c, TableInfo table, @Nullable Aud Map row = rows.get(i); Map existingRow = null==existingRows ? null : existingRows.get(i); // note switched order (oldRecord, newRecord) - var event = createDetailedAuditRecord(user, c, (AuditConfigurable)table, action, userComment, row, existingRow, null); + var event = createDetailedAuditRecord(user, c, (AuditConfigurable)table, action, userComment, row, existingRow, null, List.of()); batch.add(event); - if (batch.size() > 1000) + if (batch.size() > AbstractAuditHandler.AUDIT_BATCH_SIZE) { auditLog.addEvents(user, batch); batch.clear(); @@ -1808,7 +1808,7 @@ public void addAuditEvent(User user, Container c, TableInfo table, @Nullable Aud } @Override - protected AuditTypeEvent createSummaryAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, int rowCount, @Nullable Map row) + protected AuditTypeEvent createSummaryAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, int rowCount, @Nullable Map row, List sideEffectEvents) { throw new UnsupportedOperationException(); } @@ -1817,7 +1817,7 @@ protected AuditTypeEvent createSummaryAuditRecord(User user, Container c, AuditC * NOTE: userComment field is not supported for this domain and will be ignored */ @Override - protected DatasetAuditProvider.DatasetAuditEvent createDetailedAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, @Nullable Map record, Map existingRecord, Map providedValues) + protected DatasetAuditProvider.DatasetAuditEvent createDetailedAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, @Nullable Map record, Map existingRecord, Map providedValues, List sideEffectEvents) { String auditComment = switch (action) {