Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(jit): input arguments required error #2756

Merged
merged 15 commits into from
Sep 19, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/core/blueprint/index.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use indexmap::IndexMap;

use super::InputObjectTypeDefinition;
use crate::core::blueprint::{
Blueprint, Definition, FieldDefinition, InputFieldDefinition, SchemaDefinition,
};
Expand Down Expand Up @@ -78,6 +79,13 @@ impl Index {
false
}
}

pub fn get_input_type_definition(&self, type_name: &str) -> Option<&InputObjectTypeDefinition> {
match self.map.get(type_name) {
Some((Definition::InputObject(input), _)) => Some(input),
_ => None,
}
}
}

impl From<&Blueprint> for Index {
Expand Down
9 changes: 1 addition & 8 deletions src/core/jit/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,16 +70,9 @@ impl<'a, Input: Clone, Output> Context<'a, Input, Output> {

for arg in field.args.iter() {
let name = arg.name.as_str();
let value = arg
.value
.clone()
// TODO: default value resolution should happen in the InputResolver
.or_else(|| arg.default_value.clone());
let value = arg.value.clone();
if let Some(value) = value {
arg_map.insert(Name::new(name), value);
} else if !arg.type_of.is_nullable() {
// TODO: throw error here
todo!()
}
}
Some(arg_map)
Expand Down
5 changes: 5 additions & 0 deletions src/core/jit/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ pub enum BuildError {
pub enum ResolveInputError {
#[error("Variable `{0}` is not defined")]
VariableIsNotFound(String),
#[error("Argument `{arg_name}` for field `{field_name}` is required")]
ArgumentIsRequired {
arg_name: String,
field_name: String,
},
}

#[derive(Error, Debug, Clone)]
Expand Down
119 changes: 114 additions & 5 deletions src/core/jit/input_resolver.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use async_graphql_value::{ConstValue, Value};

use super::{OperationPlan, ResolveInputError, Variables};
use super::{Arg, Field, OperationPlan, ResolveInputError, Variables};
use crate::core::json::{JsonLikeOwned, JsonObjectLike};
use crate::core::Type;

/// Trait to represent conversion from some dynamic type (with variables)
/// to the resolved variant based on the additional provided info.
Expand All @@ -18,8 +20,6 @@
impl InputResolvable for Value {
type Output = ConstValue;

// TODO:
// - provide default values
fn resolve(self, variables: &Variables<ConstValue>) -> Result<Self::Output, ResolveInputError> {
self.into_const_with(|name| {
variables
Expand All @@ -30,6 +30,19 @@
}
}

pub trait OutputHelpers {
karatakis marked this conversation as resolved.
Show resolved Hide resolved
type Output;

fn from_json(value: Option<serde_json::Value>) -> Option<Self::Output>;
}

impl OutputHelpers for ConstValue {
type Output = ConstValue;
fn from_json(value: Option<serde_json::Value>) -> Option<Self::Output> {
value.map(ConstValue::from_json).and_then(Result::ok)
}
}

/// Transforms [OperationPlan] values the way that all the input values
/// are transformed to const variant with the help of [InputResolvable] trait
pub struct InputResolver<Input> {
Expand All @@ -45,7 +58,7 @@
impl<Input, Output> InputResolver<Input>
where
Input: Clone,
Output: Clone,
Output: Clone + JsonLikeOwned + OutputHelpers<Output = Output>,
Input: InputResolvable<Output = Output>,
{
pub fn resolve_input(
Expand All @@ -57,7 +70,28 @@
.as_parent()
.iter()
.map(|field| field.clone().try_map(|value| value.resolve(variables)))
.collect::<Result<_, _>>()?;
.map(|field| match field {
Ok(field) => {
let args = field
.args
.into_iter()
.map(|arg| {
let value = self.recursive_parse_arg(
&field.name,
&arg.name,
&arg.type_of,
&arg.default_value,
arg.value,
)?;
Ok(Arg { value, ..arg })
})
.collect::<Result<_, _>>()?;

Ok(Field { args, ..field })
}
Err(err) => Err(err),
})
.collect::<Result<Vec<_>, _>>()?;

Ok(OperationPlan::new(
new_fields,
Expand All @@ -66,4 +100,79 @@
self.plan.is_introspection_query,
))
}

#[allow(clippy::too_many_arguments)]
fn recursive_parse_arg(
&self,
parent_name: &str,
arg_name: &str,
type_of: &Type,
default_value: &Option<Output>,
value: Option<Output>,
) -> Result<Option<Output>, ResolveInputError> {
let is_value_null = value.as_ref().map(|val| val.is_null()).unwrap_or(true);
let value: Result<Option<Output>, ResolveInputError> =
if !type_of.is_nullable() && value.is_none() {
let default_value = default_value.clone();
match default_value {
Some(value) => Ok(Some(value)),
None => Err(ResolveInputError::ArgumentIsRequired {
arg_name: arg_name.to_string(),
field_name: parent_name.to_string(),
}),
}
} else if !type_of.is_nullable() && is_value_null {
karatakis marked this conversation as resolved.
Show resolved Hide resolved
Err(ResolveInputError::ArgumentIsRequired {
arg_name: arg_name.to_string(),
field_name: parent_name.to_string(),
})
} else if value.is_none() {
let default_value = default_value.clone();
Ok(default_value)
} else {
Ok(value)
};
karatakis marked this conversation as resolved.
Show resolved Hide resolved

match value? {
Some(mut value) => match self.plan.index.get_input_type_definition(type_of.name()) {
Some(def) => {
if let Some(obj) = value.as_object_mut() {
for arg_field in &def.fields {
let parent_name = format!("{}.{}", parent_name, arg_name);
let field_value = obj.get_key(&arg_field.name).cloned();
let field_default = arg_field.default_value.clone();
let field_default = Output::from_json(field_default);
let value = self.recursive_parse_arg(
&parent_name,
&arg_field.name,
&arg_field.of_type,
&field_default,
field_value,
)?;
if let Some(value) = value {
obj.insert_key(&arg_field.name, value);
}
}
} else if let Some(arr) = value.as_array_mut() {
for (index, item) in arr.iter_mut().enumerate() {
let parent_name = format!("{}.{}.{}", parent_name, arg_name, index);

*item = self
.recursive_parse_arg(
&parent_name,
&index.to_string(),
type_of,
default_value,
karatakis marked this conversation as resolved.
Show resolved Hide resolved
Some(item.clone()),
)?
.expect("Because we start with `Some`, we will end with `Some`");
}
}

Check warning on line 170 in src/core/jit/input_resolver.rs

View check run for this annotation

Codecov / codecov/patch

src/core/jit/input_resolver.rs#L170

Added line #L170 was not covered by tests
Ok(Some(value))
}
None => Ok(Some(value)),
},
None => Ok(None),
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ expression: plan.into_nested()
): String(
"tailcall test",
),
Name(
"id",
): Number(
Number(101),
),
},
),
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ expression: plan.into_nested()
): String(
"test-12",
),
Name(
"id",
): Number(
Number(101),
),
},
),
),
Expand Down
7 changes: 7 additions & 0 deletions src/core/json/borrow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,13 @@
}
}

fn as_array_mut(&mut self) -> Option<&mut Vec<Self>> {
match self {
Value::Array(arr) => Some(arr),
_ => None,

Check warning on line 53 in src/core/json/borrow.rs

View check run for this annotation

Codecov / codecov/patch

src/core/json/borrow.rs#L50-L53

Added lines #L50 - L53 were not covered by tests
}
}

Check warning on line 55 in src/core/json/borrow.rs

View check run for this annotation

Codecov / codecov/patch

src/core/json/borrow.rs#L55

Added line #L55 was not covered by tests

fn into_array(self) -> Option<Vec<Self>> {
match self {
Value::Array(array) => Some(array),
Expand Down
7 changes: 7 additions & 0 deletions src/core/json/graphql.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,13 @@
}
}

fn as_array_mut(&mut self) -> Option<&mut Vec<Self>> {
match self {
ConstValue::List(seq) => Some(seq),
_ => None,

Check warning on line 39 in src/core/json/graphql.rs

View check run for this annotation

Codecov / codecov/patch

src/core/json/graphql.rs#L39

Added line #L39 was not covered by tests
}
}

fn into_array(self) -> Option<Vec<Self>> {
match self {
ConstValue::List(seq) => Some(seq),
Expand Down
1 change: 1 addition & 0 deletions src/core/json/json_like.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ pub trait JsonLike<'json>: Sized {

// Operators
fn as_array(&self) -> Option<&Vec<Self>>;
fn as_array_mut(&mut self) -> Option<&mut Vec<Self>>;
fn into_array(self) -> Option<Vec<Self>>;
fn as_object(&self) -> Option<&Self::JsonObject>;
fn as_object_mut(&mut self) -> Option<&mut Self::JsonObject>;
Expand Down
4 changes: 4 additions & 0 deletions src/core/json/serde.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@
self.as_array()
}

fn as_array_mut(&mut self) -> Option<&mut Vec<Self>> {
self.as_array_mut()
}

Check warning on line 31 in src/core/json/serde.rs

View check run for this annotation

Codecov / codecov/patch

src/core/json/serde.rs#L29-L31

Added lines #L29 - L31 were not covered by tests

fn into_array(self) -> Option<Vec<Self>> {
if let Self::Array(vec) = self {
Some(vec)
Expand Down
18 changes: 18 additions & 0 deletions tests/core/snapshots/graphql-conformance-001.md_5.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
---
source: tests/core/spec.rs
expression: response
---
{
"status": 200,
"headers": {
"content-type": "application/json"
},
"body": {
"data": null,
"errors": [
{
"message": "Build error: ResolveInputError: Argument `id` for field `user` is required"
}
]
}
}
18 changes: 18 additions & 0 deletions tests/core/snapshots/graphql-conformance-015.md_10.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
---
source: tests/core/spec.rs
expression: response
---
{
"status": 200,
"headers": {
"content-type": "application/json"
},
"body": {
"data": null,
"errors": [
{
"message": "Build error: ResolveInputError: Argument `size` for field `profilePic` is required"
}
]
}
}
19 changes: 19 additions & 0 deletions tests/core/snapshots/graphql-conformance-015.md_11.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
---
source: tests/core/spec.rs
expression: response
---
{
"status": 200,
"headers": {
"content-type": "application/json"
},
"body": {
"data": {
"user": {
"id": 4,
"name": "User 4",
"spam": "FIZZ: [{\"bar\":\"BUZZ\"},{\"bar\":\"test\"}]"
}
}
}
}
2 changes: 1 addition & 1 deletion tests/core/snapshots/graphql-conformance-015.md_5.snap
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ expression: response
"user": {
"id": 4,
"name": "User 4",
"featuredVideo": "video_4_1600_900_"
"featuredVideo": "video_4_1600_900_true"
}
}
}
Expand Down
18 changes: 18 additions & 0 deletions tests/core/snapshots/graphql-conformance-015.md_9.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
---
source: tests/core/spec.rs
expression: response
---
{
"status": 200,
"headers": {
"content-type": "application/json"
},
"body": {
"data": null,
"errors": [
{
"message": "Build error: ResolveInputError: Argument `height` for field `featuredVideoPreview.video` is required"
}
]
}
}
5 changes: 5 additions & 0 deletions tests/core/snapshots/graphql-conformance-015.md_client.snap
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ scalar Email

scalar Empty

input Foo {
bar: String! = "BUZZ"
}

scalar Int128

scalar Int16
Expand Down Expand Up @@ -49,6 +53,7 @@ type User {
name: String!
profilePic(size: Int! = 100, width: Int, height: Int = 100): String!
searchComments(query: [[String!]!]! = [["today"]]): String!
spam(foo: [Foo!]!): String!
}

input VideoSize {
Expand Down
Loading
Loading