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

Simplify errors with thiserror #1533

Merged
merged 3 commits into from
Feb 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions pgrx-pg-config/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ edition = "2021"
cargo_toml.workspace = true
eyre.workspace = true
owo-colors.workspace = true
thiserror.workspace = true
url.workspace = true

home = "0.5.9"
Expand Down
28 changes: 7 additions & 21 deletions pgrx-pg-config/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,13 @@ use eyre::{eyre, WrapErr};
use owo_colors::OwoColorize;
use serde_derive::{Deserialize, Serialize};
use std::collections::{BTreeMap, HashMap};
use std::error::Error;
use std::ffi::OsString;
use std::fmt::{self, Debug, Display, Formatter};
use std::io::ErrorKind;
use std::path::PathBuf;
use std::process::{Command, Stdio};
use std::str::FromStr;
use thiserror::Error;
use url::Url;

pub mod cargo;
Expand Down Expand Up @@ -491,17 +491,15 @@ impl<'a> PgConfigSelector<'a> {
}
}

#[derive(Debug)]
#[derive(Debug, Error)]
pub enum PgrxHomeError {
#[error("You don't seem to have a home directory")]
NoHomeDirectory,
// allow caller to decide whether it is safe to enumerate paths
#[error("$PGRX_HOME does not exist")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note that the PathBuf here is not printed. Should it?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, no. We want to be careful about printing paths or otherwise enumerating the environment in a way that could automatically pass-through to IO, because this crate doesn't know what the security context is for this error being formatted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx, i will add a comment about that

MissingPgrxHome(PathBuf),
IoError(std::io::Error),
}

impl From<std::io::Error> for PgrxHomeError {
fn from(value: std::io::Error) -> Self {
PgrxHomeError::IoError(value)
}
#[error(transparent)]
IoError(#[from] std::io::Error),
}

impl From<PgrxHomeError> for std::io::Error {
Expand All @@ -518,18 +516,6 @@ impl From<PgrxHomeError> for std::io::Error {
}
}

impl Display for PgrxHomeError {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
match self {
PgrxHomeError::NoHomeDirectory => write!(f, "You don't seem to have a home directory"),
PgrxHomeError::MissingPgrxHome(_) => write!(f, "$PGRX_HOME does not exist"),
PgrxHomeError::IoError(e) => write!(f, "{e}"),
}
}
}

impl Error for PgrxHomeError {}

impl Pgrx {
pub fn new(base_port: u16, base_testing_port: u16) -> Self {
Pgrx { pg_configs: vec![], base_port, base_testing_port }
Expand Down
2 changes: 0 additions & 2 deletions pgrx-pg-sys/src/submodules/errcodes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -391,8 +391,6 @@ impl Display for PgSqlErrorCode {
}
}

impl std::error::Error for PgSqlErrorCode {}

impl From<i32> for PgSqlErrorCode {
fn from(error_code: i32) -> Self {
(error_code as isize).into()
Expand Down
1 change: 1 addition & 0 deletions pgrx-sql-entity-graph/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ no-schema-generation = []
[dependencies]
eyre.workspace = true
unescape.workspace = true
thiserror.workspace = true

convert_case = "0.6.0"
petgraph = "0.6.4"
Expand Down
21 changes: 4 additions & 17 deletions pgrx-sql-entity-graph/src/control_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ to the `pgrx` framework and very subject to change between versions. While you m
use super::{SqlGraphEntity, SqlGraphIdentifier, ToSql};
use core::convert::TryFrom;
use std::collections::HashMap;
use thiserror::Error;

/// The parsed contents of a `.control` file.
///
Expand Down Expand Up @@ -107,28 +108,14 @@ impl From<ControlFile> for SqlGraphEntity {
}

/// An error met while parsing a `.control` file.
#[derive(Debug, Clone)]
#[derive(Debug, Clone, Error)]
pub enum ControlFileError {
#[error("Missing field in control file! Please add `{field}`.")]
MissingField { field: &'static str },
#[error("Redundant field in control file! Please remove `{field}`.")]
RedundantField { field: &'static str },
}

impl std::fmt::Display for ControlFileError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
ControlFileError::MissingField { field } => {
write!(f, "Missing field in control file! Please add `{field}`.")?;
}
ControlFileError::RedundantField { field } => {
write!(f, "Redundant field in control file! Please remove `{field}`.")?;
}
};
Ok(())
}
}

impl std::error::Error for ControlFileError {}

impl TryFrom<&str> for ControlFile {
type Error = ControlFileError;

Expand Down
52 changes: 11 additions & 41 deletions pgrx-sql-entity-graph/src/metadata/return_variant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ Return value specific metadata for Rust to SQL translation
to the `pgrx` framework and very subject to change between versions. While you may use this, please do it with caution.

*/
use std::error::Error;
use thiserror::Error;

use super::sql_translatable::SqlMapping;

Expand All @@ -29,54 +29,24 @@ pub enum Returns {
Table(Vec<SqlMapping>),
}

#[derive(Clone, Copy, Debug, Hash, Ord, PartialOrd, PartialEq, Eq)]
#[derive(Clone, Copy, Debug, Hash, Ord, PartialOrd, PartialEq, Eq, Error)]
pub enum ReturnsError {
#[error("Nested SetOfIterator in return type")]
NestedSetOf,
#[error("Nested TableIterator in return type")]
NestedTable,
#[error("SetOfIterator containing TableIterator in return type")]
SetOfContainingTable,
#[error("TableIterator containing SetOfIterator in return type")]
TableContainingSetOf,
#[error("SetofIterator inside Array is not valid")]
SetOfInArray,
#[error("TableIterator inside Array is not valid")]
TableInArray,
#[error("Cannot use bare u8")]
BareU8,
#[error("SqlMapping::Skip inside Array is not valid")]
SkipInArray,
#[error("A Datum as a return means that `sql = \"...\"` must be set in the declaration")]
Datum,
}

impl std::fmt::Display for ReturnsError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
ReturnsError::NestedSetOf => {
write!(f, "Nested SetOfIterator in return type")
}
ReturnsError::NestedTable => {
write!(f, "Nested TableIterator in return type")
}
ReturnsError::SetOfContainingTable => {
write!(f, "SetOfIterator containing TableIterator in return type")
}
ReturnsError::TableContainingSetOf => {
write!(f, "TableIterator containing SetOfIterator in return type")
}
ReturnsError::SetOfInArray => {
write!(f, "SetofIterator inside Array is not valid")
}
ReturnsError::TableInArray => {
write!(f, "TableIterator inside Array is not valid")
}
ReturnsError::SkipInArray => {
write!(f, "SqlMapping::Skip inside Array is not valid")
}
ReturnsError::BareU8 => {
write!(f, "Cannot use bare u8")
}
ReturnsError::Datum => {
write!(
f,
"A Datum as a return means that `sql = \"...\"` must be set in the declaration"
)
}
}
}
}

impl Error for ReturnsError {}
37 changes: 8 additions & 29 deletions pgrx-sql-entity-graph/src/metadata/sql_translatable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,47 +16,28 @@ to the `pgrx` framework and very subject to change between versions. While you m

*/
use std::any::Any;
use std::error::Error;
use std::fmt::Display;
use thiserror::Error;

use super::return_variant::ReturnsError;
use super::{FunctionMetadataTypeEntity, Returns};

#[derive(Clone, Copy, Debug, Hash, Ord, PartialOrd, PartialEq, Eq)]
#[derive(Clone, Copy, Debug, Hash, Ord, PartialOrd, PartialEq, Eq, Error)]
pub enum ArgumentError {
#[error("Cannot use SetOfIterator as an argument")]
SetOf,
#[error("Cannot use TableIterator as an argument")]
Table,
#[error("Cannot use bare u8")]
BareU8,
#[error("SqlMapping::Skip inside Array is not valid")]
SkipInArray,
#[error("A Datum as an argument means that `sql = \"...\"` must be set in the declaration")]
Datum,
#[error("`{0}` is not able to be used as a function argument")]
NotValidAsArgument(&'static str),
}

impl std::fmt::Display for ArgumentError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
ArgumentError::SetOf => {
write!(f, "Cannot use SetOfIterator as an argument")
}
ArgumentError::Table => {
write!(f, "Cannot use TableIterator as an argument")
}
ArgumentError::BareU8 => {
write!(f, "Cannot use bare u8")
}
ArgumentError::SkipInArray => {
write!(f, "SqlMapping::Skip inside Array is not valid")
}
ArgumentError::Datum => {
write!(f, "A Datum as an argument means that `sql = \"...\"` must be set in the declaration")
}
ArgumentError::NotValidAsArgument(type_name) => {
write!(f, "`{}` is not able to be used as a function argument", type_name)
}
}
}
}

/// Describes ways that Rust types are mapped into SQL
#[derive(Clone, Debug, Hash, Eq, PartialEq, Ord, PartialOrd)]
pub enum SqlMapping {
Expand All @@ -75,8 +56,6 @@ impl SqlMapping {
}
}

impl Error for ArgumentError {}

/**
A value which can be represented in SQL

Expand Down
20 changes: 5 additions & 15 deletions pgrx/src/datum/numeric_support/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,32 +7,22 @@
//LICENSE All rights reserved.
//LICENSE
//LICENSE Use of this source code is governed by the MIT license that can be found in the LICENSE file.
use std::fmt;
use std::fmt::{Display, Formatter};
use thiserror::Error;

/// Represents some kind of conversion error when working with Postgres numerics
#[derive(Debug, Eq, PartialEq)]
#[derive(Debug, Eq, PartialEq, Error)]
#[non_exhaustive]
pub enum Error {
/// A conversion to Numeric would produce a value outside the precision and scale constraints
/// of the target Numeric
#[error("{0}")]
OutOfRange(String),

/// A provided value is not also a valid Numeric
#[error("{0}")]
Invalid(String),

/// Postgres versions less than 14 do not support `Infinity` and `-Infinity` values
#[error("{0}")]
ConversionNotSupported(String),
}

impl Display for Error {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
match self {
Error::OutOfRange(s) => write!(f, "{}", s),
Error::Invalid(s) => write!(f, "{}", s),
Error::ConversionNotSupported(s) => write!(f, "{}", s),
}
}
}

impl std::error::Error for Error {}
Loading