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
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .azure-pipelines/generate-docs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ stages:
versionSpec: 18
checkLatest: true

- script: npm install -g pnpm@7.27.0
- script: npm install -g pnpm@9.14.2
displayName: Install pnpm

- script: pnpm install
Expand Down
12 changes: 6 additions & 6 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@ jobs:
- name: Install pnpm
uses: pnpm/action-setup@v4
with:
version: 7.27.0
version: 9.14.2

- name: Use Node.js 18
nick4598 marked this conversation as resolved.
Show resolved Hide resolved
uses: actions/setup-node@v3
with:
node-version: 18.16.0
node-version: 20.18.1
cache: 'pnpm'

- name: Install dependencies
Expand All @@ -46,12 +46,12 @@ jobs:
- name: Install pnpm
uses: pnpm/action-setup@v4
with:
version: 7.27.0
version: 9.14.2

- name: Use Node.js 18
uses: actions/setup-node@v3
with:
node-version: 18.16.0
node-version: 20.18.1
cache: 'pnpm'

- id: set-matrix
Expand Down Expand Up @@ -102,12 +102,12 @@ jobs:
- name: Install pnpm
uses: pnpm/action-setup@v4
with:
version: 7.27.0
version: 9.14.2

- name: Use Node.js 18
nick4598 marked this conversation as resolved.
Show resolved Hide resolved
uses: actions/setup-node@v3
with:
node-version: 18.16.0
node-version: 20.18.1
cache: 'pnpm'

- name: Force dependency resolution
Expand Down
6 changes: 3 additions & 3 deletions .github/workflows/extract-api.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,12 @@ jobs:
- name: Install pnpm
uses: pnpm/action-setup@v4
with:
version: 7.27.0
version: 9.14.2

- name: Use Node.js 18
- name: Use Node.js 20
uses: actions/setup-node@v3
with:
node-version: 18.16.0
node-version: 20.18.1
cache: 'pnpm'

- name: Pnpm install
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/release-dev.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,12 @@ jobs:
- name: Install pnpm
uses: pnpm/action-setup@v4
with:
version: 7.27.0
version: 9.14.2

- name: Use Node.js 18
uses: actions/setup-node@v3
with:
node-version: 18.16.0
node-version: 20.18.1
registry-url: https://registry.npmjs.org/

- name: Install dependencies
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ jobs:
- name: Use Node.js 18
uses: actions/setup-node@v3
with:
node-version: 18.16.0
node-version: 20.18.1
registry-url: https://registry.npmjs.org/

- name: Install dependencies
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "handle queryViews with navigationProperties in the ECreferenceTypesCache",
"packageName": "@itwin/imodel-transformer",
"email": "22119573+nick4598@users.noreply.github.com",
"dependentChangeType": "patch"
}
6 changes: 3 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@
"node": ">=16"
},
"pnpm": {
"overrides": {
"semver": "^7.5.2"
}
"overrides": {
"semver": "^7.5.2"
}
}
}
35 changes: 34 additions & 1 deletion packages/transformer/src/ECReferenceTypesCache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* @module iModels
*/

import { DbResult, TupleKeyedMap } from "@itwin/core-bentley";
import { DbResult, Logger, TupleKeyedMap } from "@itwin/core-bentley";
import {
ConcreteEntityTypes,
IModelError,
Expand All @@ -24,6 +24,7 @@ import {
} from "@itwin/ecschema-metadata";
import * as assert from "assert";
import { IModelDb } from "@itwin/core-backend";
import { TransformerLoggerCategory } from "./TransformerLoggerCategory";

/** The context for transforming a *source* Element to a *target* Element and remapping internal identifiers to the target iModel.
* @internal
Expand Down Expand Up @@ -221,6 +222,38 @@ 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 */
  };

ECReferenceTypesCache.bisRootClassToRefType[targetRootBisClass.name];

const sourceCAs = sourceClass.getCustomAttributesSync();
const targetCAs = targetClass.getCustomAttributesSync();
for (const [customAttributeName, _customAttribute] of sourceCAs) {
// I've essentially captured the case that will cause us problems, but now the question is what to do about it?
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.

sourceType === undefined
) {
// no sourceType means the rootBisClass.name is not in the bisRootClassToRefType map.
Logger.logInfo(
TransformerLoggerCategory.IModelTransformer,
`sourceClass: ${sourceClass.schema.name}:${sourceClass.name} has customAttribute which is QueryView and no sourceType in the rootClassToRefType map`
nick4598 marked this conversation as resolved.
Show resolved Hide resolved
);
return undefined;
}
}

for (const [customAttributeName, _customAttribute] of targetCAs) {
if (
customAttributeName.toLowerCase() === "ecdbmap.queryview" &&
targetType === undefined
) {
// I've essentially captured the case that will cause us problems, but now the question is what to do about it?
Logger.logInfo(
TransformerLoggerCategory.IModelTransformer,
`targetClass: ${targetClass.schema.name}:${targetClass.name} has customAttribute which is QueryView and no sourceType in the rootClassToRefType map`
nick4598 marked this conversation as resolved.
Show resolved Hide resolved
);
return undefined;
}
}

const makeAssertMsg = (root: ECClass, cls: ECClass) =>
[
`An unknown root class '${root.fullName}' was encountered while populating`,
Expand Down
35 changes: 35 additions & 0 deletions packages/transformer/src/test/assets/TestQueryView.ecschema.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?xml version="1.0" encoding="UTF-8"?>
<ECSchema schemaName="TestGeneratedClassesNew" alias="tgcn" version="1.0.0"
xmlns="http://www.bentley.com/schemas/Bentley.ECXML.3.2">
<ECSchemaReference name="CoreCustomAttributes" version="01.00.03" alias="CoreCA"/>
<ECSchemaReference name="BisCustomAttributes" version="01.00.00" alias="bisCA"/>
<ECSchemaReference name="ECdbMap" version="02.00.04" alias="ecdbmap"/>
<ECSchemaReference name="BisCore" version="01.00.16" alias="bis"/>

<ECEntityClass typeName="TestView" modifier="Abstract" displayLabel="Test View" description="a sample view">
<ECCustomAttributes>
<QueryView xmlns="ECDbMap.02.00.04">
<Query>
SELECT
pe.ECInstanceId,
pe.ECClassId,
pe.Yaw as Yaw,
NAVIGATION_VALUE(bis.PhysicalElement.Parent, pe.Parent.Id) as Parent
FROM bis.PhysicalElement pe
</Query>
</QueryView>
</ECCustomAttributes>
<ECProperty propertyName="Yaw" typeName="double" description="Yaw of the PhysicalElement"/>
<ECNavigationProperty propertyName="Parent" relationshipName="PhysicalElementOwnsTestView" direction="backward" description="The parent Element that owns this element"/>
</ECEntityClass>

<ECRelationshipClass typeName="PhysicalElementOwnsTestView" strength="embedding" modifier="None" description="Relationship between an PhysicalElement and a TestView.">
<Source multiplicity="(1..1)" roleLabel="owns" polymorphic="true">
<Class class="bis:PhysicalElement"/>
</Source>
<Target multiplicity="(1..*)" roleLabel="is owned by" polymorphic="true">
<Class class="TestView"/>
</Target>
</ECRelationshipClass>

</ECSchema>
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,18 @@ import { KnownTestLocations as BackendTestsKnownLocations } from "../TestUtils/K
import * as Semver from "semver";
import { Schema, SchemaItemType, SchemaLoader } from "@itwin/ecschema-metadata";
import * as sinon from "sinon";
import { version as iTwinCoreBackendVersion } from "@itwin/core-backend/package.json";

describe("ECReferenceTypesCache", () => {
let testIModel: SnapshotDb;
const testSchemaPath = path.join(
BackendTestsKnownLocations.assetsDir,
"TestGeneratedClasses.ecschema.xml"
);
const testSchemaPathWithQueryView = path.join(
BackendTestsKnownLocations.assetsDir,
"TestQueryView.ecschema.xml"
);
const testFixtureRefCache = new ECReferenceTypesCache();
let pathForEmpty: string;
let emptyWithBrandNewBiscore: SnapshotDb;
Expand Down Expand Up @@ -185,6 +190,22 @@ describe("ECReferenceTypesCache", () => {
).to.equal(ConcreteEntityTypes.Element);
});

it("should handle QueryView", async () => {
if (Semver.gte(iTwinCoreBackendVersion, "4.6.0")) {
const thisTestRefCache = new ECReferenceTypesCache();
const ecdbMapVersion =
emptyWithBrandNewBiscore.querySchemaVersion("ECdbMap");
assert(ecdbMapVersion !== undefined);
assert(Semver.gte(ecdbMapVersion, "2.0.4"));
await emptyWithBrandNewBiscore.importSchemas([
testSchemaPathWithQueryView,
]);
await thisTestRefCache.initAllSchemasInIModel(emptyWithBrandNewBiscore);
} else {
assert(true); // Pre 4.6.0 does not have QueryView support. https://www.itwinjs.org/bis/domains/ecdbmap.ecschema/#queryview
}
});

it("should not init schemas of a lower or equal version", async () => {
const thisTestRefCache = new ECReferenceTypesCache();

Expand Down
Loading
Loading