Skip to content

Commit 53e962e

Browse files
labkey-nickalabkey-nicka
committed
Review feedback, remove manageColumns flag
- no longer process non-data iterator pathway for managed columns
1 parent 7299b8a commit 53e962e

7 files changed

Lines changed: 18 additions & 31 deletions

File tree

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

Lines changed: 9 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1987,8 +1987,7 @@ public void fireRowTrigger(
19871987
@Nullable Map<String, Object> newRow,
19881988
@Nullable Map<String, Object> oldRow,
19891989
Map<String, Object> extraContext,
1990-
@Nullable Map<String, Object> existingRecord,
1991-
boolean manageColumns
1990+
@Nullable Map<String, Object> existingRecord
19921991
) throws ValidationException
19931992
{
19941993
ValidationException errors = new ValidationException();
@@ -2002,8 +2001,9 @@ public void fireRowTrigger(
20022001
// Wrap the row once before the loop
20032002
DeltaTrackingMap<Object> trackedRow = null;
20042003
Map<String, Object> newRowTracked = newRow;
2004+
boolean manageColumns = before && insertOption != null && isTriggerManagedColumnsEnabled();
20052005

2006-
if (newRow != null && before && isTriggerManagedColumnsEnabled())
2006+
if (newRow != null && manageColumns)
20072007
{
20082008
trackedRow = new DeltaTrackingMap<>(newRow);
20092009
newRowTracked = trackedRow;
@@ -2018,6 +2018,7 @@ public void fireRowTrigger(
20182018
if (errors.hasErrors())
20192019
break;
20202020

2021+
// trackedRow should only be null when not manageColumns
20212022
if (trackedRow != null)
20222023
{
20232024
var managed = script.getManagedColumns();
@@ -2050,13 +2051,8 @@ public void fireRowTrigger(
20502051
diffs.add("remove: " + removed.stream().map(col -> "'" + col + "'").collect(Collectors.joining(", ")));
20512052

20522053
String message = "Trigger '" + script.getName() + "' attempted to " + String.join(", ", diffs) + ". Declare managed columns to include them in the column set.";
2053-
if (manageColumns)
2054-
{
2055-
errors.addGlobalError(message);
2056-
break;
2057-
}
2058-
2059-
LOG.warn(message + " This will be an error if invoked via data iteration.");
2054+
errors.addGlobalError(message);
2055+
break;
20602056
}
20612057
}
20622058

@@ -2070,15 +2066,10 @@ public void fireRowTrigger(
20702066
{
20712067
if (!newRowTracked.containsKey(col))
20722068
{
2069+
// Not using errors.addFieldError() here as the managed column may not be visible
20732070
String message = "Trigger '" + script.getName() + "' declared the managed column '" + col + "' but did not set a value for it. Set null to clear or provide a value.";
2074-
if (manageColumns)
2075-
{
2076-
// Not using errors.addFieldError() here as the managed column may not be visible
2077-
errors.addGlobalError(message);
2078-
break;
2079-
}
2080-
2081-
LOG.warn(message + " This will be an error if invoked via data iteration.");
2071+
errors.addGlobalError(message);
2072+
break;
20822073
}
20832074
}
20842075
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -913,7 +913,7 @@ public void fireBatchTrigger(Container c, User user, TriggerType type, @Nullable
913913
}
914914

915915
@Override
916-
public void fireRowTrigger(Container c, User user, TriggerType type, @Nullable QueryUpdateService.InsertOption insertOption, boolean before, int rowNumber, Map<String, Object> newRow, Map<String, Object> oldRow, Map<String, Object> extraContext, @Nullable Map<String, Object> existingRecord, boolean manageColumns)
916+
public void fireRowTrigger(Container c, User user, TriggerType type, @Nullable QueryUpdateService.InsertOption insertOption, boolean before, int rowNumber, Map<String, Object> newRow, Map<String, Object> oldRow, Map<String, Object> extraContext, @Nullable Map<String, Object> existingRecord)
917917
{
918918
throw new UnsupportedOperationException("Table triggers not yet supported on schema tables");
919919
}

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

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,6 @@
4040
import org.labkey.api.security.HasPermission;
4141
import org.labkey.api.security.User;
4242
import org.labkey.api.security.permissions.ReadPermission;
43-
import org.labkey.api.settings.AppProps;
4443
import org.labkey.api.util.ContainerContext;
4544
import org.labkey.api.util.Pair;
4645
import org.labkey.api.util.Path;
@@ -569,9 +568,7 @@ default void fireRowTrigger(Container c, User user, TriggerType type, boolean be
569568
@Nullable Map<String, Object> newRow, @Nullable Map<String, Object> oldRow, Map<String, Object> extraContext)
570569
throws ValidationException
571570
{
572-
// In production, columns are not managed for non-data iterator invoked triggers and will log a warning.
573-
// 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());
571+
fireRowTrigger(c, user, type, null, before, rowNumber, newRow, oldRow, extraContext, null);
575572
}
576573

577574
/**
@@ -610,7 +607,6 @@ default void fireRowTrigger(Container c, User user, TriggerType type, boolean be
610607
* @param oldRow The previous row for UPDATE and DELETE
611608
* @param extraContext Optional additional bindings to set in the script's context when evaluating.
612609
* @param existingRecord Optional existing record for the row, used for merge operation to differentiate new vs existing row
613-
* @param manageColumns Whether to manage columns for the row.
614610
* @throws ValidationException if the trigger function returns false or the errors map isn't empty.
615611
*/
616612
void fireRowTrigger(
@@ -623,8 +619,7 @@ void fireRowTrigger(
623619
@Nullable Map<String, Object> newRow,
624620
@Nullable Map<String, Object> oldRow,
625621
Map<String, Object> extraContext,
626-
@Nullable Map<String, Object> existingRecord,
627-
boolean manageColumns
622+
@Nullable Map<String, Object> existingRecord
628623
) throws ValidationException;
629624

630625
/**

‎api/src/org/labkey/api/data/triggers/Trigger.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ default void setInsertManagedColumns(
174174
if (insertOption != null && insertOption.mergeRows && existingRecord == null)
175175
throw new IllegalArgumentException("An existing record must be supplied for all MERGE triggers");
176176

177-
setManagedColumns(newRow, null, TableInfo.TriggerType.INSERT);
177+
setManagedColumns(newRow, existingRecord, TableInfo.TriggerType.INSERT);
178178
}
179179

180180
/**

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,7 @@ public boolean next() throws BatchValidationException
227227
_currentRow = getInput().getMap();
228228
try
229229
{
230-
_target.fireRowTrigger(_c, _user, triggerType, _context.getInsertOption(), true, rowNumber, _currentRow, getOldRow(), _extraContext, getExistingRecord(), true);
230+
_target.fireRowTrigger(_c, _user, triggerType, _context.getInsertOption(), true, rowNumber, _currentRow, getOldRow(), _extraContext, getExistingRecord());
231231
return true;
232232
}
233233
catch (ValidationException vex)
@@ -294,7 +294,7 @@ public boolean next() throws BatchValidationException
294294
Map<String,Object> newRow = getInput().getMap();
295295
try
296296
{
297-
_target.fireRowTrigger(_c, _user, getTriggerType(), _context.getInsertOption(), false, rowNumber, newRow, getOldRow(), _extraContext, getExistingRecord(), false);
297+
_target.fireRowTrigger(_c, _user, getTriggerType(), _context.getInsertOption(), false, rowNumber, newRow, getOldRow(), _extraContext, getExistingRecord());
298298
}
299299
catch (ValidationException vex)
300300
{

‎core/src/org/labkey/core/query/UsersTable.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -453,9 +453,9 @@ public SQLFragment toSQLFragment(Map<FieldKey, ? extends ColumnInfo> columnMap,
453453
}
454454

455455
@Override
456-
public void fireRowTrigger(Container c, User user, TriggerType type, @Nullable QueryUpdateService.InsertOption insertOption, boolean before, int rowNumber, @Nullable Map<String, Object> newRow, @Nullable Map<String, Object> oldRow, Map<String, Object> extraContext, @Nullable Map<String, Object> existingRecord, boolean manageColumns) throws ValidationException
456+
public void fireRowTrigger(Container c, User user, TriggerType type, @Nullable QueryUpdateService.InsertOption insertOption, boolean before, int rowNumber, @Nullable Map<String, Object> newRow, @Nullable Map<String, Object> oldRow, Map<String, Object> extraContext, @Nullable Map<String, Object> existingRecord) throws ValidationException
457457
{
458-
super.fireRowTrigger(c, user, type, insertOption, before, rowNumber, newRow, oldRow, extraContext, existingRecord, manageColumns);
458+
super.fireRowTrigger(c, user, type, insertOption, before, rowNumber, newRow, oldRow, extraContext, existingRecord);
459459
Integer userId = null!=oldRow ? asInteger(oldRow.get("UserId")) : null!=newRow ? asInteger(newRow.get("UserId")) : null;
460460
if (null != userId && !before)
461461
UserManager.fireUserPropertiesChanged(userId);

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3576,6 +3576,7 @@ public boolean isProductFoldersDataListingScopedToProject()
35763576
return AppProps.getInstance().isOptionalFeatureEnabled(EXPERIMENTAL_PRODUCT_PROJECT_DATA_LISTING_SCOPED);
35773577
}
35783578

3579+
@Override
35793580
public boolean isTriggerManagedColumnsEnabled()
35803581
{
35813582
return !AppProps.getInstance().isOptionalFeatureEnabled(EXPERIMENTAL_DISABLE_MANAGED_TRIGGER_COLUMNS);

0 commit comments

Comments
 (0)