From ca0a8bbfc623ce4b4d3d39c837c539fb64732478 Mon Sep 17 00:00:00 2001 From: Kiryl Mialeshka <8974488+meskill@users.noreply.github.com> Date: Fri, 3 May 2024 13:10:51 +0200 Subject: [PATCH] fix(grpc): show additional error info from expression (#1809) Co-authored-by: Tushar Mathur --- src/blueprint/into_schema.rs | 7 ++-- src/http/request_context.rs | 3 +- src/lambda/cache.rs | 7 ++-- src/lambda/concurrent.rs | 8 +++-- src/lambda/eval.rs | 6 ++-- src/lambda/expression.rs | 38 +++++++++++++------- src/lambda/io.rs | 34 ++++++++---------- src/serde_value_ext.rs | 44 ++++++++++++----------- tailcall-query-plan/src/resolver.rs | 2 +- tests/core/snapshots/grpc-error.md_0.snap | 12 ++++++- tests/expression_spec.rs | 6 ++-- 11 files changed, 97 insertions(+), 70 deletions(-) diff --git a/src/blueprint/into_schema.rs b/src/blueprint/into_schema.rs index 9b886bb848..6102314f6a 100644 --- a/src/blueprint/into_schema.rs +++ b/src/blueprint/into_schema.rs @@ -2,6 +2,7 @@ use std::borrow::Cow; use std::sync::Arc; use async_graphql::dynamic::{self, FieldFuture, FieldValue, SchemaBuilder}; +use async_graphql::ErrorExtensions; use async_graphql_value::ConstValue; use futures_util::TryFutureExt; use tracing::Instrument; @@ -68,8 +69,10 @@ fn to_type(def: &Definition) -> dynamic::Type { let ctx: ResolverContext = ctx.into(); let ctx = EvaluationContext::new(req_ctx, &ctx); - let const_value = - expr.eval(ctx, &Concurrent::Sequential).await?; + let const_value = expr + .eval(ctx, &Concurrent::Sequential) + .await + .map_err(|err| err.extend())?; let p = match const_value { ConstValue::List(a) => Some(FieldValue::list(a)), ConstValue::Null => FieldValue::NONE, diff --git a/src/http/request_context.rs b/src/http/request_context.rs index 158f52440c..7a3e8da519 100644 --- a/src/http/request_context.rs +++ b/src/http/request_context.rs @@ -15,6 +15,7 @@ use crate::graphql::GraphqlDataLoader; use crate::grpc; use crate::grpc::data_loader::GrpcDataLoader; use crate::http::{AppContext, DataLoaderRequest, HttpDataLoader}; +use crate::lambda::EvaluationError; use crate::runtime::TargetRuntime; #[derive(Setters)] @@ -33,7 +34,7 @@ pub struct RequestContext { pub min_max_age: Arc>>, pub cache_public: Arc>>, pub runtime: TargetRuntime, - pub cache: AsyncCache, + pub cache: AsyncCache, } impl RequestContext { diff --git a/src/lambda/cache.rs b/src/lambda/cache.rs index 2e0829f587..de53392b82 100644 --- a/src/lambda/cache.rs +++ b/src/lambda/cache.rs @@ -3,10 +3,11 @@ use std::num::NonZeroU64; use std::ops::Deref; use std::pin::Pin; -use anyhow::Result; use async_graphql_value::ConstValue; -use super::{Concurrent, Eval, EvaluationContext, Expression, ResolverContextLike}; +use super::{ + Concurrent, Eval, EvaluationContext, EvaluationError, Expression, ResolverContextLike, +}; pub trait CacheKey { fn cache_key(&self, ctx: &Ctx) -> u64; @@ -39,7 +40,7 @@ impl Eval for Cache { &'a self, ctx: EvaluationContext<'a, Ctx>, conc: &'a Concurrent, - ) -> Pin> + 'a + Send>> { + ) -> Pin> + 'a + Send>> { Box::pin(async move { if let Expression::IO(io) = self.expr.deref() { let key = io.cache_key(&ctx); diff --git a/src/lambda/concurrent.rs b/src/lambda/concurrent.rs index 792cf43ce1..b0aad7c1c2 100644 --- a/src/lambda/concurrent.rs +++ b/src/lambda/concurrent.rs @@ -1,6 +1,8 @@ use futures_util::stream::FuturesUnordered; use futures_util::{Future, StreamExt}; +use super::EvaluationError; + /// /// Concurrent controls the concurrency of a fold or foreach operation on lists. /// It's a flag that is set based on operators that are applied on a list. @@ -19,7 +21,7 @@ impl Concurrent { iter: impl Iterator, acc: B, f: impl Fn(B, A) -> anyhow::Result, - ) -> anyhow::Result + ) -> Result where F: Future, { @@ -46,9 +48,9 @@ impl Concurrent { &self, iter: impl Iterator, f: impl Fn(A) -> B, - ) -> anyhow::Result> + ) -> Result, EvaluationError> where - F: Future>, + F: Future>, { self.fold(iter, vec![], |mut acc, val| { acc.push(f(val?)); diff --git a/src/lambda/eval.rs b/src/lambda/eval.rs index fe1aef7156..9c784d514f 100644 --- a/src/lambda/eval.rs +++ b/src/lambda/eval.rs @@ -1,9 +1,7 @@ use core::future::Future; use std::pin::Pin; -use anyhow::Result; - -use super::{Concurrent, EvaluationContext, ResolverContextLike}; +use super::{Concurrent, EvaluationContext, EvaluationError, ResolverContextLike}; pub trait Eval where @@ -13,7 +11,7 @@ where &'a self, ctx: EvaluationContext<'a, Ctx>, conc: &'a Concurrent, - ) -> Pin> + 'a + Send>> + ) -> Pin> + 'a + Send>> where Output: 'a; } diff --git a/src/lambda/expression.rs b/src/lambda/expression.rs index d15deb109f..7910565de2 100644 --- a/src/lambda/expression.rs +++ b/src/lambda/expression.rs @@ -3,12 +3,12 @@ use std::fmt::{Debug, Display}; use std::pin::Pin; use std::sync::Arc; -use anyhow::{anyhow, Result}; use async_graphql::ErrorExtensions; use async_graphql_value::ConstValue; use thiserror::Error; use super::{Concurrent, Eval, EvaluationContext, ResolverContextLike, IO}; +use crate::auth; use crate::blueprint::DynamicValue; use crate::json::JsonLike; use crate::lambda::cache::Cache; @@ -67,10 +67,19 @@ pub enum EvaluationError { #[error("APIValidationError: {0:?}")] APIValidationError(Vec), - #[error("ExprEvalError: {0:?}")] + #[error("ExprEvalError: {0}")] ExprEvalError(String), + + #[error("DeserializeError: {0}")] + DeserializeError(String), + + #[error("Authentication Failure: {0}")] + AuthError(auth::error::Error), } +// TODO: remove conversion from anyhow and don't use anyhow to pass errors +// since it loses potentially valuable information that could be later provided +// in the error extensions impl From for EvaluationError { fn from(value: anyhow::Error) -> Self { match value.downcast::() { @@ -99,19 +108,19 @@ impl ErrorExtensions for EvaluationError { grpc_status_details, } = self { - e.set("grpc_code", *grpc_code); - e.set("grpc_description", grpc_description); - e.set("grpc_status_message", grpc_status_message); - e.set("grpc_status_details", grpc_status_details.clone()); + e.set("grpcCode", *grpc_code); + e.set("grpcDescription", grpc_description); + e.set("grpcStatusMessage", grpc_status_message); + e.set("grpcStatusDetails", grpc_status_details.clone()); } }) } } impl<'a> From> for EvaluationError { - fn from(_value: crate::valid::ValidationError<&'a str>) -> Self { + fn from(value: crate::valid::ValidationError<&'a str>) -> Self { EvaluationError::APIValidationError( - _value + value .as_vec() .iter() .map(|e| e.message.to_owned()) @@ -120,6 +129,12 @@ impl<'a> From> for EvaluationError { } } +impl From for EvaluationError { + fn from(value: auth::error::Error) -> Self { + EvaluationError::AuthError(value) + } +} + impl Expression { pub fn and_then(self, next: Self) -> Self { Expression::Context(Context::PushArgs { expr: Box::new(self), and_then: Box::new(next) }) @@ -136,7 +151,7 @@ impl Eval for Expression { &'a self, ctx: EvaluationContext<'a, Ctx>, conc: &'a Concurrent, - ) -> Pin> + 'a + Send>> { + ) -> Pin> + 'a + Send>> { Box::pin(async move { match self { Expression::Context(op) => match op { @@ -165,14 +180,13 @@ impl Eval for Expression { .unwrap_or(&async_graphql::Value::Null) .clone()) } - Expression::Dynamic(value) => value.render_value(&ctx), + Expression::Dynamic(value) => Ok(value.render_value(&ctx)), Expression::Protect(expr) => { ctx.request_ctx .auth_ctx .validate(ctx.request_ctx) .await - .to_result() - .map_err(|e| anyhow!("Authentication Failure: {}", e.to_string()))?; + .to_result()?; expr.eval(ctx, conc).await } Expression::IO(operation) => operation.eval(ctx, conc).await, diff --git a/src/lambda/io.rs b/src/lambda/io.rs index 97d101b379..06ac2f0355 100644 --- a/src/lambda/io.rs +++ b/src/lambda/io.rs @@ -2,7 +2,7 @@ use core::future::Future; use std::pin::Pin; use std::sync::Arc; -use anyhow::Result; +use async_graphql::from_value; use async_graphql_value::ConstValue; use reqwest::Request; @@ -49,20 +49,15 @@ impl Eval for IO { &'a self, ctx: super::EvaluationContext<'a, Ctx>, _conc: &'a super::Concurrent, - ) -> Pin> + 'a + Send>> { + ) -> Pin> + 'a + Send>> { let key = self.cache_key(&ctx); Box::pin(async move { ctx.request_ctx .cache .get_or_eval(key, move || { - Box::pin(async { - self.eval_inner(ctx, _conc) - .await - .map_err(|err| err.to_string()) - }) + Box::pin(async { self.eval_inner(ctx, _conc).await }) }) .await - .map_err(|err| anyhow::anyhow!(err)) }) } } @@ -72,7 +67,7 @@ impl IO { &'a self, ctx: super::EvaluationContext<'a, Ctx>, _conc: &'a super::Concurrent, - ) -> Pin> + 'a + Send>> { + ) -> Pin> + 'a + Send>> { Box::pin(async move { match self { IO::Http { req_template, dl_id, .. } => { @@ -190,7 +185,7 @@ fn set_cookie_headers<'ctx, Ctx: ResolverContextLike<'ctx>>( async fn execute_raw_request<'ctx, Ctx: ResolverContextLike<'ctx>>( ctx: &EvaluationContext<'ctx, Ctx>, req: Request, -) -> Result> { +) -> Result, EvaluationError> { let response = ctx .request_ctx .runtime @@ -207,12 +202,10 @@ async fn execute_raw_grpc_request<'ctx, Ctx: ResolverContextLike<'ctx>>( ctx: &EvaluationContext<'ctx, Ctx>, req: Request, operation: &ProtobufOperation, -) -> Result> { - Ok( - execute_grpc_request(&ctx.request_ctx.runtime, operation, req) - .await - .map_err(EvaluationError::from)?, - ) +) -> Result, EvaluationError> { + execute_grpc_request(&ctx.request_ctx.runtime, operation, req) + .await + .map_err(EvaluationError::from) } async fn execute_grpc_request_with_dl< @@ -227,7 +220,7 @@ async fn execute_grpc_request_with_dl< ctx: &EvaluationContext<'ctx, Ctx>, rendered: RenderedRequestTemplate, data_loader: Option<&DataLoader>, -) -> Result> { +) -> Result, EvaluationError> { let headers = ctx .request_ctx .upstream @@ -253,7 +246,7 @@ async fn execute_request_with_dl< ctx: &EvaluationContext<'ctx, Ctx>, req: Request, data_loader: Option<&DataLoader>, -) -> Result> { +) -> Result, EvaluationError> { let headers = ctx .request_ctx .upstream @@ -275,8 +268,9 @@ fn parse_graphql_response<'ctx, Ctx: ResolverContextLike<'ctx>>( ctx: &EvaluationContext<'ctx, Ctx>, res: Response, field_name: &str, -) -> Result { - let res: async_graphql::Response = serde_json::from_value(res.body.into_json()?)?; +) -> Result { + let res: async_graphql::Response = + from_value(res.body).map_err(|err| EvaluationError::DeserializeError(err.to_string()))?; for error in res.errors { ctx.add_error(error); diff --git a/src/serde_value_ext.rs b/src/serde_value_ext.rs index 1b0f46f6de..9751b70f48 100644 --- a/src/serde_value_ext.rs +++ b/src/serde_value_ext.rs @@ -1,6 +1,5 @@ use std::borrow::Cow; -use anyhow::Result; use async_graphql::{Name, Value as GraphQLValue}; use indexmap::IndexMap; @@ -8,13 +7,13 @@ use crate::blueprint::DynamicValue; use crate::path::PathString; pub trait ValueExt { - fn render_value(&self, ctx: &impl PathString) -> Result; + fn render_value(&self, ctx: &impl PathString) -> GraphQLValue; } impl ValueExt for DynamicValue { - fn render_value<'a>(&self, ctx: &'a impl PathString) -> Result { + fn render_value<'a>(&self, ctx: &'a impl PathString) -> GraphQLValue { match self { - DynamicValue::Value(value) => Ok(value.to_owned()), + DynamicValue::Value(value) => value.to_owned(), DynamicValue::Mustache(m) => { let rendered: Cow<'a, str> = Cow::Owned(m.render(ctx)); @@ -22,21 +21,24 @@ impl ValueExt for DynamicValue { // parsing can fail when Mustache::render returns bare string and since // that string is not wrapped with quotes serde_json will fail to parse it // but, we can just use that string as is - .or_else(|_| Ok(GraphQLValue::String(rendered.into_owned()))) + .unwrap_or_else(|_| GraphQLValue::String(rendered.into_owned())) } DynamicValue::Object(obj) => { - let out: Result> = obj + let out: IndexMap<_, _> = obj .iter() .map(|(k, v)| { let key = Cow::Borrowed(k.as_str()); - v.render_value(ctx).map(|val| (Name::new(&key), val)) + let val = v.render_value(ctx); + + (Name::new(key), val) }) .collect(); - out.map(GraphQLValue::Object) + + GraphQLValue::Object(out) } DynamicValue::Array(arr) => { - let out: Result> = arr.iter().map(|v| v.render_value(ctx)).collect(); - out.map(GraphQLValue::List) + let out: Vec<_> = arr.iter().map(|v| v.render_value(ctx)).collect(); + GraphQLValue::List(out) } } } @@ -56,7 +58,7 @@ mod tests { let ctx = json!({"foo": {"bar": "baz"}}); let result = value.render_value(&ctx); let expected = async_graphql::Value::from_json(json!({"a": {"bar": "baz"}})).unwrap(); - assert_eq!(result.unwrap(), expected); + assert_eq!(result, expected); } #[test] @@ -66,7 +68,7 @@ mod tests { let ctx = json!({"foo": {"bar": {"baz": 1}}}); let result = value.render_value(&ctx); let expected = async_graphql::Value::from_json(json!({"a": 1})).unwrap(); - assert_eq!(result.unwrap(), expected); + assert_eq!(result, expected); } #[test] @@ -76,7 +78,7 @@ mod tests { let ctx = json!({"foo": {"bar": {"baz": "foo"}}}); let result = value.render_value(&ctx); let expected = async_graphql::Value::from_json(json!({"a": "foo"})).unwrap(); - assert_eq!(result.unwrap(), expected); + assert_eq!(result, expected); } #[test] @@ -86,7 +88,7 @@ mod tests { let ctx = json!({"foo": {"bar": {"baz": null}}}); let result = value.render_value(&ctx); let expected = async_graphql::Value::from_json(json!(null)).unwrap(); - assert_eq!(result.unwrap(), expected); + assert_eq!(result, expected); } #[test] @@ -96,7 +98,7 @@ mod tests { let ctx = json!({"foo": {"bar": {"baz": true}}}); let result = value.render_value(&ctx); let expected = async_graphql::Value::from_json(json!({"a": true})).unwrap(); - assert_eq!(result.unwrap(), expected); + assert_eq!(result, expected); } #[test] @@ -106,7 +108,7 @@ mod tests { let ctx = json!({"foo": {"bar": {"baz": 1.1}}}); let result = value.render_value(&ctx); let expected = async_graphql::Value::from_json(json!({"a": 1.1})).unwrap(); - assert_eq!(result.unwrap(), expected); + assert_eq!(result, expected); } #[test] @@ -116,7 +118,7 @@ mod tests { let ctx = json!({"foo": {"bar": {"baz": [1,2,3]}}}); let result = value.render_value(&ctx); let expected = async_graphql::Value::from_json(json!({"a": [1, 2, 3]})).unwrap(); - assert_eq!(result.unwrap(), expected); + assert_eq!(result, expected); } #[test] @@ -126,7 +128,7 @@ mod tests { let ctx = json!({"foo": {"bar": {"baz": 1, "qux": 2}}}); let result = value.render_value(&ctx); let expected = async_graphql::Value::from_json(json!({"a": [1, 2]})).unwrap(); - assert_eq!(result.unwrap(), expected); + assert_eq!(result, expected); } #[test] @@ -134,7 +136,7 @@ mod tests { let value = json!("{{foo}}"); let value = DynamicValue::try_from(&value).unwrap(); let ctx = json!({"foo": "bar"}); - let result = value.render_value(&ctx).unwrap(); + let result = value.render_value(&ctx); let expected = async_graphql::Value::String("bar".to_owned()); assert_eq!(result, expected); } @@ -146,7 +148,7 @@ mod tests { let ctx = json!({"foo": {"bar": {"baz": 1, "qux": 2}}}); let result = value.render_value(&ctx); let expected = async_graphql::Value::from_json(json!([{"a": 1}, {"a":2}])).unwrap(); - assert_eq!(result.unwrap(), expected); + assert_eq!(result, expected); } #[test] @@ -158,6 +160,6 @@ mod tests { let expected = async_graphql::Value::from_json(json!([{"a": [{"aa": 1}]}, {"a":[{"aa": 2}]}])) .unwrap(); - assert_eq!(result.unwrap(), expected); + assert_eq!(result, expected); } } diff --git a/tailcall-query-plan/src/resolver.rs b/tailcall-query-plan/src/resolver.rs index c7868e8a79..9b875be77d 100644 --- a/tailcall-query-plan/src/resolver.rs +++ b/tailcall-query-plan/src/resolver.rs @@ -59,7 +59,7 @@ impl FieldPlan { &'a self, ctx: EvaluationContext<'a, Ctx>, ) -> Result { - self.resolver.eval(ctx, &Concurrent::Sequential).await + Ok(self.resolver.eval(ctx, &Concurrent::Sequential).await?) } } diff --git a/tests/core/snapshots/grpc-error.md_0.snap b/tests/core/snapshots/grpc-error.md_0.snap index e69720283c..cca7ca0d8c 100644 --- a/tests/core/snapshots/grpc-error.md_0.snap +++ b/tests/core/snapshots/grpc-error.md_0.snap @@ -17,7 +17,17 @@ expression: response "line": 1, "column": 9 } - ] + ], + "extensions": { + "grpcCode": 3, + "grpcDescription": "Client specified an invalid argument", + "grpcStatusDetails": { + "code": 3, + "message": "error message", + "details": [] + }, + "grpcStatusMessage": "grpc message" + } } ] } diff --git a/tests/expression_spec.rs b/tests/expression_spec.rs index 45e07d2b3e..05b43aac4f 100644 --- a/tests/expression_spec.rs +++ b/tests/expression_spec.rs @@ -5,10 +5,12 @@ mod tests { use serde_json::json; use tailcall::blueprint::{Blueprint, DynamicValue}; use tailcall::http::RequestContext; - use tailcall::lambda::{Concurrent, EmptyResolverContext, Eval, EvaluationContext, Expression}; + use tailcall::lambda::{ + Concurrent, EmptyResolverContext, Eval, EvaluationContext, EvaluationError, Expression, + }; use tailcall::mustache::Mustache; - async fn eval(expr: &Expression) -> anyhow::Result { + async fn eval(expr: &Expression) -> Result { let runtime = tailcall::cli::runtime::init(&Blueprint::default()); let req_ctx = RequestContext::new(runtime); let res_ctx = EmptyResolverContext {};