From e98225aaf6c58b4971693aba3ec49ef375250f58 Mon Sep 17 00:00:00 2001 From: Jiarui Zhang <91682445+jiaruiz717@users.noreply.github.com> Date: Mon, 24 Jun 2024 10:52:46 -0400 Subject: [PATCH] skipPropagateChangesToRootElements should be default true (#188) fix iTwin/imodel-transformer#177 set the default value of skipPropagateChangesToRootElements to be true --------- Co-authored-by: Nick Tessier <22119573+nick4598@users.noreply.github.com> --- ...-53ad9100-036c-41c0-83b9-d954608888cf.json | 7 ++ packages/transformer/src/IModelImporter.ts | 5 +- packages/transformer/src/IModelTransformer.ts | 9 +- .../test/standalone/IModelTransformer.test.ts | 102 ++++++++++-------- .../standalone/IModelTransformerHub.test.ts | 4 +- 5 files changed, 76 insertions(+), 51 deletions(-) create mode 100644 change/@itwin-imodel-transformer-53ad9100-036c-41c0-83b9-d954608888cf.json diff --git a/change/@itwin-imodel-transformer-53ad9100-036c-41c0-83b9-d954608888cf.json b/change/@itwin-imodel-transformer-53ad9100-036c-41c0-83b9-d954608888cf.json new file mode 100644 index 00000000..03a8f944 --- /dev/null +++ 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 index 3109bd5c..44b18231 100644 --- 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(); } diff --git a/packages/transformer/src/IModelTransformer.ts b/packages/transformer/src/IModelTransformer.ts index 1d218ce5..16e49b07 100644 --- 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 index 4e5c261f..89e684a0 100644 --- 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 index 68ce53ab..f62174fc 100644 --- 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