Propagating Changes to Sets of Element

[Ellord207]

We are having an issue with elements persisting after the transaction. We understand why, but want to:

1. Make sure the reason is a design choice and not a bug
2. Validate the workflow and find an alternative solution

Explain workflow:

We are trying to create an automated system to maintain a Sheet Index and its Sheet Numbers (a unique string describing a Sheet). The Sheet Index for this purpose can be thought of as an ordered filing system or a tree graph. We have a Sheet Index that acts as the root, and SheetIndexEntries make up the ordered children. SheetIndexEntries can either be a SheetIndexFolder or a SheetReference. These are both bis structures from iTwin.js Core. We have created a SheetNumberingAspect, which describes the “recipe” for the Sheet Number. The system will then traverse the ordered tree and use that recipe to calculate the Sheet Number and update the SheetReference’s Code Value.

Currently we are triggering this system to process the Sheet Index using the iModel.txns.onCommit event.

The Motivation for using onCommit

We decided on this approach for two main reasons.

We wanted a central hook for the system since there are wide, varied and not yet well-defined systems that could trigger these changes. - A central event monitoring the changes to the iModel reduces the overhead on those systems and the possibility of the SheetIndex becoming outdated.

• The transaction manager has a system for filtering the classes that trigger an event, and the event is raised once for each transaction, this avoid unnecessarily processing intermediate steps during a transaction (such as multiple sheets are inserted into the Sheet Index).

We did consider some alternatives.

The element drives element relationship doesn’t seem to work for this case. The tree needs to be processed as a whole, meaning that driving individual elements doesn’t really work, but we have considered a few dependance graphs.

1. Linked list; seems redundant given the ordered nature of the tree. Plus, it would have to be maintained, which defeats the purpose.
2. There is also the possibility of all elements in a Sheet Index tree “driving” the Sheet Index element and using that relationship to trigger a tree process, which also seemed awkward, since the driven element is not actually updated.
3. Building on 2, including a driving relationship from parent to child would create a cyclical dependency and can’t be used.

We have also considered using the class “on” event handles, such as “onInsert” or “onUpdate,” but are not sure if this is possible since we don’t own those classes outside of iTwin.js Core.

Explaining the findings around the issue for why it is unclear how this should work

During our investigation of the bug, we found a few comments implying this workflow should not work.

• This comment calls out the exact scenario we are observing:
  BESQLLite.cpp // save briefcase local values again after _OnCommit. It can cause additional changes
• This comment seems to hint at why it’s happening:
  BriefcaseTxns.test.ts // NB: onCommit is produced after we process all changes. onModelGeometryChanged is produced during change processing.

Our theory based on these comments, reading the code, and assistance from CoPiliot is that the onCommit event is raised to the typescript level on a different thread. By the time the typescript was executed, the native code has already finalized the transaction.
While these comments reveal that this behavior was understood by the core developers at some point, we wanted to confirm this was a design decision.

Questions

1. Are changes made during the “onCommit” event intended to be included in a different change set?
2. Is there a more appropriate trigger to kick off the processing for Sheet Numbering?
3. If not, is there a possibility of a “beforeCommit” event being introduced to easily support this workflow.

CC: @Alfonso-Martello & @nimam-bentley

[ColinKerr]

@khanaffan

[khanaffan]

We should have documented onCommit() more thoroughly. It is invoked after SaveTxn() is called, meaning all changes in the current session have already been written to the txns table. Any modifications made within onCommit() will be included in the next call to saveChanges().

However, this behavior is misleading and potentially dangerous. The onCommit() hook is triggered before SQLite has actually committed anything to disk. This creates a false sense of finality, especially since saveChanges() typically ensures both:

1. The transaction is recorded in the txns table.
2. The database changes are flushed to disk.

In this case, any changes made inside onCommit() are persisted to disk but not recorded in the txns table, breaking the consistency model.

In short: it is unsafe to use onCommit() to modify the iModel. It’s simply called at the wrong point in the lifecycle.

---

✅ Proposed Fix

Introduce a more structured commit lifecycle:

1. saveChanges() triggers onBeginCommit(changes: SqliteChangesetReader)
2. Any changes made between onBeginCommit() and onEndCommit() are marked as indirect, similar to how ElementDrivesElement or propagation changes are handled.
3. At the end, onEndCommit() is called. This hook squashes the current session changes with propagated and indirect changes, ensuring consistency.

---

📊 Mermaid Sequence Diagram

sequenceDiagram
    participant App
    participant iModel
    participant SQLite

    App->>iModel: saveChanges()
    iModel->>App: onBeginCommit(changes)
    App->>iModel: Make indirect/propagated changes
    iModel->>App: onEndCommit()
    iModel->>SQLite: Write all changes to txns table
    iModel->>SQLite: Commit to disk

Loading

This diagram shows a cleaner separation of responsibilities and ensures that all changes—direct or indirect—are properly tracked and committed.

[khanaffan]

Or it could be as simple as moving the onCommit() at right point.

diff --git a/iModelCore/iModelPlatform/DgnCore/TxnManager.cpp b/iModelCore/iModelPlatform/DgnCore/TxnManager.cpp
index a75c945f8d..7d30706ddf 100644
--- a/iModelCore/iModelPlatform/DgnCore/TxnManager.cpp
+++ b/iModelCore/iModelPlatform/DgnCore/TxnManager.cpp
@@ -797,6 +797,18 @@ ChangeTracker::OnCommitStatus TxnManager::_OnCommit(bool isCommit, Utf8CP operat
             }
         }
     }
+    // At this point, all of the changes to all tables have been applied. Tell TxnMonitors
+    NotifyOnCommit();
+    if (HasDataChanges()) { 
+        UndoChangeSet onCommitChanges;
+        DbResult result = onCommitChanges.FromChangeTrack(*this);
+        if (BE_SQLITE_OK != result) {
+            LOG.errorv("failed to create changeset from onCommitChanges: %s", BeSQLiteLib::GetErrorName(result));
+            return OnCommitStatus::Abort;
+        }
+        dataChanges.ConcatenateWith(onCommitChanges);
+        Restart(); // clear the change tracker since we copied any changes to dataChanges
+    }
 
     if (!ddlChanges._IsEmpty()) {
         rc = SaveTxn(ddlChanges, operation, TxnType::Ddl);
@@ -822,9 +834,6 @@ ChangeTracker::OnCommitStatus TxnManager::_OnCommit(bool isCommit, Utf8CP operat
         }
     }
 
-    // At this point, all of the changes to all tables have been applied. Tell TxnMonitors
-    NotifyOnCommit();
-
     if (propagateChanges) {
         OnEndValidate();
     }

[Ellord207]

I feel think the second solution is cleaner, but I'm also a worried it being a breaking change for users. If they are using it for something dependent on timing, trying to tracking this change could be VERY difficult as it was for me to find it.

I do like the idea of aborting a changeset due to these propagated changes failing. That is another issue in the current system, but we have been largely ignoring that if it does happen.

Either solution would work for us, doubly so if it can allow for aborting the save on failure.

[MichaelSwigerAtBentley]

Hi Nathan

Would it be possible to extend the BIS Core class in order to create a class definition that you own and then use the domain handler functions that you mentioned (onInsert, onUpdate, ect)? Those functions are definable for class-specific scenarios like this. It is also potentially possible we can add this logic into the domain handlers for the BIS core classes.

[MichaelSwigerAtBentley]

There are also handlers available for more specific scenarios, like onChildInsert/onSubModelInsert

[Ellord207]

I did not think to test this, but I prefer solutions that act on a "per commit" bases rather than reacting to individual elements. There can be large sets of changes to a sheet index in a single commit. To propagate changes, the whole sheet index needs to be traversed. This could be hundreds of elements. Traversing the Sheet Index multiple times per change could create performance issues, and expanding the algorithm to cache some of that state might be able to help, but would also add development time and maintenance cost.

[pmconne]

Our theory based on these comments, reading the code, and assistance from CoPiliot is that the onCommit event is raised to the typescript level on a different thread.

CoPilot lied to you - all of this is happening synchronously on a single thread, though execution may pass from native to JS and back.

Pardon a potentially dumb question but do the sheet numbers actually need to be stored in the iModel? Couldn't they be computed as needed?

[Ellord207]

Our theory based on these comments, reading the code, and assistance from CoPiliot is that the onCommit event is raised to the typescript level on a different thread.

CoPilot lied to you - all of this is happening synchronously on a single thread, though execution may pass from native to JS and back.

Thanks for the clarification on that. I know there are multiple threads when querying, so I wasn't sure how synchronous the backend was.

Pardon a potentially dumb question but do the sheet numbers actually need to be stored in the iModel? Couldn't they be computed as needed?

That's what we were initially planning, but it was determined through conversations with the Bis-working group and project management that they should be stored as Code Values on the Sheet Reference elements. This is for 3 main reasons.

1. Conceptually, this is make sense from a BIS perspective
2. The nature of the Codes ensures they are unique.
3. The value will probably need to be stored in the iModel to work with text fields (there is the possibility using a ECView, but that doesn't allow for storing in the Code or for easily ensuring uniqueness).

There are the counter arguments that storing the sheet numbers is actually redundant since all the information to calculate it is also stored in the iModel and if users REALLY care that these values are unique.

[pmconne]

The "Booster" team has a similar situation wherein a change to a single element can cascade to affect many other elements. Have you talked to them about how they are solving that problem? Are their solutions applicable to your problem?

[Ellord207]

We have not. Thank you for the suggestion.