Skip to content

Commit

Permalink
Revert "save and push changes after transformer is complete (#153)" (#…
Browse files Browse the repository at this point in the history 
…185)

Leaving the responsibility up to the consumer. We already require them
to download the iModels and acquire locks themselves, so we should also
let them push changes as pushing changes releases locks.

There have also been mentions of some transforms intentionally inserting
data that isn't meant to be transformed back over, which would not be
possible if we pushed changes for consumers, because then they wouldn't
be able to add that data as part of the pending changeset indices.
  • Loading branch information
@nick4598
nick4598 authored Jun 4, 2024
1 parent a9e3ac3 commit 8ae591e
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 166 deletions.
7 changes: 7 additions & 0 deletions change/@itwin-imodel-transformer-359b2d25-3f51-4eab-b5d4-a6602ba69819.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "Revert \"save and push changes after transformer is complete (#153)\"",
"packageName": "@itwin/imodel-transformer",
"email": "[email protected]",
"dependentChangeType": "patch"
}
86 changes: 18 additions & 68 deletions packages/transformer/src/IModelTransformer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -374,37 +374,10 @@ export interface InitOptions {
/**
* Arguments for [[IModelTransformer.processChanges]]
*/
export type ProcessChangesOptions = ExportChangesOptions &
FinalizeTransformationOptions;

/**
* Options which modify the behavior of [[IModelTransformer.finalizeTransformation]], called at the end of [[IModelTransformer.processAll]] and [[IModelTransformer.processChanges]].
* @beta
*/
export interface FinalizeTransformationOptions {
/**
* This description will be used when the transformer needs to push changes to the branch during a reverse sync.
* @default `Update provenance in response to a reverse sync to iModel: ${this.targetDb.iModelId}`
*/
reverseSyncBranchChangesetDescription?: string;
/**
* This description will be used when the transformer needs to push changes to master during a reverse sync.
* @default `Reverse sync of iModel: ${this.sourceDb.iModelId}`
*/
reverseSyncMasterChangesetDescription?: string;
/**
* This description will be used when the transformer needs to push changes to the branch during a forward sync.
* @default `Forward sync of iModel: ${this.sourceDb.iModelId}`
*/
forwardSyncBranchChangesetDescription?: string;
/**
* This description will be used when the transformer needs to push changes to the branch during a provenance init transform.
* @default `initialized branch provenance with master iModel: ${this.sourceDb.iModelId}`
*/
provenanceInitTransformChangesetDescription?: string;
export type ProcessChangesOptions = ExportChangesOptions & {
/** how to call saveChanges on the target. Must call targetDb.saveChanges, should not edit the iModel */
saveTargetChanges?: (transformer: IModelTransformer) => Promise<void>;
}
};

type ChangeDataState =
| "uninited"
Expand Down Expand Up @@ -2345,6 +2318,10 @@ export class IModelTransformer extends IModelExportHandler {
pendingReverseSyncChangesetIndicesKey,
];

// NOTE that as documented in [[processChanges]], this assumes that right after
// transformation finalization, the work will be saved immediately, otherwise we've
// just marked this changeset as a synchronization to ignore, and the user can add other
// stuff to it which would break future synchronizations
for (
let i = this._startingChangesetIndices.target + 1;
i <= this.targetDb.changeset.index + 1;
Expand Down Expand Up @@ -2392,9 +2369,7 @@ export class IModelTransformer extends IModelExportHandler {
}

// FIXME<MIKE>: is this necessary when manually using low level transform APIs? (document if so)
private async finalizeTransformation(
options?: FinalizeTransformationOptions
) {
private finalizeTransformation() {
this.importer.finalize();
this.updateSynchronizationVersion();

Expand Down Expand Up @@ -2426,36 +2401,6 @@ export class IModelTransformer extends IModelExportHandler {
(this.targetDb as any).codeValueBehavior = "trim-unicode-whitespace";
}
/* eslint-enable @itwin/no-internal */

const defaultSaveTargetChanges = () => this.targetDb.saveChanges();
await (options?.saveTargetChanges ?? defaultSaveTargetChanges)(this);
if (this.isReverseSynchronization) this.sourceDb.saveChanges();

const description = `${
this._isProvenanceInitTransform
? options?.provenanceInitTransformChangesetDescription ??
`initialized branch provenance with master iModel: ${this.sourceDb.iModelId}`
: this.isForwardSynchronization
? options?.forwardSyncBranchChangesetDescription ??
`Forward sync of iModel: ${this.sourceDb.iModelId}`
: options?.reverseSyncMasterChangesetDescription ??
`Reverse sync of iModel: ${this.sourceDb.iModelId}`
}`;

if (this.targetDb.isBriefcaseDb()) {
// This relies on authorizationClient on iModelHost being defined, otherwise this will fail
await this.targetDb.pushChanges({
description,
});
}
if (this.isReverseSynchronization && this.sourceDb.isBriefcaseDb()) {
// This relies on authorizationClient on iModelHost being defined, otherwise this will fail
await this.sourceDb.pushChanges({
description:
options?.reverseSyncBranchChangesetDescription ??
`Update provenance in response to a reverse sync to iModel: ${this.targetDb.iModelId}`,
});
}
}

/** Imports all relationships that subclass from the specified base class.
Expand Down Expand Up @@ -3280,9 +3225,7 @@ export class IModelTransformer extends IModelExportHandler {
/** Export everything from the source iModel and import the transformed entities into the target iModel.
* @note [[processSchemas]] is not called automatically since the target iModel may want a different collection of schemas.
*/
public async processAll(
options?: FinalizeTransformationOptions
): Promise<void> {
public async processAll(): Promise<void> {
this.logSettings();
this.initScopeProvenance();
await this.initialize();
Expand Down Expand Up @@ -3320,7 +3263,7 @@ export class IModelTransformer extends IModelExportHandler {
this.importer.optimizeGeometry(this._options.optimizeGeometry);

this.importer.computeProjectExtents();
await this.finalizeTransformation(options);
this.finalizeTransformation();
}

/** previous provenance, either a federation guid, a `${sourceFedGuid}/${targetFedGuid}` pair, or required aspect props */
Expand All @@ -3346,7 +3289,9 @@ export class IModelTransformer extends IModelExportHandler {

/** Export changes from the source iModel and import the transformed entities into the target iModel.
* Inserts, updates, and deletes are determined by inspecting the changeset(s).
* @note the transformer saves and pushes changes when its work is complete.
* @note the transformer assumes that you saveChanges after processing changes. You should not
* modify the iModel after processChanges until saveChanges, failure to do so may result in corrupted
* data loss in future branch operations
* @note if no startChangesetId or startChangeset option is provided as part of the ProcessChangesOptions, the next unsynchronized changeset
* will automatically be determined and used
* @note To form a range of versions to process, set `startChangesetId` for the start (inclusive) of the desired range and open the source iModel as of the end (inclusive) of the desired range.
Expand All @@ -3366,8 +3311,13 @@ export class IModelTransformer extends IModelExportHandler {
this.importer.optimizeGeometry(this._options.optimizeGeometry);

this.importer.computeProjectExtents();
this.finalizeTransformation();

const defaultSaveTargetChanges = () => {
this.targetDb.saveChanges();
};

await this.finalizeTransformation(options);
await (options.saveTargetChanges ?? defaultSaveTargetChanges)(this);
}

/** Changeset data must be initialized in order to build correct changeOptions.
Expand Down
16 changes: 11 additions & 5 deletions packages/transformer/src/test/TestUtils/TimelineTestUtil.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import {
} from "@itwin/core-common";
import { Point3d, YawPitchRollAngles } from "@itwin/core-geometry";
import {
FinalizeTransformationOptions,
IModelTransformer,
IModelTransformOptions,
} from "../../IModelTransformer";
Expand Down Expand Up @@ -259,7 +258,6 @@ export type TimelineStateChange =
assert?: {
afterProcessChanges?: (transformer: IModelTransformer) => void;
};
finalizeTransformationOptions?: FinalizeTransformationOptions;
},
];
}
Expand Down Expand Up @@ -364,7 +362,6 @@ export async function runTimeline(
assert?: {
afterProcessChanges?: (transformer: IModelTransformer) => void;
};
finalizeTransformationOptions?: FinalizeTransformationOptions;
},
]
| undefined;
Expand Down Expand Up @@ -453,6 +450,11 @@ export async function runTimeline(
});
await provenanceInserter.processAll();
provenanceInserter.dispose();
await saveAndPushChanges(
accessToken,
branchDb,
"initialized branch provenance"
);
} else if ("seed" in newIModelEvent) {
await saveAndPushChanges(
accessToken,
Expand Down Expand Up @@ -499,7 +501,6 @@ export async function runTimeline(
initTransformer,
expectThrow,
assert: assertFxns,
finalizeTransformationOptions,
},
] = getSync(event)!;
// if the synchronization source is master, it's a normal sync
Expand All @@ -520,7 +521,6 @@ export async function runTimeline(
await syncer.processChanges({
accessToken,
startChangeset: startIndex ? { index: startIndex } : undefined,
...finalizeTransformationOptions,
});
expect(
expectThrow === false || expectThrow === undefined,
Expand Down Expand Up @@ -558,6 +558,12 @@ export async function runTimeline(
}

target.state = getIModelState(target.db); // update the tracking state

if (!expectThrow) {
if (!isForwardSync)
await saveAndPushChanges(accessToken, source.db, stateMsg);
await saveAndPushChanges(accessToken, target.db, stateMsg);
}
} else {
const alreadySeenIModel = trackedIModels.get(iModelName)!;
let stateMsg: string;
Expand Down
102 changes: 9 additions & 93 deletions packages/transformer/src/test/standalone/IModelTransformerHub.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ import {
} from "@itwin/core-common";
import { Point3d, YawPitchRollAngles } from "@itwin/core-geometry";
import {
FinalizeTransformationOptions,
IModelExporter,
IModelImporter,
IModelTransformer,
Expand Down Expand Up @@ -265,6 +264,8 @@ describe("IModelTransformerHub", () => {
startChangeset: { id: sourceDb.changeset.id },
});
transformer.dispose();
targetDb.saveChanges();
await targetDb.pushChanges({ accessToken, description: "Import #1" });
TransformerExtensiveTestScenario.assertTargetDbContents(
sourceDb,
targetDb
Expand Down Expand Up @@ -331,8 +332,6 @@ describe("IModelTransformerHub", () => {
targetDb,
ElementRefersToElements.classFullName
);
const changesetIndexOfTargetDbBeforeChangelessTransform =
targetDb.changeset.index;
const targetImporter = new CountingIModelImporter(targetDb);
const transformer = new TestIModelTransformer(sourceDb, targetImporter);
await transformer.processChanges({ accessToken });
Expand Down Expand Up @@ -360,14 +359,12 @@ describe("IModelTransformerHub", () => {
count(targetDb, ElementRefersToElements.classFullName),
"Second import should not add relationships"
);
targetDb.saveChanges();
assert.isFalse(targetDb.nativeDb.hasPendingTxns());
// Validate same changeset index, because there were no changes in this run of the transform.
expect(changesetIndexOfTargetDbBeforeChangelessTransform).to.not.be
.undefined;
expect(targetDb.changeset.index).to.equal(
changesetIndexOfTargetDbBeforeChangelessTransform
);

await targetDb.pushChanges({
accessToken,
description: "Should not actually push because there are no changes",
});
transformer.dispose();
}

Expand Down Expand Up @@ -420,6 +417,8 @@ describe("IModelTransformerHub", () => {
const transformer = new TestIModelTransformer(sourceDb, targetDb);
await transformer.processChanges({ accessToken });
transformer.dispose();
targetDb.saveChanges();
await targetDb.pushChanges({ accessToken, description: "Import #2" });
TestUtils.ExtensiveTestScenario.assertUpdatesInDb(targetDb);

// Use IModelExporter.exportChanges to verify the changes to the targetDb
Expand Down Expand Up @@ -3244,89 +3243,6 @@ describe("IModelTransformerHub", () => {
sinon.restore();
});

it("should allow custom changeset descriptions", async () => {
const pushChangeset = sinon.spy(HubMock, "pushChangeset");
const csDescriptions: FinalizeTransformationOptions = {
forwardSyncBranchChangesetDescription:
"this is a test forward sync on the branch",
reverseSyncBranchChangesetDescription:
"this is a test reverse sync on the branch",
reverseSyncMasterChangesetDescription:
"this is a test reverse sync on the master",
};
const makeCsPropsDescriptionMatcher = (description: string) => {
return sinon.match.has(
"changesetProps",
sinon.match.has("description", description)
);
};
const timeline: Timeline = [
{ master: { 1: 1, 2: 2, 3: 1 } },
{ branch: { branch: "master" } },
{ branch: { 1: 2, 4: 1 } },
{
master: {
sync: ["branch", { finalizeTransformationOptions: csDescriptions }],
},
},
{
assert() {
expect(
pushChangeset.calledWith(
makeCsPropsDescriptionMatcher(
csDescriptions.reverseSyncBranchChangesetDescription!
)
)
).to.be.true;
expect(
pushChangeset.calledWith(
makeCsPropsDescriptionMatcher(
csDescriptions.reverseSyncMasterChangesetDescription!
)
)
).to.be.true;

// We haven't passed csDescriptions to a forward sync yet so expect false
expect(
pushChangeset.calledWith(
makeCsPropsDescriptionMatcher(
csDescriptions.forwardSyncBranchChangesetDescription!
)
)
).to.be.false;
},
},
{ master: { 5: 1 } },
{
branch: {
sync: ["master", { finalizeTransformationOptions: csDescriptions }],
},
},
{
assert() {
expect(
pushChangeset.calledWith(
makeCsPropsDescriptionMatcher(
csDescriptions.forwardSyncBranchChangesetDescription!
)
)
).to.be.true;
},
},
];

const { tearDown } = await runTimeline(timeline, {
iTwinId,
accessToken,
transformerOpts: {
// force aspects so that reverse sync has to edit the target
forceExternalSourceAspectProvenance: true,
},
});

await tearDown();
});

it("should be able to handle a transformation which deletes a relationship and then elements of that relationship", async () => {
const masterIModelName = "MasterDeleteRelAndEnds";
const masterSeedFileName = path.join(outputDir, `${masterIModelName}.bim`);
Expand Down

0 comments on commit 8ae591e

Please sign in to comment.