Merge pull request #109 from amtrack/fix/retry-when-logged-in

fix: retry opening pages when already logged in

package.json

@@ -12,6 +12,7 @@
     "@oclif/errors": "^1",
     "@salesforce/command": "^1.2.0",
     "json-merge-patch": "^0.2.3",
+    "p-retry": "^3.0.1",
     "puppeteer": "^1.12.2",
     "tslib": "^1"
   },

src/browserforce.ts

@@ -1,11 +1,7 @@
 import { core } from '@salesforce/command';
+import * as pRetry from 'p-retry';
 import * as puppeteer from 'puppeteer';
 import { URL } from 'url';
-import { retry } from './plugins/utils';
-
-class FrontdoorRedirectError extends Error {}
-class LoginError extends Error {}
-class RetryableError extends Error {}
 
 const PERSONAL_INFORMATION_PATH =
   'setup/personalInformationSetup.apexp?nooverride=1';
@@ -28,10 +24,11 @@ export default class Browserforce {
       args: ['--no-sandbox', '--disable-setuid-sandbox'],
       headless: !(process.env.BROWSER_DEBUG === 'true')
     });
-    await this.retryLogin(
+    await this.openPage(
       `secur/frontdoor.jsp?sid=${
         this.org.getConnection().accessToken
-      }&retURL=${encodeURIComponent(PERSONAL_INFORMATION_PATH)}`
+      }&retURL=${encodeURIComponent(PERSONAL_INFORMATION_PATH)}`,
+      { waitUntil: ['load', 'domcontentloaded', 'networkidle0'] }
     );
     return this;
   }
@@ -51,32 +48,6 @@ export default class Browserforce {
     }
   }
 
-  public async throwResponseErrors(response) {
-    if (!response) {
-      throw new Error('no response');
-    }
-    if (!response.ok()) {
-      throw new RetryableError(
-        `${response.status()}: ${response.statusText()}`
-      );
-    }
-    if (response.url().indexOf('/?ec=302') > 0) {
-      if (
-        !response.url().startsWith(this.getInstanceUrl()) &&
-        !response.url().startsWith(this.getLightningUrl())
-      ) {
-        const redactedUrl = response
-          .url()
-          .replace(/sid=(.*)/, 'sid=<REDACTED>')
-          .replace(/sid%3D(.*)/, 'sid=<REDACTED>');
-        throw new FrontdoorRedirectError(
-          `expected instance or lightning URL but got: ${redactedUrl}`
-        );
-      }
-      throw new LoginError('login failed');
-    }
-  }
-
   public async throwPageErrors(page) {
     const errorHandle = await page.$(ERROR_DIV_SELECTOR);
     if (errorHandle) {
@@ -106,29 +77,58 @@ export default class Browserforce {
 
   // path instead of url
   public async openPage(urlPath, options?) {
-    const result = await retry(
+    const result = await pRetry(
       async () => {
-        try {
-          await this.resolveDomains();
-        } catch (error) {
-          throw new RetryableError(error);
-        }
+        await this.resolveDomains();
         const page = await this.browser.newPage();
         page.setDefaultNavigationTimeout(
           parseInt(process.env.BROWSERFORCE_NAVIGATION_TIMEOUT_MS, 10) || 90000
         );
         await page.setViewport({ width: 1024, height: 768 });
         const url = `${this.getInstanceUrl()}/${urlPath}`;
         const response = await page.goto(url, options);
-        await this.throwResponseErrors(response);
+        if (response) {
+          if (!response.ok()) {
+            throw new Error(`${response.status()}: ${response.statusText()}`);
+          }
+          if (response.url().indexOf('/?ec=302') > 0) {
+            if (
+              response.url().startsWith(this.getInstanceUrl()) ||
+              response.url().startsWith(this.getLightningUrl())
+            ) {
+              // the url looks ok so it is a login error
+              throw new pRetry.AbortError('login failed');
+            } else {
+              // the url is not as expected
+              const redactedUrl = response
+                .url()
+                .replace(/sid=(.*)/, 'sid=<REDACTED>')
+                .replace(/sid%3D(.*)/, 'sid=<REDACTED>');
+              if (this.logger) {
+                this.logger.warn(
+                  `expected ${this.getInstanceUrl()} or ${this.getLightningUrl()} but got: ${redactedUrl}`
+                );
+                this.logger.warn('refreshing auth...');
+              }
+              await this.org.refreshAuth();
+              throw new Error('redirection failed');
+            }
+          }
+        }
         // await this.throwPageErrors(page);
         return page;
       },
-      5,
-      4000,
-      true,
-      RetryableError.prototype,
-      this.logger
+      {
+        onFailedAttempt: error => {
+          if (this.logger) {
+            this.logger.warn(
+              `retrying ${error.retriesLeft} more time(s) because of "${error}"`
+            );
+          }
+        },
+        retries: 4,
+        minTimeout: 4 * 1000
+      }
     );
     return result;
   }
@@ -141,44 +141,4 @@ export default class Browserforce {
     const myDomain = this.getInstanceUrl().match(/https?\:\/\/([^.]*)/)[1];
     return `https://${myDomain}.lightning.force.com`;
   }
-
-  private async retryLogin(
-    loginUrl,
-    refreshAuthLeft = 1,
-    frontdoorWorkaroundLeft = 1
-  ) {
-    try {
-      await this.openPage(loginUrl, {
-        waitUntil: ['load', 'domcontentloaded', 'networkidle0']
-      });
-    } catch (err) {
-      if (err instanceof LoginError && refreshAuthLeft > 0) {
-        if (this.logger) {
-          this.logger.warn(`retrying with refreshed auth because of "${err}"`);
-        }
-        await this.org.refreshAuth();
-        await this.retryLogin(
-          loginUrl,
-          refreshAuthLeft - 1,
-          frontdoorWorkaroundLeft
-        );
-      } else if (
-        err instanceof FrontdoorRedirectError &&
-        frontdoorWorkaroundLeft > 0
-      ) {
-        if (this.logger) {
-          this.logger.warn(
-            `retrying open page with instance url because of "${err}"`
-          );
-        }
-        await this.retryLogin(
-          PERSONAL_INFORMATION_PATH,
-          refreshAuthLeft,
-          frontdoorWorkaroundLeft - 1
-        );
-      } else {
-        throw err;
-      }
-    }
-  }
 }

src/plugins/security/identity-provider/index.ts

@@ -1,7 +1,8 @@
 import { SalesforceId } from 'jsforce';
 import * as jsonMergePatch from 'json-merge-patch';
+import * as pRetry from 'p-retry';
 import { BrowserforcePlugin } from '../../../plugin';
-import { removeNullValues, retry } from '../../utils';
+import { removeNullValues } from '../../utils';
 
 const PATHS = {
   EDIT_VIEW: 'setup/secur/idp/IdpPage.apexp'
@@ -45,7 +46,7 @@ export default class IdentityProvider extends BrowserforcePlugin {
   public async apply(plan) {
     if (plan.enabled && plan.certificate && plan.certificate !== '') {
       // wait for cert to become available in Identity Provider UI
-      await retry(
+      await pRetry(
         async () => {
           const certsResponse = await this.org
             .getConnection()
@@ -95,8 +96,10 @@ export default class IdentityProvider extends BrowserforcePlugin {
             page.click(SELECTORS.SAVE_BUTTON)
           ]);
         },
-        5,
-        2000
+        {
+          retries: 5,
+          minTimeout: 2 * 1000
+        }
       );
     } else {
       const page = await this.browserforce.openPage(PATHS.EDIT_VIEW);

src/plugins/utils.ts

@@ -1,36 +1,3 @@
-// source: https://gitlab.com/snippets/1775781 Daniel Iñigo <[email protected]>
-export async function retry(
-  fn,
-  retriesLeft = 5,
-  interval = 1000,
-  exponential = false,
-  errorPrototype = Error.prototype,
-  logger?
-) {
-  try {
-    const val = await fn();
-    return val;
-  } catch (error) {
-    if (errorPrototype.isPrototypeOf(error) && retriesLeft) {
-      if (logger) {
-        logger.warn(
-          `retrying ${retriesLeft} more time(s) because of "${error}"`
-        );
-      }
-      // tslint:disable-next-line no-string-based-set-timeout
-      await new Promise(r => setTimeout(r, interval));
-      return retry(
-        fn,
-        retriesLeft - 1,
-        exponential ? interval * 2 : interval,
-        exponential,
-        errorPrototype,
-        logger
-      );
-    } else throw error;
-  }
-}
-
 export function removeEmptyValues(obj) {
   if (!obj) {
     obj = {};

test/browserforce.e2e-spec.ts

@@ -5,7 +5,7 @@ import Browserforce from '../src/browserforce';
 describe('Browser', () => {
   describe('login()', () => {
     it('should successfully login with valid credentials', async function() {
-      this.timeout(1000 * 180);
+      this.timeout(1000 * 300);
       this.slow(1000 * 30);
       const defaultScratchOrg = await core.Org.create({});
       const ux = await UX.create();
@@ -16,11 +16,12 @@ describe('Browser', () => {
     });
 
     it('should fail login with invalid credentials', async function() {
-      this.timeout(1000 * 180);
+      this.timeout(1000 * 300);
       this.slow(1000 * 30);
       const fakeOrg = await core.Org.create({});
       fakeOrg.getConnection().accessToken = 'invalid';
-      const bf = new Browserforce(fakeOrg);
+      const ux = await UX.create();
+      const bf = new Browserforce(fakeOrg, ux.cli);
       await assert.rejects(async () => {
         await bf.login();
       }, /login failed/);

test/utils.test.ts

@@ -1,79 +1,5 @@
 import * as assert from 'assert';
-import { retry, semanticallyCleanObject } from '../src/plugins/utils';
-
-class FooError extends Error {}
-
-async function sayHello() {
-  return 'hi';
-}
-
-describe('retry', () => {
-  it('should return on first try', async function() {
-    const res = await retry(sayHello);
-    assert.deepStrictEqual(res, 'hi');
-  });
-  it('should return on third try', async function() {
-    let helloCounter = 0;
-    const res = await retry(async function() {
-      helloCounter++;
-      if (helloCounter >= 3) {
-        return 'hi';
-      }
-      throw new Error('not yet');
-    });
-    assert.deepStrictEqual(res, 'hi');
-    assert.deepEqual(helloCounter, 3);
-  });
-  it('should return on third try for specific error', async function() {
-    let helloCounter = 0;
-    const res = await retry(
-      async function() {
-        helloCounter++;
-        if (helloCounter >= 3) {
-          return 'hi';
-        }
-        throw new FooError('not yet');
-      },
-      5,
-      1000,
-      false,
-      FooError.prototype
-    );
-    assert.deepStrictEqual(res, 'hi');
-    assert.deepEqual(helloCounter, 3);
-  });
-  it('should retry on any Error', async function() {
-    let helloCounter = 0;
-    const res = await retry(
-      async function() {
-        helloCounter++;
-        if (helloCounter >= 3) {
-          return 'hi';
-        }
-        throw new FooError('not yet');
-      },
-      5,
-      1000,
-      false,
-      Error.prototype
-    );
-    assert.deepStrictEqual(res, 'hi');
-    assert.deepEqual(helloCounter, 3);
-  });
-  it('should throw immediately if specific error is not thrown', async function() {
-    await assert.rejects(async () => {
-      await retry(
-        async function() {
-          throw new Error('hi');
-        },
-        5,
-        1000,
-        false,
-        FooError.prototype
-      );
-    }, /hi/);
-  });
-});
+import { semanticallyCleanObject } from '../src/plugins/utils';
 
 describe('semanticallyCleanObject', () => {
   it('should clean object', async function() {

yarn.lock

@@ -5843,6 +5843,13 @@ p-locate@^3.0.0:
   dependencies:
     p-limit "^2.0.0"
 
+p-retry@^3.0.1:
+  version "3.0.1"
+  resolved "https://registry.yarnpkg.com/p-retry/-/p-retry-3.0.1.tgz#316b4c8893e2c8dc1cfa891f406c4b422bebf328"
+  integrity sha512-XE6G4+YTTkT2a0UWb2kjZe8xNwf8bIbnqpc/IS/idOBVhyves0mK5OJgeocjx7q5pvX/6m23xuzVPYT1uGM73w==
+  dependencies:
+    retry "^0.12.0"
+
 p-timeout@^1.1.1:
   version "1.2.1"
   resolved "https://registry.yarnpkg.com/p-timeout/-/p-timeout-1.2.1.tgz#5eb3b353b7fce99f101a1038880bb054ebbea386"
@@ -6595,6 +6602,11 @@ ret@~0.1.10:
   resolved "https://registry.yarnpkg.com/ret/-/ret-0.1.15.tgz#b8a4825d5bdb1fc3f6f53c2bc33f81388681c7bc"
   integrity sha512-TTlYpa+OL+vMMNG24xSlQGEJ3B/RzEfUlLct7b5G/ytav+wPrplCpVMFuwzXbkecJrb6IYo1iFb0S9v37754mg==
 
+retry@^0.12.0:
+  version "0.12.0"
+  resolved "https://registry.yarnpkg.com/retry/-/retry-0.12.0.tgz#1b42a6266a21f07421d1b0b54b7dc167b01c013b"
+  integrity sha1-G0KmJmoh8HQh0bC1S33BZ7AcATs=
+
 rgb2hex@~0.1.0:
   version "0.1.9"
   resolved "https://registry.yarnpkg.com/rgb2hex/-/rgb2hex-0.1.9.tgz#5d3e0e14b0177b568e6f0d5b43e34fbfdb670346"