From ce6f97d38e3294a8c2e1ed17e837e845f57bd890 Mon Sep 17 00:00:00 2001 From: Pierre Ricadat Date: Sat, 16 Oct 2021 09:16:58 +0900 Subject: [PATCH] Implement "Values of Correct Type" validation (#1093) * Implement variable validation spec * Implement value validation spec --- .../scala/caliban/validation/Validator.scala | 72 ++++++++----- ...ueValidator.scala => ValueValidator.scala} | 100 ++++++++++-------- .../caliban/execution/DefaultValueSpec.scala | 4 +- .../caliban/execution/ExecutionSpec.scala | 7 +- 4 files changed, 106 insertions(+), 77 deletions(-) rename core/src/main/scala/caliban/validation/{DefaultValueValidator.scala => ValueValidator.scala} (57%) diff --git a/core/src/main/scala/caliban/validation/Validator.scala b/core/src/main/scala/caliban/validation/Validator.scala index 9c4aa731e2..259947f56f 100644 --- a/core/src/main/scala/caliban/validation/Validator.scala +++ b/core/src/main/scala/caliban/validation/Validator.scala @@ -261,7 +261,13 @@ object Validator { s"Argument '$arg' is not defined on directive '${d.name}' ($location).", "Every argument provided to a field or directive must be defined in the set of possible arguments of that field or directive." ) - case Some(inputValue) => validateInputValues(inputValue, argValue, context) + case Some(inputValue) => + validateInputValues( + inputValue, + argValue, + context, + s"InputValue '${inputValue.name}' of Directive '${d.name}'" + ) } } *> IO.when(!directive.locations.contains(location))( @@ -456,43 +462,51 @@ object Validator { currentType: __Type, context: Context ): IO[ValidationError, Unit] = - IO.foreach_(field.arguments) { case (arg, argValue) => - f.args.find(_.name == arg) match { - case None => + IO.foreach_(f.args.filter(a => a.`type`().kind == __TypeKind.NON_NULL))(arg => + (arg.defaultValue, field.arguments.get(arg.name)) match { + case (None, None) | (None, Some(NullValue)) => + failValidation( + s"Required argument '${arg.name}' is null or missing on field '${field.name}' of type '${currentType.name + .getOrElse("")}'.", + "Arguments can be required. An argument is required if the argument type is non‐null and does not have a default value. Otherwise, the argument is optional." + ) + + case (Some(_), Some(NullValue)) => failValidation( - s"Argument '$arg' is not defined on field '${field.name}' of type '${currentType.name.getOrElse("")}'.", - "Every argument provided to a field or directive must be defined in the set of possible arguments of that field or directive." + s"Required argument '${arg.name}' is null on '${field.name}' of type '${currentType.name + .getOrElse("")}'.", + "Arguments can be required. An argument is required if the argument type is non‐null and does not have a default value. Otherwise, the argument is optional." ) - case Some(inputValue) => validateInputValues(inputValue, argValue, context) + case _ => IO.unit } - } *> - IO.foreach_(f.args.filter(a => a.`type`().kind == __TypeKind.NON_NULL))(arg => - (arg.defaultValue, field.arguments.get(arg.name)) match { - case (None, None) | (None, Some(NullValue)) => + ) *> + IO.foreach_(field.arguments) { case (arg, argValue) => + f.args.find(_.name == arg) match { + case None => failValidation( - s"Required argument '${arg.name}' is null or missing on field '${field.name}' of type '${currentType.name - .getOrElse("")}'.", - "Arguments can be required. An argument is required if the argument type is non‐null and does not have a default value. Otherwise, the argument is optional." + s"Argument '$arg' is not defined on field '${field.name}' of type '${currentType.name.getOrElse("")}'.", + "Every argument provided to a field or directive must be defined in the set of possible arguments of that field or directive." ) - - case (Some(_), Some(NullValue)) => - failValidation( - s"Required argument '${arg.name}' is null on '${field.name}' of type '${currentType.name - .getOrElse("")}'.", - "Arguments can be required. An argument is required if the argument type is non‐null and does not have a default value. Otherwise, the argument is optional." + case Some(inputValue) => + validateInputValues( + inputValue, + argValue, + context, + s"InputValue '${inputValue.name}' of Field '${field.name}'" ) - case _ => IO.unit } - ) + } private[caliban] def validateInputValues( inputValue: __InputValue, argValue: InputValue, - context: Context + context: Context, + errorContext: String ): IO[ValidationError, Unit] = { val t = inputValue.`type`() val inputType = if (t.kind == __TypeKind.NON_NULL) t.ofType.getOrElse(t) else t val inputFields = inputType.inputFields.getOrElse(Nil) + argValue match { case InputValue.ObjectValue(fields) if inputType.kind == __TypeKind.INPUT_OBJECT => IO.foreach_(fields) { case (k, v) => @@ -502,7 +516,13 @@ object Validator { s"Input field '$k' is not defined on type '${inputType.name.getOrElse("?")}'.", "Every input field provided in an input object value must be defined in the set of possible fields of that input object’s expected type." ) - case Some(value) => validateInputValues(value, v, context) + case Some(value) => + validateInputValues( + value, + v, + context, + s"InputValue '${inputValue.name}' of Field '$k' of InputObject '${t.name.getOrElse("")}'" + ) } } *> IO .foreach_(inputFields)(inputField => @@ -528,7 +548,7 @@ object Validator { } case _ => IO.unit } - } + } *> ValueValidator.validateInputTypes(inputValue, argValue, errorContext) private def checkVariableUsageAllowed( variableDefinition: VariableDefinition, @@ -741,7 +761,7 @@ object Validator { private[caliban] def validateInputValue(inputValue: __InputValue, errorContext: String): IO[ValidationError, Unit] = { val fieldContext = s"InputValue '${inputValue.name}' of $errorContext" for { - _ <- DefaultValueValidator.validateDefaultValue(inputValue, fieldContext) + _ <- ValueValidator.validateDefaultValue(inputValue, fieldContext) _ <- doesNotStartWithUnderscore(inputValue, fieldContext) _ <- onlyInputType(inputValue.`type`(), fieldContext) } yield () diff --git a/core/src/main/scala/caliban/validation/DefaultValueValidator.scala b/core/src/main/scala/caliban/validation/ValueValidator.scala similarity index 57% rename from core/src/main/scala/caliban/validation/DefaultValueValidator.scala rename to core/src/main/scala/caliban/validation/ValueValidator.scala index 1529dd48ce..8515a9a082 100644 --- a/core/src/main/scala/caliban/validation/DefaultValueValidator.scala +++ b/core/src/main/scala/caliban/validation/ValueValidator.scala @@ -9,7 +9,7 @@ import caliban.parsing.Parser import caliban.{ InputValue, Value } import zio.IO -object DefaultValueValidator { +object ValueValidator { def validateDefaultValue(field: __InputValue, errorContext: String): IO[ValidationError, Unit] = IO.whenCase(field.defaultValue) { case Some(v) => for { @@ -21,8 +21,7 @@ object DefaultValueValidator { "The default value for a field must be written using GraphQL input syntax." ) ) - _ <- Validator.validateInputValues(field, value, Context.empty) - _ <- validateInputTypes(field, value, errorContext) + _ <- Validator.validateInputValues(field, value, Context.empty, errorContext) } yield () } @@ -33,55 +32,62 @@ object DefaultValueValidator { ): IO[ValidationError, Unit] = validateType(inputValue.`type`(), argValue, errorContext) def validateType(inputType: __Type, argValue: InputValue, errorContext: String): IO[ValidationError, Unit] = - inputType.kind match { - case NON_NULL => - argValue match { - case NullValue => - failValidation(s"$errorContext is null", "Input field was null but was supposed to be non-null.") - case x => validateType(inputType.ofType.getOrElse(inputType), x, errorContext) - } - case LIST => - argValue match { - case ListValue(values) => - IO.foreach_(values)(v => - validateType(inputType.ofType.getOrElse(inputType), v, s"List item in $errorContext") - ) - case _ => - failValidation(s"$errorContext has invalid type: $argValue", "Input field was supposed to be a list.") - } + argValue match { + case _: VariableValue => IO.unit + case _ => + inputType.kind match { + case NON_NULL => + argValue match { + case NullValue => + failValidation(s"$errorContext is null", "Input field was null but was supposed to be non-null.") + case x => validateType(inputType.ofType.getOrElse(inputType), x, errorContext) + } + case LIST => + argValue match { + case ListValue(values) => + IO.foreach_(values)(v => + validateType(inputType.ofType.getOrElse(inputType), v, s"List item in $errorContext") + ) + case other => + // handle item as the first item in the list + validateType(inputType.ofType.getOrElse(inputType), other, s"List item in $errorContext") + } - case INPUT_OBJECT => - argValue match { - case ObjectValue(fields) => - IO.foreach_(inputType.inputFields.getOrElse(List.empty)) { f => - val value = - fields.collectFirst({ case (name, fieldValue) if name == f.name => fieldValue }).getOrElse(NullValue) - validateType(f.`type`(), value, s"Field ${f.name} in $errorContext") + case INPUT_OBJECT => + argValue match { + case ObjectValue(fields) => + IO.foreach_(inputType.inputFields.getOrElse(List.empty)) { f => + val value = + fields + .collectFirst({ case (name, fieldValue) if name == f.name => fieldValue }) + .getOrElse(NullValue) + validateType(f.`type`(), value, s"Field ${f.name} in $errorContext") + } + case _ => + failValidation( + s"$errorContext has invalid type: $argValue", + "Input field was supposed to be an input object." + ) } - case _ => - failValidation( - s"$errorContext has invalid type: $argValue", - "Input field was supposed to be an input object." - ) - } - case ENUM => - argValue match { - case EnumValue(value) => - validateEnum(value, inputType, errorContext) - case StringValue(value) => - validateEnum(value, inputType, errorContext) - case _ => + case ENUM => + argValue match { + case EnumValue(value) => + validateEnum(value, inputType, errorContext) + case StringValue(value) => + validateEnum(value, inputType, errorContext) + case _ => + failValidation( + s"$errorContext has invalid type: $argValue", + "Input field was supposed to be an enum value." + ) + } + case SCALAR => validateScalar(inputType, argValue, errorContext) + case _ => failValidation( - s"$errorContext has invalid type: $argValue", - "Input field was supposed to be an enum value." + s"$errorContext has invalid type $inputType", + "Input value is invalid, should be a scalar, list or input object." ) } - case SCALAR => validateScalar(inputType, argValue, errorContext) - case _ => - failValidation( - s"$errorContext has invalid type $inputType", - "Input value is invalid, should be a scalar, list or input object." - ) } def validateEnum(value: String, inputType: __Type, errorContext: String): IO[ValidationError, Unit] = { diff --git a/core/src/test/scala/caliban/execution/DefaultValueSpec.scala b/core/src/test/scala/caliban/execution/DefaultValueSpec.scala index 495cc60689..ea8175cad9 100644 --- a/core/src/test/scala/caliban/execution/DefaultValueSpec.scala +++ b/core/src/test/scala/caliban/execution/DefaultValueSpec.scala @@ -101,7 +101,7 @@ object DefaultValueSpec extends DefaultRunnableSpec { assertM(gql.interpreter)(anything) }, testM("invalid list validation") { - case class TestInput(@GQLDefault("\"string\"") string: List[String]) + case class TestInput(@GQLDefault("3") string: List[String]) case class Query(test: TestInput => List[String]) val gql = graphQL(RootResolver(Query(i => i.string))) assertM(gql.interpreter.run)( @@ -180,7 +180,7 @@ object DefaultValueSpec extends DefaultRunnableSpec { int <- gql.interpreter res <- int.execute(query) } yield assert(res.errors.headOption)( - isSome((isSubtype[CalibanError.ValidationError](anything))) + isSome(isSubtype[CalibanError.ValidationError](anything)) ) }, test("it should render default values in the SDL") { diff --git a/core/src/test/scala/caliban/execution/ExecutionSpec.scala b/core/src/test/scala/caliban/execution/ExecutionSpec.scala index bf7eee5771..e16bea1454 100644 --- a/core/src/test/scala/caliban/execution/ExecutionSpec.scala +++ b/core/src/test/scala/caliban/execution/ExecutionSpec.scala @@ -4,7 +4,7 @@ import java.util.UUID import caliban.CalibanError.ExecutionError import caliban.GraphQL._ import caliban.Macros.gqldoc -import caliban.{ CalibanError, GraphQL, RootResolver } +import caliban.{ CalibanError, GraphQL, InputValue, RootResolver } import caliban.TestUtils._ import caliban.Value.{ BooleanValue, IntValue, StringValue } import caliban.introspection.adt.__Type @@ -195,6 +195,7 @@ object ExecutionSpec extends DefaultRunnableSpec { case i if i > 0 => Right(NonNegInt(i)) case neg => Left(CalibanError.ExecutionError(s"$neg is negative")) } + implicit val nonNegIntSchema: Schema[Any, NonNegInt] = Schema.intSchema.contramap(_.value) } case class Args(int: NonNegInt, value: String) case class Test(q: Args => Unit) @@ -585,13 +586,15 @@ object ExecutionSpec extends DefaultRunnableSpec { case class User(test: UserArgs => String) case class Mutations(user: Task[User]) case class Queries(a: Int) + implicit val intArgBuilder: ArgBuilder[Int] = (_: InputValue) => Left(ExecutionError("nope")) + val api = graphQL(RootResolver(Queries(1), Mutations(ZIO.succeed(User(_.toString))))) val interpreter = api.interpreter val query = """mutation { | user { - | test(id: "wrong") + | test(id: 1) | } |}""".stripMargin interpreter