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 all 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 @@ -80,16 +80,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
106 changes: 101 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 @@ -45,8 +45,9 @@
impl<Input, Output> InputResolver<Input>
where
Input: Clone,
Output: Clone,
Output: Clone + JsonLikeOwned + TryFrom<serde_json::Value>,
Input: InputResolvable<Output = Output>,
<Output as TryFrom<serde_json::Value>>::Error: std::fmt::Debug,
{
pub fn resolve_input(
&self,
Expand All @@ -57,7 +58,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 +88,78 @@
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 = if !type_of.is_nullable() && value.is_none() {
let default_value = default_value.clone();

Some(default_value.ok_or(ResolveInputError::ArgumentIsRequired {
arg_name: arg_name.to_string(),
field_name: parent_name.to_string(),
})?)
} else if !type_of.is_nullable() && is_value_null {
return Err(ResolveInputError::ArgumentIsRequired {
arg_name: arg_name.to_string(),
field_name: parent_name.to_string(),
});
} else if value.is_none() {
default_value.clone()
} else {
value
};

let Some(mut value) = value else {
return Ok(None);
};

let Some(def) = self.plan.index.get_input_type_definition(type_of.name()) else {
return Ok(Some(value));
};

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()
.map(|value| Output::try_from(value).expect("The conversion cannot fail"));
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,
&None,
Some(item.clone()),
)?
.expect("Because we start with `Some`, we will end with `Some`");
}
}

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

View check run for this annotation

Codecov / codecov/patch

src/core/jit/input_resolver.rs#L161

Added line #L161 was not covered by tests

Ok(Some(value))
}
}
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
5 changes: 5 additions & 0 deletions tests/core/snapshots/graphql-conformance-015.md_merged.snap
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ schema
query: Query
}

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

input VideoSize {
hdr: Boolean = true
height: Int!
Expand All @@ -28,4 +32,5 @@ type User {
profilePic(size: Int! = 100, width: Int, height: Int = 100): String!
@expr(body: "{{.value.id}}_{{.args.size}}_{{.args.width}}_{{.args.height}}")
searchComments(query: [[String!]!]! = [["today"]]): String! @expr(body: "video_{{.value.id}}_{{.args.query}}")
spam(foo: [Foo!]!): String! @expr(body: "FIZZ: {{.args.foo}}")
}
Loading
Loading