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

Add test which introduces a EntityClass that has a CA 'QueryView' and a navigation property to exercise ECReferenceTypesCache #222

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

Conversation

nick4598
Copy link
Collaborator

@nick4598 nick4598 commented Nov 22, 2024

Right now the only place this cache appears to be used is cloneElementAspect.

if (sourceNavProp?.id) {
          const navPropRefType = this._refTypesCache.getNavPropRefType(
            sourceElementAspect.schemaName,
            sourceElementAspect.className,
            propertyName
          );

I believe there could never be a navigation property with an id which pointed to an Entity which had the customattribute of QueryView, but I'm not sure. My logic being that the modifiers I saw for all other examples of QueryView being used were abstract. The QueryView is built off of the elements in the database, so I assume the view itself would never be represented as an id. I suppose theres a possibility that there could be a non abstract entity with the custom attribute QueryView? Or is that somehow prevented with the way the schema is written?

I briefly tried this out by adding a base class to the testview entity in my testQueryView schema and changing the modifier from Abstract to Sealed, but then I would get

Error | ECObjectsNative | The NavigationECProperty BisCore:Element:Parent cannot be overridden by TestGeneratedClassesNew:TestView:Parent because the relationship was changed. A derived property cannot change the referenced relationship.
Error | ECObjectsNative | Failed to read class 'TestView' from schema 'TestGeneratedClassesNew'
Error | ECObjectsNative | Failed to read XML file: D:\repos\imodel-transformer\packages\transformer\lib\cjs\test\assets\TestQueryView.ecschema.xml

Are there potentially any other EC concepts that would cause this code to fail? Or are QueryViews unique?

@nick4598 nick4598 requested review from a team as code owners November 22, 2024 16:52
* Since queryViews are typically associated with abstract classes and have no specific table in the database, we can ignore them.
*/
if (
customAttributeName.toLowerCase() === "ecdbmap.queryview" &&
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Worried about potential performance implications, if I am checking the customattributes of every relationshipClass for the queryView attribute, will that be slow? Potential optimization is to only check if the sourceType or targetType are undefined?

I imagine this is probably quite a small %age of any given transformer run, so maybe it doesn't matter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Started timing it: Info | core-backend.IModelTransformer | Checked 1444 classes for QueryView custom attribute. This took 4ms.

Its maybe overkill.

@@ -221,6 +274,24 @@ export class ECReferenceTypesCache {
ECReferenceTypesCache.bisRootClassToRefType[sourceRootBisClass.name];
const targetType =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ColinKerr What is the recommended way to tell if a class is abstract or not?
Right now if targetType is undefined from calling ECReferenceTypesCache.bisRootClassToRefType[targetRootBisClass.name]; That seems like a pretty good indicator that something is abstract, because it didn't have any matches for Element, Model, ElementAspect, or Relationship. But I do worry that it isn't foolproof. What if something else went wrong to cause us to get undefined? Right now the code assumes it is an error to get undefined and throws.

private static bisRootClassToRefType: Record<
    string,
    ConcreteEntityTypes | undefined
  > = {
    /* eslint-disable quote-props, @typescript-eslint/naming-convention */
    Element: ConcreteEntityTypes.Element,
    Model: ConcreteEntityTypes.Model,
    ElementAspect: ConcreteEntityTypes.ElementAspect,
    ElementRefersToElements: ConcreteEntityTypes.Relationship,
    ElementDrivesElement: ConcreteEntityTypes.Relationship,
    // code spec is technically a potential root class but it is ignored currently
    // see [ConcreteEntityTypes]($common)
    /* eslint-enable quote-props, @typescript-eslint/naming-convention */
  };

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
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.

1 participant