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

Pass HTTP status code & errcode from CS-API errors #100

Merged
merged 12 commits into from
Nov 8, 2024

Conversation

AndrewFerr
Copy link
Member

Currently, error responses from CS-API requests are re-thrown/rejected without their HTTP status code or errcode string. This PR passes them along, because otherwise clients that may run either standalone or as a widget cannot reliably use HTTP status codes on error responses, as the latter would not receive them.

Signed-off-by: Andrew Ferrazzutti [email protected]

message: string;
}

interface IMatrixErrorData {
Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps there's a better place to put this, or a better way to have it as part of IWidgetApiErrorResponseData.

Also, are interface types still supposed to start with I?

Allow client widget drivers to serialize Matrix API error responses into
JSON to be received by the requesting widget.
because Typescript has its own version of that rule
Mock client-side responses to test deserializing them on the widget side
As long as the error details payload is extensible, let drivers put more
data in them than just the key for Matrix API error responses.
as this makes it too easy for drivers to put data in the wrong section.

Still define the payload type as an interface so that it can be
extended in a future version of the API.

Also don't use a subfield now that non-extensibility makes the format of
the details fields unambiguous.
@AndrewFerr
Copy link
Member Author

With code coverage finally high enough, this is now ready to review.

@AndrewFerr
Copy link
Member Author

Code coverage is as high as it can reasonably get now.

Copy link

sonarcloud bot commented Nov 6, 2024

}
}

private handleDriverError(e: unknown, request: IWidgetApiRequest, message: string) {
const data = this.driver.processError(e);
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not a fan of making driver implementations responsible for serializing errors. I'd much rather have it be done by something akin to a method on the error object itself, but that's not simple because errors are of an unknown type here. For now, the current solution works, but I'm open to suggestions.

@AndrewFerr AndrewFerr requested a review from fkwp November 6, 2024 18:08
@fkwp fkwp requested a review from toger5 November 8, 2024 12:55
Copy link
Contributor

@toger5 toger5 left a comment

Choose a reason for hiding this comment

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

This looks good.
I left a couple of comments but most of them have been addressed in the meeting.

src/interfaces/IWidgetApiErrorResponse.ts Show resolved Hide resolved
src/driver/WidgetDriver.ts Show resolved Hide resolved
src/transport/ITransport.ts Show resolved Hide resolved
src/WidgetApi.ts Show resolved Hide resolved
Copy link
Contributor

@toger5 toger5 left a comment

Choose a reason for hiding this comment

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

I approve this now since the things we discussed could also be done in a follow up pr:

  • Allow the driver methods to throw the correct error directly.

@AndrewFerr AndrewFerr merged commit e62698f into master Nov 8, 2024
3 checks passed
@AndrewFerr AndrewFerr deleted the af/errors-with-http-status branch November 8, 2024 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants