Skip to content

Commit 61286bc

Browse files
labkey-nickalabkey-nicka
committed
Experimental feature flag, review feedback
1 parent 5ff3f06 commit 61286bc

File tree

7 files changed

+78
-9
lines changed

7 files changed

+78
-9
lines changed

‎api/src/org/labkey/api/collections/DeltaTrackingMap.java‎

Lines changed: 57 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -172,22 +172,34 @@ public V get(Object key)
172172
return delegate.get(key);
173173
}
174174

175+
/**
176+
* Returns an unmodifiable view of the key set. Use {@link #put} and {@link #remove} to
177+
* mutate the map so that structural changes are correctly tracked.
178+
*/
175179
@Override
176180
public @NotNull Set<String> keySet()
177181
{
178-
return delegate.keySet();
182+
return Collections.unmodifiableSet(delegate.keySet());
179183
}
180184

185+
/**
186+
* Returns an unmodifiable view of the values collection. Use {@link #put} and {@link #remove} to
187+
* mutate the map so that structural changes are correctly tracked.
188+
*/
181189
@Override
182190
public @NotNull Collection<V> values()
183191
{
184-
return delegate.values();
192+
return Collections.unmodifiableCollection(delegate.values());
185193
}
186194

195+
/**
196+
* Returns an unmodifiable view of the entry set. Use {@link #put} and {@link #remove} to
197+
* mutate the map so that structural changes are correctly tracked.
198+
*/
187199
@Override
188200
public @NotNull Set<Entry<String, V>> entrySet()
189201
{
190-
return delegate.entrySet();
202+
return Collections.unmodifiableSet(delegate.entrySet());
191203
}
192204

193205
public static class TestCase extends Assert
@@ -333,6 +345,48 @@ public void testEncapsulationOfTrackingSets()
333345
}
334346
}
335347

348+
@Test
349+
public void testMapViewsAreUnmodifiable()
350+
{
351+
DeltaTrackingMap<String> map = createTracker();
352+
353+
// keySet().remove() must not bypass tracking
354+
try
355+
{
356+
map.keySet().remove("ExistingKey1");
357+
fail("keySet() should return an unmodifiable view.");
358+
}
359+
catch (UnsupportedOperationException e)
360+
{
361+
// Expected behavior
362+
}
363+
assertFalse("keySet().remove() must not have modified the map", map.hasStructuralChanges());
364+
assertTrue(map.containsKey("ExistingKey1"));
365+
366+
// entrySet() iterator remove() must not bypass tracking
367+
try
368+
{
369+
map.entrySet().iterator().remove();
370+
fail("entrySet() should return an unmodifiable view.");
371+
}
372+
catch (UnsupportedOperationException e)
373+
{
374+
// Expected behavior
375+
}
376+
377+
// values() remove() must not bypass tracking
378+
try
379+
{
380+
map.values().remove("Value1");
381+
fail("values() should return an unmodifiable view.");
382+
}
383+
catch (UnsupportedOperationException e)
384+
{
385+
// Expected behavior
386+
}
387+
assertFalse("No mutations should have been tracked", map.hasStructuralChanges());
388+
}
389+
336390
@Test
337391
public void testCrossCaseCancellations()
338392
{

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2050,8 +2050,8 @@ public void fireRowTrigger(
20502050
}
20512051
}
20522052

2053-
// Verify that update triggers handle every managed column
2054-
if (managedCols != null && type == TriggerType.UPDATE)
2053+
// Verify the trigger handles every managed column
2054+
if (managedCols != null)
20552055
{
20562056
for (var col : managedCols)
20572057
{

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -571,7 +571,7 @@ default void fireRowTrigger(Container c, User user, TriggerType type, boolean be
571571
{
572572
// In production, columns are not managed for non-data iterator invoked triggers and will log a warning.
573573
// In development, columns are managed for all non-data iterator triggers and will throw an error.
574-
fireRowTrigger(c, user, type, null, before, rowNumber, newRow, oldRow, extraContext, null, AppProps.getInstance().isDevMode());
574+
fireRowTrigger(c, user, type, null, before, rowNumber, newRow, oldRow, extraContext, null, AppProps.getInstance().isDevMode() && QueryService.get().isTriggerManagedColumnsEnabled());
575575
}
576576

577577
/**

‎api/src/org/labkey/api/dataiterator/TriggerDataBuilderHelper.java‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import org.labkey.api.data.triggers.Trigger;
2222
import org.labkey.api.exp.query.ExpTable;
2323
import org.labkey.api.query.BatchValidationException;
24+
import org.labkey.api.query.QueryService;
2425
import org.labkey.api.query.QueryUpdateService;
2526
import org.labkey.api.query.ValidationException;
2627
import org.labkey.api.security.User;
@@ -192,10 +193,12 @@ class BeforeIterator extends TriggerDataIterator
192193
{
193194
boolean _firstRow = true;
194195
Map<String, Object> _currentRow = null;
196+
private final boolean _manageColumns;
195197

196198
BeforeIterator(DataIterator di, DataIteratorContext context)
197199
{
198200
super(di, context);
201+
_manageColumns = QueryService.get().isTriggerManagedColumnsEnabled();
199202
}
200203

201204
@Override
@@ -223,7 +226,7 @@ public boolean next() throws BatchValidationException
223226
_currentRow = getInput().getMap();
224227
try
225228
{
226-
_target.fireRowTrigger(_c, _user, triggerType, _context.getInsertOption(), true, rowNumber, _currentRow, getOldRow(), _extraContext, getExistingRecord(), true);
229+
_target.fireRowTrigger(_c, _user, triggerType, _context.getInsertOption(), true, rowNumber, _currentRow, getOldRow(), _extraContext, getExistingRecord(), _manageColumns);
227230
return true;
228231
}
229232
catch (ValidationException vex)
@@ -290,7 +293,7 @@ public boolean next() throws BatchValidationException
290293
Map<String,Object> newRow = getInput().getMap();
291294
try
292295
{
293-
_target.fireRowTrigger(_c, _user, getTriggerType(), _context.getInsertOption(), false, rowNumber, newRow, getOldRow(), _extraContext, getExistingRecord(), true);
296+
_target.fireRowTrigger(_c, _user, getTriggerType(), _context.getInsertOption(), false, rowNumber, newRow, getOldRow(), _extraContext, getExistingRecord(), false);
294297
}
295298
catch (ValidationException vex)
296299
{

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@
5454
public interface QueryService
5555
{
5656
String EXPERIMENTAL_LAST_MODIFIED = "queryMetadataLastModified";
57+
String EXPERIMENTAL_DISABLE_MANAGED_TRIGGER_COLUMNS = "queryDisableManagedTriggerColumns";
5758
String EXPERIMENTAL_PRODUCT_ALL_FOLDER_LOOKUPS = "queryProductAllFolderLookups";
5859
String EXPERIMENTAL_PRODUCT_PROJECT_DATA_LISTING_SCOPED = "queryProductProjectDataListingScoped";
5960
String MAX_QUERY_SELECTION = "maxQuerySelection";
@@ -697,4 +698,9 @@ default Results select()
697698
* Returns true if the "Product projects display project-specific data" experimental feature is enabled.
698699
*/
699700
boolean isProductFoldersDataListingScopedToProject();
701+
702+
/**
703+
* Returns false if the "Disable managed columns in query triggers" experimental feature is enabled.
704+
*/
705+
boolean isTriggerManagedColumnsEnabled();
700706
}

‎query/src/org/labkey/query/QueryModule.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,8 @@ public QuerySchema createSchema(DefaultSchema schema, Module module)
247247
"Allow for lookup fields in product folders to query across all folders within the top-level folder.", false);
248248
OptionalFeatureService.get().addExperimentalFeatureFlag(QueryServiceImpl.EXPERIMENTAL_PRODUCT_PROJECT_DATA_LISTING_SCOPED, "Product folders display folder-specific data",
249249
"Only list folder-specific data within product folders.", false);
250+
OptionalFeatureService.get().addExperimentalFeatureFlag(QueryService.EXPERIMENTAL_DISABLE_MANAGED_TRIGGER_COLUMNS, "Disable managed columns in query triggers",
251+
"By default LabKey enforces managed columns for triggers and errors when the data does not align. Enabling this feature will result in them only logging warnings.", false);
250252

251253
McpService.get().register(new QueryMcp());
252254
}

‎query/src/org/labkey/query/QueryServiceImpl.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@
3333
import org.json.JSONObject;
3434
import org.junit.Assert;
3535
import org.junit.Test;
36-
import org.labkey.api.action.ApiUsageException;
3736
import org.labkey.api.assay.AssayService;
3837
import org.labkey.api.audit.AbstractAuditHandler;
3938
import org.labkey.api.audit.AuditHandler;
@@ -3577,6 +3576,11 @@ public boolean isProductFoldersDataListingScopedToProject()
35773576
return AppProps.getInstance().isOptionalFeatureEnabled(EXPERIMENTAL_PRODUCT_PROJECT_DATA_LISTING_SCOPED);
35783577
}
35793578

3579+
public boolean isTriggerManagedColumnsEnabled()
3580+
{
3581+
return !AppProps.getInstance().isOptionalFeatureEnabled(EXPERIMENTAL_DISABLE_MANAGED_TRIGGER_COLUMNS);
3582+
}
3583+
35803584
public static class TestCase extends Assert
35813585
{
35823586
@Test

0 commit comments

Comments
 (0)