From 0e49403c2892c06b8d812ba2a34b556a5f923425 Mon Sep 17 00:00:00 2001 From: David Pedersen <david.pdrsn@gmail.com> Date: Fri, 23 Aug 2019 16:44:46 +0200 Subject: [PATCH 1/8] Implement `std::error::Error` for all error types --- juniper/src/lib.rs | 24 ++++++++++++++++++++++++ juniper/src/parser/lexer.rs | 2 ++ juniper/src/parser/parser.rs | 2 ++ juniper/src/parser/utils.rs | 14 ++++++++++++++ juniper/src/validation/context.rs | 17 ++++++++++++++++- 5 files changed, 58 insertions(+), 1 deletion(-) diff --git a/juniper/src/lib.rs b/juniper/src/lib.rs index bcf257f63..07e540f07 100644 --- a/juniper/src/lib.rs +++ b/juniper/src/lib.rs @@ -155,6 +155,7 @@ use crate::{ parser::{parse_document_source, ParseError, Spanning}, validation::{validate_input_values, visit_all_rules, ValidatorContext}, }; +use std::fmt; pub use crate::{ ast::{FromInputValue, InputValue, Selection, ToInputValue, Type}, @@ -188,6 +189,29 @@ pub enum GraphQLError<'a> { IsSubscription, } +impl<'a> fmt::Display for GraphQLError<'a> { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + GraphQLError::ParseError(error) => write!(f, "{}", error), + GraphQLError::ValidationError(errors) => { + let msgs = errors + .iter() + .map(|error| format!("{}", error)) + .collect::<Vec<_>>() + .join("\n"); + + write!(f, "{}", msgs) + } + GraphQLError::NoOperationProvided => write!(f, "No operation provided"), + GraphQLError::MultipleOperationsProvided => write!(f, "No operation provided"), + GraphQLError::UnknownOperationName => write!(f, "Unknown operation name"), + GraphQLError::IsSubscription => write!(f, "Subscription are not currently supported"), + } + } +} + +impl<'a> std::error::Error for GraphQLError<'a> {} + /// Execute a query in a provided schema pub fn execute<'a, S, CtxT, QueryT, MutationT>( document_source: &'a str, diff --git a/juniper/src/parser/lexer.rs b/juniper/src/parser/lexer.rs index 5d8854e9c..4a18afee4 100644 --- a/juniper/src/parser/lexer.rs +++ b/juniper/src/parser/lexer.rs @@ -538,3 +538,5 @@ impl fmt::Display for LexerError { } } } + +impl std::error::Error for LexerError {} diff --git a/juniper/src/parser/parser.rs b/juniper/src/parser/parser.rs index 095fc1e7e..198c24322 100644 --- a/juniper/src/parser/parser.rs +++ b/juniper/src/parser/parser.rs @@ -203,3 +203,5 @@ impl<'a> fmt::Display for ParseError<'a> { } } } + +impl<'a> std::error::Error for ParseError<'a> {} diff --git a/juniper/src/parser/utils.rs b/juniper/src/parser/utils.rs index d5bd2c915..5ea12817c 100644 --- a/juniper/src/parser/utils.rs +++ b/juniper/src/parser/utils.rs @@ -90,6 +90,14 @@ impl<T> Spanning<T> { } } +impl<T: fmt::Display> fmt::Display for Spanning<T> { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "{}. At {}", self.item, self.start) + } +} + +impl<T: std::error::Error> std::error::Error for Spanning<T> {} + impl SourcePosition { #[doc(hidden)] pub fn new(index: usize, line: usize, col: usize) -> SourcePosition { @@ -142,3 +150,9 @@ impl SourcePosition { self.col } } + +impl fmt::Display for SourcePosition { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "{}:{}", self.line, self.col) + } +} diff --git a/juniper/src/validation/context.rs b/juniper/src/validation/context.rs index 9498bbdae..ef0b3782d 100644 --- a/juniper/src/validation/context.rs +++ b/juniper/src/validation/context.rs @@ -1,4 +1,4 @@ -use std::{collections::HashSet, fmt::Debug}; +use std::{collections::HashSet, fmt::{self, Debug}}; use crate::ast::{Definition, Document, Type}; @@ -48,6 +48,21 @@ impl RuleError { } } +impl fmt::Display for RuleError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + // this is fine since all `RuleError`s should have at least one source position + let locations = self + .locations + .iter() + .map(|location| format!("{}", location)) + .collect::<Vec<_>>() + .join(", "); + write!(f, "{}. At {}", self.message, locations) + } +} + +impl std::error::Error for RuleError {} + impl<'a, S: Debug> ValidatorContext<'a, S> { #[doc(hidden)] pub fn new(schema: &'a SchemaType<S>, document: &Document<'a, S>) -> ValidatorContext<'a, S> { From 5d6b87ca4168d6ed276f5af088992ebfe118e349 Mon Sep 17 00:00:00 2001 From: David Pedersen <david.pdrsn@gmail.com> Date: Tue, 27 Aug 2019 08:41:41 +0200 Subject: [PATCH 2/8] Fix copy-paste --- juniper/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/juniper/src/lib.rs b/juniper/src/lib.rs index 07e540f07..7af75a8bf 100644 --- a/juniper/src/lib.rs +++ b/juniper/src/lib.rs @@ -203,7 +203,7 @@ impl<'a> fmt::Display for GraphQLError<'a> { write!(f, "{}", msgs) } GraphQLError::NoOperationProvided => write!(f, "No operation provided"), - GraphQLError::MultipleOperationsProvided => write!(f, "No operation provided"), + GraphQLError::MultipleOperationsProvided => write!(f, "Multiple operations provided"), GraphQLError::UnknownOperationName => write!(f, "Unknown operation name"), GraphQLError::IsSubscription => write!(f, "Subscription are not currently supported"), } From bec1f99b90316f9ac7850598a36c03a41908cded Mon Sep 17 00:00:00 2001 From: David Pedersen <david.pdrsn@gmail.com> Date: Thu, 31 Oct 2019 20:59:47 +0100 Subject: [PATCH 3/8] Implement `Display` for `Value` This is required for implementing `Display` for `FieldError` --- juniper/src/value/mod.rs | 119 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) diff --git a/juniper/src/value/mod.rs b/juniper/src/value/mod.rs index 3a61ccc78..a6621f346 100644 --- a/juniper/src/value/mod.rs +++ b/juniper/src/value/mod.rs @@ -2,6 +2,7 @@ use crate::{ ast::{InputValue, ToInputValue}, parser::Spanning, }; +use std::fmt::{self, Display, Formatter}; mod object; mod scalar; @@ -178,6 +179,46 @@ impl<S: ScalarValue> ToInputValue<S> for Value<S> { } } +impl<S: ScalarValue> Display for Value<S> { + fn fmt(&self, f: &mut Formatter) -> fmt::Result { + match self { + Value::Null => write!(f, "null"), + Value::Scalar(s) => { + if let Some(string) = s.as_string() { + write!(f, "\"{}\"", string) + } else { + write!(f, "{}", s) + } + } + Value::List(list) => { + write!(f, "[")?; + for (idx, item) in list.iter().enumerate() { + write!(f, "{}", item)?; + if idx < list.len() - 1 { + write!(f, ", ")?; + } + } + write!(f, "]")?; + + Ok(()) + } + Value::Object(obj) => { + write!(f, "{{")?; + for (idx, (key, value)) in obj.iter().enumerate() { + write!(f, "\"{}\": {}", key, value)?; + + if idx < obj.field_count() - 1 { + write!(f, ", ")?; + } + } + write!(f, "}}")?; + + Ok(()) + } + } + } +} + impl<S, T> From<Option<T>> for Value<S> where S: ScalarValue, @@ -345,4 +386,82 @@ mod tests { ) ); } + + #[test] + fn display_null() { + let s: Value<DefaultScalarValue> = graphql_value!(None); + assert_eq!("null", format!("{}", s)); + } + + #[test] + fn display_int() { + let s: Value<DefaultScalarValue> = graphql_value!(123); + assert_eq!("123", format!("{}", s)); + } + + #[test] + fn display_float() { + let s: Value<DefaultScalarValue> = graphql_value!(123.456); + assert_eq!("123.456", format!("{}", s)); + } + + #[test] + fn display_string() { + let s: Value<DefaultScalarValue> = graphql_value!("foo"); + assert_eq!("\"foo\"", format!("{}", s)); + } + + #[test] + fn display_bool() { + let s: Value<DefaultScalarValue> = graphql_value!(false); + assert_eq!("false", format!("{}", s)); + + let s: Value<DefaultScalarValue> = graphql_value!(true); + assert_eq!("true", format!("{}", s)); + } + + #[test] + fn display_list() { + let s: Value<DefaultScalarValue> = graphql_value!([1, None, "foo"]); + assert_eq!("[1, null, \"foo\"]", format!("{}", s)); + } + + #[test] + fn display_list_one_element() { + let s: Value<DefaultScalarValue> = graphql_value!([1]); + assert_eq!("[1]", format!("{}", s)); + } + + #[test] + fn display_list_empty() { + let s: Value<DefaultScalarValue> = graphql_value!([]); + assert_eq!("[]", format!("{}", s)); + } + + #[test] + fn display_object() { + let s: Value<DefaultScalarValue> = graphql_value!({ + "int": 1, + "null": None, + "string": "foo", + }); + assert_eq!( + r#"{"int": 1, "null": null, "string": "foo"}"#, + format!("{}", s) + ); + } + + #[test] + fn display_object_one_field() { + let s: Value<DefaultScalarValue> = graphql_value!({ + "int": 1, + }); + assert_eq!(r#"{"int": 1}"#, format!("{}", s)); + } + + #[test] + fn display_object_empty() { + let s = Value::<DefaultScalarValue>::object(Object::with_capacity(0)); + assert_eq!(r#"{}"#, format!("{}", s)); + } } From 0e715bddc88c0222d0a642d54a1f1b9662de836b Mon Sep 17 00:00:00 2001 From: David Pedersen <david.pdrsn@gmail.com> Date: Thu, 31 Oct 2019 21:00:28 +0100 Subject: [PATCH 4/8] Implement `std::error::Error` for `FieldError` This required removing `impl From<T> for FieldError where T: Display` because it would otherwise cause a conflicting implementation. That is because `impl From<T> for T` already exists. Instead I added `impl From<String> for FieldError` and `impl From<&str> for FieldError` which should cover most use cases of the previous `impl`. I also added `FieldError::from_error` so users can convert from any error they may have. --- juniper/src/executor/mod.rs | 62 ++++++++++++++++++++++++++++++++----- 1 file changed, 55 insertions(+), 7 deletions(-) diff --git a/juniper/src/executor/mod.rs b/juniper/src/executor/mod.rs index d74893933..3e80ba36f 100644 --- a/juniper/src/executor/mod.rs +++ b/juniper/src/executor/mod.rs @@ -1,4 +1,10 @@ -use std::{borrow::Cow, cmp::Ordering, collections::HashMap, fmt::Display, sync::RwLock}; +use std::{ + borrow::Cow, + cmp::Ordering, + collections::HashMap, + fmt::{self, Debug, Display}, + sync::RwLock, +}; use fnv::FnvHashMap; @@ -123,14 +129,26 @@ where /// Field errors are represented by a human-readable error message and an /// optional `Value` structure containing additional information. /// -/// They can be converted to from any type that implements `std::fmt::Display`, -/// which makes error chaining with the `?` operator a breeze: +/// They can be converted to from `&str` or `String` which makes error +/// chaining with the `?` operator a breeze: /// /// ```rust /// # use juniper::{FieldError, ScalarValue}; /// fn get_string(data: Vec<u8>) -> Result<String, FieldError> /// { -/// let s = String::from_utf8(data)?; +/// let s = String::from_utf8(data).map_err(|e| e.to_string())?; +/// Ok(s) +/// } +/// ``` +/// +/// You can also use `FieldError::from_error` with anything that +/// implements `std::error::Error`: +/// +/// ```rust +/// # use juniper::{FieldError, ScalarValue}; +/// fn get_string(data: Vec<u8>) -> Result<String, FieldError> +/// { +/// let s = String::from_utf8(data).map_err(FieldError::from_error)?; /// Ok(s) /// } /// ``` @@ -140,13 +158,43 @@ pub struct FieldError<S = DefaultScalarValue> { extensions: Value<S>, } -impl<T: Display, S> From<T> for FieldError<S> +impl<S> FieldError<S> +where + S: ScalarValue, +{ + /// Create a `FieldError` from another error. + pub fn from_error<E>(error: E) -> FieldError<S> + where + E: std::error::Error, + { + FieldError::from(error.to_string()) + } +} + +impl<S: Display + Debug + ScalarValue> std::error::Error for FieldError<S> {} + +impl<S: Display + ScalarValue> fmt::Display for FieldError<S> { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "{} with extensions {}", self.message, self.extensions) + } +} + +impl<S> From<&str> for FieldError<S> where S: crate::value::ScalarValue, { - fn from(e: T) -> FieldError<S> { + fn from(message: &str) -> FieldError<S> { + FieldError::from(message.to_owned()) + } +} + +impl<S> From<String> for FieldError<S> +where + S: crate::value::ScalarValue, +{ + fn from(message: String) -> FieldError<S> { FieldError { - message: format!("{}", e), + message, extensions: Value::null(), } } From 30e9c003cbf851d160f59940d5744ff2a26523a5 Mon Sep 17 00:00:00 2001 From: David Pedersen <david.pdrsn@gmail.com> Date: Thu, 31 Oct 2019 21:03:03 +0100 Subject: [PATCH 5/8] Update changelog --- juniper/CHANGELOG.md | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/juniper/CHANGELOG.md b/juniper/CHANGELOG.md index dc6ad7ed2..587e01c1f 100644 --- a/juniper/CHANGELOG.md +++ b/juniper/CHANGELOG.md @@ -1,6 +1,17 @@ # master -- No changes yet +- All error types now implement `std::error::Error`: + - `FieldError` + - `GraphQLError` + - `LexerError` + - `ParseError` + - `RuleError` +- `impl From<T> for FieldError where T: Display` has been removed in favor of: + - `impl From<String> for FieldError` + - `impl From<&str> for FieldError` + - `FieldError::from_error` + +See [#419](https://github.com/graphql-rust/juniper/pull/419). # [[0.14.1] 2019-10-24](https://github.com/graphql-rust/juniper/releases/tag/juniper-0.14.1) From 0bc0523e0a0e64ffa41bf4699f1240613b14251d Mon Sep 17 00:00:00 2001 From: David Pedersen <david.pdrsn@gmail.com> Date: Fri, 1 Nov 2019 17:26:05 +0100 Subject: [PATCH 6/8] Format --- juniper/src/validation/context.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/juniper/src/validation/context.rs b/juniper/src/validation/context.rs index ef0b3782d..9a02ae15e 100644 --- a/juniper/src/validation/context.rs +++ b/juniper/src/validation/context.rs @@ -1,4 +1,7 @@ -use std::{collections::HashSet, fmt::{self, Debug}}; +use std::{ + collections::HashSet, + fmt::{self, Debug}, +}; use crate::ast::{Definition, Document, Type}; From 5e3d1462d9d72fb8f09a5bc9d27eb17492b3af70 Mon Sep 17 00:00:00 2001 From: David Pedersen <david.pdrsn@gmail.com> Date: Tue, 5 Nov 2019 09:22:01 +0100 Subject: [PATCH 7/8] Bring back `impl<T: Display, S> From<T> for FieldError<S>` We cannot have this and `impl<S> std::error::Error for FieldError<S>` so we agreed this is more valuable. More context https://github.com/graphql-rust/juniper/pull/419 --- juniper/src/executor/mod.rs | 54 +++++-------------------------------- 1 file changed, 6 insertions(+), 48 deletions(-) diff --git a/juniper/src/executor/mod.rs b/juniper/src/executor/mod.rs index 3e80ba36f..dac40274f 100644 --- a/juniper/src/executor/mod.rs +++ b/juniper/src/executor/mod.rs @@ -129,26 +129,14 @@ where /// Field errors are represented by a human-readable error message and an /// optional `Value` structure containing additional information. /// -/// They can be converted to from `&str` or `String` which makes error -/// chaining with the `?` operator a breeze: +/// They can be converted to from any type that implements `std::fmt::Display`, +/// which makes error chaining with the `?` operator a breeze: /// /// ```rust /// # use juniper::{FieldError, ScalarValue}; /// fn get_string(data: Vec<u8>) -> Result<String, FieldError> /// { -/// let s = String::from_utf8(data).map_err(|e| e.to_string())?; -/// Ok(s) -/// } -/// ``` -/// -/// You can also use `FieldError::from_error` with anything that -/// implements `std::error::Error`: -/// -/// ```rust -/// # use juniper::{FieldError, ScalarValue}; -/// fn get_string(data: Vec<u8>) -> Result<String, FieldError> -/// { -/// let s = String::from_utf8(data).map_err(FieldError::from_error)?; +/// let s = String::from_utf8(data)?; /// Ok(s) /// } /// ``` @@ -158,43 +146,13 @@ pub struct FieldError<S = DefaultScalarValue> { extensions: Value<S>, } -impl<S> FieldError<S> -where - S: ScalarValue, -{ - /// Create a `FieldError` from another error. - pub fn from_error<E>(error: E) -> FieldError<S> - where - E: std::error::Error, - { - FieldError::from(error.to_string()) - } -} - -impl<S: Display + Debug + ScalarValue> std::error::Error for FieldError<S> {} - -impl<S: Display + ScalarValue> fmt::Display for FieldError<S> { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "{} with extensions {}", self.message, self.extensions) - } -} - -impl<S> From<&str> for FieldError<S> +impl<T: Display, S> From<T> for FieldError<S> where S: crate::value::ScalarValue, { - fn from(message: &str) -> FieldError<S> { - FieldError::from(message.to_owned()) - } -} - -impl<S> From<String> for FieldError<S> -where - S: crate::value::ScalarValue, -{ - fn from(message: String) -> FieldError<S> { + fn from(e: T) -> FieldError<S> { FieldError { - message, + message: format!("{}", e), extensions: Value::null(), } } From b7eb36a858217f5b74ca5841f84b537e24739851 Mon Sep 17 00:00:00 2001 From: David Pedersen <david.pdrsn@gmail.com> Date: Mon, 6 Jan 2020 09:12:48 +0100 Subject: [PATCH 8/8] Write errors without allocations --- juniper/src/lib.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/juniper/src/lib.rs b/juniper/src/lib.rs index 7af75a8bf..f82bc3bec 100644 --- a/juniper/src/lib.rs +++ b/juniper/src/lib.rs @@ -194,13 +194,10 @@ impl<'a> fmt::Display for GraphQLError<'a> { match self { GraphQLError::ParseError(error) => write!(f, "{}", error), GraphQLError::ValidationError(errors) => { - let msgs = errors - .iter() - .map(|error| format!("{}", error)) - .collect::<Vec<_>>() - .join("\n"); - - write!(f, "{}", msgs) + for error in errors { + writeln!(f, "{}", error)?; + } + Ok(()) } GraphQLError::NoOperationProvided => write!(f, "No operation provided"), GraphQLError::MultipleOperationsProvided => write!(f, "Multiple operations provided"),