skipPropagateChangesToRootElements should be default true (#188)

fix #177 set the default value of skipPropagateChangesToRootElements to be true --------- Co-authored-by: Nick Tessier <22119573+nick4598@users.noreply.github.com>
iTwin · Jun 24, 2024 · e98225a · e98225a
1 parent bdfd569
commit e98225a
Show file tree

Hide file tree

Showing 5 changed files with 76 additions and 51 deletions.
diff --git a/change/@itwin-imodel-transformer-53ad9100-036c-41c0-83b9-d954608888cf.json b/change/@itwin-imodel-transformer-53ad9100-036c-41c0-83b9-d954608888cf.json
@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "skipPropagateChangesToRootElements should be default true",
+  "packageName": "@itwin/imodel-transformer",
+  "email": "91682445+jiaruiz717@users.noreply.github.com",
+  "dependentChangeType": "patch"
+}
diff --git a/packages/transformer/src/IModelImporter.ts b/packages/transformer/src/IModelImporter.ts
@@ -71,7 +71,8 @@ export interface IModelImportOptions {
   simplifyElementGeometry?: boolean;
   /**
    * Skip propagating changes made to the root subject, dictionaryModel and IModelImporter._realityDataSourceLinkPartitionStaticId (0xe)
-   * @default false
+   * If it is set to false, changes to root elements are propagated, the root subject name gets changed and leads to the iModelDb.name property being updated in .initializeiModelDb
+   * @default true
    */
   skipPropagateChangesToRootElements?: boolean;
 }
@@ -136,7 +137,7 @@ export class IModelImporter {
         options?.preserveElementIdsForFiltering ?? false,
       simplifyElementGeometry: options?.simplifyElementGeometry ?? false,
       skipPropagateChangesToRootElements:
-        options?.skipPropagateChangesToRootElements ?? false,
+        options?.skipPropagateChangesToRootElements ?? true,
     };
     this._duplicateCodeValueMap = new Map<Id64String, string>();
   }

diff --git a/packages/transformer/src/IModelTransformer.ts b/packages/transformer/src/IModelTransformer.ts
@@ -267,7 +267,8 @@ export interface IModelTransformOptions {
 
   /**
    * Skip propagating changes made to the root subject, dictionaryModel and IModelImporter._realityDataSourceLinkPartitionStaticId (0xe)
-   * @default false
+   * If it is set to false, changes to root elements are propagated, the root subject name gets changed and leads to the iModelDb.name property being updated in .initializeiModelDb
+   * @default true
    */
   skipPropagateChangesToRootElements?: boolean;
 }
@@ -634,6 +635,8 @@ export class IModelTransformer extends IModelExportHandler {
         options?.danglingReferencesBehavior ?? "reject",
       branchRelationshipDataBehavior:
         options?.branchRelationshipDataBehavior ?? "reject",
+      skipPropagateChangesToRootElements:
+        options?.skipPropagateChangesToRootElements ?? true,
     };
     this._isProvenanceInitTransform = this._options
       .wasSourceIModelCopiedToTarget
@@ -1335,7 +1338,7 @@ export class IModelTransformer extends IModelExportHandler {
       isReverseSynchronization: this.isReverseSynchronization,
       fn,
       skipPropagateChangesToRootElements:
-        this._options.skipPropagateChangesToRootElements ?? false,
+        this._options.skipPropagateChangesToRootElements ?? true,
     });
   }
 
@@ -3327,7 +3330,7 @@ export class IModelTransformer extends IModelExportHandler {
     if (!this._isSynchronization) return {};
     return {
       skipPropagateChangesToRootElements:
-        this._options.skipPropagateChangesToRootElements ?? false,
+        this._options.skipPropagateChangesToRootElements,
       accessToken: opts.accessToken,
       ...(this._csFileProps
         ? { csFileProps: this._csFileProps }

diff --git a/packages/transformer/src/test/standalone/IModelTransformer.test.ts b/packages/transformer/src/test/standalone/IModelTransformer.test.ts
@@ -366,7 +366,7 @@ describe("IModelTransformer", () => {
       assert.equal(targetImporter.numModelsUpdated, 0);
       assert.equal(targetImporter.numElementsInserted, 0);
       // TODO: explain which elements are updated
-      assert.equal(targetImporter.numElementsUpdated, 41);
+      assert.equal(targetImporter.numElementsUpdated, 38);
       assert.equal(targetImporter.numElementsExplicitlyDeleted, 0);
       assert.equal(targetImporter.numElementAspectsInserted, 0);
       assert.equal(targetImporter.numElementAspectsUpdated, 0);
@@ -420,7 +420,7 @@ describe("IModelTransformer", () => {
       assert.equal(targetImporter.numModelsInserted, 0);
       assert.equal(targetImporter.numModelsUpdated, 0);
       assert.equal(targetImporter.numElementsInserted, 1);
-      assert.equal(targetImporter.numElementsUpdated, 36);
+      assert.equal(targetImporter.numElementsUpdated, 33);
       /**
        * There are 5 elements deleted in TransformerExtensiveTestScenario.updateDb, but only 4 detected.
        * This is because PhysicalObject6's code is scoped to PhysicalObject5. When PhysicalObject5 is deleted, PhysicalObject6 is also deleted in the superclasses
@@ -1016,6 +1016,7 @@ describe("IModelTransformer", () => {
         {
           targetScopeElementId: subjectId,
           danglingReferencesBehavior: "ignore",
+          skipPropagateChangesToRootElements: false,
         }
       );
       transformerA2S.context.remapElement(IModel.rootSubjectId, subjectId);
@@ -3183,51 +3184,64 @@ describe("IModelTransformer", () => {
     targetDb.close();
   });
 
-  it("should not update root elements when skipPropagateChangesToRootElements is set to true", async () => {
-    const iModelShared: SnapshotDb =
-      IModelTransformerTestUtils.createSharedIModel(outputDir, ["A", "B"]);
-    const iModelA: SnapshotDb = IModelTransformerTestUtils.createTeamIModel(
-      outputDir,
-      "A",
-      Point3d.create(0, 0, 0),
-      ColorDef.green
-    );
-    IModelTransformerTestUtils.assertTeamIModelContents(iModelA, "A");
-    const iModelExporterA = new IModelExporter(iModelA);
-
-    const subjectId: Id64String = IModelTransformerTestUtils.querySubjectId(
-      iModelShared,
-      "A"
-    );
-    const transformerA2S = new IModelTransformer(
-      iModelExporterA,
-      iModelShared,
-      {
-        targetScopeElementId: subjectId,
-        danglingReferencesBehavior: "ignore",
-        skipPropagateChangesToRootElements: true,
-      }
-    );
-    transformerA2S.context.remapElement(IModel.rootSubjectId, subjectId);
-
-    // Act
-    await transformerA2S.processAll();
+  for (const skipPropagateChangesToRootElements of [true, undefined, false]) {
+    it(`should ${
+      skipPropagateChangesToRootElements === false ? "update" : "not update"
+    } root elements when skipPropagateChangesToRootElements is set to ${skipPropagateChangesToRootElements}`, async () => {
+      const iModelShared: SnapshotDb =
+        IModelTransformerTestUtils.createSharedIModel(outputDir, ["A", "B"]);
+      const iModelA: SnapshotDb = IModelTransformerTestUtils.createTeamIModel(
+        outputDir,
+        "A",
+        Point3d.create(0, 0, 0),
+        ColorDef.green
+      );
+      IModelTransformerTestUtils.assertTeamIModelContents(iModelA, "A");
+      const iModelExporterA = new IModelExporter(iModelA);
 
-    // Assert
-    const rootElements = ["0x10", "0xe"];
-    rootElements.forEach((rootElementId) => {
-      const dictionary = iModelShared.elements.getElement(rootElementId);
-      assert.equal(
-        dictionary.parent?.id,
-        "0x1",
-        `Root element '${rootElementId}' parent should not be remapped to '${dictionary.parent?.id}'.`
+      const subjectId: Id64String = IModelTransformerTestUtils.querySubjectId(
+        iModelShared,
+        "A"
       );
-    });
+      const transformerA2S = new IModelTransformer(
+        iModelExporterA,
+        iModelShared,
+        {
+          targetScopeElementId: subjectId,
+          danglingReferencesBehavior: "ignore",
+          skipPropagateChangesToRootElements,
+        }
+      );
+      transformerA2S.context.remapElement(IModel.rootSubjectId, subjectId);
+      // Act
+      await transformerA2S.processAll();
+      // Assert
+      const rootElements = ["0x10", "0xe"];
+      rootElements.forEach((rootElementId) => {
+        const rootElement = iModelShared.elements.getElement(rootElementId);
+        if (
+          skipPropagateChangesToRootElements === undefined ||
+          skipPropagateChangesToRootElements === true
+        ) {
+          assert.equal(
+            rootElement.parent?.id,
+            "0x1",
+            `Root element '${rootElementId}' parent should not be remapped to '${rootElement.parent?.id}'.`
+          );
+        } else {
+          assert.equal(
+            rootElement.parent?.id,
+            subjectId,
+            `Root element '${rootElementId}' parent should be remapped to '${rootElement.parent?.id}'.`
+          );
+        }
+      });
 
-    transformerA2S.dispose();
-    iModelA.close();
-    iModelShared.close();
-  });
+      transformerA2S.dispose();
+      iModelA.close();
+      iModelShared.close();
+    });
+  }
 
   it("IModelTransformer handles generated class nav property cycle", async () => {
     const sourceDbPath = IModelTransformerTestUtils.prepareOutputFile(

diff --git a/packages/transformer/src/test/standalone/IModelTransformerHub.test.ts b/packages/transformer/src/test/standalone/IModelTransformerHub.test.ts
@@ -4153,7 +4153,7 @@ describe("IModelTransformerHub", () => {
             0
           );
           expect(count(branch.db, ExternalSourceAspect.classFullName)).to.equal(
-            10
+            9
           );
 
           const scopeProvenanceCandidates = branch.db.elements
@@ -4191,7 +4191,7 @@ describe("IModelTransformerHub", () => {
           );
           // added because the root was modified
           expect(count(branch.db, ExternalSourceAspect.classFullName)).to.equal(
-            11
+            10
           );
 
           const scopeProvenanceCandidates = branch.db.elements