Skip to content

Commit 0ba7e8b

Browse files
tyranrontyranron
committed
Fix also sync execution
1 parent 25cfbcb commit 0ba7e8b

File tree

3 files changed

+66
-23
lines changed

3 files changed

+66
-23
lines changed

‎juniper/src/types/async_await.rs‎

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -330,21 +330,23 @@ where
330330
})),
331331
));
332332
}
333-
} else if let Ok(Value::Null) = sub_result {
334-
// NOTE: Executing a fragment cannot really result in a `Value::Null`,
335-
// because it represents a set of fields, so normal execution always
336-
// results in a `Value::Object`. However, a `Value::Null` is used here
337-
// to indicate that fragment execution failed somewhere and, because
338-
// of non-`null` types involved, its error should be propagated to the
339-
// parent field.
333+
} else {
334+
if let Err(e) = sub_result {
335+
sub_exec.push_error_at(e, span.start);
336+
}
337+
// NOTE: Executing a fragment cannot really result in anything other
338+
// than `Value::Object`, because it represents a set of fields.
339+
// So, if an error happens or a `Value::Null` is returned, it's an
340+
// indication that the fragment execution failed somewhere and,
341+
// because of non-`null` types involved, its error should be
342+
// propagated to the parent field, which is done here by returning
343+
// a `Value::Null`.
340344
async_values.push_back(AsyncValueFuture::Field2(future::ready(
341345
AsyncValue::Field(AsyncField {
342346
name: String::new(), // doesn't matter here
343347
value: None,
344348
}),
345349
)));
346-
} else if let Err(e) = sub_result {
347-
sub_exec.push_error_at(e, span.start);
348350
}
349351
}
350352
}
@@ -387,21 +389,23 @@ where
387389
})),
388390
));
389391
}
390-
} else if let Ok(Value::Null) = sub_result {
391-
// NOTE: Executing a fragment cannot really result in a `Value::Null`,
392-
// because it represents a set of fields, so normal execution
393-
// always results in a `Value::Object`. However, a `Value::Null`
394-
// is used here to indicate that fragment execution failed
395-
// somewhere and, because of non-`null` types involved, its error
396-
// should be propagated to the parent field.
392+
} else {
393+
if let Err(e) = sub_result {
394+
sub_exec.push_error_at(e, span.start);
395+
}
396+
// NOTE: Executing a fragment cannot really result in anything other
397+
// than `Value::Object`, because it represents a set of fields.
398+
// So, if an error happens or a `Value::Null` is returned, it's an
399+
// indication that the fragment execution failed somewhere and,
400+
// because of non-`null` types involved, its error should be
401+
// propagated to the parent field, which is done here by returning
402+
// a `Value::Null`.
397403
async_values.push_back(AsyncValueFuture::Field2(future::ready(
398404
AsyncValue::Field(AsyncField {
399405
name: String::new(), // doesn't matter here
400406
value: None,
401407
}),
402408
)));
403-
} else if let Err(e) = sub_result {
404-
sub_exec.push_error_at(e, span.start);
405409
}
406410
}
407411
} else {

‎juniper/src/types/base.rs‎

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -537,8 +537,18 @@ where
537537
for (k, v) in object {
538538
merge_key_into(result, &k, v);
539539
}
540-
} else if let Err(e) = sub_result {
541-
sub_exec.push_error_at(e, span.start);
540+
} else {
541+
if let Err(e) = sub_result {
542+
sub_exec.push_error_at(e, span.start);
543+
}
544+
// NOTE: Executing a fragment cannot really result in anything other
545+
// than `Value::Object`, because it represents a set of fields.
546+
// So, if an error happens or a `Value::Null` is returned, it's an
547+
// indication that the fragment execution failed somewhere and,
548+
// because of non-`null` types involved, its error should be
549+
// propagated to the parent field, which is done here by returning
550+
// a `false`.
551+
return false;
542552
}
543553
}
544554
}
@@ -573,8 +583,18 @@ where
573583
for (k, v) in object {
574584
merge_key_into(result, &k, v);
575585
}
576-
} else if let Err(e) = sub_result {
577-
sub_exec.push_error_at(e, span.start);
586+
} else {
587+
if let Err(e) = sub_result {
588+
sub_exec.push_error_at(e, span.start);
589+
}
590+
// NOTE: Executing a fragment cannot really result in anything other
591+
// than `Value::Object`, because it represents a set of fields.
592+
// So, if an error happens or a `Value::Null` is returned, it's an
593+
// indication that the fragment execution failed somewhere and,
594+
// because of non-`null` types involved, its error should be
595+
// propagated to the parent field, which is done here by returning
596+
// a `false`.
597+
return false;
578598
}
579599
}
580600
} else if !resolve_selection_set_into(

‎tests/integration/tests/issue_1287.rs‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,5 +67,24 @@ async fn error_propagates_same_way() {
6767
.await
6868
.unwrap();
6969

70-
assert_eq!(actual, expected);
70+
assert_eq!(actual, expected, "async execution mismatch");
71+
72+
let (expected, _) = juniper::execute_sync(
73+
without_fragment,
74+
None,
75+
&Schema::new(Query, EmptyMutation::new(), EmptySubscription::new()),
76+
&Variables::new(),
77+
&(),
78+
)
79+
.unwrap();
80+
let (actual, _) = juniper::execute_sync(
81+
with_fragment,
82+
None,
83+
&Schema::new(Query, EmptyMutation::new(), EmptySubscription::new()),
84+
&Variables::new(),
85+
&(),
86+
)
87+
.unwrap();
88+
89+
assert_eq!(actual, expected, "sync execution mismatch");
7190
}

0 commit comments

Comments
 (0)