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.

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

chore(client): use application errors instead of PlatformErrors #2337

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
11 changes: 6 additions & 5 deletions client/electron/routing_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ import {TunnelStatus} from '../src/www/app/outline_server_repository/vpn';
import {ErrorCode} from '../src/www/model/errors';
import {
PlatformError,
ROUTING_SERVICE_NOT_RUNNING,
serializeForIpc,
GoErrorCode,
} from '../src/www/model/platform_error';

const isLinux = platform() === 'linux';
Expand Down Expand Up @@ -135,10 +136,10 @@ export class RoutingDaemon {
cleanup();
newSocket.destroy();
const perr = new PlatformError(
ROUTING_SERVICE_NOT_RUNNING,
GoErrorCode.ROUTING_SERVICE_NOT_RUNNING,
'routing daemon service stopped before started'
);
reject(new Error(perr.toJSON()));
reject(new Error(serializeForIpc(perr)));
} else {
fulfill();
}
Expand All @@ -159,11 +160,11 @@ export class RoutingDaemon {
console.error('Routing daemon socket setup failed', err);
this.socket = null;
const perr = new PlatformError(
ROUTING_SERVICE_NOT_RUNNING,
GoErrorCode.ROUTING_SERVICE_NOT_RUNNING,
'routing daemon is not running',
{cause: err}
);
reject(new Error(perr.toJSON()));
reject(new Error(serializeForIpc(perr)));
};
newSocket.once('error', initialErrorHandler);
});
Expand Down
6 changes: 3 additions & 3 deletions client/go/outline/fetch.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func fetchResource(url string) (string, error) {
resp, err := client.Get(url)
if err != nil {
return "", platerrors.PlatformError{
Code: platerrors.FetchConfigFailed,
Code: platerrors.GenericErr,
Message: "failed to fetch the URL",
Details: platerrors.ErrorDetails{"url": url},
Cause: platerrors.ToPlatformError(err),
Expand All @@ -45,7 +45,7 @@ func fetchResource(url string) (string, error) {
resp.Body.Close()
if resp.StatusCode > 299 {
return "", platerrors.PlatformError{
Code: platerrors.FetchConfigFailed,
Code: platerrors.GenericErr,
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be useful to expose this error details? Especially the HTTP response code?

Message: "non-successful HTTP status",
Details: platerrors.ErrorDetails{
"status": resp.Status,
Expand All @@ -55,7 +55,7 @@ func fetchResource(url string) (string, error) {
}
if err != nil {
return "", platerrors.PlatformError{
Code: platerrors.FetchConfigFailed,
Code: platerrors.GenericErr,
Message: "failed to read the body",
Details: platerrors.ErrorDetails{"url": url},
Cause: platerrors.ToPlatformError(err),
Expand Down
4 changes: 2 additions & 2 deletions client/go/outline/fetch_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func TestFetchResource_HTTPStatusError(t *testing.T) {
content, err := fetchResource(server.URL)
require.Empty(t, content)
require.ErrorAs(t, err, &perr)
require.Equal(t, platerrors.FetchConfigFailed, perr.Code)
require.Equal(t, platerrors.GenericErr, perr.Code)
require.Error(t, perr.Cause)
}
}
Expand All @@ -98,7 +98,7 @@ func TestFetchResource_BodyReadError(t *testing.T) {
content, err := fetchResource(server.URL)
require.Empty(t, content)
require.ErrorAs(t, err, &perr)
require.Equal(t, platerrors.FetchConfigFailed, perr.Code)
require.Equal(t, platerrors.GenericErr, perr.Code)
require.Error(t, perr.Cause)
}

Expand Down
4 changes: 2 additions & 2 deletions client/go/outline/platerrors/error_code.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@ const (
//////////

const (
// FetchConfigFailed means we failed to fetch a config from a remote location.
FetchConfigFailed ErrorCode = "ERR_FETCH_CONFIG_FAILURE"
// GenericErr indicates a generic error that is not directly surfaced to the user.
GenericErr ErrorCode = "ERR_GENERIC"
Copy link
Contributor

@jyyi1 jyyi1 Jan 23, 2025

Choose a reason for hiding this comment

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

Can we use the InternalError ErrorCode = "ERR_INTERNAL_ERROR" defined above, which has already been widely used across the code base?


// IllegalConfig indicates an invalid config to connect to a remote server.
IllegalConfig ErrorCode = "ERR_ILLEGAL_CONFIG"
Expand Down
30 changes: 0 additions & 30 deletions client/go/outline/vpn/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,33 +27,3 @@ func errCancelled(cause error) error {
Cause: perrs.ToPlatformError(cause),
}
}

func errIllegalConfig(msg string, params ...any) error {
return errPlatError(perrs.IllegalConfig, msg, nil, params...)
}

func errSetupVPN(msg string, cause error, params ...any) error {
return errPlatError(perrs.SetupSystemVPNFailed, msg, cause, params...)
}

func errCloseVPN(msg string, cause error, params ...any) error {
return errPlatError(perrs.DisconnectSystemVPNFailed, msg, cause, params...)
}

func errPlatError(code perrs.ErrorCode, msg string, cause error, params ...any) error {
logParams := append(params, "err", cause)
slog.Error(msg, logParams...)

details := perrs.ErrorDetails{}
for i := 1; i < len(params); i += 2 {
if key, ok := params[i-1].(string); ok {
details[key] = params[i]
}
}
return perrs.PlatformError{
Code: code,
Message: msg,
Details: details,
Cause: perrs.ToPlatformError(cause),
}
}
26 changes: 10 additions & 16 deletions client/src/www/app/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,14 @@ import {OperationTimedOut} from '@outline/infrastructure/timeout_promise';

import {Clipboard} from './clipboard';
import {EnvironmentVariables} from './environment';
import {localizeErrorCode} from './error_localizer';
import * as config from './outline_server_repository/config';
import {Settings, SettingsKey} from './settings';
import {Updater} from './updater';
import {UrlInterceptor} from './url_interceptor';
import {VpnInstaller} from './vpn_installer';
import * as errors from '../model/errors';
import * as events from '../model/events';
import {
PlatformError,
ROUTING_SERVICE_NOT_RUNNING,
} from '../model/platform_error';
import {PlatformError, GoErrorCode} from '../model/platform_error';
import {Server, ServerRepository} from '../model/server';
import {OutlineErrorReporter} from '../shared/error_reporter';
import {ServerConnectionState, ServerListItem} from '../views/servers_view';
Expand Down Expand Up @@ -310,18 +306,19 @@ export class App {
};
} else if (error instanceof errors.SessionConfigFetchFailed) {
toastMessage = this.localize('error-connection-configuration-fetch');
buttonMessage = this.localize('error-details');
buttonHandler = () => {
this.showErrorCauseDialog(error);
};
if (error?.cause instanceof Error) {
const cause = error.cause;
buttonMessage = this.localize('error-details');
buttonHandler = () => {
this.showErrorCauseDialog(cause);
};
}
} else if (error instanceof errors.ProxyConnectionFailure) {
toastMessage = this.localize('error-connection-proxy');
buttonMessage = this.localize('error-details');
buttonHandler = () => {
this.showErrorCauseDialog(error);
};
} else if (error instanceof errors.SessionConfigError) {
toastMessage = error.message;
} else if (error instanceof errors.SessionProviderError) {
toastMessage = error.message;
buttonMessage = this.localize('error-details');
Expand All @@ -330,10 +327,6 @@ export class App {
buttonHandler = () => {
this.showErrorDetailsDialog(error.details);
};
} else if (error instanceof PlatformError) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep this. The default handler (showErrorCauseDialog) will print weird outputs (for example, duplicated causes). PlatformError's toString already handles the format and prints a cause chain.

Also the default handler doesn't print out error details Map.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in showErrorCauseDialog.

We shouldn't be using localizeErrorCode anymore, it's not needed.

Copy link
Contributor

@sbruens sbruens Jan 23, 2025

Choose a reason for hiding this comment

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

We shouldn't be using localizeErrorCode anymore, it's not needed.

Where is localization of errors happening now? Are we no longer localizing these errors and just throwing a generic localized "unknown error" for them? That's a regression and is a worse user experience. We should localize the top-level user-facing error that can tell the user what went wrong (like the one suggesting to contact their service provider with details).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We localize the errors in App.showLocalizedError:

showLocalizedError(error?: Error, toastDuration = 10000) {

I'm actually removing the duplicate logic in this PR.

toastMessage = localizeErrorCode(error.code, this.localize);
buttonMessage = this.localize('error-details');
buttonHandler = () => this.showErrorDetailsDialog(error.toString());
} else {
const hasErrorDetails = Boolean(error.message || error.cause);
toastMessage = this.localize('error-unexpected');
Expand Down Expand Up @@ -558,8 +551,9 @@ export class App {
});
console.error(`could not connect to server ${serverId}: ${e}`);
if (
// TODO(fortuna): Use typed errors instead.
e instanceof PlatformError &&
e.code === ROUTING_SERVICE_NOT_RUNNING
e.code === GoErrorCode.ROUTING_SERVICE_NOT_RUNNING
) {
const confirmation =
this.localize('outline-services-installation-confirmation') +
Expand Down
34 changes: 0 additions & 34 deletions client/src/www/app/error_localizer.ts

This file was deleted.

4 changes: 1 addition & 3 deletions client/src/www/app/outline_server_repository/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,9 +165,7 @@ export async function parseAccessKey(

throw new TypeError('Access Key is not a ss:// or ssconf:// URL');
} catch (e) {
throw new errors.ServerAccessKeyInvalid('Invalid static access key.', {
cause: e,
});
throw new errors.ServerAccessKeyInvalid(e);
}
}

Expand Down
28 changes: 15 additions & 13 deletions client/src/www/app/outline_server_repository/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,27 +141,29 @@ class OutlineServer implements Server {
async function fetchTunnelConfig(
configLocation: URL
): Promise<TunnelConfigJson> {
const responseBody = (
await getDefaultMethodChannel().invokeMethod(
'FetchResource',
configLocation.toString()
)
).trim();
let responseBody: string;
try {
responseBody = (
await getDefaultMethodChannel().invokeMethod(
'FetchResource',
configLocation.toString()
)
).trim();
} catch (e) {
throw new errors.SessionConfigFetchFailed(e);
}
if (!responseBody) {
throw new errors.ServerAccessKeyInvalid(
'Got empty config from dynamic key.'
new Error('Got empty config from dynamic key.')
);
}
try {
return parseTunnelConfig(responseBody);
return await parseTunnelConfig(responseBody);
} catch (cause) {
if (cause instanceof errors.SessionProviderError) {
throw cause;
}

throw new errors.ServerAccessKeyInvalid(
'Failed to parse VPN information fetched from dynamic access key.',
{cause}
);
// TODO(fortuna): Use a more specific "invalid tunnel config" error.
throw new errors.ServerAccessKeyInvalid(cause);
}
}
14 changes: 4 additions & 10 deletions client/src/www/model/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,8 @@ export class ServerUrlInvalid extends CustomError {
}

export class SessionConfigFetchFailed extends CustomError {
constructor(message: string, options?: {cause?: Error}) {
super(message, options);
}
}

export class SessionConfigError extends CustomError {
constructor(message: string, options?: {cause?: Error}) {
super(message, options);
constructor(cause?: Error) {
super(undefined, {cause});
Copy link
Contributor

Choose a reason for hiding this comment

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

The undefined message seems to be off from the best practice of JavaScript's Error object:

The message data property of an Error instance is a human-readable description of the error.

Maybe we can have a default human-readable message?

}
}

Expand All @@ -63,8 +57,8 @@ export class SessionProviderError extends CustomError {
}

export class ServerAccessKeyInvalid extends CustomError {
constructor(message: string, options?: {cause?: Error}) {
super(message, options);
constructor(cause: Error) {
super(undefined, {cause});
}
}

Expand Down
Loading
Loading