From 907fb5b32c554733dec15b867d9cd8093565f850 Mon Sep 17 00:00:00 2001 From: Nick Tessier <22119573+nick4598@users.noreply.github.com> Date: Thu, 16 May 2024 16:05:31 -0400 Subject: [PATCH] Support providing the reversesync and sync versions when unsafe-migrate is set. (#179) Support providing the reversesync and sync versions when unsafe-migrate is set. This is to support svcs team and their use cases so they no longer have to have code surrounding this. A change we made to make syncDirection determined automatically by the transformer has made some of their patches no longer work. I also ran into a bug when trying to test this which had 2 issues, the first being the hasElementChangedCache was getting aspect ids added to it. I believe it was ALWAYS supposed to be only element ids, but I made that mistake when converting to use Affan's changeset reader. The query that used to be used for the hasElementChangedCache had a where clause: [`ic.ChangedInstance.ClassId IS (ONLY BisCore.Element)`](https://github.com/iTwin/imodel-transformer/commit/576295137997aa8911e5589f1b1377a19902221d#diff-d07eab136b812db5c475ddb91cbc589d5fd6e5943638c6e2028994d6f58fe80bR687) The second part of the bug: I believe we should no longer be checking `if Id64.isValid(targetElementId)` when deciding if we should return early because an element has not changed. This seems to be leftover from a time when you needed the targetElementId to find the ESA which described the relationship between the source and target element. I believe at the time of Mike making his change to no longer search for ESAs he didn't want to remove targetElementId (so he changed it to _targetElementId), because that COULD be a breaking change if someone was overriding the hasElementChanged function. The services team does not appear to be overriding the hasElementChanged function, and this change will be part of a 1.0.0 major release. --- ...-6d5fba74-5711-471e-9f99-4ef766c48902.json | 7 + packages/transformer/src/IModelTransformer.ts | 110 ++++++-- .../standalone/IModelTransformerHub.test.ts | 256 ++++++++++++++++++ 3 files changed, 350 insertions(+), 23 deletions(-) create mode 100644 change/@itwin-imodel-transformer-6d5fba74-5711-471e-9f99-4ef766c48902.json diff --git a/change/@itwin-imodel-transformer-6d5fba74-5711-471e-9f99-4ef766c48902.json b/change/@itwin-imodel-transformer-6d5fba74-5711-471e-9f99-4ef766c48902.json new file mode 100644 index 00000000..9836aceb --- /dev/null +++ b/change/@itwin-imodel-transformer-6d5fba74-5711-471e-9f99-4ef766c48902.json @@ -0,0 +1,7 @@ +{ + "type": "prerelease", + "comment": "Support setting the reversesync and sync version when unsafe-migrate is set. Also fix bug with haselementchangedcache", + "packageName": "@itwin/imodel-transformer", + "email": "22119573+nick4598@users.noreply.github.com", + "dependentChangeType": "patch" +} diff --git a/packages/transformer/src/IModelTransformer.ts b/packages/transformer/src/IModelTransformer.ts index d9176f07..2d5617d2 100644 --- a/packages/transformer/src/IModelTransformer.ts +++ b/packages/transformer/src/IModelTransformer.ts @@ -248,6 +248,23 @@ export interface IModelTransformOptions { * @default "reject" */ branchRelationshipDataBehavior?: "unsafe-migrate" | "reject"; + + /** + * The forward sync 'version' to set on the scoping ESA @see ExternalSourceAspectProps upon startup, if the version property on the scoping ESA is undefined or empty string. + * @note This option is not without risk! You must also set @see branchRelationshipDataBehavior to "unsafe-migrate". + * @note This value is ignored if the version property on the scoping ESA is NOT undefined or empty string. + * @default "" + */ + unsafeFallbackSyncVersion?: string; + + /** + * The reverse sync version to set on the scoping ESA @see TargetScopeProvenanceJsonProps upon startup, if the reverseSync property on the scoping ESA is undefined or empty string. + * @note This option is not without risk! You must also set @see branchRelationshipDataBehavior to "unsafe-migrate". + * @note This value is ignored if the reverseSyncVersion property on the scoping ESA is NOT undefined or empty string. + * @default "" + */ + unsafeFallbackReverseSyncVersion?: string; + /** * Skip propagating changes made to the root subject, dictionaryModel and IModelImporter._realityDataSourceLinkPartitionStaticId (0xe) * @default false @@ -1125,26 +1142,71 @@ export class IModelTransformer extends IModelExportHandler { } else { // foundEsaProps is defined. aspectProps.id = foundEsaProps.aspectId; - aspectProps.version = - foundEsaProps.version ?? - (this._options.branchRelationshipDataBehavior === "unsafe-migrate" - ? "" - : undefined); + aspectProps.version = foundEsaProps.version; aspectProps.jsonProperties = foundEsaProps.jsonProperties ? JSON.parse(foundEsaProps.jsonProperties) - : this._options.branchRelationshipDataBehavior === "unsafe-migrate" - ? { - pendingReverseSyncChangesetIndices: [], - pendingSyncChangesetIndices: [], - reverseSyncVersion: "", - } - : undefined; + : undefined; + this.handleUnsafeMigrate(aspectProps); } this._targetScopeProvenanceProps = aspectProps as typeof this._targetScopeProvenanceProps; } + private handleUnsafeMigrate(aspectProps: { + version?: string; + jsonProperties?: TargetScopeProvenanceJsonProps; + }): void { + if (this._options.branchRelationshipDataBehavior !== "unsafe-migrate") + return; + const fallbackSyncVersionToUse = + this._options.unsafeFallbackSyncVersion ?? ""; + const fallbackReverseSyncVersionToUse = + this._options.unsafeFallbackReverseSyncVersion ?? ""; + + if (aspectProps.version === undefined || aspectProps.version === "") + aspectProps.version = fallbackSyncVersionToUse; + + if (aspectProps.jsonProperties === undefined) { + aspectProps.jsonProperties = { + pendingReverseSyncChangesetIndices: [], + pendingSyncChangesetIndices: [], + reverseSyncVersion: fallbackReverseSyncVersionToUse, + }; + } else if ( + aspectProps.jsonProperties.reverseSyncVersion === undefined || + aspectProps.jsonProperties.reverseSyncVersion === "" + ) { + aspectProps.jsonProperties.reverseSyncVersion = + fallbackReverseSyncVersionToUse; + } + + /** + * This case will only be hit when: + * - first transformation was performed on pre-fedguid transformer. + * - a second processAll transformation was performed on the same target-source iModels post-fedguid transformer. + * - change processing was invoked on for the second 'initial' transformation. + * NOTE: This case likely does not exist anymore, but we will keep it just to be sure. + */ + if ( + aspectProps.jsonProperties.pendingReverseSyncChangesetIndices === + undefined + ) { + Logger.logWarning( + loggerCategory, + "Property pendingReverseSyncChangesetIndices missing on the jsonProperties of the scoping ESA. Setting to []." + ); + aspectProps.jsonProperties.pendingReverseSyncChangesetIndices = []; + } + if (aspectProps.jsonProperties.pendingSyncChangesetIndices === undefined) { + Logger.logWarning( + loggerCategory, + "Property pendingSyncChangesetIndices missing on the jsonProperties of the scoping ESA. Setting to []." + ); + aspectProps.jsonProperties.pendingSyncChangesetIndices = []; + } + } + /** * Iterate all matching federation guids and ExternalSourceAspects in the provenance iModel (target unless reverse sync) * and call a function for each one. @@ -1570,13 +1632,9 @@ export class IModelTransformer extends IModelExportHandler { /** Returns true if a change within sourceElement is detected. * @param sourceElement The Element from the source iModel - * @param targetElementId The Element from the target iModel to compare against. * @note A subclass can override this method to provide custom change detection behavior. */ - protected hasElementChanged( - sourceElement: Element, - _targetElementId: Id64String - ): boolean { + protected hasElementChanged(sourceElement: Element): boolean { if (this._sourceChangeDataState === "no-changes") return false; if (this._sourceChangeDataState === "unconnected") return true; nodeAssert( @@ -1902,11 +1960,7 @@ export class IModelTransformer extends IModelExportHandler { } } - if ( - Id64.isValid(targetElementId) && - !this.hasElementChanged(sourceElement, targetElementId) - ) - return; + if (!this.hasElementChanged(sourceElement)) return; this.collectUnmappedReferences(sourceElement); @@ -2883,6 +2937,12 @@ export class IModelTransformer extends IModelExportHandler { )) { relationshipECClassIds.add(row.ECInstanceId); } + const elementECClassIds = new Set(); + for await (const row of this.sourceDb.createQueryReader( + "SELECT ECInstanceId FROM ECDbMeta.ECClassDef where ECInstanceId IS (BisCore.Element)" + )) { + elementECClassIds.add(row.ECInstanceId); + } // For later use when processing deletes. const alreadyImportedElementInserts = new Set(); @@ -2935,7 +2995,11 @@ export class IModelTransformer extends IModelExportHandler { change.Scope.Id === this.targetScopeElementId ) { elemIdToScopeEsa.set(change.Element.Id, change); - } else if (changeType === "Inserted" || changeType === "Updated") + } else if ( + (changeType === "Inserted" || changeType === "Updated") && + change.ECClassId !== undefined && + elementECClassIds.has(change.ECClassId) + ) hasElementChangedCache.add(change.ECInstanceId); } diff --git a/packages/transformer/src/test/standalone/IModelTransformerHub.test.ts b/packages/transformer/src/test/standalone/IModelTransformerHub.test.ts index 42947777..b85478a1 100644 --- a/packages/transformer/src/test/standalone/IModelTransformerHub.test.ts +++ b/packages/transformer/src/test/standalone/IModelTransformerHub.test.ts @@ -3518,6 +3518,262 @@ describe("IModelTransformerHub", () => { masterSeedDb.close(); }); + it("should throw when pendingSyncChangesetIndices and pendingReverseSyncChangesetIndices are undefined and then not throw when they're undefined, but 'unsafe-migrate' is set.", async () => { + let targetScopeProvenanceProps: ExternalSourceAspectProps | undefined; + const setBranchRelationshipDataBehaviorToUnsafeMigrate = ( + transformer: IModelTransformer + ) => + (transformer["_options"]["branchRelationshipDataBehavior"] = + "unsafe-migrate"); + + const timeline: Timeline = [ + { master: { 1: 1, 2: 2, 3: 1 } }, + { branch: { branch: "master" } }, + { branch: { 1: 2, 4: 1 } }, + // eslint-disable-next-line @typescript-eslint/no-shadow + { + assert({ master, branch }) { + const scopeProvenanceCandidates = branch.db.elements + .getAspects( + IModelDb.rootSubjectId, + ExternalSourceAspect.classFullName + ) + .filter( + (a) => + (a as ExternalSourceAspect).identifier === master.db.iModelId + ); + expect(scopeProvenanceCandidates).to.have.length(1); + const targetScopeProvenance = + scopeProvenanceCandidates[0].toJSON() as ExternalSourceAspectProps; + + expect(targetScopeProvenance).to.deep.subsetEqual({ + identifier: master.db.iModelId, + version: `${master.db.changeset.id};${master.db.changeset.index}`, + jsonProperties: JSON.stringify({ + pendingReverseSyncChangesetIndices: [1], + pendingSyncChangesetIndices: [], + reverseSyncVersion: ";0", // not synced yet + }), + } as ExternalSourceAspectProps); + targetScopeProvenanceProps = targetScopeProvenance; + }, + }, + { + branch: { + manualUpdate(branch) { + // Check it fails without pendingReverseSync and pendingSync + const missingPendings = JSON.stringify({ + pendingReverseSyncChangesetIndices: undefined, + pendingSyncChangesetIndices: undefined, + reverseSyncVersion: ";0", + }); + branch.elements.updateAspect({ + ...targetScopeProvenanceProps!, + jsonProperties: missingPendings as any, + }); + }, + }, + }, + { + master: { + // Our pendingReverseSyncChangesetIndices are undefined, so we expect to throw when we try to read them. + sync: ["branch", { expectThrow: true }], + }, + }, + { + assert({ branch }) { + const aspect = branch.db.elements.getAspect( + targetScopeProvenanceProps!.id! + ); + expect(aspect).to.not.be.undefined; + expect((aspect as ExternalSourceAspect).jsonProperties).to.equal( + JSON.stringify({ + pendingReverseSyncChangesetIndices: undefined, + pendingSyncChangesetIndices: undefined, + reverseSyncVersion: ";0", + }) + ); + }, + }, + { + master: { + // Our pendingReverseSyncChangesetIndices are undefined, but our branchrelationshipdatabehavior is 'unsafe-migrate' so we expect the transformer to correct the issue. + sync: [ + "branch", + { + expectThrow: false, + initTransformer: setBranchRelationshipDataBehaviorToUnsafeMigrate, + }, + ], + }, + }, + { + assert({ branch }) { + const aspect = branch.db.elements.getAspect( + targetScopeProvenanceProps!.id! + ); + expect(aspect).to.not.be.undefined; + const jsonProps = JSON.parse( + (aspect as ExternalSourceAspect).jsonProperties! + ); + expect((aspect as any).version).to.match(/;1$/); + expect(jsonProps.reverseSyncVersion).to.match(/;3$/); + expect(jsonProps).to.deep.subsetEqual({ + pendingReverseSyncChangesetIndices: [4], + pendingSyncChangesetIndices: [2], + }); + }, + }, + ]; + + const { tearDown } = await runTimeline(timeline, { + iTwinId, + accessToken, + }); + + await tearDown(); + }); + + it("should set unsafeVersions correctly when branchRelationshipDataBehavior is 'unsafe-migrate'", async () => { + let targetScopeProvenanceProps: ExternalSourceAspectProps | undefined; + const setBranchRelationshipDataBehaviorToUnsafeMigrate = ( + transformer: IModelTransformer + ) => { + transformer["_options"]["branchRelationshipDataBehavior"] = + "unsafe-migrate"; + transformer["_options"]["unsafeFallbackReverseSyncVersion"] = ";2"; + transformer["_options"]["unsafeFallbackSyncVersion"] = ";3"; + }; + + const timeline: Timeline = [ + { master: { 1: 1, 2: 2, 3: 1 } }, + { branch: { branch: "master" } }, + { branch: { 1: 2, 4: 1 } }, + { branch: { 5: 1 } }, + // eslint-disable-next-line @typescript-eslint/no-shadow + { + assert({ master, branch }) { + const scopeProvenanceCandidates = branch.db.elements + .getAspects( + IModelDb.rootSubjectId, + ExternalSourceAspect.classFullName + ) + .filter( + (a) => + (a as ExternalSourceAspect).identifier === master.db.iModelId + ); + expect(scopeProvenanceCandidates).to.have.length(1); + const targetScopeProvenance = + scopeProvenanceCandidates[0].toJSON() as ExternalSourceAspectProps; + + expect(targetScopeProvenance).to.deep.subsetEqual({ + identifier: master.db.iModelId, + version: `${master.db.changeset.id};${master.db.changeset.index}`, + jsonProperties: JSON.stringify({ + pendingReverseSyncChangesetIndices: [1], + pendingSyncChangesetIndices: [], + reverseSyncVersion: ";0", // not synced yet + }), + } as ExternalSourceAspectProps); + targetScopeProvenanceProps = targetScopeProvenance; + }, + }, + { + branch: { + manualUpdate(branch) { + // Check it fails without version now. + branch.elements.updateAspect({ + ...targetScopeProvenanceProps!, + jsonProperties: undefined, + } as ExternalSourceAspectProps); + }, + }, + }, + { + master: { + // Reverse sync passing along our unsafeReverseSyncVersion which intentionally skips the changeset that added the element with userlabel 4. + sync: [ + "branch", + { + initTransformer: setBranchRelationshipDataBehaviorToUnsafeMigrate, + }, + ], + }, + }, + { + assert({ master, branch }) { + // Assert that we skipped the changeset: { branch: { 1: 2, 4: 1 } } during our reverse sync. + const expectedState = { 1: 1, 2: 2, 3: 1, 5: 1 }; + expect(master.state).to.deep.equal(expectedState); + expect(branch.state).to.deep.equal({ ...expectedState, 1: 2, 4: 1 }); + assertElemState(master.db, expectedState); + assertElemState(branch.db, { ...expectedState, 1: 2, 4: 1 }); + }, + }, + // repeat all above for forward sync scenario! + { master: { 2: 4, 6: 1 } }, + { master: { 7: 1 } }, + // Update our targetscopeprovenanceprops + { + assert({ master, branch }) { + const scopeProvenanceCandidates = branch.db.elements + .getAspects( + IModelDb.rootSubjectId, + ExternalSourceAspect.classFullName + ) + .filter( + (a) => + (a as ExternalSourceAspect).identifier === master.db.iModelId + ); + expect(scopeProvenanceCandidates).to.have.length(1); + const targetScopeProvenance = + scopeProvenanceCandidates[0].toJSON() as ExternalSourceAspectProps; + targetScopeProvenanceProps = targetScopeProvenance; + }, + }, + { + branch: { + manualUpdate(branch) { + // Check it fails without version now. + branch.elements.updateAspect({ + ...targetScopeProvenanceProps!, + version: undefined, + } as ExternalSourceAspectProps); + }, + }, + }, + { + branch: { + // Reverse sync passing along our unsafeReverseSyncVersion which intentionally skips the changeset that added the element with userlabel 4. + sync: [ + "master", + { + initTransformer: setBranchRelationshipDataBehaviorToUnsafeMigrate, + }, + ], + }, + }, + { + assert({ master, branch }) { + // Assert that we skipped the changeset: { master: { 2: 4, 6: 1, } }, during our forward sync.. making it so those properties didn't make it to the branch. + const expectedMasterState = { 1: 1, 2: 4, 3: 1, 5: 1, 6: 1, 7: 1 }; + const expectedBranchState = { 1: 2, 2: 2, 3: 1, 4: 1, 5: 1, 7: 1 }; + expect(master.state).to.deep.equal(expectedMasterState); + expect(branch.state).to.deep.equal(expectedBranchState); + assertElemState(master.db, expectedMasterState); + assertElemState(branch.db, expectedBranchState); + }, + }, + ]; + + const { tearDown } = await runTimeline(timeline, { + iTwinId, + accessToken, + }); + + await tearDown(); + }); + it("should fail processingChanges on pre-version-tracking forks unless branchRelationshipDataBehavior is 'unsafe-migrate'", async () => { let targetScopeProvenanceProps: ExternalSourceAspectProps | undefined; let targetScopeElementId: Id64String | undefined;