TDbContextEntityDataSetStore.ApplyChanges: one SaveChanges per item and no transaction, so a failing row leaves the batch half-applied

[Stemonik]

Summary

TDbContextEntityDataSetStore.ApplyChanges (Sources/Web/Dext.Web.EntityDataSetApi.pas) applies a batch of dataset changes one item at a time, with one SaveChanges per item and no transaction around the batch. If item 7 fails, items 1–6 are already committed, and the caller gets a per-item result list back, not a failed request.

Where

In the for i := 0 to AChanges.Count - 1 loop, each branch ends with its own ADbContext.SaveChanges (inserted, modified, deleted). The exception is caught per item and stored in ItemResult.ErrorMessage, and the loop carries on. Neither ApplyChanges nor the TEntityDataSetApi.Map<T> endpoint that calls it opens a transaction.

Why it matters

A client dataset's ApplyUpdates is usually understood as all-or-nothing: a master row plus its details, or a set of rows that only make sense together. With the current behaviour a half-applied batch is a normal outcome, and the database is left in a state no client ever asked for. It is also one round trip per item.

We learned the same lesson in our own server: batches that committed per statement looked fine until a real failure in the middle of one.

If partial success per item is intended, it may be worth documenting and making it opt-in. Something to check along the way, which we have not verified: after a failed SaveChanges, is the failed entity still in the ChangeTracker? If it is, the next item's SaveChanges would retry it.

Suggested fix

• Wrap the loop in a transaction on ADbContext.
• Track all items first and call SaveChanges once.
• On failure, roll back and return the failing index.
• If per-item results must stay, make that an explicit option rather than the default.

Checked against f440e79a.

🤖 Generated with Claude Code

[cesarliws]

Thank you for this thorough analysis! This is an important catch regarding transactional integrity and data consistency.

You are completely right on all counts:

1. Lack of Atomicity: Client dataset update batches are fundamentally expected to be all-or-nothing (atomic). Committing per-row without a transaction can easily leave the database in an inconsistent, partially-applied state upon intermediate failures.
2. ChangeTracker Pollution: Your intuition is spot-on — if an entity fails during SaveChanges, it remains tracked in the ChangeTracker in a dirty state (esAdded, esModified, etc.), causing subsequent iterations to re-attempt saving the failed entity.
3. Performance Overhead: Calling SaveChanges per item creates unnecessary round-trips to the database instead of leveraging batch execution.

Proposed Solution

I agree with your suggested approach:

1. Default to Atomic / Transactional Execution:
   • Open a transaction (ADbContext.Database.BeginTransaction).
   • Stage all operations into the ChangeTracker first.
   • Execute a single ADbContext.SaveChanges inside a try ... finally block, calling Commit on success and Rollback on failure.
   • Populate key mappings for generated/identity keys after the batch succeeds.
2. Opt-in Partial Updates (if needed):
   • If per-item partial success ("continue on error") is desired for specific use cases, provide an explicit option for it. If an item fails in that mode, explicitly detach the failed entity (ADbContext.ChangeTracker.Detach(EntityObj)) before proceeding to avoid contaminating subsequent saves.

Would you be open to contributing a PR for this? If so, we'd be delighted to review it! Otherwise, I will prioritize this fix internally.