-
Notifications
You must be signed in to change notification settings - Fork 25
Use contextual exn-based errors in DEEPWELL #2661
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
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e5a3a64 to
5686bb5
Compare
This should produce very nice error messages that show the relevant code path and assist with debugging.
Contains the basic string line for the kind of error previously simply rendered via Display using thiserror.
Now the new one is default, and all the prior uses are OldError, OldResult, etc.
Last one! Woo!
Also rearrange some of these cases.
Helps us discover issues related to particular languages, such as missing translations.
Zokhoi
approved these changes
Feb 8, 2026
Member
Author
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This uses the
exncrate to change all of DEEPWELL's errors to be context. This strategy is described in Stop Forwarding Errors, Start Designing Them, which discusses how approaches likethiserror, despite being easy, simply passing it on as-is, with no context of how you got there.It uses the example of:
Which I think is a compelling argument at the start against error forwarding.
A number of ideas are mentioned there, such as human-readable vs machine-processable errors (like if a request can be retried or not), but for our purposes we have a few needs:
As such, the solution I came up with, there are two parts, the string message (free-form for describing the current stage, intended for developers), and the "error type", which embodies both the error code / type of operation, and has any ancillary data to be presented in the JSONRPC error.
Speaking of for developers, the key distinction for
exnis that simply doing?will not accept the error, you need to do.or_raise()to add context about this layer. This is technically optional if the error type is the same, meaning that it's less enforced for our crate here, but we should endeavor to add an error layer for every function that's not the most simple wrapper. This way we can see the logic that resulted in the current error.You will notice that throughout the code, I make use of a
make_errorclosure to create an error - note that forexnit is an anti-pattern for you to make the error message the particular step you're trying to do. Instead it should describe what this current layer wants to do, thus the identical error message for the layer.The only exception for this are bottom-tier errors, where it should be the specific item that failed. We can manually kick off errors with
bail!, and you can see there are a few root-level errors for things like "page not found". This is the exception to the rule.So, if I'm in a function called
rerender_page(), then the error message should be something likefailed to rerender page [...more context here]and a more specific error like "page not found" would be wrapped by this higher level.With this information, we can look at an example error response:
With the error JSON itself being:
{ "code": 4300, "message": "Page slug cannot be empty", "data": { "call_trace": "[1001] method 'page_move' failed, at src/api.rs:309:5\n|\n|-> [1009] failed to move page, at src/endpoints/page.rs:235:10\n|\n|-> [1009] failed to move page 'start' to '' (ID 1) in site ID 3, performed by user ID 2, at src/services/page/service.rs:359:14\n|\n|-> [4300] cannot create page with empty slug, at src/services/page/service.rs:1152:13", "code_trace": [ 1001, 1009, 1009, 4300 ], "extra": null } }Artifically-added error deeper in the stack:
This is pretty long, but most of the changes involve adding
make_erroror.or_raise(...)to places where we run operations returning aResult, so it should be skimmable.For future development, feel free to add additional context to error messages if you feel that it's necessary. For instance, if you cannot tell what the issue is from a glance at the trace, then there might be a parameter we should be logging, even if it means needing to clone some data. Ease of debugability will make long-term maintenance on the project easier.