Allow post-validation `FromInputValue` errors by tyranron · Pull Request #987 · graphql-rust/juniper

tyranron · 2021-10-15T10:16:08Z

Required for #975

Synopsis

At the moment, FromInputValue fallibility is considered during input operation validation only. Also, the real error is erased, due to Option<T> usage, thus argument absence is unobviously mixed with convertion errors over the code base.

Being applicable to most cases, still there are some situations where the error may occur only during resolution after passing the validation (from #975 it's deserializing json::Value into Json<T: Deserialize>). At the moment, we're ending up with runtime panics in such situation, which is not good.

Solution

Make FromInputValue returning Result with the actual error preservation. Require this error be IntoFieldError convertible, so it may naturally bubble-up during operation execution.

Checklist

tests renewed
documentation updated
CHANGELOG entry described

tyranron · 2021-11-22T13:00:14Z

cc @ilslv

Before continuing this, make sure master is built OK on latest nightly, and update it (if required) in a separate PR.

…errors

# Conflicts: # juniper/src/ast.rs # juniper_actix/src/lib.rs

ilslv · 2021-12-01T14:15:07Z

@tyranron PR is ready for review

tyranron

@ilslv I've refactored base machinery in juniper:

more errors propagation;
simplifying code with using String in implementations rather than FieldError<S>.

Codegen needs some updates, though, described below.

tyranron · 2021-12-10T15:34:22Z

juniper/src/types/containers.rs

+    NotEnough(usize),
+
+    /// Too many elements. Contains __all__ [`InputValue`] elements.
+    TooMuch(Vec<T>),


I do think keeping Vec gives no actual use here, while complicates relative unsafe code (and so reasoning) a lot.

tyranron · 2021-12-10T15:54:38Z

juniper/src/types/containers.rs

+            <Vec<i32>>::from_input_value(&V::list(vec![V::scalar(1), V::scalar(2), V::scalar(3)])),
+            Ok(vec![1, 2, 3]),
+        );
+        // TODO: all examples


@ilslv we should provide here a complete test suite. But let's do it in a separate PR after merging this.

tyranron · 2021-12-10T16:31:48Z

juniper/src/schema/meta.rs

    T: FromInputValue<S>,
 {
-    <T as FromInputValue<S>>::from_input_value(v).is_some()
+    T::from_input_value(v).is_ok()


We don't actually propagate any errors here, only silence them as we did before. Let's do propagation.

tyranron · 2021-12-10T16:39:24Z

juniper/src/types/scalars.rs

-                .map(ID),
-            _ => None,
-        }
+    fn from_input_value(v: &InputValue) -> Result<ID, FieldError> {


We should be more accurate here and use FieldError<S> to omit redundant scalar conversions.

tyranron · 2021-12-10T17:27:51Z

juniper_codegen/src/graphql_union/mod.rs

+                    }
+                }
+                None => return Box::pin(::juniper::futures::future::ready(
+                    Err(::juniper::FieldError::from("This GraphQLType has no name"))


@ilslv to DRYing the generated code a bit, let's move such things to helper functions inside juniper crate, and reuse them here.

…nput-value-resolve-errors # Conflicts: # juniper/CHANGELOG.md

# Conflicts: # integration_tests/juniper_tests/src/issue_372.rs # juniper/CHANGELOG.md # juniper/src/validation/rules/fields_on_correct_type.rs # juniper_actix/src/lib.rs

ilslv · 2021-12-14T09:17:04Z

@tyranron this PR is ready for review

tyranron · 2021-12-14T16:06:58Z

juniper_codegen/src/common/field/arg.rs

+                                },
+                                Ok,
+                            )
+                        })?


@ilslv I don't follow the logic here.

It was:

args.get::<#ty>(#name) returns Option<T>.

If it's None, then ::juniper::FromInputValue::<#scalar>::from_implicit_null returns Option<T>.

If it's None we panic.

Now it is:

args.get::<#ty>(#name) returns Result<Option<T>, FieldError>.

If it's Ok, we check the inner opt.

If it Some we map it to Some(Ok(v)) and so have Result<Option<Result<T>, FieldError>, FieldError>.

Why?

It seems the more natural way is:

Unpack the Result early: args.get::<#ty>(#name)?

.transpose() the inner error in Option and return it with ?.

Oh... understood. .map_or_else doesn't map an inner Option value, but rather maps the whole Option into something.

.ok_or_else will do better here.

Meh... .ok_or_else() cannot map absense to Result::Ok.

tyranron

@ilslv LGTM

tyranron · 2021-12-14T16:26:43Z

juniper_codegen/src/common/field/arg.rs

+                                            let e = ::juniper::IntoFieldError::into_field_error(e);
+                                            ::juniper::FieldError::new(
+                                                format!(#err_text, e.message()),
+                                                e.extensions().clone(),


This mess is better be replaced with .map_message method on FieldError.

tyranron · 2021-12-14T16:55:10Z

juniper_codegen/src/common/field/arg.rs

+                                },
+                                Ok,
+                            )
+                        })?


Meh... .ok_or_else() cannot map absense to Result::Ok.

tyranron added 2 commits October 15, 2021 13:05

Change FromInputValue signature and boostrap refactoring

0cb9248

Refactor primitive types, vol.1

584f9ae

tyranron added enhancement Improvement of existing features or bugfix semver::breaking Breaking change in terms of SemVer labels Oct 15, 2021

tyranron self-assigned this Oct 15, 2021

tyranron marked this pull request as draft October 15, 2021 10:18

tyranron assigned ilslv and unassigned tyranron Nov 22, 2021

ilslv added 12 commits November 23, 2021 10:55

Update actix beta

e5481a0

Merge branch 'update-actix-beta' into allow-from-input-value-resolve-…

fdac7c7

…errors

Merge branch 'master' into allow-from-input-value-resolve-errors

1c0ce20

# Conflicts: # juniper/src/ast.rs # juniper_actix/src/lib.rs

Support FromInputValue for arrays

acbdf97

Fix existing tests

7092bf2

Remove more unwraps and panics

111e464

Fix ui codegen tests

555db09

Fix book example and docs

1350245

Merge branch 'master' into allow-from-input-value-resolve-errors

3692c19

Corrections

ea2fe64

CHANGELOG

115c55f

Correction

06e71c3

ilslv and others added 5 commits December 8, 2021 15:19

Forbid __typename on subscription root

53a7c69

CHANGELOG

c1d5631

Minor corrections

64f7f8e

Merge branch 'master' into allow-from-input-value-resolve-errors

ff7c3d7

Corrections

5cc7555

tyranron commented Dec 10, 2021

View reviewed changes

ilslv added 2 commits December 13, 2021 08:05

Add tests

3e43886

Merge branch 'master' into forbid_typename_on_subscription_root

0a7b7d7

ilslv added 10 commits December 13, 2021 08:41

Tests fixes

efca2c0

Upgrade actix

0112c1a

Use poll_fn()

7006d58

Merge branch 'forbid_typename_on_subscription_root' into allow-from-i…

dd3a43c

…nput-value-resolve-errors # Conflicts: # juniper/CHANGELOG.md

Add boxed_field_err_fut

f2dbec3

Corrections

91a91a1

Corrections

dd80735

Corrections

00ff1df

Merge branch 'master' into allow-from-input-value-resolve-errors

1f69939

# Conflicts: # integration_tests/juniper_tests/src/issue_372.rs # juniper/CHANGELOG.md # juniper/src/validation/rules/fields_on_correct_type.rs # juniper_actix/src/lib.rs

Corrections

982ec42

tyranron commented Dec 14, 2021

View reviewed changes

tyranron marked this pull request as ready for review December 14, 2021 16:55

Polishing

afd607e

tyranron commented Dec 14, 2021

View reviewed changes

tyranron self-assigned this Dec 14, 2021

tyranron merged commit 46be97a into master Dec 14, 2021

tyranron deleted the allow-from-input-value-resolve-errors branch December 14, 2021 17:30

ilslv mentioned this pull request Jan 10, 2022

Redesign #[graphql_scalar] macro #1014

Merged

15 tasks

tyranron added this to the 0.16.0 milestone Apr 13, 2022

tyranron mentioned this pull request Nov 30, 2023

Add descriptive value validation errors #811

Merged

Conversation

tyranron commented Oct 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Synopsis

Solution

Checklist

Uh oh!

tyranron commented Nov 22, 2021

Uh oh!

ilslv commented Dec 1, 2021

Uh oh!

tyranron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilslv commented Dec 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyranron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tyranron commented Oct 15, 2021 •

edited

Loading