Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Supporting changing exclude criteria alongside entity recreation #223

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
"comment": "support changing exclude criteria alongside entity recreations.",
"packageName": "@itwin/imodel-transformer",
"email": "22119573+nick4598@users.noreply.github.com",
"dependentChangeType": "patch"
}
1 change: 1 addition & 0 deletions common/api/imodel-transformer.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,7 @@ export class IModelTransformer extends IModelExportHandler {
onExportModel(sourceModel: Model): void;
onExportRelationship(sourceRelationship: Relationship): void;
onExportSchema(schema: ECSchemaMetaData.Schema): Promise<void | ExportSchemaResult>;
onSkipElement(elementId: Id64String): void;
onTransformElement(sourceElement: Element_2): ElementProps;
protected onTransformElementAspect(sourceElementAspect: ElementAspect): ElementAspectProps;
onTransformModel(sourceModel: Model, targetModeledElementId: Id64String): ModelProps;
Expand Down
41 changes: 40 additions & 1 deletion packages/transformer/src/IModelTransformer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,14 @@ export class IModelTransformer extends IModelExportHandler {
public readonly context: IModelCloneContext;
private _syncType?: SyncType;

// The following two maps are used to aid in entity recreation when changing exclude criteria. A recreated entity is one which has been deleted in one changeset and then re-inserted in another changeset. They both would point to the same targetId by virtue of having the same federationGuid.
// If the transformer is processing changesets which include an entity recreation and IModelExporter.excludeElement is called on the re-inserted entity id then the initial delete should propagate to the target.
// These maps aid in propagating the initial delete.
/** a map of the id in the target to the id of the re-inserted entity id */
private _targetIdToSourceInsertId = new Map<Id64String, Id64String>();
/** a map of the inserted entity Id to the deleted entity id. Note these ids are different but they are considered the same entity due to having the same federationguid. */
private _sourceInsertIdToSourceDeleteId = new Map<Id64String, Id64String>();

/** The Id of the Element in the **target** iModel that represents the **source** repository as a whole and scopes its [ExternalSourceAspect]($backend) instances. */
public get targetScopeElementId(): Id64String {
return this._options.targetScopeElementId;
Expand Down Expand Up @@ -2375,6 +2383,19 @@ export class IModelTransformer extends IModelExportHandler {
}
}

/** Override of [IModelExportHandler.onSkipElement]($transformer) that is called when [IModelExporter]($transformer) excludes an element from being exported. */
public override onSkipElement(elementId: Id64String): void {
if (this._sourceInsertIdToSourceDeleteId.has(elementId)) {
const deleteId = this._sourceInsertIdToSourceDeleteId.get(elementId)!;
Logger.logInfo(
TransformerLoggerCategory.IModelTransformer,
"Adding element back to sourceDbChanges.deleteIds because it was excluded from being exported and detected as an entity recreation.",
{ elementId, deleteId }
);
this.exporter.sourceDbChanges?.element.deleteIds.add(deleteId);
}
}

/** Override of [IModelExportHandler.onDeleteRelationship]($transformer) that is called when [IModelExporter]($transformer) detects that a [Relationship]($backend) has been deleted from the source iModel.
* This override propagates the delete to the target iModel via [IModelImporter.deleteRelationship]($transformer).
*/
Expand Down Expand Up @@ -2820,8 +2841,13 @@ export class IModelTransformer extends IModelExportHandler {
const targetElementId = this.context.findTargetElementId(
insertedSourceElementId
);
if (Id64.isValid(targetElementId))
if (Id64.isValid(targetElementId)) {
alreadyImportedElementInserts.add(targetElementId);
this._targetIdToSourceInsertId.set(
targetElementId,
insertedSourceElementId
);
}
}
);
this.exporter.sourceDbChanges?.model.insertIds.forEach(
Expand Down Expand Up @@ -3021,6 +3047,19 @@ export class IModelTransformer extends IModelExportHandler {
this.exporter.sourceDbChanges?.element.deleteIds.delete(
changedInstanceId
);
const insertId = this._targetIdToSourceInsertId.get(targetId!);
// All IDs in alreadyImportedElementInserts are also added as a key to targetIdtoSourceInsertId map.
nodeAssert(insertId);
this._sourceInsertIdToSourceDeleteId.set(insertId, changedInstanceId);
Logger.logTrace(
TransformerLoggerCategory.IModelTransformer,
"Element recreation detected.",
{
targetId,
idWhenDeleted: changedInstanceId,
idWhenInserted: insertId,
}
);
}
if (alreadyImportedModelInserts.has(targetId!)) {
this.exporter.sourceDbChanges?.model.deleteIds.delete(
Expand Down
36 changes: 32 additions & 4 deletions packages/transformer/src/test/TestUtils/TimelineTestUtil.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
} from "../IModelTransformerUtils";
import { IModelTestUtils } from "./IModelTestUtils";
import { omit } from "@itwin/core-bentley";
import { IModelExporter } from "../../IModelExporter";

const saveAndPushChanges = async (
accessToken: string,
Expand Down Expand Up @@ -253,7 +254,15 @@ export type TimelineStateChange =
source: string,
opts?: {
since?: number;
initTransformer?: (transformer: IModelTransformer) => void;
init?: {
initTransformer?: (transformer: IModelTransformer) => void;
initExporterBeforeTransformerInitialize?: (
exporter: IModelExporter
) => Promise<void>;
initExporterAfterTransformerInitialize?: (
exporter: IModelExporter
) => Promise<void>;
};
expectThrow?: boolean;
assert?: {
afterProcessChanges?: (transformer: IModelTransformer) => void;
Expand Down Expand Up @@ -357,7 +366,15 @@ export async function runTimeline(
src: string,
opts: {
since?: number;
initTransformer?: (transformer: IModelTransformer) => void;
init?: {
initTransformer?: (transformer: IModelTransformer) => void;
initExporterBeforeTransformerInitialize?: (
exporter: IModelExporter
) => Promise<void>;
initExporterAfterTransformerInitialize?: (
exporter: IModelExporter
) => Promise<void>;
};
expectThrow?: boolean;
assert?: {
afterProcessChanges?: (transformer: IModelTransformer) => void;
Expand Down Expand Up @@ -498,7 +515,7 @@ export async function runTimeline(
syncSource,
{
since: startIndex,
initTransformer,
init: initFxns,
expectThrow,
assert: assertFxns,
},
Expand All @@ -520,7 +537,18 @@ export async function runTimeline(
: { index: undefined },
},
});
initTransformer?.(syncer);
await initFxns?.initExporterBeforeTransformerInitialize?.(
syncer.exporter
);

initFxns?.initTransformer?.(syncer);
if (initFxns?.initExporterAfterTransformerInitialize) {
// only call initialize if initExporterAfterTransformerInitialize is set, otherwise process() will take care of it.
// The expectThrow logic currently relies on process calling initialize
await initFxns?.initExporterAfterTransformerInitialize?.(
syncer.exporter
);
}
try {
await syncer.process();
expect(
Expand Down
140 changes: 130 additions & 10 deletions packages/transformer/src/test/standalone/IModelTransformerHub.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1302,7 +1302,11 @@ describe("IModelTransformerHub", () => {
master: {
sync: [
"branch1",
{ initTransformer: setForceOldRelationshipProvenanceMethod },
{
init: {
initTransformer: setForceOldRelationshipProvenanceMethod,
},
},
],
},
}, // first master<-branch1 reverse sync picking up new relationship from branch imodel
Expand Down Expand Up @@ -1346,7 +1350,11 @@ describe("IModelTransformerHub", () => {
branch1: {
sync: [
"master",
{ initTransformer: setForceOldRelationshipProvenanceMethod },
{
init: {
initTransformer: setForceOldRelationshipProvenanceMethod,
},
},
],
},
}, // forward sync master->branch1 to pick up delete of relationship
Expand Down Expand Up @@ -2975,6 +2983,101 @@ describe("IModelTransformerHub", () => {
}
});

for (const excludeBeforeTransformerInitialize of [true, false]) {
it.only(`should be able to handle entity recreation and a switching of exclude criteria ${excludeBeforeTransformerInitialize ? "before transformer initialize" : "after transformer initialize"}`, async () => {
// Create imodel, insert element
// Fork iModel
// push two changesets to source iModel, first one deletes element. second inserts element with same fed guid as the deleted element.
// Run transformation calling excludeElement on the inserted element id.
// expect that the delete from the first changeset makes its way to the target iModel.

const syncMasterToBranch = excludeBeforeTransformerInitialize
? {
branch: {
sync: [
"master",
{
init: {
initExporterBeforeTransformerInitialize: async (
exporter: IModelExporter
) => {
exporter.excludeElement(idOfElementAfterRecreation);
},
},
},
],
},
}
: {
branch: {
sync: [
"master",
{
init: {
initExporterAfterTransformerInitialize: async (
exporter: IModelExporter
) => {
exporter.excludeElement(idOfElementAfterRecreation);
},
},
},
],
},
};

let propsOfElementToRecreate: ElementProps;
let idOfElementAfterRecreation: Id64String;

const timeline: Timeline = {
0: { master: { 1: 1 } },
1: { branch: { branch: "master" } },
2: {
assert({ master, branch }) {
const sourceId = IModelTestUtils.queryByUserLabel(master.db, "1");
expect(sourceId).to.not.be.undefined;
const elem = master.db.elements.getElement(sourceId);
expect(elem).to.not.be.undefined;
expect(elem.federationGuid).to.not.be.undefined;
propsOfElementToRecreate = elem.toJSON();
expect(IModelTestUtils.queryByUserLabel(branch.db, "1")).to.not.be
.undefined;
},
},
3: { master: { 1: deleted } },
4: {
master: {
manualUpdate(db) {
delete propsOfElementToRecreate.id;
propsOfElementToRecreate.userLabel = "recreated";
idOfElementAfterRecreation = db.elements.insertElement(
propsOfElementToRecreate
);
expect(idOfElementAfterRecreation).to.not.be.undefined;
},
},
},
5: syncMasterToBranch as any,
6: {
assert({ branch }) {
const elem = IModelTestUtils.queryByUserLabel(branch.db, "1");
const newElemLabel = IModelTestUtils.queryByUserLabel(
branch.db,
"recreated"
); // we changed the userlabel in master so make sure that didnt make it to target
expect(elem).to.equal(Id64.invalid);
expect(newElemLabel).to.equal(Id64.invalid);
},
},
};

const { tearDown } = await runTimeline(timeline, {
iTwinId,
accessToken,
});
await tearDown();
});
}

it("should preserve FederationGuid when element is recreated within the same changeset and across changesets", async () => {
const sourceIModelName: string =
IModelTransformerTestUtils.generateUniqueName("Source");
Expand Down Expand Up @@ -3797,7 +3900,10 @@ describe("IModelTransformerHub", () => {
"branch",
{
expectThrow: false,
initTransformer: setBranchRelationshipDataBehaviorToUnsafeMigrate,
init: {
initTransformer:
setBranchRelationshipDataBehaviorToUnsafeMigrate,
},
},
],
},
Expand Down Expand Up @@ -3894,7 +4000,10 @@ describe("IModelTransformerHub", () => {
sync: [
"branch",
{
initTransformer: setBranchRelationshipDataBehaviorToUnsafeMigrate,
init: {
initTransformer:
setBranchRelationshipDataBehaviorToUnsafeMigrate,
},
},
],
},
Expand Down Expand Up @@ -3947,7 +4056,10 @@ describe("IModelTransformerHub", () => {
sync: [
"master",
{
initTransformer: setBranchRelationshipDataBehaviorToUnsafeMigrate,
init: {
initTransformer:
setBranchRelationshipDataBehaviorToUnsafeMigrate,
},
},
],
},
Expand Down Expand Up @@ -4033,9 +4145,11 @@ describe("IModelTransformerHub", () => {
sync: [
"branch",
{
initTransformer: (transformer) =>
(transformer["_options"]["branchRelationshipDataBehavior"] =
"unsafe-migrate"),
init: {
initTransformer: (transformer) =>
(transformer["_options"]["branchRelationshipDataBehavior"] =
"unsafe-migrate"),
},
},
], // Sync again with no changes except for ones which may get made by unsafe-migrate.
},
Expand Down Expand Up @@ -4174,7 +4288,10 @@ describe("IModelTransformerHub", () => {
"master",
{
expectThrow: false,
initTransformer: setBranchRelationshipDataBehaviorToUnsafeMigrate,
init: {
initTransformer:
setBranchRelationshipDataBehaviorToUnsafeMigrate,
},
},
],
},
Expand All @@ -4185,7 +4302,10 @@ describe("IModelTransformerHub", () => {
"branch",
{
expectThrow: false,
initTransformer: setBranchRelationshipDataBehaviorToUnsafeMigrate,
init: {
initTransformer:
setBranchRelationshipDataBehaviorToUnsafeMigrate,
},
},
],
},
Expand Down
Loading