[MM-53295] Fix crash in diagnostics when server is unreachable #2771
Conversation
| @@ -24,7 +25,7 @@ const run = async (logger: ElectronLog): Promise<DiagnosticStepResponse> => { | |||
| throw new Error(`Invalid server configuration. Server Url: ${server.url}, server name: ${server.name}`); | |||
| } | |||
|
|
|||
| const serverOnline = await isOnline(logger, `${server.url}/api/v4/system/ping`); | |||
| const serverOnline = await isOnline(logger, parseURL(`${server.url}/api/v4/system/ping`)?.toString()); | |||
There was a problem hiding this comment.
If the parseUrl here returns undefined, will it get defaulted above in the isOnline function? Do we need to check for undefined?
There was a problem hiding this comment.
Not in this case no, it's very unlikely that this is undefined but if it ever is (would only happen if someone tried to break something) the default case would catch it from crashing.
There was a problem hiding this comment.
I'm not familiar what this is trying to do just want to make sure that is OK to default to `'https://community.mattermost.com/api/v4/system/ping' in the rare case it's an invalid url here
There was a problem hiding this comment.
Yeah that's fine, this is all part of a health check in case users experience issues. Worst case scenario it pings community, which is only an indication that their server URL is wrong/unavailable. It's actually potentially helpful for it to work this way :P
There was a problem hiding this comment.
Trying to understand the difference here, the use of parseURL here is only to validate if its a current URL, right? If so, are we including all the basic stuff such as removing trailing slashes, whitespaces, missing protocols, etc?
There was a problem hiding this comment.
parseURL is here to remove extra slashes, since we assume that one is needed before the api route. But often the slash is already there so we run it through parseURL to make sure it's all correct.
The whitespace and missing protocols should already all be taken care of at this point before the server was even added.
| @@ -24,7 +25,7 @@ const run = async (logger: ElectronLog): Promise<DiagnosticStepResponse> => { | |||
| throw new Error(`Invalid server configuration. Server Url: ${server.url}, server name: ${server.name}`); | |||
| } | |||
|
|
|||
| const serverOnline = await isOnline(logger, `${server.url}/api/v4/system/ping`); | |||
| const serverOnline = await isOnline(logger, parseURL(`${server.url}/api/v4/system/ping`)?.toString()); | |||
There was a problem hiding this comment.
I'm not familiar what this is trying to do just want to make sure that is OK to default to `'https://community.mattermost.com/api/v4/system/ping' in the rare case it's an invalid url here
| @@ -24,7 +25,7 @@ const run = async (logger: ElectronLog): Promise<DiagnosticStepResponse> => { | |||
| throw new Error(`Invalid server configuration. Server Url: ${server.url}, server name: ${server.name}`); | |||
| } | |||
|
|
|||
| const serverOnline = await isOnline(logger, `${server.url}/api/v4/system/ping`); | |||
| const serverOnline = await isOnline(logger, parseURL(`${server.url}/api/v4/system/ping`)?.toString()); | |||
There was a problem hiding this comment.
Trying to understand the difference here, the use of parseURL here is only to validate if its a current URL, right? If so, are we including all the basic stuff such as removing trailing slashes, whitespaces, missing protocols, etc?
Summary
When running diagnostics, we check the current configured servers to ensure reachability. Unfortunately, there was a bug introduced when refactoring for the
ServerManagerin which the wrong URL was provided and would always crash. This revealed a further bug where if the server was unreachable, diagnostics would always crash.This PR fixes the issue with the wrong URL, as well as catches the potential crash that could happen.
Ticket Link
https://mattermost.atlassian.net/browse/MM-53295
Closes #2769