[WIP] - Aquire locks during onElement and onModel api's#284
[WIP] - Aquire locks during onElement and onModel api's#284DanRod1999 wants to merge 20 commits intoitwinjs-v5from
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Pull request overview
This PR advances the transformer’s async API migration (notably model export/import and delete callbacks) and introduces an option to automatically acquire element locks during import/provenance updates to support hub-locked workflows (per issue #263).
Changes:
- Make
IModelExportHandler.onExportModelasync and update exporter flow to await it. - Make
IModelImportermodel/element/aspect import + element delete APIs async, updating transformer + test overrides accordingly. - Add an
aquireElementLocksoption and a new hub test that exercises transformation with locks enabled.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/transformer/src/IModelExporter.ts | Makes onExportModel async and awaits handler execution during export. |
| packages/transformer/src/IModelImporter.ts | Converts key import/delete APIs to async and adds optional lock acquisition during import. |
| packages/transformer/src/IModelTransformer.ts | Awaits importer calls, adds transformer-level lock behavior for provenance aspects, and plumbs new option into importer. |
| packages/transformer/src/test/IModelTransformerUtils.ts | Updates importer/exporter subclasses to match new async method signatures. |
| packages/transformer/src/test/standalone/IModelTransformer.test.ts | Updates test export handler override for async onExportModel. |
| packages/transformer/src/test/standalone/IModelTransformerHub.test.ts | Adds lock-enabled end-to-end hub test and updates helper to support lock/no-lock creation. |
| common/api/imodel-transformer.api.md | Updates public API declarations for new async signatures and new lock option. |
Comments suppressed due to low confidence (1)
packages/transformer/src/IModelImporter.ts:233
- With
aquireElementLocksenabled,onInsertModelcurrently doesn't acquire any lock before callingmodels.insertModel, while update/delete do. If locks are required by the hub, inserting models may fail. Consider acquiring the appropriate lock(s) for the model's id/modeledElement before insertion (consistent withonUpdateModel/onDeleteModel).
protected async onInsertModel(modelProps: ModelProps): Promise<Id64String> {
try {
const modelId: Id64String = this.targetDb.models.insertModel(modelProps);
Logger.logInfo(
loggerCategory,
`Inserted ${this.formatModelForLogger(modelProps)}`
);
this.trackProgress();
return modelId;
} catch (error) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
If option is enabled, we should also acquire schema lock in |
If the schema lock is needed that means we would be locking the entire imodel anyway right? Does that change anything? It slightly defeats the purpose of only locking what you need, but I guess in cases where they need to processSchema() there's no way to avoid it anyway |
Trying to resolve: #263
Make
onElement...()andonModel...()apis async. Add transformer option to acquire locks when these functions are called. If the transformation fails abandon changes and release locks acquired during transformation process, but only do so if the option flag for acquiring locks is set.