Handler class indexes Trigger.new / Trigger.old directly
ID |
apex.correctness.direct_trigger_map |
Severity |
high |
Remediation Complexity |
medium |
Remediation Risk |
medium |
Remediation Effort |
medium |
Resource |
Api Design |
Language |
Apex |
Description
Reports indexed access to the trigger context from a class - Trigger.new[i], Trigger.old[i], Trigger.newMap.get(…) or Trigger.oldMap.get(…).
Rationale
Half of those collections are null in half of the trigger contexts: Trigger.old and Trigger.oldMap are null on insert, Trigger.new and Trigger.newMap are null on delete, so an unguarded index throws a null dereference the first time the class runs in the other context. Reading the context inside the class also welds it to the trigger: the same logic cannot be called from a batch, from a Queueable or from a test that builds its own records, so the only way to exercise it is to fire a real DML statement and hope the right context happens.
The following code illustrates the pattern detected by this rule:
public class AccountTriggerHandler {
public void onAfterUpdate() {
// FLAGGED: Handler class indexes Trigger.new / Trigger.old directly
Account before = Trigger.old[0];
// FLAGGED: Handler class indexes Trigger.new / Trigger.old directly
Account after = Trigger.new[0];
System.debug(before.Name + after.Name);
}
public void compareOwners() {
for (Account a : (List<Account>) Trigger.new) {
// FLAGGED: Handler class indexes Trigger.new / Trigger.old directly
Account previous = Trigger.oldMap.get(a.Id);
if (previous.OwnerId != a.OwnerId) {
System.debug('owner changed');
}
}
}
public void handleAfterUpdate(List<Account> updated, Map<Id, Account> previousById) {
for (Account a : updated) {
Account previous = previousById.get(a.Id);
if (previous.OwnerId != a.OwnerId) {
System.debug('owner changed');
}
}
}
public void handleAfterInsert(List<Account> inserted) {
for (Account a : inserted) {
System.debug(a.Name);
}
}
}Remediation
Take the records as parameters - handleAfterUpdate(List<Account> updated, Map<Id, Account> previousById) - and let the trigger itself pass Trigger.new and Trigger.oldMap in, once, from the context where they are known to exist. The handler is then callable from a batch and testable without DML.