Add support for unique aspects without ECInstanceId (#45)

core/src/DMO.ts

@@ -119,6 +119,16 @@ export interface ElementAspectDMO extends DMO {
    * References the BIS class of this element aspect. See {@link ElementDMO#ecElement}.
    */
   readonly ecElementAspect: ECDomainClassFullName | ECDynamicEntityClassProps;
+
+  /**
+   * The attribute of IR instances of this DMO that points to the IR instance each will attach to.
+   *
+   * If omitted, the connector author (you) must specify supply the
+   * [`element`](https://www.itwinjs.org/reference/core-common/entities/elementaspectprops)
+   * property which has type [`RelatedElementProps`](https://www.itwinjs.org/reference/core-common/entities/relatedelementprops)
+   * in {@link DMO#modifyProps}.
+   */
+  elementAttr?: string;
 }
 
 /**

core/src/Node.ts

@@ -8,10 +8,10 @@ import * as fs from "fs";
 import { Code, CodeSpec, ElementAspectProps, ElementProps, IModel, InformationPartitionElementProps, ModelProps, RelatedElement, RelatedElementProps, RelationshipProps, RepositoryLinkProps, SubjectProps } from "@itwin/core-common";
 import { ElementAspectDMO, ElementDMO, ElementWithParentDMO, RelatedElementDMO, RelationshipDMO } from "./DMO";
 import { IModelDb, InformationPartitionElement, Model, RepositoryLink, Subject, SubjectOwnsPartitionElements, SubjectOwnsSubjects } from "@itwin/core-backend";
+import { Id64, Id64String } from "@itwin/core-bentley";
 
 import { DynamicEntityMap } from "./DynamicSchema";
 import { IRInstance } from "./IRModel";
-import { Id64String } from "@itwin/core-bentley";
 import { JobArgs } from "./BaseApp";
 import { Loader } from "./loaders";
 import { PConnector } from "./PConnector";
@@ -738,6 +738,13 @@ export interface ElementAspectNodeProps extends NodeProps {
 
   /** The subject from which this element aspect descends. */
   subject: SubjectNode;
+
+  /**
+   * The element to which the unique aspect attaches.
+   *
+   * If omitted, please see the corresponding property {@link DMO!ElementAspectDMO#elementAttr}.
+   */
+  element?: ElementNode;
 }
 
 /**
@@ -749,11 +756,13 @@ export class ElementAspectNode extends Node implements ElementAspectNodeProps {
 
   public dmo: ElementAspectDMO;
   public subject: SubjectNode;
+  public element?: ElementNode;
 
   constructor(pc: PConnector, props: ElementAspectNodeProps) {
     super(pc, props);
     this.dmo = props.dmo;
     this.subject = props.subject;
+    this.element = props.element;
     this.pc.tree.insert<ElementAspectNode>(this);
   }
 
@@ -772,18 +781,24 @@ export class ElementAspectNode extends Node implements ElementAspectNodeProps {
       const classFullName = typeof ecElementAspect === "string" ? ecElementAspect : `${this.pc.dynamicSchemaName}:${ecElementAspect.name}`;
 
       const props: ElementAspectProps = {
-        element: { id: "" },
+        element: { id: Id64.invalid },
         classFullName,
       };
 
       if (typeof this.dmo.modifyProps === "function")
         await this.dmo.modifyProps(this.pc, props, instance);
 
-      if (!props.element || !props.element.id)
-        throw new Error("You must attach \"props.element = { ... } as RelatedElementProps\" in ElementAspectDMO.modifyProps()");
+      if (this.element && this.dmo.elementAttr) {
+        const elementKey = IRInstance.createKey(this.element.dmo.irEntity, instance.get(this.dmo.elementAttr));
+        props.element = { id: this.pc.elementCache[elementKey] };
+      } else if (this.element || this.dmo.elementAttr) {
+        throw Error(`Your aspect node '${this.key}' must have an element, and its DMO must specify the \`elementAttr\` property.`);
+      } else if (!(props.element && Id64.isValid(props.element.id))) {
+        throw Error(`You must attach \`props.element = { ... } as RelatedElementProps\` in ElementAspectDMO#modifyProps in your aspect node '${this.key}'.`);
+      }
 
       const result = this.pc.syncElementUniqueAspect({
-        props: props,
+        props,
         version: instance.version,
         checksum: instance.checksum,
         scope: this.pc.jobSubjectId,
@@ -880,7 +895,7 @@ export class RelationshipNode extends Node {
       const { sourceId, targetId } = pair;
       const existing = this.pc.db.relationships.tryGetInstance(classFullName, { sourceId, targetId });
       if (existing) {
-        results.push({ entityId: existing.id, state: ItemState.Unchanged, comment: "" })
+        results.push({ entityId: existing.id, state: ItemState.Unchanged, comment: "" });
         continue;
       }
 
@@ -889,7 +904,7 @@ export class RelationshipNode extends Node {
         await this.dmo.modifyProps(this.pc, props, instance);
 
       const relId = this.pc.db.relationships.insertInstance(props);
-      results.push({ entityId: relId, state: ItemState.New, comment: "" })
+      results.push({ entityId: relId, state: ItemState.New, comment: "" });
     }
     return results;
   }

core/src/PConnector.ts

@@ -2,7 +2,7 @@
 * Copyright (c) Bentley Systems, Incorporated. All rights reserved.
 * See LICENSE.md in the project root for license terms and full copyright notice.
 *--------------------------------------------------------------------------------------------*/
-import { Id64String, Logger } from "@itwin/core-bentley";
+import { assert, Id64String, Logger } from "@itwin/core-bentley";
 import { Code, CodeScopeSpec, CodeSpec, ElementAspectProps, ExternalSourceAspectProps, IModel, IModelError, RelatedElementProps } from "@itwin/core-common";
 import { BriefcaseDb, Category, ComputeProjectExtentsOptions, DefinitionElement, ElementAspect, ElementUniqueAspect, ExternalSourceAspect, IModelDb, IModelHost, PushChangesArgs, SnapshotDb, StandaloneDb, SubjectOwnsPartitionElements } from "@itwin/core-backend";
 import { ItemState, ModelNode, SubjectNode, SyncResult, IRInstance, IRInstanceKey, IRModel, JobArgs, LoaderNode, RelatedElementNode, RelationshipNode, RepoTree, SyncArg, syncDynamicSchema, tryGetSchema } from "./pcf";
@@ -553,7 +553,7 @@ export abstract class PConnector {
       return ItemState.New;
     }
 
-    const xsa: ExternalSourceAspect = this.db.elements.getAspect(aspectId) as ExternalSourceAspect;
+    const xsa = this.db.elements.getAspect(aspectId) as ExternalSourceAspect;
     const existing = (xsa.version ?? "") + (xsa.checksum ?? "");
     const current = (version ?? "") + (checksum ?? "");
     if (existing === current)
@@ -590,23 +590,42 @@ export abstract class PConnector {
     const aspects = this.db.elements.getAspects(props.element.id, props.classFullName);
     const existingAspect = aspects.length === 1 ? aspects[0] : undefined;
 
-    let state: ItemState;
+    // The `id` of the `props` argument becomes the element to which the external source aspect
+    // attaches in {@link PConnector#syncProvenance}. It's confusing.
+
+    // Store provenance on the element that the aspect attaches to; okay because
+    // bis:ExternalSourceAspect derives from bis:ElementMultiAspect.
+
+    const state = this.syncProvenance({ ...arg, props: { ...props, id: props.element.id } });
+
+    // Detect if the unique aspect has _moved_ to a different element in the source data. This means
+    // we'll be unable to locate the original aspect in the iModel using the updated aspect in the
+    // source. However, it will get cleaned up by the deletion pass of the synchronizer. We still
+    // have to manually move the provenance over to the new element. Unfortunately, `updateAspect`
+    // does not update the element's navigation property.
+
+    // - A test for this particular failure: https://github.com/iTwin/itwinjs-core/pull/4227.
+    // - Known issues with the aspect API: https://github.com/iTwin/itwinjs-core/issues/3969.
+
+    if (!existingAspect && state === ItemState.Changed) {
+      const { scope, kind, identifier } = arg;
+      const { aspectId: foundId } = ExternalSourceAspect.findBySource(this.db, scope, kind, identifier);
+      assert(foundId !== undefined);
+      this.db.elements.deleteAspect(foundId);
+      this.syncProvenance({ ...arg, props: { ...props, id: props.element.id } });
+    }
 
     if (!existingAspect) {
       this.db.elements.insertAspect(props);
-
-      // store provenance on the element that the aspect attaches to
-      // this is ok because ExternalSourceAspect (provenance) is a ElementMultiAspect
-      state = this.syncProvenance({ ...arg, props: { ...props, id: props.element.id} });
       const newAspect = this.db.elements.getAspects(props.element.id, props.classFullName)[0];
       props.id = newAspect.id;
     } else {
       props.id = existingAspect.id;
-      state = this.syncProvenance({ ...arg, props });
     }
 
-    if (state === ItemState.Changed)
+    if (state === ItemState.Changed) {
       this.db.elements.updateAspect(props);
+    }
 
     return { entityId: props.id, state, comment: "" };
   }

core/src/test/ExpectedTestResults.ts

@@ -69,19 +69,22 @@ const TestResults: {[sourceFile: string]: QueryToCount} = {
     "select * from TestSchema:ExtPhysicalType where UserLabel='new_mock_user_label'": 3, // attribute update (from v1)
     "select * from TestSchema:ExtPhysicalElement": 3,                 // -2+1 (from v1)
     "select * from TestSchema:ExtGroupInformationElement": 1,         // -1 (from v1)
+    "select * from bis:SubCategory as subcategories inner join bis:Category as categories on subcategories.Parent.id = categories.ECInstanceId where subcategories.Description like '%moved%' and categories.CodeValue = 'ExtSpatialCategory-1'": 1,
     // Element Aspect
     "select * from TestSchema:ExtElementAspectA": 1,
     "select * from TestSchema:ExtElementAspectB": 1,
     "select * from TestSchema:ExtElementAspectA where Name='a-name'": 1,
-    "select * from BisCore:ExternalSourceAspect where identifier='ExtElementAspectA-1'": 1, // provenance of ExtElementAspect
+    "select * from BisCore:ExternalSourceAspect where identifier='ExtElementAspectA-1' and Element.id in (0x20000000010, 0x20) ": 1, // provenance of ExtElementAspect
+    "select * from bis:ChannelRootAspect as aspects inner join TestSchema:ExtPhysicalElement as elements on aspects.Element.id = elements.ECInstanceId where elements.CodeValue in ('ExtPhysicalElement-1', 'ExtPhysicalElement-5')": 2,
+    "select * from bis:ExternalSourceAspect as metas inner join TestSchema:ExtPhysicalElement as elements on metas.Element.id = elements.ECInstanceId where metas.Identifier = 'ElementAspectC-1' and elements.CodeValue = 'ExtPhysicalElement-1'": 1,
+    "select * from bis:ExternalSourceAspect as metas inner join TestSchema:ExtPhysicalElement as elements on metas.Element.id = elements.ECInstanceId where metas.Identifier = 'ElementAspectC-2' and elements.CodeValue = 'ExtPhysicalElement-5'": 1,
     // Relationship
     "select * from TestSchema:ExtElementGroupsMembers": 0,            // -1 (from v1)
     "select * from TestSchema:ExtElementRefersToElements": 2,         // +1 (from v1)
     "select * from TestSchema:ExtElementRefersToExistingElements": 1, // +1 (from v1)
     "select * from TestSchema:ExtExistingElementRefersToElements": 1,
     "select * from TestSchema:ExtPhysicalElementAssemblesElements": 1,
     "select categories.ECInstanceId from bis:ElementOwnsChildElements as relationships inner join bis:SpatialCategory as categories on relationships.SourceECInstanceId = categories.ECInstanceId": 2, // +1 default subcategory
-    "select * from bis:SubCategory where Description like '%moved%'": 1,
     // Domain Class
     "select * from BuildingSpatial:Space": 1,
     // Nested models, pro bono projects deleted and sushi project added to backlog
@@ -101,13 +104,20 @@ const TestResults: {[sourceFile: string]: QueryToCount} = {
     "select * from TestSchema:ExtElementAspectA": 1,
     "select * from TestSchema:ExtElementAspectB": 1,
     "select * from TestSchema:ExtElementAspectA where Name='a-new-name'": 1, // attribute update
+    "select * from BisCore:ExternalSourceAspect where Identifier='ExtElementAspectA-1'": 1, // provenance update
+    "select * from BisCore:ExternalSourceAspect where Identifier='ExtElementAspectA-1' and element.id in (0x20000000010, 0x20)": 1, // provenance update
+    "select * from bis:ChannelRootAspect": 1,
+    "select * from bis:ChannelRootAspect as aspects inner join TestSchema:ExtPhysicalElement as elements on aspects.Element.id = elements.ECInstanceId where elements.CodeValue = 'ExtPhysicalElement-2'": 1,
+    "select * from bis:ExternalSourceAspect where Identifier = 'ElementAspectC-1'": 1,
+    "select * from bis:ExternalSourceAspect as metas inner join TestSchema:ExtPhysicalElement as elements on metas.Element.id = elements.ECInstanceId where metas.Identifier = 'ElementAspectC-1' and elements.CodeValue = 'ExtPhysicalElement-2'": 1,
     // Nested models, everything deleted; 1 default, 1 for the loader
     "select * from bis:LinkModel": 2,
   },
   "v4.json": {
     // Element Aspect
     "select * from TestSchema:ExtElementAspectA": 0, // -1 (from v3)
     "select * from TestSchema:ExtElementAspectB": 0, // -1 (from v3)
+    "select * from bis:ChannelRootAspect": 0,        // -1 (from v3)
   },
   "v5.json": { // introduce a new Subject
     "select * from BisCore:Subject": 3,

core/src/test/JSONConnector/JSONConnector.ts

@@ -111,7 +111,7 @@ export class JSONConnector extends pcf.PConnector {
       loader: new pcf.JSONLoader({
         format: "json",
         entities: [
-          "ExtElementAspectA", "ExtElementAspectB",
+          "ExtElementAspectA", "ExtElementAspectB", "ElementAspectC",
           "ExtGroupInformationElement",
           "ExtPhysicalElement",
           "ExtPhysicalType",
@@ -190,6 +190,13 @@ export class JSONConnector extends pcf.PConnector {
       category: sptCategory
     });
 
+    new pcf.ElementAspectNode(this, {
+      key: "ElementAspectC",
+      subject: subject1,
+      element: extPhysicalElement,
+      dmo: aspects.ElementAspectC,
+    });
+
     const extGroupInformationElement = new pcf.ElementNode(this, {
       key: "ExtGroupInformationElement",
       model: grpModel,

core/src/test/JSONConnector/dmos/ElementAspects.ts

@@ -1,4 +1,4 @@
-import { BriefcaseDb, ElementUniqueAspect, StandaloneDb } from "@itwin/core-backend";
+import { BriefcaseDb, ChannelRootAspect, ElementUniqueAspect, StandaloneDb } from "@itwin/core-backend";
 import { PrimitiveType, primitiveTypeToString } from "@itwin/ecschema-metadata";
 import { IRInstance, ElementAspectDMO, PConnector } from "../../../pcf";
 
@@ -44,4 +44,11 @@ export const ExtElementAspectB: ElementAspectDMO = {
     else if (pc.db instanceof BriefcaseDb)
       props.element = { id: instance.get("BriefcaseExistingElementId") };
   },
+
+};
+
+export const ElementAspectC: ElementAspectDMO = {
+  irEntity: "ElementAspectC",
+  ecElementAspect: ChannelRootAspect.classFullName,
+  elementAttr: "attachTo",
 };

core/src/test/assets/v2.json

@@ -71,6 +71,16 @@
       "StandaloneExistingElementId": "0x20"
     }
   ],
+  "ElementAspectC": [
+    {
+      "id": "1",
+      "attachTo": "1"
+    },
+    {
+      "id": "2",
+      "attachTo": "5"
+    }
+  ],
   "ExtElementRefersToElements": [
     {
       "id": "1",

core/src/test/assets/v3.json

@@ -67,6 +67,12 @@
       "StandaloneExistingElementId": "0x20"
     }
   ],
+  "ElementAspectC": [
+    {
+      "id": "1",
+      "attachTo": "2"
+    }
+  ],
   "ExtElementRefersToElements": [
     {
       "id": "1",