itwinjs-core icon indicating copy to clipboard operation
itwinjs-core copied to clipboard

Is there a reason `insertAspect` doesn't return the `ECInstanceId` of the new aspect?

Open jackson-at-bentley opened this issue 3 years ago • 7 comments

Is your feature request related to a problem? Please describe.

I'm working on a synchronizer library and would like to be able to locate an ElementAspect in the iModel after I've inserted it. Unfortunately insertAspect has a void return type, unlike insertElement and insertModel. Is this due to an implementation detail of aspects?

I assume updateAspect requires the iModel ID of the aspect like the other object types to perform an update.

public insertAspect(aspectProps: ElementAspectProps): void {
  try {
    this._iModel.nativeDb.insertElementAspect(aspectProps);
  } catch (err: any) { ... }
}
public insertElement(elProps: ElementProps): Id64String {
  try {
    return elProps.id = this._iModel.nativeDb.insertElement(elProps);
  } catch (err: any) { ... }
}

Describe the solution you'd like.

public insertAspect(aspectProps: ElementAspectProps): Id64String { ... };

Describe alternatives you've considered.

I considered a linear pass over the aspects to locate the one I'm looking for, but I can't write application data to an ElementAspect because it lacks a jsonProperties property. I can however store a map from source identifiers to iModel identifiers in the related element's jsonProperties.

jackson-at-bentley avatar Jul 25 '22 23:07 jackson-at-bentley

An "iModel Id" identifies an iModel, not a thing inside an iModel. The term you are looking for is "ECInstanceId" which is a 64-bit unsigned integer identifying a row in a table associated with an ECClass within the iModel.

Can you explain what kind of application data you want to store in the iModel and why?

pmconne avatar Jul 26 '22 08:07 pmconne

My synchronizer's job is mapping source files to an iModel. In order to insert or update an element, model, or aspect I need to know its ECInstanceID. For elements and models, this is easy.

  • Elements can be located by their Code or a (kind, external identifier) tuple in a given ExternalSourceAspect scope using ExternalSourceAspect.findBySource.
  • Models have exactly one modeled element and the modeled element can be fetched.

For aspects, there's updateAspect which accepts an ElementAspectProps with a nullable id property but there's no way to obtain that ECInstanceID from the iTwin API. ~~I guess I could create instances of ElementUniqueAspect or ElementMultiAspect because those seem to populate that id property for you, but then the classFullName property is wrong and they're also abstract in BIS.~~ Right now the API feels inconsistent because insertAspect swallows the ECInstanceID.

I'm planning on storing a JSON object in the related element's jsonProperties that maps some external identifier of the aspect (my library calls it anchor) to its ECInstanceID so that it can be accessed in constant time after parsing the map (which I guess is actually linear in the number of aspects, but it seems faster than IModelDb.Elements.getAspects).

jackson-at-bentley avatar Jul 26 '22 15:07 jackson-at-bentley

The implementation of insertElementAspect inside the addon could return you the ECInstanceId. However, there is no single Aspect table - there are two: ElementMultiAspect and ElementUniqueAspect - so the ECInstanceId alone will not fully identify the aspect.

@jffmarker isn't ExternalSourceAspect already the right way to track the provenance of element aspects?

pmconne avatar Jul 26 '22 15:07 pmconne

@jffmarker isn't ExternalSourceAspect already the right way to track the provenance of element aspects?

Yup. Look for references to ExternalSourceAspect on https://www.itwinjs.org/learning/writeaconnector/... so that'll give you a concrete aspect class to work with. @abeesh

jffmarker avatar Jul 26 '22 18:07 jffmarker

I thought there isn't a built-in way to track the provenance of element aspects, because the source of ElementOwnsExternalSourceAspects is an Element, and aspects don't derive from elements, they're a separate "thing." The core tests use getAspects with a class that extends ElementAspect and has a custom property to locate an aspect.

A synchronizer could do this as well, but it seems like a hack.

edit: I'm just trying to locate an aspect in an iModel using getAspect. There doesn't seem to be a way to do that with the current API unless I extend the class and do a linear pass.

jackson-at-bentley avatar Jul 26 '22 18:07 jackson-at-bentley

@jackson-at-bentley your suggestion to update insertAspect to return the aspects instanceId is a good suggestion. Since we have other API which requires the aspect instance Id we should return the id when we insert an aspect. @calebmshafer do you agree to adding this to the backlog?

ColinKerr avatar Aug 01 '22 17:08 ColinKerr

#4137's corresponding native PR added this (returning of the ECInstanceId), but was reverted. When it is remerged that could be considered done and we can consider closing this?

MichaelBelousov avatar Oct 03 '22 15:10 MichaelBelousov