Skip to content
This repository has been archived by the owner on Nov 25, 2024. It is now read-only.

Refactor cross-boundary error handling #1441

Open
neilalexander opened this issue Sep 24, 2020 · 2 comments
Open

Refactor cross-boundary error handling #1441

neilalexander opened this issue Sep 24, 2020 · 2 comments
Labels
C-Polylith T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. X-Fix-With-Monolith This issue can be resolved with a monolith-only arch

Comments

@neilalexander
Copy link
Contributor

Errors in Dendrite are a mess.

The client API has jsonerror.MatrixError, which is nice because those are the things that are returned to clients, although it is maybe misplaced since we also might want to use those in the federation API.

gomatrix also has *gomatrix.HTTPError, and then in some places we have roomserverAPI.PerformError, and in other places we just throw regular opaque errors around. In some API functions we put PerformErrors into the response struct, in others we just return an error from the function itself.

This makes it really difficult to return meaningful clients and other federated servers to errors consistently in any place that an API query is performed.

We should do the work to ensure that if we provide a jsonerror.MatrixError, that it is correctly marshalled and unmarshalled across API boundaries and that they make it all the way back to clients/federation endpoints, and tidy up the rest of the mess so that it's consistent and usable.

@neilalexander neilalexander added this to the Beta milestone Sep 24, 2020
@ara4n
Copy link
Member

ara4n commented Sep 26, 2020

I agree this is super desirable, but i'm not sure it should technically block entering beta.

@kegsay
Copy link
Member

kegsay commented Oct 1, 2020

that it is correctly marshalled and unmarshalled across API boundaries and that they make it all the way back to clients/federation endpoints

iirc this broadly does happen, and there's sytests to prove it. The internal APIs are a mixture of error return / error struct though which should definitely be fixed. Agreed it shouldn't block beta.

@neilalexander neilalexander removed this from the Beta milestone Oct 1, 2020
@kegsay kegsay added T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. C-Polylith and removed maintenance labels Dec 5, 2022
brianathere pushed a commit to HereNotThere/dendrite that referenced this issue Feb 9, 2023
@kegsay kegsay added the X-Fix-With-Monolith This issue can be resolved with a monolith-only arch label Feb 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C-Polylith T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. X-Fix-With-Monolith This issue can be resolved with a monolith-only arch
Projects
None yet
Development

No branches or pull requests

3 participants