Allow Entity.forEach() to Support Relationship Classes

[MichaelSwigerAtBentley]

When calling forEach() on a Relationship Class, a Cannot get metadata error would be thrown as it was expecting only an EntityClass.

Adds a test for this case as well

[rschili]

Our Entity.getMetadata() method has the same problem. It returns an EntityClass. I never thought the type called "Entity" would be used for anything other than Entity classes....

Now I'm thinking to change that to EntityClass | RelationshipClass. But I feel that would add quite a bit of confusion.

@ColinKerr @StefanApfel-Bentley @RohitPtnkr1996 : thoughts?

I see two options:

• Entity should be limited to Entities only, and other use is illegal, so this reported issue would be WAD.
• Entity can return EntityClass or RelationshipClass. This is the more pragmatic approach, still confusing, but I prefer this.

I guess the root of this issue is this:

export class Relationship extends Entity {}

That's a mismatch with what we do in ECSchema, where Entity and Relationship are concepts on the same level that don't depend on each other.

[rschili]

After some discussion with Affan, I feel we should do something like this:

export class Entity {
    public async getMetaData(): Promise<EntityClass | RelationshipClass> {
        const ecClass = await this.iModel.schemaContext.getSchemaItem(this.schemaItemKey, ECClass);
        if (EntityClass.isEntityClass(ecClass) || RelationshipClass.isRelationshipClass(ecClass)) {
            return ecClass;
        }
        throw new Error(`Cannot get metadata for ${this.classFullName}`);
    }
}

export class Element extends Entity {
    public async override getMetaData(): Promise<EntityClass> {
        const entity = await this.iModel.schemaContext.getSchemaItem(this.schemaItemKey, EntityClass);
        if (entity !== undefined) {
            return entity;
        }
        throw new Error(`Cannot get metadata for ${this.classFullName}`);
    }
}

export class Relationship extends Entity {
    public async override getMetaData(): Promise<RelationshipClass> {
        const relationship = await this.iModel.schemaContext.getSchemaItem(this.schemaItemKey, RelationshipClass);
        if (relationship !== undefined) {
            return relationship;
        }
        throw new Error(`Cannot get metadata for ${this.classFullName}`);
    }
}

However, that is a breaking change. It will probably not affect anybody because everybody is working with subclasses of Entity. Still, we need to make sure we override this properly on every subclass.

[ColinKerr]

After some discussion with Affan, I feel we should do something like this:

export class Entity {
    public async getMetaData(): Promise<EntityClass | RelationshipClass> {
        const ecClass = await this.iModel.schemaContext.getSchemaItem(this.schemaItemKey, ECClass);
        if (EntityClass.isEntityClass(ecClass) || RelationshipClass.isRelationshipClass(ecClass)) {
            return ecClass;
        }
        throw new Error(`Cannot get metadata for ${this.classFullName}`);
    }
}

export class Element extends Entity {
    public async override getMetaData(): Promise<EntityClass> {
        const entity = await this.iModel.schemaContext.getSchemaItem(this.schemaItemKey, EntityClass);
        if (entity !== undefined) {
            return entity;
        }
        throw new Error(`Cannot get metadata for ${this.classFullName}`);
    }
}

export class Relationship extends Entity {
    public async override getMetaData(): Promise<RelationshipClass> {
        const relationship = await this.iModel.schemaContext.getSchemaItem(this.schemaItemKey, RelationshipClass);
        if (relationship !== undefined) {
            return relationship;
        }
        throw new Error(`Cannot get metadata for ${this.classFullName}`);
    }
}

However, that is a breaking change. It will probably not affect anybody because everybody is working with subclasses of Entity. Still, we need to make sure we override this properly on every subclass.

The problem stems from the fact that the class hierarchy between ecschema-metadata and core-backend are different. ecschema uses ECClass as a common base class for entity and relationship while backend has relationship directly derive from entity.

It does kind of look like the only way to fix getMetaData is to break the API.

It's a real shame that we've capitalized the D in MetaData

I guess we could deprecate getMetaData and replace with a correct getMetadata function.

[MichaelSwigerAtBentley]

@rschili @ColinKerr

Should I create a new issue for getMetadata? or do we want to fix it in this PR

[rschili]

I'd fix it in this PR, the bugs are very closely related.

[MichaelSwigerAtBentley]

I'd fix it in this PR, the bugs are very closely related.

I've prepared a new getMetadata() function and deprecated getMetaData(). While the errors are closely related, I still think we should merge that separately as it is a more drastic change than this PR's simple fix, and they are two separate issues. This PR fixes @simihartstein's issue and it might need to be backported. I'm worried tacking on more breaking changes will make that more difficult.

@aruniverse What do you think? Should we get this in now or fix both issues in one go?

[aruniverse]

I'm worried tacking on more breaking changes will make that more difficult.

Id hope there are no breaking changes.

I'm fine if you want to split it up, as long as we are tracking work for the other part in an issue

[MichaelSwigerAtBentley]

@rschili @ColinKerr

I've created a new issue for getMetaData so this PR won't be held up: https://github.com/iTwin/itwinjs-core/issues/8288

Should have a new PR soon

[simihartstein]

@MichaelSwigerAtBentley Thanks for taking care of this. Looks like the PR is approved. Any reason it's being held up?