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

Allow custom error type in server_fn return type for enhanced ergonomics and security #3270

Open
ryo33 opened this issue Nov 21, 2024 · 6 comments · May be fixed by #3274
Open

Allow custom error type in server_fn return type for enhanced ergonomics and security #3270

ryo33 opened this issue Nov 21, 2024 · 6 comments · May be fixed by #3274

Comments

@ryo33
Copy link

ryo33 commented Nov 21, 2024

Is your feature request related to a problem? Please describe.

  • Any ? operator in the server function might inadvertently expose sensitive error messages from the server side because the From is implemented for ServerFnError which uses error.to_string().
  • Functions that return anyhow::Error are cumbersome to use because they need explicit conversion (e.g., using the server_fn_error! macro).
  • It's difficult to share structured error types between server and client because ServerFnError has its own serialization strategy utilizing FromStr and Display.
  • Since ServerFnError is an external type from applications and has tailored trait implementations, users cannot implement arbitrary traits such as From<MyError>.

Describe the solution you'd like

To address these issues, I propose the following:

  1. Make ServerFnError only intended for using it in server_fn return type just for the ease of error conversion with ?.
  2. Use ServerFnErrorErr instead of ServerFnError everywhere internally.
  3. Allow users to specify any error type for the server fn result where E: Serialize + DeserializeOwned + From<ServerFnErrorErr> + Debug + Display.
  4. Depricate ServerFnError(Err)::WrappedServerError variant, ViaError, server_fn_error!, and NoCustomError.

This maintains backward compatibility in the typical server_fn use case that uses ServerFnError.

Describe alternatives you've considered

An alternative solution could simplify the implementation further but would sacrifice backward compatibility. In this scenario, users can still use ServerFnError as the return type of server_fns, but error conversion with ? from any E: Error types is no longer available.

  1. Remove the WrappedServerError variant, ServerFnErrorErr, and error conversion toolings, including ViaError.
  2. Implement Error for ServerFnError. This removes the support of From<E: Error> for ServerFnError, but it's still allowed to use it as the error type because it fulfills the below bounds.
  3. Allow users to specify any error type for the server fn result where E: Serialize + DeserializeOwned + From<ServerFnError> + Debug + Display.

Additional context

@benwis
Copy link
Contributor

benwis commented Nov 21, 2024

This is definitely a tricky thing to play with.

The idea that ? makes it easy to return Errors is essentially the point, and the idea of storing sensitive data in Error messages seems silly to me.

By removing that impl we'll be breaking tons of people's apps that just use it, and forcing them to write From or map_err() in a bunch of places. Once someone does, it'll be just as easy anyway.

I'm not sure I understand how we can still use ServerFnError for the server function result and let them define an error

We can't just have a generic error return type because there are a bunch of errors returned from Leptos itself that would be incompatible

I've been considering trying to simplify the situation but I haven't found a good answer yet

@ryo33
Copy link
Author

ryo33 commented Nov 21, 2024

Thank you for your feedback!

The purpose of this is to provide users with an additional option to use custom errors in the return type without breaking the existing code (in best effort), so it actually reduces the cases that users need to write manual error conversion with map_err() or server_fn_error!. For users who don't need custom errors, ServerFnError remains available without breakage of existing implicit error conversion with ?.

the idea of storing sensitive data in Error messages seems silly to me.

By "sensitive", I mean detailed internal information that users, including potentially malicious ones, should not have access to. I don't know the correct term for this.

I'm not sure I understand how we can still use ServerFnError for the server function result and let them define an error

The key idea is the ServerFnError itself satisfies the trait bounds required for the user-defined custom type.

We can't just have a generic error return type because there are a bunch of errors returned from Leptos itself that would be incompatible

From my understanding, requiring From<ServerFnErrorErr> for the user-defined errors is sufficient to convert Leptos's error both on the server-side and client-side as a server_fn result. Additionally, I plan to leverage the existing ServerFn::Error associated type in the ServerFn trait.

@benwis
Copy link
Contributor

benwis commented Nov 21, 2024

I think I'll have to discuss this with Greg, but that'd be easier with a PR to play with, so if you feel like trying it feel free

@ryo33
Copy link
Author

ryo33 commented Nov 21, 2024

Thanks! I've started to work on it.

@gbj
Copy link
Collaborator

gbj commented Nov 21, 2024

I'm a little confused -- Don't we already support custom error types in the ServerFnError type? (With the generic that defaults to NoCustomError?)

@ryo33
Copy link
Author

ryo33 commented Nov 21, 2024

Yes, but there are several limitations:

  • Users cannot implement From<anyhow::Error> or any similar things for ServerFnError.
  • Users cannot limit automatic type conversion for only specific types since ServerFnError has blanket implementation for all E: Error.
  • Even if MyCustomError: From<ErrorA>, the user cannot use ? operator for Result<T, ErrorA>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants