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

Return Result<T, E> from trait methods #81

Closed
Jesse-Bakker opened this issue Apr 7, 2023 · 1 comment
Closed

Return Result<T, E> from trait methods #81

Jesse-Bakker opened this issue Apr 7, 2023 · 1 comment
Labels
bug Something isn't working

Comments

@Jesse-Bakker
Copy link

Jesse-Bakker commented Apr 7, 2023

Currently, error handling is implemented the way it is in pgx, which is by panicing and catching the panic at the transaction boundary. This is good at the interface with postgres, but for a higher level API, using the standard rust Result method of error handling is generally more ergonomic. This because interfaces signal fallibility and it allows using the standard machinery for handling and converting errors.

struct HelloWorldFdw{ url: String }

impl ForeignDataWrapper for HelloWorldFdw {
    fn new(options: &HashMap<String, String>) -> Self {
        Self {
            url: require_option("url", options).unwrap()
        }
    }
}

Here, fn new is fallible, but nothing signals its fallibility to the casual reader or implementer.
require_option diverges, so the unwrap is safe (actually require_option could return String instead of Option<String>, because if it would be None, it would panic through report_error).
If new returned Result<Self, WrappersError>, this could be rewritten as, for example:

struct HelloWorldFdw{ url: String }

impl ForeignDataWrapper for HelloWorldFdw {
    fn new(options: &HashMap<String, String>) -> Result<Self, WrappersError> {
        Ok(Self {
            url: require_option("url", options)?
        })
    }
}

Where require_option() returns a Result<String, WrappersError>, with the error being a WrappersError::OptionNameNotFound(String), with the string representing the option name. It is now clear that fn new is fallible, and require_option is actually just options.get(<name>).ok_or(WrappersError::OptionNameNotFound(<name>))
Of course other errors could also be returned from new(). I imagine WrappersError to look something like:

enum WrappersError {
    OptionNameNotFound(String),
    ColumnNameNotFound(String),
    InvalidDataType(String),
    // ... etc
    Other(Box<dyn Error>),
}

impl WrappersError {
    fn error_code(&self) -> PgSqlErrorCode {
        match self {
            OptionNameNotFound(_) => PgSqlErrorCode::ERROR_CODE_FDW_OPTION_NAME_NOT_FOUND
        }
    }
}

impl Display for WrappersError{ /*...*/}

In FdwState, errors would be converted to the panic-y kind:

match Wrapper::new(options) {
    Err(e) => report_error(e.error_code(), &e.to_string()),
    Ok(wrapper) => wrapper,
}

I'd be happy to make a PR implementing this by the way.

@Jesse-Bakker Jesse-Bakker added the bug Something isn't working label Apr 7, 2023
@imor
Copy link
Contributor

imor commented Jan 15, 2024

Implemented in #144

@imor imor closed this as completed Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants