From 3b51d279681c77fe3a57921c45048377c5700a12 Mon Sep 17 00:00:00 2001 From: kyri-petrou <67301607+kyri-petrou@users.noreply.github.com> Date: Mon, 23 Sep 2024 17:34:31 +0300 Subject: [PATCH] Fix TODOs in SchemaValidator (#2415) --- core/src/main/scala/caliban/GraphQL.scala | 4 +- .../caliban/validation/SchemaValidator.scala | 41 ++++++++----------- .../scala/caliban/validation/Validator.scala | 2 +- .../validation/ValidationSchemaSpec.scala | 8 ++-- 4 files changed, 25 insertions(+), 30 deletions(-) diff --git a/core/src/main/scala/caliban/GraphQL.scala b/core/src/main/scala/caliban/GraphQL.scala index e0ee53303..af12eb32f 100644 --- a/core/src/main/scala/caliban/GraphQL.scala +++ b/core/src/main/scala/caliban/GraphQL.scala @@ -11,7 +11,7 @@ import caliban.parsing.{ Parser, SourceMapper, VariablesCoercer } import caliban.rendering.DocumentRenderer import caliban.schema._ import caliban.transformers.Transformer -import caliban.validation.Validator +import caliban.validation.{ SchemaValidator, Validator } import caliban.wrappers.Wrapper import caliban.wrappers.Wrapper._ import zio.stacktracer.TracingImplicits.disableAutoTrace @@ -32,7 +32,7 @@ trait GraphQL[-R] { self => protected val transformer: Transformer[R] private[caliban] def validateRootSchema: Either[ValidationError, RootSchema[R]] = - Validator.validateSchema(schemaBuilder) + SchemaValidator.validateSchema(schemaBuilder) /** * Returns a string that renders the API types into the GraphQL SDL. diff --git a/core/src/main/scala/caliban/validation/SchemaValidator.scala b/core/src/main/scala/caliban/validation/SchemaValidator.scala index da0059410..3222cc260 100644 --- a/core/src/main/scala/caliban/validation/SchemaValidator.scala +++ b/core/src/main/scala/caliban/validation/SchemaValidator.scala @@ -10,12 +10,7 @@ import caliban.schema.{ RootSchema, RootSchemaBuilder, Types } import caliban.validation.Utils.isObjectType import caliban.validation.ValidationOps._ -/* - * TODO In the next major version: - * 1. Make `SchemaValidator` an object and remove inheritance from Validator - * 2. Make all methods private except `validateSchema` and `validateType` - */ -private[caliban] trait SchemaValidator { +private[caliban] object SchemaValidator { /** * Verifies that the given schema is valid. Fails with a [[caliban.CalibanError.ValidationError]] otherwise. @@ -43,7 +38,7 @@ private[caliban] trait SchemaValidator { case _ => unit }) - private[caliban] def validateClashingTypes(types: List[__Type]): Either[ValidationError, Unit] = { + private def validateClashingTypes(types: List[__Type]): Either[ValidationError, Unit] = { val check = types.groupBy(_.name).collectFirst { case (Some(name), v) if v.size > 1 => (name, v) } check match { case None => unit @@ -108,7 +103,7 @@ private[caliban] trait SchemaValidator { } } - private[caliban] def validateEnum(t: __Type): Either[ValidationError, Unit] = + private def validateEnum(t: __Type): Either[ValidationError, Unit] = t.allEnumValues match { case _ :: _ => unit case Nil => @@ -118,7 +113,7 @@ private[caliban] trait SchemaValidator { ) } - private[caliban] def validateUnion(t: __Type): Either[ValidationError, Unit] = + private def validateUnion(t: __Type): Either[ValidationError, Unit] = t.possibleTypes match { case None | Some(Nil) => failValidation( @@ -134,7 +129,7 @@ private[caliban] trait SchemaValidator { case _ => unit } - private[caliban] def validateInputObject(t: __Type): Either[ValidationError, Unit] = { + private def validateInputObject(t: __Type): Either[ValidationError, Unit] = { lazy val inputObjectContext = s"""${if (t._isOneOfInput) "OneOf " else ""}InputObject '${t.name.getOrElse("")}'""" def noDuplicateInputValueName( @@ -184,7 +179,7 @@ private[caliban] trait SchemaValidator { } } - private[caliban] def validateInputValue( + private def validateInputValue( inputValue: __InputValue, errorContext: => String ): Either[ValidationError, Unit] = { @@ -196,7 +191,7 @@ private[caliban] trait SchemaValidator { } yield () } - private[caliban] def validateInterface(t: __Type): Either[ValidationError, Unit] = { + private def validateInterface(t: __Type): Either[ValidationError, Unit] = { lazy val interfaceContext = s"Interface '${t.name.getOrElse("")}'" t.allFields match { @@ -209,7 +204,7 @@ private[caliban] trait SchemaValidator { } } - def validateObject(obj: __Type): Either[ValidationError, Unit] = { + private def validateObject(obj: __Type): Either[ValidationError, Unit] = { lazy val objectContext = s"Object '${obj.name.getOrElse("")}'" def validateInterfaceFields(obj: __Type) = { @@ -315,7 +310,7 @@ private[caliban] trait SchemaValidator { private def isListField(field: __Field) = field._type.kind == __TypeKind.LIST - private[caliban] def onlyInputType(`type`: __Type, errorContext: => String): Either[ValidationError, Unit] = { + private def onlyInputType(`type`: __Type, errorContext: => String): Either[ValidationError, Unit] = { // https://spec.graphql.org/June2018/#IsInputType() def isInputType(t: __Type): Either[__Type, Unit] = { import __TypeKind._ @@ -336,7 +331,7 @@ private[caliban] trait SchemaValidator { } } - private[caliban] def validateFields(fields: List[__Field], context: => String): Either[ValidationError, Unit] = + private def validateFields(fields: List[__Field], context: => String): Either[ValidationError, Unit] = noDuplicateFieldName(fields, context) *> validateAllDiscard(fields) { field => lazy val fieldContext = s"Field '${field.name}' of $context" @@ -347,14 +342,14 @@ private[caliban] trait SchemaValidator { } yield () } - private[caliban] def noDuplicateFieldName(fields: List[__Field], errorContext: => String) = { + private def noDuplicateFieldName(fields: List[__Field], errorContext: => String) = { val messageBuilder = (f: __Field) => s"$errorContext has repeated fields: ${f.name}" def explanatory = "The field must have a unique name within that Interface type; no two fields may share the same name" noDuplicateName[__Field](fields, _.name, messageBuilder, explanatory) } - private[caliban] def onlyOutputType(`type`: __Type, errorContext: => String): Either[ValidationError, Unit] = { + private def onlyOutputType(`type`: __Type, errorContext: => String): Either[ValidationError, Unit] = { // https://spec.graphql.org/June2018/#IsOutputType() def isOutputType(t: __Type): Either[__Type, Unit] = { import __TypeKind._ @@ -375,7 +370,7 @@ private[caliban] trait SchemaValidator { } } - private[caliban] def noDuplicateName[T]( + private def noDuplicateName[T]( listOfNamed: List[T], nameExtractor: T => String, messageBuilder: T => String, @@ -388,7 +383,7 @@ private[caliban] trait SchemaValidator { failValidation(messageBuilder(duplicate), explanatoryText) ) - private[caliban] def checkName(name: String, fieldContext: => String): Either[ValidationError, Unit] = + private def checkName(name: String, fieldContext: => String): Either[ValidationError, Unit] = Parser .parseName(name) .left @@ -399,7 +394,7 @@ private[caliban] trait SchemaValidator { ) ) *> doesNotStartWithUnderscore(name, fieldContext) - private[caliban] def doesNotStartWithUnderscore( + private def doesNotStartWithUnderscore( name: String, errorContext: => String ): Either[ValidationError, Unit] = @@ -408,7 +403,7 @@ private[caliban] trait SchemaValidator { """Names can not begin with the characters "__" (two underscores)""" ) - private[caliban] def validateRootQuery[R]( + private def validateRootQuery[R]( schema: RootSchemaBuilder[R] ): Either[ValidationError, RootSchema[R]] = schema.query match { @@ -427,7 +422,7 @@ private[caliban] trait SchemaValidator { ) } - private[caliban] def validateRootMutation[R](schema: RootSchemaBuilder[R]): Either[ValidationError, Unit] = + private def validateRootMutation[R](schema: RootSchemaBuilder[R]): Either[ValidationError, Unit] = schema.mutation match { case Some(mutation) if mutation.opType.kind != __TypeKind.OBJECT => failValidation( @@ -437,7 +432,7 @@ private[caliban] trait SchemaValidator { case _ => unit } - private[caliban] def validateRootSubscription[R](schema: RootSchemaBuilder[R]): Either[ValidationError, Unit] = + private def validateRootSubscription[R](schema: RootSchemaBuilder[R]): Either[ValidationError, Unit] = schema.subscription match { case Some(subscription) if subscription.opType.kind != __TypeKind.OBJECT => failValidation( diff --git a/core/src/main/scala/caliban/validation/Validator.scala b/core/src/main/scala/caliban/validation/Validator.scala index 2d472e9b2..b19334c14 100644 --- a/core/src/main/scala/caliban/validation/Validator.scala +++ b/core/src/main/scala/caliban/validation/Validator.scala @@ -25,7 +25,7 @@ import scala.collection.compat._ import scala.collection.mutable import scala.collection.mutable.ListBuffer -object Validator extends SchemaValidator { +object Validator { import ValidationOps._ import caliban.syntax._ diff --git a/core/src/test/scala/caliban/validation/ValidationSchemaSpec.scala b/core/src/test/scala/caliban/validation/ValidationSchemaSpec.scala index 5ae7c907c..2a8154296 100644 --- a/core/src/test/scala/caliban/validation/ValidationSchemaSpec.scala +++ b/core/src/test/scala/caliban/validation/ValidationSchemaSpec.scala @@ -21,7 +21,7 @@ object ValidationSchemaSpec extends ZIOSpecDefault { def checkTypeError(t: __Type, expectedMessage: String): IO[ValidationError, TestResult] = ZIO - .fromEither(Validator.validateType(t)) + .fromEither(SchemaValidator.validateType(t)) .exit .map(assert(_)(fails(hasField("msg", _.msg, equalTo(expectedMessage))))) @@ -29,7 +29,7 @@ object ValidationSchemaSpec extends ZIOSpecDefault { suite("ValidationSchemaSpec")( suite("Enum")( test("non-empty enum is ok") { - val res = Validator.validateType( + val res = SchemaValidator.validateType( Types.makeEnum( name = Some("nonEmptyEnum"), description = None, @@ -54,7 +54,7 @@ object ValidationSchemaSpec extends ZIOSpecDefault { ), suite("Union")( test("union containing object types is ok") { - val res = Validator.validateType( + val res = SchemaValidator.validateType( Types.makeUnion( name = Some("GoodUnion"), description = None, @@ -298,7 +298,7 @@ object ValidationSchemaSpec extends ZIOSpecDefault { graphQL(resolverFieldWithArg).interpreter.exit.map(assert(_)(succeeds(anything))) }, test("fields with additional nullable args are valid") { - assert(Validator.validateObject(nullableExtraArgsObject))(isRight) + assert(SchemaValidator.validateType(nullableExtraArgsObject))(isRight) }, test("fields with additional non-nullable args are invalid") { checkTypeError(