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

Send/receive error details with widgets #4492

Merged
merged 15 commits into from
Nov 9, 2024
Merged
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
"jwt-decode": "^4.0.0",
"loglevel": "^1.7.1",
"matrix-events-sdk": "0.0.1",
"matrix-widget-api": "^1.8.2",
"matrix-widget-api": "^1.10.0",
"oidc-client-ts": "^3.0.1",
"p-retry": "4",
"sdp-transform": "^2.14.1",
Expand Down
85 changes: 83 additions & 2 deletions spec/unit/embedded.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,10 @@ import {
ITurnServer,
IRoomEvent,
IOpenIDCredentials,
WidgetApiResponseError,
} from "matrix-widget-api";

import { createRoomWidgetClient, MsgType, UpdateDelayedEventAction } from "../../src/matrix";
import { createRoomWidgetClient, MatrixError, MsgType, UpdateDelayedEventAction } from "../../src/matrix";
import { MatrixClient, ClientEvent, ITurnServer as IClientTurnServer } from "../../src/client";
import { SyncState } from "../../src/sync";
import { ICapabilities, RoomWidgetClient } from "../../src/embedded";
Expand Down Expand Up @@ -90,7 +91,11 @@ class MockWidgetApi extends EventEmitter {
public getTurnServers = jest.fn(() => []);
public sendContentLoaded = jest.fn();

public transport = { reply: jest.fn() };
public transport = {
reply: jest.fn(),
send: jest.fn(),
sendComplete: jest.fn(),
};
}

declare module "../../src/types" {
Expand Down Expand Up @@ -187,6 +192,46 @@ describe("RoomWidgetClient", () => {
.map((e) => e.getEffectiveEvent()),
).toEqual([event]);
});

it("handles widget errors with generic error data", async () => {
const error = new Error("failed to send");
widgetApi.transport.send.mockRejectedValue(error);

await makeClient({ sendEvent: ["org.matrix.rageshake_request"] });
widgetApi.sendRoomEvent.mockImplementation(widgetApi.transport.send);

await expect(
client.sendEvent("!1:example.org", "org.matrix.rageshake_request", { request_id: 123 }),
).rejects.toThrow(error);
});

it("handles widget errors with Matrix API error response data", async () => {
const errorStatusCode = 400;
const errorUrl = "http://example.org";
const errorData = {
errcode: "M_BAD_JSON",
error: "Invalid body",
};

const widgetError = new WidgetApiResponseError("failed to send", {
matrix_api_error: {
http_status: errorStatusCode,
http_headers: {},
url: errorUrl,
response: errorData,
},
});
const matrixError = new MatrixError(errorData, errorStatusCode, errorUrl);

widgetApi.transport.send.mockRejectedValue(widgetError);

await makeClient({ sendEvent: ["org.matrix.rageshake_request"] });
widgetApi.sendRoomEvent.mockImplementation(widgetApi.transport.send);

await expect(
client.sendEvent("!1:example.org", "org.matrix.rageshake_request", { request_id: 123 }),
).rejects.toThrow(matrixError);
});
});

describe("delayed events", () => {
Expand Down Expand Up @@ -598,6 +643,42 @@ describe("RoomWidgetClient", () => {
await makeClient({});
expect(await client.getOpenIdToken()).toStrictEqual(testOIDCToken);
});

it("handles widget errors with generic error data", async () => {
const error = new Error("failed to get token");
widgetApi.transport.sendComplete.mockRejectedValue(error);

await makeClient({});
widgetApi.requestOpenIDConnectToken.mockImplementation(widgetApi.transport.sendComplete as any);

await expect(client.getOpenIdToken()).rejects.toThrow(error);
});

it("handles widget errors with Matrix API error response data", async () => {
const errorStatusCode = 400;
const errorUrl = "http://example.org";
const errorData = {
errcode: "M_UNKNOWN",
error: "Bad request",
};

const widgetError = new WidgetApiResponseError("failed to get token", {
matrix_api_error: {
http_status: errorStatusCode,
http_headers: {},
url: errorUrl,
response: errorData,
},
});
const matrixError = new MatrixError(errorData, errorStatusCode, errorUrl);

widgetApi.transport.sendComplete.mockRejectedValue(widgetError);

await makeClient({});
widgetApi.requestOpenIDConnectToken.mockImplementation(widgetApi.transport.sendComplete as any);

await expect(client.getOpenIdToken()).rejects.toThrow(matrixError);
});
});

it("gets TURN servers", async () => {
Expand Down
95 changes: 93 additions & 2 deletions spec/unit/http-api/errors.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ describe("MatrixError", () => {
headers = new Headers({ "Content-Type": "application/json" });
});

function makeMatrixError(httpStatus: number, data: IErrorJson): MatrixError {
return new MatrixError(data, httpStatus, undefined, undefined, headers);
function makeMatrixError(httpStatus: number, data: IErrorJson, url?: string): MatrixError {
return new MatrixError(data, httpStatus, url, undefined, headers);
}

it("should accept absent retry time from rate-limit error", () => {
Expand Down Expand Up @@ -95,4 +95,95 @@ describe("MatrixError", () => {
const err = makeMatrixError(429, { errcode: "M_LIMIT_EXCEEDED" });
expect(() => err.getRetryAfterMs()).toThrow("integer value is too large");
});

describe("can be converted to data compatible with the widget api", () => {
it("from default values", () => {
const matrixError = new MatrixError();

const widgetApiErrorData = {
http_status: 400,
http_headers: {},
url: "",
response: {
errcode: "M_UNKNOWN",
error: "Unknown message",
},
};

expect(matrixError.asWidgetApiErrorData()).toEqual(widgetApiErrorData);
});

it("from non-default values", () => {
headers.set("Retry-After", "120");
const statusCode = 429;
const data = {
errcode: "M_LIMIT_EXCEEDED",
error: "Request is rate-limited.",
retry_after_ms: 120000,
};
const url = "http://example.net";

const matrixError = makeMatrixError(statusCode, data, url);

const widgetApiErrorData = {
http_status: statusCode,
http_headers: {
"content-type": "application/json",
"retry-after": "120",
},
url,
response: data,
};

expect(matrixError.asWidgetApiErrorData()).toEqual(widgetApiErrorData);
});
});

describe("can be created from data received from the widget api", () => {
it("from minimal data", () => {
const statusCode = 400;
const data = {
errcode: "M_UNKNOWN",
error: "Something went wrong.",
};
const url = "";

const widgetApiErrorData = {
http_status: statusCode,
http_headers: {},
url,
response: data,
};

headers.delete("Content-Type");
const matrixError = makeMatrixError(statusCode, data, url);

expect(MatrixError.fromWidgetApiErrorData(widgetApiErrorData)).toEqual(matrixError);
});

it("from more data", () => {
const statusCode = 429;
const data = {
errcode: "M_LIMIT_EXCEEDED",
error: "Request is rate-limited.",
retry_after_ms: 120000,
};
const url = "http://example.net";

const widgetApiErrorData = {
http_status: statusCode,
http_headers: {
"content-type": "application/json",
"retry-after": "120",
},
url,
response: data,
};

headers.set("Retry-After", "120");
const matrixError = makeMatrixError(statusCode, data, url);

expect(MatrixError.fromWidgetApiErrorData(widgetApiErrorData)).toEqual(matrixError);
});
});
});
41 changes: 41 additions & 0 deletions src/embedded.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,17 @@ limitations under the License.
import {
WidgetApi,
WidgetApiToWidgetAction,
WidgetApiResponseError,
MatrixCapabilities,
IWidgetApiRequest,
IWidgetApiAcknowledgeResponseData,
ISendEventToWidgetActionRequest,
ISendToDeviceToWidgetActionRequest,
ISendEventFromWidgetResponseData,
IWidgetApiRequestData,
WidgetApiAction,
IWidgetApiResponse,
IWidgetApiResponseData,
} from "matrix-widget-api";

import { MatrixEvent, IEvent, IContent, EventStatus } from "./models/event.ts";
Expand All @@ -45,6 +50,7 @@ import {
} from "./client.ts";
import { SyncApi, SyncState } from "./sync.ts";
import { SlidingSyncSdk } from "./sliding-sync-sdk.ts";
import { MatrixError } from "./http-api/errors.ts";
import { User } from "./models/user.ts";
import { Room } from "./models/room.ts";
import { ToDeviceBatch, ToDevicePayload } from "./models/ToDeviceMessage.ts";
Expand Down Expand Up @@ -147,6 +153,33 @@ export class RoomWidgetClient extends MatrixClient {
) {
super(opts);

const transportSend = this.widgetApi.transport.send.bind(this.widgetApi.transport);
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this bind?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is used as a way to preserve the original send method so it can still be used by the function that replaces it.

Alternatively this could have been an arrow function, but that would require specifying a parameter list. Using bind gets around that.

Copy link
Member

Choose a reason for hiding this comment

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

This seems like a really nasty pattern, this looks like somewhere a subclass makes far more sense rather than overwriting class methods, what's the rationale for not doing it in a more conventional way?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's because the send methods don't belong to WidgetApi, but to ITransport, meaning that subclassing (or even a composition-based approach) would have to work harder to get to them.

Any non-bind approach I can think of ends up being more complicated / requires much more copied code than this.

this.widgetApi.transport.send = async <
T extends IWidgetApiRequestData,
R extends IWidgetApiResponseData = IWidgetApiAcknowledgeResponseData,
>(
action: WidgetApiAction,
data: T,
): Promise<R> => {
try {
return await transportSend<T, R>(action, data);
} catch (error) {
processAndThrow(error);
}
};

const transportSendComplete = this.widgetApi.transport.sendComplete.bind(this.widgetApi.transport);
this.widgetApi.transport.sendComplete = async <T extends IWidgetApiRequestData, R extends IWidgetApiResponse>(
action: WidgetApiAction,
data: T,
): Promise<R> => {
try {
return await transportSendComplete<T, R>(action, data);
} catch (error) {
processAndThrow(error);
}
};

this.widgetApiReady = new Promise<void>((resolve) => this.widgetApi.once("ready", resolve));

// Request capabilities for the functionality this client needs to support
Expand Down Expand Up @@ -523,3 +556,11 @@ export class RoomWidgetClient extends MatrixClient {
}
}
}

function processAndThrow(error: unknown): never {
if (error instanceof WidgetApiResponseError && error.data.matrix_api_error) {
throw MatrixError.fromWidgetApiErrorData(error.data.matrix_api_error);
} else {
throw error;
}
}
33 changes: 33 additions & 0 deletions src/http-api/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import { IMatrixApiError as IWidgetMatrixError } from "matrix-widget-api";

import { IUsageLimit } from "../@types/partials.ts";
import { MatrixEvent } from "../models/event.ts";

Expand Down Expand Up @@ -131,6 +133,37 @@ export class MatrixError extends HTTPError {
}
return null;
}

/**
* @returns this error expressed as a JSON payload
* for use by Widget API error responses.
*/
public asWidgetApiErrorData(): IWidgetMatrixError {
const headers: Record<string, string> = {};
if (this.httpHeaders) {
for (const [name, value] of this.httpHeaders) {
headers[name] = value;
}
}
return {
http_status: this.httpStatus ?? 400,
http_headers: headers,
url: this.url ?? "",
response: {
errcode: this.errcode ?? "M_UNKNOWN",
error: this.data.error ?? "Unknown message",
...this.data,
},
};
}

/**
* @returns a new {@link MatrixError} from a JSON payload
* received from Widget API error responses.
*/
public static fromWidgetApiErrorData(data: IWidgetMatrixError): MatrixError {
return new MatrixError(data.response, data.http_status, data.url, undefined, new Headers(data.http_headers));
}
}

/**
Expand Down
8 changes: 4 additions & 4 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -4940,10 +4940,10 @@ matrix-mock-request@^2.5.0:
dependencies:
expect "^28.1.0"

matrix-widget-api@^1.8.2:
version "1.9.0"
resolved "https://registry.yarnpkg.com/matrix-widget-api/-/matrix-widget-api-1.9.0.tgz#884136b405bd3c56e4ea285095c9e01ec52b6b1f"
integrity sha512-au8mqralNDqrEvaVAkU37bXOb8I9SCe+ACdPk11QWw58FKstVq31q2wRz+qWA6J+42KJ6s1DggWbG/S3fEs3jw==
matrix-widget-api@^1.10.0:
version "1.10.0"
resolved "https://registry.yarnpkg.com/matrix-widget-api/-/matrix-widget-api-1.10.0.tgz#d31ea073a5871a1fb1a511ef900b0c125a37bf55"
integrity sha512-rkAJ29briYV7TJnfBVLVSKtpeBrBju15JZFSDP6wj8YdbCu1bdmlplJayQ+vYaw1x4fzI49Q+Nz3E85s46sRDw==
dependencies:
"@types/events" "^3.0.0"
events "^3.2.0"
Expand Down
Loading