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 for attaching aspect to a different element #4227

Closed
wants to merge 2 commits into from

Conversation

jackson-at-bentley
Copy link

Hey core team. I'm working on aspect support in the declarative connector framework @itwin/pcf. The aspect API gave me a hard time with my declarative synchronizer fir and I suspect @itwin/connector-framework is going to run into the same problems when it eventually supports aspects.

This may be the same problem as #3969: currently the aspect API doesn't give you a handle on the aspect you insert, and it's just gone forever in the iModel, because aspects don't have codes or provenance.

It seems like updateAspect fails when you change the Element navigation property of its argument, which has type RelatedElementProps. In the attached test, the backend seems to consider its updated argument a different aspect even though it has the same ECInstanceId.

I'm confused and will appreciate any insight. My workaround to this in pcf is just to delete and reinsert the aspect.

@jackson-at-bentley jackson-at-bentley added the help wanted Extra attention is needed label Sep 2, 2022
@jackson-at-bentley jackson-at-bentley requested a review from a team as a code owner September 2, 2022 20:26
@jackson-at-bentley
Copy link
Author

Sorry Paul the CI got upset with me when I didn't include one last time.

@pmconne
Copy link
Member

pmconne commented Sep 2, 2022

I think the premise "should be able to change the element to which an aspect attaches" is flawed. @swwilson-bsi does it make sense to talk about taking an existing aspect and relocating it to a different element?

@pmconne
Copy link
Member

pmconne commented Sep 2, 2022

Sorry Paul the CI got upset with me when I didn't include one last time.

No worries - you still gotta run rush change, just enter an empty string for changes uninteresting to end users.

@jackson-at-bentley
Copy link
Author

I haven't had problems changing the navigation properties (e.g., Parent) of elements when using updateElement so I thought updateAspect would behave the same. But I don't understand the implementation of aspects so this is from the perspective of an API consumer.

@swwilson-bsi
Copy link
Contributor

swwilson-bsi commented Sep 6, 2022

The call to updateAspect threw a NotFound error, right? That is because the boats element does not have that aspect.

Continuing with Paul's point, an aspect is like an optional property of an element. An element has one or more aspects. Just as you don't "move" a property from one element to another, you don't "move" an aspect either.

It may be a little confusing because an ECRelationship is used to connect an aspect to its owning element. Note that the ElementOwnsUniqueUniqueAspect ECRelationship has "embedding" strength. (Ditto for ElementOwnsMultiAspects.) That means that an element aspect is conceptually part of the element that owns it.

An aspect's identity is best thought of as the ID of the owning element, plus the aspect's ECClassId. That is why insertAspect does not return the the internal ecinstanceid of the aspect. I think the BIS core docs could be a little clearer on that point. The backend API docs are a little clearer.

At a mechanical level, I can understand why one might think of moving an aspect from one element to another. But there is no API for that. You'd have call deleteAspect on the harbor and then insertAspect on the boats.

A connector that updates aspects must look them up on the owning element by ECClassId. If the source repository has changed such that a certain BIS element should no longer have a given aspect, then the connector must call deleteAspect, identifying that element. If some other BIS element should have a similar aspect, then the connect must call insertAspect, identifying that other element.

Do you think the BIS core docs should be expanded to make this clearer? Are we missing some overview docs? @diegoalexdiaz

@diegoalexdiaz
Copy link
Contributor

Thanks for bringing up the need for more clarity on the article about element-aspects, @swwilson-bsi. We'll improve it for the next round of enhancements for the BIS guide.

@jackson-at-bentley
Copy link
Author

jackson-at-bentley commented Sep 6, 2022

Closing as a misunderstanding, thank you all for helping me understand how aspects are identified in the backend. I especially appreciate the advice for implementing connectors from Sam. I do still think the private ID of bis:ElementMultiAspect should be exposed by the API (because how else is updateAspect supposed to work with bis:ElementMultiAspect?).

I think Sam's linked section is clear. I didn't read it closely enough to realize that changing the Element navigation property identifies a different aspect.

It's the same situation for link-table relationships and updateInstance. Changing the source, target, or class identifies a different relationship.

So updateAspect and updateInstance are only for updating properties, not navigation properties.

@jackson-at-bentley jackson-at-bentley deleted the attach-aspect-elsewhere branch September 6, 2022 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants