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 support for unique aspects without requiring ECInstanceId #45

Merged
merged 1 commit into from
Sep 9, 2022

Conversation

jackson-at-bentley
Copy link
Contributor

@jackson-at-bentley jackson-at-bentley commented Sep 2, 2022

This PR brings the API of ElementAspectNode closer to the other nodes that create elements with navigation properties.

Keep in mind that ElementAspectNode only supports the creation of unique aspects. Tracking the provenance of bis:ElementMultiAspect is trickier and not really doable without changes to the iTwin APIs. fir doesn't make any attempt to do so and just reinserts all aspects attached to an element.

As originally written, the connector author has to intervene in the DMO and specify the ECInstanceId of the element to attach the unique aspect to. Now there's an optional elementAttr in ElementAspectDMO. It's optional to allow compatibility with the old API, which means if you forget to specify both elementAttr and the element property of your node PCF will throw at runtime. If we can confirm anyone isn't using the old API (very likely) I'd like to drop it.

I've also made changes to the synchronization of aspects to properly track the provenance of an aspect if it attaches itself to a different element. This solution is a hack, but I have a PR open to discuss alternatives.

@jchick-bentley
Copy link
Collaborator

@akshay-madhukar @admccarthy1 Can you please have a look at these diffs esp. *.ts changes for unique element aspect enhancements? Thx.

@jackson-at-bentley
Copy link
Contributor Author

jackson-at-bentley commented Sep 6, 2022

Yeah, sorry. In the future I'm going to move documentation changes to their own PR even if they're documenting this PR's changes. Too noisy otherwise.

Here's a link to what changed.

Rebased onto main and removed docs.

@jchick-bentley
Copy link
Collaborator

@akshay-madhukar Hi Akshay, could you please have a look at this PR? We're hoping to get this merge before Jackson's internship is complete.

@jackson-at-bentley jackson-at-bentley added the enhancement New feature or request label Sep 8, 2022
@jackson-at-bentley jackson-at-bentley force-pushed the unique-aspects branch 2 times, most recently from 642a3f2 to dc5d77b Compare September 8, 2022 22:37
@jackson-at-bentley
Copy link
Contributor Author

jackson-at-bentley commented Sep 8, 2022

I've reverted the changes I made to Zach's syncProvenance. Updating an external source aspect from the perspective of the one in the source data (the one we construct from the IRInstance) means relying on unspecified behavior from the backend if the element property changes. The way I've modified syncElementUniqueAspect means we may try to update an external source aspect using a different element in that first call to syncProvenance.

Ideally we'd break up syncProvenance into two functions: one that does the external source aspect comparision, maybe

function compareExternalSourceAspect(arg: SyncArg): ItemState { ... }

and syncProvenance which actually writes the aspect if it's changed. Anywho, less code to review now.

@jchick-bentley jchick-bentley merged commit 6fe5f0f into main Sep 9, 2022
@jchick-bentley jchick-bentley deleted the unique-aspects branch September 9, 2022 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants