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

Derived class caching #7429

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
8 changes: 4 additions & 4 deletions common/api/ecschema-editing.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1928,25 +1928,25 @@ export const SchemaCompareDiagnostics: {
diagnosticType: DiagnosticType;
};
FormatUnitMissing: {
new (ecDefinition: SchemaItem, messageArgs: [Unit | InvertedUnit], category?: DiagnosticCategory): {
new (ecDefinition: SchemaItem, messageArgs: [InvertedUnit | Unit], category?: DiagnosticCategory): {
readonly code: string;
readonly messageText: string;
readonly schema: Schema;
readonly diagnosticType: DiagnosticType;
ecDefinition: Format;
messageArgs?: [Unit | InvertedUnit] | undefined;
messageArgs?: [InvertedUnit | Unit] | undefined;
category: DiagnosticCategory;
};
diagnosticType: DiagnosticType;
};
UnitLabelOverrideDelta: {
new (ecDefinition: SchemaItem, messageArgs: [Unit | InvertedUnit, string | undefined, string | undefined], category?: DiagnosticCategory): {
new (ecDefinition: SchemaItem, messageArgs: [InvertedUnit | Unit, string | undefined, string | undefined], category?: DiagnosticCategory): {
readonly code: string;
readonly messageText: string;
readonly schema: Schema;
readonly diagnosticType: DiagnosticType;
ecDefinition: Format;
messageArgs?: [Unit | InvertedUnit, string | undefined, string | undefined] | undefined;
messageArgs?: [InvertedUnit | Unit, string | undefined, string | undefined] | undefined;
category: DiagnosticCategory;
};
diagnosticType: DiagnosticType;
Expand Down
6 changes: 4 additions & 2 deletions common/api/ecschema-metadata.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -257,9 +257,7 @@ export abstract class ECClass extends SchemaItem implements CustomAttributeConta
// (undocumented)
protected addCustomAttribute(customAttribute: CustomAttribute): void;
protected addProperty<T extends Property>(prop: T): T;
// (undocumented)
get baseClass(): LazyLoadedECClass | undefined;
set baseClass(baseClass: LazyLoadedECClass | undefined);
// (undocumented)
protected _baseClass?: LazyLoadedECClass;
// (undocumented)
Expand Down Expand Up @@ -293,6 +291,8 @@ export abstract class ECClass extends SchemaItem implements CustomAttributeConta
// @alpha
protected deletePropertySync(name: string): void;
// (undocumented)
protected _derivedClasses?: Map<string, LazyLoadedECClass>;
// (undocumented)
fromJSON(classProps: ClassProps): Promise<void>;
// (undocumented)
fromJSONSync(classProps: ClassProps): void;
Expand All @@ -303,6 +303,7 @@ export abstract class ECClass extends SchemaItem implements CustomAttributeConta
getBaseClassSync(): ECClass | undefined;
getCustomAttributes(): Promise<CustomAttributeSet>;
getCustomAttributesSync(): CustomAttributeSet;
getDerivedClasses(): Promise<ECClass[] | undefined>;
getInheritedProperty(name: string): Promise<Property | undefined>;
getInheritedPropertySync(name: string): Property | undefined;
getProperties(resetCache?: boolean): Promise<Property[]>;
Expand Down Expand Up @@ -333,6 +334,7 @@ export abstract class ECClass extends SchemaItem implements CustomAttributeConta
get properties(): IterableIterator<Property> | undefined;
// (undocumented)
protected _properties?: Map<string, Property>;
setBaseClass(baseClass: LazyLoadedECClass | undefined): Promise<void>;
// @alpha
protected setModifier(modifier: ECClassModifier): void;
toJSON(standalone?: boolean, includeSchemaVersion?: boolean): ClassProps;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@itwin/ecschema-editing",
"comment": "Replaced ECClass.baseClass setter with setBaseClass method due to underlying changes in the ecschema-metadata package.",
"type": "none"
}
],
"packageName": "@itwin/ecschema-editing"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@itwin/ecschema-metadata",
"comment": "ECClass.baseClass setter has been replaced with setBaseClass method to enable derived class caching support.",
"type": "none"
}
],
"packageName": "@itwin/ecschema-metadata"
}
6 changes: 3 additions & 3 deletions core/ecschema-editing/src/Editing/ECClasses.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export class ECClasses extends SchemaItems{
if (baseClassKey !== undefined) {
const baseClassSchema = !baseClassKey.schemaKey.matches(newClass.schema.schemaKey) ? await this.getSchema(baseClassKey.schemaKey) : newClass.schema as MutableSchema;
const baseClassItem = await this.lookupSchemaItem<ECClass>(baseClassSchema, baseClassKey);
newClass.baseClass = new DelayedPromiseWithProps<SchemaItemKey, T>(baseClassKey, async () => baseClassItem as T);
await (newClass as ECClass as MutableClass).setBaseClass(new DelayedPromiseWithProps<SchemaItemKey, T>(baseClassKey, async () => baseClassItem as T));
}

return newClass;
Expand Down Expand Up @@ -275,7 +275,7 @@ export class ECClasses extends SchemaItems{
try {
const classItem = await this.getSchemaItem<ECClass>(itemKey);
if (!baseClassKey) {
classItem.baseClass = undefined;
await (classItem as MutableClass).setBaseClass(undefined);
return;
}

Expand All @@ -284,7 +284,7 @@ export class ECClasses extends SchemaItems{
if (classItem.baseClass !== undefined && !await baseClassItem.is(await classItem.baseClass))
throw new SchemaEditingError(ECEditingStatus.InvalidBaseClass, new ClassId(this.schemaItemType, baseClassKey), undefined, undefined, `Base class ${baseClassKey.fullName} must derive from ${(await classItem.baseClass).fullName}.`);

classItem.baseClass = new DelayedPromiseWithProps<SchemaItemKey, ECClass>(baseClassKey, async () => baseClassItem);
await (classItem as MutableClass).setBaseClass(new DelayedPromiseWithProps<SchemaItemKey, ECClass>(baseClassKey, async () => baseClassItem));
} catch(e: any) {
throw new SchemaEditingError(ECEditingStatus.SetBaseClass, new ClassId(this.schemaItemType, itemKey), e);
}
Expand Down
3 changes: 2 additions & 1 deletion core/ecschema-editing/src/Editing/Mutable/MutableClass.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* See LICENSE.md in the project root for license terms and full copyright notice.
*--------------------------------------------------------------------------------------------*/
import {
CustomAttribute, ECClass, ECClassModifier, Enumeration, EnumerationArrayProperty, EnumerationProperty, PrimitiveArrayProperty,
CustomAttribute, ECClass, ECClassModifier, Enumeration, EnumerationArrayProperty, EnumerationProperty, LazyLoadedECClass, PrimitiveArrayProperty,
PrimitiveProperty, PrimitiveType, Property, StructArrayProperty, StructClass, StructProperty,
} from "@itwin/ecschema-metadata";

Expand All @@ -20,6 +20,7 @@ export abstract class MutableStructClass extends StructClass {
* @internal
*/
export abstract class MutableClass extends ECClass {
public abstract override setBaseClass(baseClass: LazyLoadedECClass | undefined): Promise<void>;
public abstract override addCustomAttribute(customAttribute: CustomAttribute): void;
public abstract override setModifier(modifier: ECClassModifier): void;
public abstract override setName(name: string): void;
Expand Down
10 changes: 7 additions & 3 deletions core/ecschema-editing/src/Editing/RelationshipClasses.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/

import {
CustomAttribute, DelayedPromiseWithProps, ECClassModifier, EntityClass, LazyLoadedRelationshipConstraintClass, Mixin, NavigationPropertyProps,
CustomAttribute, DelayedPromiseWithProps, ECClass, ECClassModifier, EntityClass, LazyLoadedRelationshipConstraintClass, Mixin, NavigationPropertyProps,
RelationshipClass, RelationshipClassProps, RelationshipConstraint, RelationshipEnd, RelationshipMultiplicity, SchemaItemKey, SchemaItemType,
SchemaKey, StrengthDirection, StrengthType,
} from "@itwin/ecschema-metadata";
Expand All @@ -18,6 +18,7 @@ import * as Rules from "../Validation/ECRules";
import { AnyDiagnostic, RelationshipConstraintDiagnostic, SchemaItemDiagnostic } from "../Validation/Diagnostic";
import { NavigationProperties } from "./Properties";
import { ClassId, CustomAttributeId, ECEditingStatus, RelationshipConstraintId, SchemaEditingError } from "./Exception";
import { MutableClass } from "./Mutable/MutableClass";

/**
* @alpha
Expand Down Expand Up @@ -107,15 +108,18 @@ export class RelationshipClasses extends ECClasses {
* @param baseClassKey The SchemaItemKey of the base class. Specifying 'undefined' removes the base class.
*/
public override async setBaseClass(itemKey: SchemaItemKey, baseClassKey?: SchemaItemKey): Promise<void> {
const relClass = await this.schemaEditor.schemaContext.getSchemaItem<RelationshipClass>(itemKey);
const relClass = await this.schemaEditor.schemaContext.getSchemaItem<RelationshipClass>(itemKey)
.catch((e) => {
throw new SchemaEditingError(ECEditingStatus.SetBaseClass, new ClassId(this.schemaItemType, itemKey), e);
});
const baseClass = relClass?.baseClass;

await super.setBaseClass(itemKey, baseClassKey);

try {
await this.validate(relClass!);
} catch(e: any) {
relClass!.baseClass = baseClass;
await (relClass! as ECClass as MutableClass).setBaseClass(baseClass);
Copy link
Contributor

@jason-crow jason-crow Dec 6, 2024

Choose a reason for hiding this comment

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

why is a double cast necessary? especially given you already have the definitely assigned operator, which together makes this about the least type safe typescript way to accomplish what you seem to want to do here

Copy link
Contributor Author

@christophermlawson christophermlawson Dec 20, 2024

Choose a reason for hiding this comment

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

I know it looks odd, but it has to do with our Mutable classes, which do not derive from each other but from the class they 'mutating'. A RelationshipClass cannot be cast to a MutableClass directly, because they have no direct relationship. We have a MutableRelationshipClass but we can't use it because it does not derive from MutableClass, and we are trying to access members of MutableClass in this instance). We discussed this and there might be some TS trickery we can do to make the mutable classes more usable, but for now, this is what we have. The mutable classes are strictly internal and are not used outside the package.

throw new SchemaEditingError(ECEditingStatus.SetBaseClass, new ClassId(SchemaItemType.RelationshipClass, itemKey), e);
}
}
Expand Down
30 changes: 29 additions & 1 deletion core/ecschema-editing/src/test/Editing/Entities.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,13 @@ describe("Entities tests", () => {
const result = await testEditor.entities.create(testKey, "testEntity", ECClassModifier.None, "testLabel", testEntityBaseRes);

const testEntity = await testEditor.schemaContext.getSchemaItem<EntityClass>(result);
expect(await testEntity?.baseClass).to.eql(await testEditor.schemaContext.getSchemaItem(testEntityBaseRes));
const baseEntity = await testEditor.schemaContext.getSchemaItem<EntityClass>(testEntityBaseRes);
expect(await testEntity?.baseClass).to.eql(baseEntity);
expect(testEntity?.label).to.eql("testLabel");
const derivedClasses = await baseEntity?.getDerivedClasses();
expect(derivedClasses).to.not.be.undefined;
expect(derivedClasses![0]).to.eql(testEntity);
expect(derivedClasses?.length).to.eql(1);
});

it("should create a new entity class with a base class from different schema", async () => {
Expand All @@ -85,8 +90,13 @@ describe("Entities tests", () => {
const result = await testEditor.entities.create(testKey, "testEntity", ECClassModifier.None, "testLabel", baseClassKey);

const testEntity = await testEditor.schemaContext.getSchemaItem<EntityClass>(result);
const baseEntity = await testEditor.schemaContext.getSchemaItem<EntityClass>(baseClassKey);
expect(await testEntity?.baseClass).to.eql(await testEditor.schemaContext.getSchemaItem(baseClassKey));
expect(testEntity?.label).to.eql("testLabel");
const derivedClasses = await baseEntity?.getDerivedClasses();
expect(derivedClasses).to.not.be.undefined;
expect(derivedClasses![0]).to.eql(testEntity);
expect(derivedClasses?.length).to.eql(1);
});

it("try creating a new entity class with base class from unknown schema, returns error", async () => {
Expand Down Expand Up @@ -157,7 +167,12 @@ describe("Entities tests", () => {
await testEditor.entities.setBaseClass(result, testEntityBaseRes);

const testEntity = await testEditor.schemaContext.getSchemaItem<EntityClass>(result);
const baseEntity = await testEditor.schemaContext.getSchemaItem<EntityClass>(testEntityBaseRes);
expect(await testEntity?.baseClass).to.eql(await testEditor.schemaContext.getSchemaItem(testEntityBaseRes));
const derivedClasses = await baseEntity?.getDerivedClasses();
expect(derivedClasses).to.not.be.undefined;
expect(derivedClasses![0]).to.eql(testEntity);
expect(derivedClasses?.length).to.eql(1);
});

it("should add base class to existing entity class where base class is from a different schema", async () => {
Expand All @@ -182,7 +197,12 @@ describe("Entities tests", () => {
await testEditor.entities.setBaseClass(result, baseClassKey);

const testEntity = await testEditor.schemaContext.getSchemaItem<EntityClass>(result);
const baseEntity = await testEditor.schemaContext.getSchemaItem<EntityClass>(baseClassKey);
expect(await testEntity?.baseClass).to.eql(await testEditor.schemaContext.getSchemaItem(baseClassKey));
const derivedClasses = await baseEntity?.getDerivedClasses();
expect(derivedClasses).to.not.be.undefined;
expect(derivedClasses![0]).to.eql(testEntity);
expect(derivedClasses?.length).to.eql(1);
});

it("should change base class of existing entity class with different base class.", async () => {
Expand Down Expand Up @@ -215,8 +235,13 @@ describe("Entities tests", () => {
expect(await testEntity?.baseClass).to.eql(await testEditor.schemaContext.getSchemaItem(firstBaseClassKey));

const secondBaseClassKey = new SchemaItemKey("testEntityBase2", refSchema.schemaKey);
const secondBaseClass = await testEditor.schemaContext.getSchemaItem<EntityClass>(secondBaseClassKey);
await testEditor.entities.setBaseClass(testEntityResult, secondBaseClassKey);
expect(await testEntity?.baseClass).to.eql(await testEditor.schemaContext.getSchemaItem(secondBaseClassKey));
const derivedClasses = await secondBaseClass?.getDerivedClasses();
expect(derivedClasses).to.not.be.undefined;
expect(derivedClasses![0]).to.eql(testEntity);
expect(derivedClasses?.length).to.eql(1);
});

it("should remove base class from existing entity class.", async () => {
Expand All @@ -228,6 +253,9 @@ describe("Entities tests", () => {

await testEditor.entities.setBaseClass(result, undefined);
expect(await testEntity?.baseClass).to.eql(undefined);
const baseEntity = await testEditor.schemaContext.getSchemaItem<EntityClass>(testEntityBaseRes);
const derivedClasses = await baseEntity?.getDerivedClasses();
expect(derivedClasses).to.be.undefined;
});

it("try adding base class with unknown schema to existing entity class, returns error", async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@
*--------------------------------------------------------------------------------------------*/

import { expect } from "chai";
import { DelayedPromiseWithProps, ECClassModifier, EntityClass,
import { DelayedPromiseWithProps, ECClass, ECClassModifier, EntityClass,
RelationshipClass, Schema, SchemaContext, schemaItemTypeToString,
} from "@itwin/ecschema-metadata";
import * as Rules from "../../../Validation/ECRules";
import { DiagnosticCategory, DiagnosticType } from "../../../Validation/Diagnostic";
import { MutableClass } from "../../../Editing/Mutable/MutableClass";

/* eslint-disable @typescript-eslint/no-deprecated */

Expand All @@ -22,7 +23,7 @@ describe("ClassRule tests", () => {
it("BaseClassIsSealed, rule violated.", async () => {
const baseClass = new EntityClass(schema, "TestBase", ECClassModifier.Sealed);
const entityClass = new EntityClass(schema, "TestClass");
entityClass.baseClass = new DelayedPromiseWithProps(baseClass.key, async () => baseClass);
await (entityClass as ECClass as MutableClass).setBaseClass(new DelayedPromiseWithProps(baseClass.key, async () => baseClass));

const result = Rules.baseClassIsSealed(entityClass);

Expand All @@ -42,7 +43,7 @@ describe("ClassRule tests", () => {
it("BaseClassIsSealed, base is not sealed, rule passes.", async () => {
const baseClass = new EntityClass(schema, "TestBase");
const entityClass = new EntityClass(schema, "TestClass");
entityClass.baseClass = new DelayedPromiseWithProps(baseClass.key, async () => baseClass);
await (entityClass as ECClass as MutableClass).setBaseClass(new DelayedPromiseWithProps(baseClass.key, async () => baseClass));

const result = Rules.baseClassIsSealed(entityClass);
for await (const _diagnostic of result) {
Expand All @@ -62,7 +63,7 @@ describe("ClassRule tests", () => {
it("BaseClassIsOfDifferentType, rule violated.", async () => {
const baseClass = new RelationshipClass(schema, "TestBase");
const entityClass = new EntityClass(schema, "TestClass");
entityClass.baseClass = new DelayedPromiseWithProps(baseClass.key, async () => baseClass);
await (entityClass as ECClass as MutableClass).setBaseClass(new DelayedPromiseWithProps(baseClass.key, async () => baseClass));
const baseType = schemaItemTypeToString(baseClass.schemaItemType);

const result = Rules.baseClassIsOfDifferentType(entityClass);
Expand All @@ -82,7 +83,7 @@ describe("ClassRule tests", () => {
it("BaseClassIsOfDifferentType, same type, rule passes.", async () => {
const baseClass = new EntityClass(schema, "TestBase");
const entityClass = new EntityClass(schema, "TestClass");
entityClass.baseClass = new DelayedPromiseWithProps(baseClass.key, async () => baseClass);
await (entityClass as ECClass as MutableClass).setBaseClass(new DelayedPromiseWithProps(baseClass.key, async () => baseClass));

const result = Rules.baseClassIsOfDifferentType(entityClass);
for await (const _diagnostic of result) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,11 @@
*--------------------------------------------------------------------------------------------*/

import { expect } from "chai";
import { DelayedPromiseWithProps, EntityClass, Mixin, Schema, SchemaContext } from "@itwin/ecschema-metadata";
import { DelayedPromiseWithProps, ECClass, EntityClass, Mixin, Schema, SchemaContext } from "@itwin/ecschema-metadata";
import { MutableEntityClass } from "../../../Editing/Mutable/MutableEntityClass";
import { DiagnosticCategory, DiagnosticType } from "../../../Validation/Diagnostic";
import * as Rules from "../../../Validation/ECRules";
import { MutableClass } from "../../../Editing/Mutable/MutableClass";

describe("Mixin Rule Tests", () => {
let schema: Schema;
Expand All @@ -28,7 +29,7 @@ describe("Mixin Rule Tests", () => {
const mixin = new TestMixin(schema, "TestMixin", constraintClass);
const entityClass = new EntityClass(schema, "TestClass");
(entityClass as MutableEntityClass).addMixin(mixin);
entityClass.baseClass = new DelayedPromiseWithProps(constraintClass.key, async () => constraintClass);
await (entityClass as ECClass as MutableClass).setBaseClass(new DelayedPromiseWithProps(constraintClass.key, async () => constraintClass));

const result = Rules.mixinAppliedToClassMustDeriveFromConstraint(entityClass);
for await (const _diagnostic of result) {
Expand Down Expand Up @@ -73,7 +74,7 @@ describe("Mixin Rule Tests", () => {
const mixin = new TestMixin(schema, "TestMixin", constraintClass);
const entityClass = new EntityClass(schema, "TestClass");
(entityClass as MutableEntityClass).addMixin(mixin);
entityClass.baseClass = new DelayedPromiseWithProps(baseClass.key, async () => baseClass);
await (entityClass as ECClass as MutableClass).setBaseClass(new DelayedPromiseWithProps(baseClass.key, async () => baseClass));

const result = Rules.mixinAppliedToClassMustDeriveFromConstraint(entityClass);

Expand Down
Loading
Loading