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.

Sign up for GitHub

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

Fix web FetchRequest does not respect disableConnectivityCheck client option #1925

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix web FetchRequest does not respect disableConnectivityCheck client option #1925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f1ecab
Add "disable connectivity check" tests for browser request implementa…
VeskeR Nov 22, 2024
f4acc4a
Fix web FetchRequest does not respect `disableConnectivityCheck` clie…
VeskeR Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 19 additions & 12 deletions src/platform/web/lib/http/http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,18 +117,25 @@ const Http = class {
this.Request = async (method, uri, headers, params, body) => {
return fetchRequestImplementation(method, client ?? null, uri, headers, params, body);
};
this.checkConnectivity = async function () {
Logger.logAction(
this.logger,
Logger.LOG_MICRO,
'(Fetch)Http.checkConnectivity()',
'Sending; ' + connectivityCheckUrl,
);
const requestResult = await this.doUri(HttpMethods.Get, connectivityCheckUrl, null, null, null);
const result = !requestResult.error && (requestResult.body as string)?.replace(/\n/, '') == 'yes';
Logger.logAction(this.logger, Logger.LOG_MICRO, '(Fetch)Http.checkConnectivity()', 'Result: ' + result);
return result;
};

if (client?.options.disableConnectivityCheck) {
this.checkConnectivity = async function () {
return true;
};
} else {
this.checkConnectivity = async function () {
Logger.logAction(
this.logger,
Logger.LOG_MICRO,
'(Fetch)Http.checkConnectivity()',
'Sending; ' + connectivityCheckUrl,
);
const requestResult = await this.doUri(HttpMethods.Get, connectivityCheckUrl, null, null, null);
const result = !requestResult.error && (requestResult.body as string)?.replace(/\n/, '') == 'yes';
Logger.logAction(this.logger, Logger.LOG_MICRO, '(Fetch)Http.checkConnectivity()', 'Result: ' + result);
return result;
};
Comment on lines +126 to +137
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Inconsistent handling of custom connectivity check URLs between XHR and Fetch implementations

The Fetch implementation doesn't handle custom connectivity check URLs (connectivityUrlIsDefault) in the same way as the XHR implementation. This could lead to different behaviors when using custom connectivity check URLs.

XHR implementation:

if (!connectivityUrlIsDefault) {
  result = !requestResult.error && isSuccessCode(requestResult.statusCode as number);
} else {
  result = !requestResult.error && (requestResult.body as string)?.replace(/\n/, '') == 'yes';
}

Fetch implementation only uses the body check:

const result = !requestResult.error && (requestResult.body as string)?.replace(/\n/, '') == 'yes';

Apply this diff to make the implementations consistent:

-          const result = !requestResult.error && (requestResult.body as string)?.replace(/\n/, '') == 'yes';
+          let result = false;
+          if (!connectivityUrlIsDefault) {
+            result = !requestResult.error && isSuccessCode(requestResult.statusCode as number);
+          } else {
+            result = !requestResult.error && (requestResult.body as string)?.replace(/\n/, '') == 'yes';
+          }

}
} else {
this.Request = async () => {
const error = hasImplementation
Expand Down
57 changes: 56 additions & 1 deletion test/browser/connection.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';

define(['shared_helper', 'chai'], function (Helper, chai) {
define(['ably', 'shared_helper', 'chai'], function (Ably, Helper, chai) {
var { expect, assert } = chai;
var transportPreferenceName = 'ably-transport-preference';

Expand Down Expand Up @@ -453,6 +453,61 @@ define(['shared_helper', 'chai'], function (Helper, chai) {
}
this.test.helper.closeAndFinish(done, realtime);
});

// this is identical to disable_connectivity_check test in connectivity, but here we specifically test browser supported request implementations
for (const scenario of [
{
description: 'XHR request disable connectivity check',
ctx: { initialFetchSupported: Ably.Rest.Platform.Config.fetchSupported },
before: (ctx) => {
Ably.Rest.Platform.Config.fetchSupported = false;
},
after: (ctx) => {
Ably.Rest.Platform.Config.fetchSupported = ctx.initialFetchSupported;
},
},
{
description: 'Fetch request disable connectivity check',
ctx: { initialXhrSupported: Ably.Rest.Platform.Config.xhrSupported },
before: (ctx) => {
Ably.Rest.Platform.Config.xhrSupported = false;
},
after: (ctx) => {
Ably.Rest.Platform.Config.xhrSupported = ctx.initialXhrSupported;
},
},
]) {
Comment on lines +458 to +479
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid mutating global configuration to prevent side effects in tests

Directly modifying global configurations like Ably.Rest.Platform.Config.fetchSupported and xhrSupported may lead to side effects affecting other tests, especially if tests are run in parallel. To prevent unintended interactions:

  • Use stubbing or mocking libraries (e.g., Sinon.js) to temporarily override these properties within the test scope.
  • Ensure that any modifications are safely isolated and restored, even if the test encounters an error.

/** @nospec */
it(scenario.description, async function () {
const helper = this.test.helper;
scenario.before(scenario.ctx);

helper.recordPrivateApi('pass.clientOption.connectivityCheckUrl');
helper.recordPrivateApi('pass.clientOption.disableConnectivityCheck');
const options = {
connectivityCheckUrl: helper.unroutableHost,
disableConnectivityCheck: true,
autoConnect: false,
};

let thrownError = null;
let res = null;
try {
helper.recordPrivateApi('call.http.checkConnectivity');
res = await helper.AblyRealtime(options).http.checkConnectivity();
} catch (error) {
thrownError = error;
}

scenario.after(scenario.ctx);
expect(
res && !thrownError,
'Connectivity check completed ' +
(thrownError &&
(helper.recordPrivateApi('call.Utils.inspectError'), helper.Utils.inspectError(thrownError))),
).to.be.ok;
});
}
Comment on lines +481 to +510
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure proper cleanup in tests by using 'finally' block or test hooks

If an exception occurs before scenario.after(scenario.ctx); is called, the global configurations may not be restored, potentially affecting other tests. To guarantee cleanup:

  • Place scenario.after(scenario.ctx); inside a finally block to ensure it executes regardless of errors.
  • Alternatively, use afterEach hooks provided by the testing framework to handle teardown.

Suggested change:

 try {
   helper.recordPrivateApi('call.http.checkConnectivity');
   res = await helper.AblyRealtime(options).http.checkConnectivity();
 } catch (error) {
   thrownError = error;
+} finally {
+  scenario.after(scenario.ctx);
 }
-scenario.after(scenario.ctx);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it(scenario.description, async function () {
const helper = this.test.helper;
scenario.before(scenario.ctx);
helper.recordPrivateApi('pass.clientOption.connectivityCheckUrl');
helper.recordPrivateApi('pass.clientOption.disableConnectivityCheck');
const options = {
connectivityCheckUrl: helper.unroutableHost,
disableConnectivityCheck: true,
autoConnect: false,
};
let thrownError = null;
let res = null;
try {
helper.recordPrivateApi('call.http.checkConnectivity');
res = await helper.AblyRealtime(options).http.checkConnectivity();
} catch (error) {
thrownError = error;
}
scenario.after(scenario.ctx);
expect(
res && !thrownError,
'Connectivity check completed ' +
(thrownError &&
(helper.recordPrivateApi('call.Utils.inspectError'), helper.Utils.inspectError(thrownError))),
).to.be.ok;
});
}
it(scenario.description, async function () {
const helper = this.test.helper;
scenario.before(scenario.ctx);
helper.recordPrivateApi('pass.clientOption.connectivityCheckUrl');
helper.recordPrivateApi('pass.clientOption.disableConnectivityCheck');
const options = {
connectivityCheckUrl: helper.unroutableHost,
disableConnectivityCheck: true,
autoConnect: false,
};
let thrownError = null;
let res = null;
try {
helper.recordPrivateApi('call.http.checkConnectivity');
res = await helper.AblyRealtime(options).http.checkConnectivity();
} catch (error) {
thrownError = error;
} finally {
scenario.after(scenario.ctx);
}
expect(
res && !thrownError,
'Connectivity check completed ' +
(thrownError &&
(helper.recordPrivateApi('call.Utils.inspectError'), helper.Utils.inspectError(thrownError))),
).to.be.ok;
});
}

});
}
});
Loading