SavePoint and Rollback usage consideration

Question

I came through this Trailhead module to better understand Service Layers with some fflib examples.

Unfortunately I am struggling to understand this part:

The following example uses a SavePoint to encapsulate and wrap the database operations within a Service method. As per the design considerations, the SavePoint is used to avoid the caller catching exceptions (perhaps resulting from the second DML statement). That would then result in the Apex runtime committing updates to the opportunity lines (first DML statement), causing a partial update to the database.

Given the provided example in the applyDiscounts method of the Trailhead module, I do not see the benefits of encapsulating the 2 DML updates in a try/catch and rollback if the exception is thrown.

As far as I understand, this would be the default behaviour as well and both DML will be rolled back if the second fails.

Since it is as well throwing the catched exception after the specific rollback of those 2 DMLs, the default rollback will continue on the caller.

I see how if it does not handle the rollback inside of the Service method, the caller can also Set a SavePoint before the Service method call, wrap the call in a try/catch, and rollback if any exception has been thrown in the Service method. This would basically result in the same outcome.

Is this specifically what it aims to avoid? So that logic is encapsulated in the Service method and we can keep the caller cleaner?

What if (unlikely) from the caller perspective, we would want the first DML to be actually commited even if the second fails? This approach makes it not possible, providing claningness against flexibility.

Answer 1

The main point here is that the service layer should make no assumptions about what the caller is doing. For example, if someone wrote:

try {
  applyDiscounts(oppIds, discount);
} catch(Exception e) {
  ApexPages.addMessage(
    new ApexPages.Message(
      ApexPages.SEVERITY.ERROR,
      e.getMessage()
    )
  );
}

Without a rollback in place, they may not realize that they've left the database in a partial update condition. It would be incredibly easy to accidentally do it. Further, this particular method would almost certainly always want to be atomic.

Further, let's say they just wrote this:

public void saveData() {
  applyDiscounts(oppIds, discount);
}

In the context of a Visualforce page. Now, the user gets an ugly white screen with an error message that causes them to lose all their work. That's why a developer is more likely to use the first piece of code; hard crashes are usually not ideal.

In practice, almost every database transaction you'll ever want to do needs to be atomic. It is highly unlikely you'll ever want to allow the parents to be updated but the children not. Simply codifying this in your service layer is almost always ideal.

In fact, I can't recall a single time I've ever wanted to allow this sort of partial update to occur. It always leads to bad things happening. There are times when it would be preferable to allow a partial update with allOrNone set to false, but that typically only applies to triggers, and then you still need to coordinate passing errors back up to the parent record.