fix processSchemas potentially deleting schema export dir, and update…

… linter rules to prevent it in the future (iTwin#1786)

Co-authored-by: Mike Belousov <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

clients/projectshare/src/ProjectShareClient.ts

@@ -524,7 +524,7 @@ export class ProjectShareClient extends WsgClient {
     try {
       const accessurl = file.accessUrl !== undefined ? file.accessUrl : "";
       await this.uploadBlob(requestContext, accessurl, data);
-      return this.updateFileExistsProperty(requestContext, contextId, file.wsgId);
+      return await this.updateFileExistsProperty(requestContext, contextId, file.wsgId);
     } catch (err) {
       throw err;
     }

common/changes/@bentley/backend-itwin-client/transformer-await-returned-in-try_2021-07-06-11-52.json

@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "packageName": "@bentley/backend-itwin-client",
+      "comment": "async order change in BlobDownloader",
+      "type": "none"
+    }
+  ],
+  "packageName": "@bentley/backend-itwin-client",
+  "email": "[email protected]"
+}

common/changes/@bentley/bentleyjs-core/transformer-await-returned-in-try_2021-07-06-11-52.json

@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "packageName": "@bentley/bentleyjs-core",
+      "comment": "async return value rather than fulfilled promise",
+      "type": "none"
+    }
+  ],
+  "packageName": "@bentley/bentleyjs-core",
+  "email": "[email protected]"
+}

common/changes/@bentley/eslint-plugin/transformer-await-returned-in-try_2021-07-06-11-52.json

@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "packageName": "@bentley/eslint-plugin",
+      "comment": "enforce use of typed return-await alternative with try/catch handling",
+      "type": "none"
+    }
+  ],
+  "packageName": "@bentley/eslint-plugin",
+  "email": "[email protected]"
+}

common/changes/@bentley/imodeljs-backend/transformer-await-returned-in-try_2021-07-06-11-52.json

@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "packageName": "@bentley/imodeljs-backend",
+      "comment": "await returned promise to ensure finally block doesn't intercept it",
+      "type": "none"
+    }
+  ],
+  "packageName": "@bentley/imodeljs-backend",
+  "email": "[email protected]"
+}

common/changes/@bentley/imodeljs-common/transformer-await-returned-in-try_2021-07-06-11-52.json

@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "packageName": "@bentley/imodeljs-common",
+      "comment": "",
+      "type": "none"
+    }
+  ],
+  "packageName": "@bentley/imodeljs-common",
+  "email": "[email protected]"
+}

common/changes/@bentley/imodeljs-frontend/transformer-await-returned-in-try_2021-07-06-11-52.json

@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "packageName": "@bentley/imodeljs-frontend",
+      "comment": "internal async order change",
+      "type": "none"
+    }
+  ],
+  "packageName": "@bentley/imodeljs-frontend",
+  "email": "[email protected]"
+}

common/changes/@bentley/projectshare-client/transformer-await-returned-in-try_2021-07-06-11-52.json

@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "packageName": "@bentley/projectshare-client",
+      "comment": "async order change in uploadContentInFile",
+      "type": "none"
+    }
+  ],
+  "packageName": "@bentley/projectshare-client",
+  "email": "[email protected]"
+}

core/backend-itwin-client/src/imodelhub/BlobDownloader.ts

@@ -592,7 +592,7 @@ export class BlobDownloader {
           release();
         }
       });
-      return session.ready;
+      return await session.ready;
     } finally {
       unlock();
     }

core/backend/src/IModelTransformer.ts

@@ -773,6 +773,11 @@ export class IModelTransformer extends IModelExportHandler {
     IModelJsFs.writeFileSync(schemaPath, await schema.toXmlString());
   }
 
+  // pending PR https://github.com/typescript-eslint/typescript-eslint/pull/3601 fixes the rule @typescript-eslint/return-await
+  // to work in try/catch syntax in functions that contain a nested function
+  // until that merges and we upgrade our dependency, the callback cannot be defined inside the method it is used
+  private makeAbsolute = (s: string) => path.join(this._schemaExportDir, s);
+
   /** Cause all schemas to be exported from the source iModel and imported into the target iModel.
    * @note For performance reasons, it is recommended that [IModelDb.saveChanges]($backend) be called after `processSchemas` is complete.
    * It is more efficient to process *data* changes after the schema changes have been saved.
@@ -786,8 +791,8 @@ export class IModelTransformer extends IModelExportHandler {
       const exportedSchemaFiles = IModelJsFs.readdirSync(this._schemaExportDir);
       if (exportedSchemaFiles.length === 0)
         return;
-      const schemaFullPaths = exportedSchemaFiles.map((s) => path.join(this._schemaExportDir, s));
-      return this.targetDb.importSchemas(requestContext, schemaFullPaths);
+      const schemaFullPaths = exportedSchemaFiles.map(this.makeAbsolute);
+      return await this.targetDb.importSchemas(requestContext, schemaFullPaths);
     } finally {
       requestContext.enter();
       IModelJsFs.removeSync(this._schemaExportDir);

core/backend/src/test/standalone/IModelTransformer.test.ts

@@ -4,6 +4,7 @@
 *--------------------------------------------------------------------------------------------*/
 import { assert } from "chai";
 import * as path from "path";
+import * as sinon from "sinon";
 import { DbResult, Id64, Id64String, Logger, LogLevel } from "@bentley/bentleyjs-core";
 import { Point3d, Range3d, StandardViewIndex, Transform, YawPitchRollAngles } from "@bentley/geometry-core";
 import {
@@ -958,4 +959,37 @@ describe("IModelTransformer", () => {
     targetDb.close();
   });
 
+  it("processSchemas should wait for the schema import to finish to delete the export directory", async () => {
+    const reqCtx = new BackendRequestContext();
+    const cloneTestSchema100 = path.join(KnownTestLocations.assetsDir, "CloneTest.01.00.00.ecschema.xml");
+    const sourceDbPath = IModelTestUtils.prepareOutputFile("IModelTransformer", "FinallyFirstTest.bim");
+    const sourceDb = SnapshotDb.createEmpty(sourceDbPath, { rootSubject: { name: "FinallyFirstTest" } });
+    await sourceDb.importSchemas(reqCtx, [cloneTestSchema100]);
+    sourceDb.saveChanges();
+
+    const targetDbPath = IModelTestUtils.prepareOutputFile("IModelTransformer", "FinallyFirstTestOut.bim");
+    const targetDb = SnapshotDb.createEmpty(targetDbPath, { rootSubject: { name: "FinallyFirstTest" } });
+    const transformer = new IModelTransformer(sourceDb, targetDb);
+
+    const importSchemasResolved = sinon.spy();
+    let importSchemasPromise: Promise<void>;
+
+    sinon.replace(targetDb, "importSchemas", sinon.fake(async () => {
+      importSchemasPromise = new Promise((resolve) => setImmediate(() => {
+        importSchemasResolved();
+        resolve(undefined);
+      }));
+      return importSchemasPromise;
+    }));
+
+    const removeSyncSpy = sinon.spy(IModelJsFs, "removeSync");
+
+    await transformer.processSchemas(reqCtx);
+    assert(removeSyncSpy.calledAfter(importSchemasResolved));
+
+    sinon.restore();
+    sourceDb.close();
+    targetDb.close();
+  });
+
 });

core/bentley/src/OneAtATimeAction.ts

@@ -83,8 +83,7 @@ export class OneAtATimeAction<T> {
     }
 
     try {
-      await promise;
-      return promise;
+      return await promise;
     } finally {
       // do all of this whether promise was fulfilled or rejected
       this._active = this._pending; // see if there's a pending request waiting

core/common/src/rpc/core/RpcInvocation.ts

@@ -130,6 +130,8 @@ export class RpcInvocation {
       (impl as any)[CURRENT_INVOCATION] = this;
       const op = this.lookupOperationFunction(impl);
 
+      // @typescript-eslint/return-await doesn't agree with awaiting values that *might* be a promise
+      // eslint-disable-next-line @typescript-eslint/return-await
       return await op.call(impl, ...parameters);
     } catch (error) {
       return this.reject(error);

core/frontend/src/ContextRealityModelState.ts

@@ -28,7 +28,7 @@ async function getAccessToken(): Promise<AccessToken | undefined> {
     return undefined; // Not signed in
 
   try {
-    return IModelApp.authorizationClient.getAccessToken();
+    return await IModelApp.authorizationClient.getAccessToken();
   } catch (_) {
     return undefined;
   }

core/frontend/src/test/render/webgl/FrameBuffer.test.ts

@@ -14,9 +14,9 @@ import { TextureHandle } from "../../../render/webgl/Texture";
 import { System } from "../../../render/webgl/System";
 
 describe("FrameBuffer tests", () => {
-  // eslint-disable-next-line no-return-await
+  // eslint-disable-next-line @typescript-eslint/return-await
   before(async () => await IModelApp.startup());
-  // eslint-disable-next-line no-return-await
+  // eslint-disable-next-line @typescript-eslint/return-await
   after(async () => await IModelApp.shutdown());
 
   it("should produce and bind a valid framebuffer with single color attachment", () => {

core/frontend/src/tile/map/ImageryTileTree.ts

@@ -222,7 +222,7 @@ class ImageryTileLoader extends RealityTileLoader {
     try {
       const textureParams = new RenderTexture.Params(undefined, RenderTexture.Type.FilteredTileSection);
 
-      return imageElementFromImageSource(imageSource)
+      return await imageElementFromImageSource(imageSource)
         .then((image) => isCanceled() ? undefined : system.createTextureFromImage(image, ImageSourceFormat.Png === imageSource.format, iModel, textureParams))
         .catch((_) => undefined);
     } catch (e) {

test-apps/display-test-app/src/frontend/openStandaloneIModel.ts

@@ -8,7 +8,7 @@ import { BriefcaseConnection, IModelConnection, SnapshotConnection } from "@bent
 
 export async function openStandaloneIModel(filename: string, writable: boolean,): Promise<IModelConnection> {
   try {
-    return BriefcaseConnection.openStandalone(filename, writable ? OpenMode.ReadWrite : OpenMode.Readonly, { key: filename });
+    return await BriefcaseConnection.openStandalone(filename, writable ? OpenMode.ReadWrite : OpenMode.Readonly, { key: filename });
   } catch (err) {
     if (writable && err instanceof IModelError && err.errorNumber === IModelStatus.ReadOnly)
       return SnapshotConnection.openFile(filename);

tools/eslint-plugin/dist/configs/imodeljs-recommended.js

@@ -161,6 +161,7 @@ module.exports = {
         "allow": ["T", "args"]
       }
     ],
+    "@typescript-eslint/return-await": "error",
     "@typescript-eslint/no-this-alias": "error",
     "@typescript-eslint/no-use-before-define": "off",
     "@typescript-eslint/no-unnecessary-type-assertion": "error",
@@ -283,7 +284,7 @@ module.exports = {
       }
     ],
     "no-restricted-syntax": ["error", { selector: "TSEnumDeclaration[const=true]", message: "const enums are not allowed" }],
-    "no-return-await": "error",
+    "no-return-await": "off", // using @typescript-eslint/return-await instead
     "no-shadow": "off", // using @typescript-eslint/no-shadow instead
     "no-sparse-arrays": "error",
     "no-template-curly-in-string": "error",