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

Support 'custom' changes being added to exporter.sourceDbChanges #172

Open
wants to merge 321 commits into
base: main
Choose a base branch
from

Conversation

nick4598
Copy link
Collaborator

No description provided.

MichaelBelousov and others added 30 commits May 2, 2023 12:02
* Change files

* bump to latest manual nightly release

* fix typo
* @note Its expected that this function be overridden by a subclass of transformer if it needs to modify sourceDbChanges.
*/
protected async addCustomChanges(
_sourceDbChanges?: ChangedInstanceIds
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_sourceDbChanges? should be required. As API user I have no idea how to handle undefined source db changes. It is impossible to add custom changes and I am too worried to throw error (what if it is expected).
It should be transformers responsibility to handle case when source db change is undefined and act accordingly (throw / not to call addCustomChanges / initalize custom changes / something else).

* @note In most cases, this method does not need to be called. Its only for consumers to mimic changes as if they were found in a changeset, which should only be useful in certain cases such as the changing of filter criteria for a preexisting master branch relationship.
* @beta
*/
public addCustomModelChange(changeType: SqliteChangeOp, ids: Id64Arg): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Continuing this discussion: https://github.com/iTwin/imodel-transformer/pull/172/files/e576fa6b45d47d3996366ff9a910a9d5369ba892#r1888402014

I realized where was my logical mistake was in previous discussion, these are fixed requirements of how model can be inserted via custom changes API:

  • model id is added to insertedIds
  • modeled element id is added to inserted ids (it is same as model id). This is needed as model can not be created without modeled element.
  • model of modeled element is added to updated / inserted ids. This is needed so that modeled element is exported to target in exportModeledContents()

Turns out model of modeled element needs to be marked as changed (not parent model of inserted model as I thought previously). I got mixed up as those are the same in my test data (both equal to RepositoryModel).

Suggest to update current implementation to call addCustomElementChange instead of 'handleChange' with modelet element id:

    addCustomModelChange(changeType, ids) {
        for (const id of core_bentley_1.Id64.iterable(ids)) {
            this.handleChange(this.model, changeType, id);
            // Also add the model's modeledElement to the element changes. The modeledElement and model go hand in hand.
            addCustomElementChange(changeType, id);
        }
    }

This is test that I used for my investigation, which is failing with current implementation:

  it.only("should insert Model", async () => {
    const masterIModelName = "Master";
    const masterSeedFileName = path.join(outputDir, `${masterIModelName}.bim`);
    if (IModelJsFs.existsSync(masterSeedFileName))
      IModelJsFs.removeSync(masterSeedFileName);
    const masterSeedState = { 1: 1, 2: 1, 20: 1, 21: 1, 40: 1, 41: 2, 42: 3 };
    const masterSeedDb = SnapshotDb.createEmpty(masterSeedFileName, {
      rootSubject: { name: masterIModelName },
    });
    // eslint-disable-next-line deprecation/deprecation
    masterSeedDb.nativeDb.setITwinId(iTwinId); // workaround for "ContextId was not properly setup in the checkpoint" issue
    const schemaPathForMultiAspect =
      IModelTransformerTestUtils.getPathToSchemaWithMultiAspect();
    await masterSeedDb.importSchemas([schemaPathForMultiAspect]);

    populateTimelineSeed(masterSeedDb, masterSeedState);

    const masterSeed: TimelineIModelState = {
      // HACK: we know this will only be used for seeding via its path and performCheckpoint
      db: masterSeedDb as any as BriefcaseDb,
      id: "master-seed",
      state: masterSeedState,
    };

    let physicalModelIdInSource: Id64String | undefined;
    let modelUnderRepositoryModel: Id64String | undefined;
    const timeline: Timeline = [
      { master: { seed: masterSeed } }, // masterSeedState is above
      { branch1: { branch: "master" } },
      { master: { 100: 100, 101: 101 } },
      {
        master: {
          manualUpdate(db) {
            physicalModelIdInSource = PhysicalModel.insert(
              db,
              IModel.rootSubjectId,
              "MyPhysicalModel"
            );
            modelUnderRepositoryModel = DefinitionModel.insert(
              db,
              IModel.rootSubjectId,
              "MyModelUnderRepositoryModel"
            );
          },
        },
      },
      { branch1: { sync: ["master"] } }, // master->branch1 forward sync to pick up relationship change
      {
        branch1: {
          // delete model and element from branch so that we can attempt to add it back in as a custom 'Inserted' change
          manualUpdate(db) {
            const physicalPartitionIdInTarget =
              IModelTestUtils.queryByCodeValue(db, "MyPhysicalModel");
            expect(physicalPartitionIdInTarget).to.not.equal(Id64.invalid);
            db.models.deleteModel(physicalPartitionIdInTarget);
            db.elements.deleteElement(physicalPartitionIdInTarget);
            const modelUnderRepositoryModelId =
              IModelTestUtils.queryByCodeValue(
                db,
                "MyModelUnderRepositoryModel"
              );
            expect(modelUnderRepositoryModelId).to.not.equal(Id64.invalid);
            db.models.deleteModel(modelUnderRepositoryModelId);
            db.elements.deleteElement(modelUnderRepositoryModelId);
          },
        },
      },
      {
        assert({ branch1 }) {
          // Extra assert to make sure relationship and element are deleted in branch1
         
          // assume if element is gone, aspect is gone
          const physicalPartitionIdInTarget = IModelTestUtils.queryByCodeValue(
            branch1.db,
            "MyPhysicalModel"
          );
          expect(physicalPartitionIdInTarget).to.equal(Id64.invalid);
          const modelUnderRepositoryModelInTarget =
            IModelTestUtils.queryByCodeValue(
              branch1.db,
              "MyModelUnderRepositoryModel"
            );
          expect(modelUnderRepositoryModelInTarget).to.equal(Id64.invalid);
        },
      },
      {
        branch1: {
          sync: [
            "master",
            {
              init: {
                afterInitializeExporter: async (exporter) => {
                  exporter.sourceDbChanges?.addCustomModelChange(
                    "Inserted",
                    physicalModelIdInSource!
                  );
                  exporter.sourceDbChanges?.addCustomModelChange(
                    "Inserted",
                    modelUnderRepositoryModel!
                  );
                },
              },
            },
          ],
        },
      },
      {
        assert({ branch1 }) {
          const physicalPartitionIdInTarget = IModelTestUtils.queryByCodeValue(
            branch1.db,
            "MyPhysicalModel"
          );
          expect(physicalPartitionIdInTarget).to.not.equal(Id64.invalid);
          expect(branch1.db.elements.getElement(physicalPartitionIdInTarget)).to
            .not.be.undefined;
          expect(branch1.db.models.getModel(physicalPartitionIdInTarget)).to.not
            .be.undefined;
          const modelUnderRepositoryModelInTarget =
            IModelTestUtils.queryByCodeValue(
              branch1.db,
              "MyModelUnderRepositoryModel"
            );
          expect(modelUnderRepositoryModelInTarget).to.not.equal(Id64.invalid);
          expect(
            branch1.db.elements.getElement(modelUnderRepositoryModelInTarget)
          ).to.not.be.undefined;
          expect(branch1.db.models.getModel(modelUnderRepositoryModelInTarget))
            .to.not.be.undefined;
        },
      }
    ];
    const { tearDown } = await runTimeline(timeline, {
      iTwinId,
      accessToken,
    });
    await tearDown();
  });

): void {
// if delete unnecessary?
for (const id of Id64.iterable(ids)) {
this.addModelToUpdated(id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To improve performance, consider querying models of all elements at once in addModelToUpdated()

);
this.exporter.sourceDbChanges?.element.insertIds.forEach((id) =>
hasElementChangedCache.add(id)
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not be needed if this is fixed: #229

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants