fix: raise validation error for required fields #2535

laststylebender14 · 2024-07-26T08:26:34Z

Summary:
Briefly describe the changes made in this PR.

Issue Reference(s):
Fixes #... (Replace "..." with the issue number)

Build & Testing:

I ran cargo test successfully.
I have run ./lint.sh --mode=fix to fix all linting issues raised by ./lint.sh --mode=check.

Checklist:

I have added relevant unit & integration tests.
I have updated the documentation accordingly.
I have performed a self-review of my code.
PR follows the naming convention of <type>(<optional scope>): <title>

tests/execution/test-required-fields.md

meskill · 2024-07-29T11:36:03Z

src/core/jit/synth/synth.rs

@@ -82,7 +82,12 @@ impl<'a, Value: JsonLike<'a> + Clone + 'a> Synth<Value> {
        match parent {
            Some(parent) => {
                if !Self::is_array(&node.type_of, parent) {
-                    return Ok(Value::null());
+                    if node.type_of.is_nullable() {


let's put this link https://spec.graphql.org/October2021/#sec-Handling-Field-Errors somewhere around this code so it's faster to check the spec

meskill · 2024-07-29T11:37:32Z

src/core/jit/synth/synth.rs

+                        } else {
+                            Err(ValidationError::ValueRequired.into())
+                                .map_err(|e| self.to_location_error(e, node, data_path))
+                        }


can we just move that check at the end of the function and do it only once?
i.e. store the ouput in variable and check if shouldn't be null

meskill · 2024-07-29T11:39:11Z

src/core/jit/synth/synth.rs

-                        // IR exists, so there must be a value.
-                        // if there is no value then we must return Null
-                        Ok(Value::null())
+                        if node.type_of.is_nullable() || node.ir.is_some() {


why do we need to check ir here? Why having ir allows to have null for possibly non-nullable field?

meskill · 2024-07-29T11:42:07Z

src/core/jit/synth/synth.rs

+                    path.push(PathSegment::Index(data_path[idx]));
+                    idx += 1;


this code travels from children to parent fields to build the path, so it's built in backward, but the data_path is built in forward. So here you'll probably using the wrong indexes in case we have lists inside lists. Need a test for this

meskill · 2024-07-29T11:52:29Z

tests/execution/test-required-fields.md

+  users: [User] @http(path: "/users")
+  posts: [Post] @http(path: "/posts")


please, check the spec.

Seems like, since the field posts is nullable it should be set to null instead of propagating up to the whole data

meskill · 2024-07-29T11:53:17Z

tests/execution/test-required-fields.md

+  users: [User] @http(path: "/users")
+  posts: [Post] @http(path: "/posts")
+  post: Post! @http(path: "/posts/12")
+  foo: [Foo]! @http(path: "/foos")


Suggested change

foo: [Foo]! @http(path: "/foos")

foos: [Foo]! @http(path: "/foos")

tests/execution/test-required-fields.md

meskill · 2024-07-29T11:55:38Z

tests/execution/test-required-fields.md

+  post: Post! @http(path: "/posts/12")
+  foo: [Foo]! @http(path: "/foos")
+  fooInner: [Foo!] @http(path: "/foos-inner")
+}


need to add test case for [Foo!]!

tests/execution/test-required-fields.md

github-actions · 2024-08-03T12:03:31Z

Action required: PR inactive for 5 days.
Status update or closure in 10 days.

github-actions · 2024-08-16T07:03:13Z

Action required: PR inactive for 5 days.
Status update or closure in 10 days.

github-actions · 2024-08-22T09:02:52Z

Action required: PR inactive for 5 days.
Status update or closure in 10 days.

Co-authored-by: Kiryl Mialeshka <[email protected]>

karatakis · 2024-08-23T16:07:19Z

I will continue the development of this PR

karatakis · 2024-08-25T19:38:01Z

Because this PR has been stale for too long, I ported the changes to the current main branch. See here #2754

meskill · 2024-08-27T09:17:08Z

in favor of #2754

meskill and others added 30 commits July 18, 2024 13:41

store index inside the plan

4465b33

replace is_scalar check for field

c309562

fix(jit): add enum values validation

51a5705

fix lint

2179cce

Merge remote-tracking branch 'origin/main' into fix/jit-enum-validation

e641620

add validation for required fields

23de99b

refactor error mapping

1a8f744

- updated snaps

751ae1d

fix(jit): handle error path and position in single place (#2477)

01ebf20

Merge branch 'main' into fix/jit-enum-validation

06f0d21

- handle enum and required types

2317ddc

Merge branch 'main' into refactor/enum-validation-conflict-fixes

3b7eb32

- enable dead_code for now, disable it later

9fbd906

- use Lift

9c67d07

- lint changes

43904ae

Merge branch 'main' into refactor/enum-validation-conflict-fixes

9b4eabd

- todo complete: remove allow deadcode block

2f15bb4

- raise validation error.

eeddda1

- revert: none case

ea14df7

- added required types check

bae1cf5

Merge branch 'main' into refactor/enum-validation-conflict-fixes

07d3cc7

- snap updates

10560f2

Merge branch 'main' into refactor/enum-validation-conflict-fixes

eb24066

- lint changes

16f2c78

Merge branch 'main' into refactor/enum-validation-conflict-fixes

c38ed74

- lint formatting changes.

daf3790

- add errors found in context to response.

2814cc1

- rename method from get_errors to errors

e1fa878

- fix variable name

73564c5

- use extend instead of append

c1dc13a

laststylebender14 marked this pull request as draft July 26, 2024 12:11

laststylebender14 added 2 commits July 26, 2024 18:25

- if field has IR, then don't raise the error.

3823884

- refactor validate_field method

119cd38

laststylebender14 marked this pull request as ready for review July 26, 2024 13:07

- added more test cases

e17438b

laststylebender14 marked this pull request as draft July 26, 2024 14:46

laststylebender14 added 5 commits July 26, 2024 21:24

- renamed variable to is_response_null.

1fe3a69

- lint changes

4a3d97e

- revert changes

3e1359a

- added few more test cases

9e5ff71

- add list indexes in error.

33bd361

laststylebender14 commented Jul 26, 2024

View reviewed changes

tests/execution/test-required-fields.md Outdated Show resolved Hide resolved

meskill requested changes Jul 29, 2024

View reviewed changes

github-actions bot added the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label Aug 3, 2024

Base automatically changed from fix/handle-gql-errors to refactor/enum-validation-conflict-fixes August 11, 2024 05:38

github-actions bot removed the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label Aug 11, 2024

github-actions bot added the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label Aug 16, 2024

Base automatically changed from refactor/enum-validation-conflict-fixes to main August 17, 2024 07:06

github-actions bot removed the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label Aug 17, 2024

github-actions bot added the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label Aug 22, 2024

Update tests/execution/test-required-fields.md

7108864

Co-authored-by: Kiryl Mialeshka <[email protected]>

github-actions bot added the type: fix Iterations on existing features or infrastructure. label Aug 23, 2024

github-actions bot removed the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label Aug 23, 2024

meskill closed this Aug 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: raise validation error for required fields #2535

fix: raise validation error for required fields #2535

laststylebender14 commented Jul 26, 2024 •

edited

Loading

meskill Jul 29, 2024

meskill Jul 29, 2024

meskill Jul 29, 2024

meskill Jul 29, 2024

meskill Jul 29, 2024

meskill Jul 29, 2024

meskill Jul 29, 2024

github-actions bot commented Aug 3, 2024

github-actions bot commented Aug 16, 2024

github-actions bot commented Aug 22, 2024

karatakis commented Aug 23, 2024

karatakis commented Aug 25, 2024

meskill commented Aug 27, 2024

		users: [User] @http(path: "/users")
		posts: [Post] @http(path: "/posts")

	foo: [Foo]! @http(path: "/foos")
	foos: [Foo]! @http(path: "/foos")

fix: raise validation error for required fields #2535

fix: raise validation error for required fields #2535

Conversation

laststylebender14 commented Jul 26, 2024 • edited Loading

meskill Jul 29, 2024

Choose a reason for hiding this comment

meskill Jul 29, 2024

Choose a reason for hiding this comment

meskill Jul 29, 2024

Choose a reason for hiding this comment

meskill Jul 29, 2024

Choose a reason for hiding this comment

meskill Jul 29, 2024

Choose a reason for hiding this comment

meskill Jul 29, 2024

Choose a reason for hiding this comment

meskill Jul 29, 2024

Choose a reason for hiding this comment

github-actions bot commented Aug 3, 2024

github-actions bot commented Aug 16, 2024

github-actions bot commented Aug 22, 2024

karatakis commented Aug 23, 2024

karatakis commented Aug 25, 2024

meskill commented Aug 27, 2024

laststylebender14 commented Jul 26, 2024 •

edited

Loading