Support 'custom' changes being added to exporter.sourceDbChanges

[nick4598]

No description provided.

[JulijaRamoskiene]

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();
  });

[nick4598]

So does this mean the 'modelUnderRepositoryModel' is where we fail? expect(modelUnderRepositoryModelInTarget).to.not.equal(Id64.invalid);? because its modeled element has a model which is the repositoryModel, and we didn't add it to the updated? (Note: after rereading im usnure about tihs, but I hope my two assertions below are correct)

I didn't realize the modeled element could have a model that wasn't the model it was modelling... I started looking at the documentation for model:

Im guessing 'modeled element's model' is being refereenced by this line here? "The Element modeling the car-as-a-whole is also in a Model. What Element is that Model sub-modeling? BIS escapes from infinite regression by defining a special RepositoryModel that is not required to sub-model some other Element." docs

Is that right?

And to take it further the modeled element's model would be found right here on an element:

image taken from here

Is that also right?

[nick4598]

Thanks for figuring this out. Ive made the change that you suggested and also changed my test to no longer explicitly call addCustomElementChange on the physical and modelunderrepositorymodel, relying on the addCustomModelChange function to handle it.

My test started failing after I did that so then added your change in to call addCustomElementChange within addCustomModelChange and its now passing.

[JulijaRamoskiene]

@aruniverse , @DanRod1999, can you advise me:
getCustomRelationshipDataFromId() and hasCustomRelationshipChanges() are not meant to be called by third party code, those methods are supposed to be "internal".
How to properly handle/mark such methods in 'imodel-transformer' package?I feel we might want to remove them in future or implement in different way. Is it enough to mark them as 'beta'? Those will never be something that user should use.
I found one case where such internal method was marked as private and then called like this:

imodel-transformer/packages/transformer/src/IModelTransformer.ts

Line 3169 in 4e4a7df

await this.exporter["exportAllAspects"](); // eslint-disable-line @typescript-eslint/dot-notation

[aruniverse]

We are tracking this larger issue of using itwinjs-core internal apis in #196. We will be looking at adding a new release tag doc to use which permits internal consumers like transformers and a few other pkgs from using certain apis. This is not a problem while on 4.x, but will be on 5

[DanRod1999]

I thought this is new code that is internal to the transformer repo, not an instance of transformer using internal code? Am I understanding this wrong? If so can we label these new methods as internal? Beta may make sense too if we feel this might change though.

[aruniverse]

ah i misread, dan is right

[JulijaRamoskiene]

I am unsure why this was added. I suspect Nick stumbled into some issue similar to the one I also reported: #236

We should consider if this is really the right approach for this problem. My first instinct is that we should rather save processed version and use that as default value, instead of "process the current changeset of the source iModel being exported when startChangeset.id is undefined.". I am not sure if this change means only last (current) change set would be processed? In such case this could result in unpredictable behavior if there was more than 1 change set added since last transformation.