From 0c992026145724741b6a1f2965981fdaa0cd9ea1 Mon Sep 17 00:00:00 2001 From: kyri-petrou <67301607+kyri-petrou@users.noreply.github.com> Date: Mon, 15 Jan 2024 20:03:53 +1100 Subject: [PATCH] Ensure fields are unique after merge (#2077) * Ensure fields are unique after merge * Fix scala 2.12 compiling * Check for uniqueness within the Executor --- .../scala/caliban/execution/Executor.scala | 21 +-- .../main/scala/caliban/execution/Field.scala | 18 +++ .../schema/SchemaDerivationIssuesSpec.scala | 134 +++++++++++++++++- 3 files changed, 153 insertions(+), 20 deletions(-) diff --git a/core/src/main/scala/caliban/execution/Executor.scala b/core/src/main/scala/caliban/execution/Executor.scala index 5f37b253b..f49311b37 100644 --- a/core/src/main/scala/caliban/execution/Executor.scala +++ b/core/src/main/scala/caliban/execution/Executor.scala @@ -349,16 +349,6 @@ object Executor { ZIO.succeed(GraphQLResponse(NullValue, List(error))) private[caliban] def mergeFields(field: Field, typeName: String): List[Field] = { - def haveSameCondition(head: Field, tail: List[Field]): Boolean = { - val condition = head._condition - var remaining = tail - while (remaining ne Nil) { - if (remaining.head._condition != condition) return false - remaining = remaining.tail - } - true - } - def matchesTypename(f: Field): Boolean = f._condition.isEmpty || f._condition.get.contains(typeName) @@ -380,12 +370,11 @@ object Executor { map.values().asScala.toList } - field.fields match { - // Shortcut if all the fields have the same condition, which means we don't need to merge as that's been handled in Field.apply - case h :: t if haveSameCondition(h, t) => if (matchesTypename(h)) field.fields else Nil - case Nil => Nil - case fields => mergeFields(fields) - } + val fields = field.fields + if (field.allFieldsUniqueNameAndCondition) { + if (fields.isEmpty || !matchesTypename(fields.head)) Nil + else fields + } else mergeFields(fields) } private def fieldInfo(field: Field, path: List[PathValue], fieldDirectives: List[Directive]): FieldInfo = diff --git a/core/src/main/scala/caliban/execution/Field.scala b/core/src/main/scala/caliban/execution/Field.scala index ffd2bb18d..0c7cde964 100644 --- a/core/src/main/scala/caliban/execution/Field.scala +++ b/core/src/main/scala/caliban/execution/Field.scala @@ -45,6 +45,24 @@ case class Field( private[caliban] val aliasedName: String = alias.getOrElse(name) + private[caliban] lazy val allFieldsUniqueNameAndCondition: Boolean = { + def inner: Boolean = { + val set = new mutable.HashSet[String] + val headCondition = fields.head._condition + val _ = set.add(fields.head.aliasedName) + + var remaining = fields.tail + while (remaining ne Nil) { + val f = remaining.head + val result = set.add(f.aliasedName) && f._condition == headCondition + if (!result) return false + remaining = remaining.tail + } + true + } + if (fields.isEmpty) true else inner + } + def combine(other: Field): Field = self.copy( fields = self.fields ::: other.fields, diff --git a/core/src/test/scala/caliban/schema/SchemaDerivationIssuesSpec.scala b/core/src/test/scala/caliban/schema/SchemaDerivationIssuesSpec.scala index 3f24cb38b..ad7827cc9 100644 --- a/core/src/test/scala/caliban/schema/SchemaDerivationIssuesSpec.scala +++ b/core/src/test/scala/caliban/schema/SchemaDerivationIssuesSpec.scala @@ -1,11 +1,10 @@ package caliban.schema +import caliban.Macros._ import caliban.schema.Annotations.{ GQLDescription, GQLInterface, GQLName, GQLUnion } -import zio.Chunk -import zio.test.ZIOSpecDefault -import zio.test._ import caliban.{ graphQL, RootResolver } -import caliban.Macros._ +import zio.test.{ ZIOSpecDefault, _ } +import zio.{ Chunk, ZIO } object SchemaDerivationIssuesSpec extends ZIOSpecDefault { def spec = suite("SchemaDerivationIssuesSpec")( @@ -211,6 +210,64 @@ object SchemaDerivationIssuesSpec extends ZIOSpecDefault { res <- i.execute(gqldoc("{ e1 { b } e2 { b } e3 { b } }")) data = res.data.toString } yield assertTrue(data == """{"e1":{"b":"b"},"e2":{"b":"b"},"e3":{"b":"b"}}""") + }, + test("i2076") { + import i2076._ + val queryFragments = + gqldoc(""" + query { + widget { + __typename + ...Widgets + } + } + + fragment Widgets on Widget { + ... on WidgetA { + name + children { + total + nodes { name } + } + } + ... on WidgetB { + name + children { + total + nodes { name foo } + } + } + } + """) + + val queryInlined = gqldoc(""" + query { + widget { + __typename + ... on WidgetA { + name + children { + total + nodes { name } + } + } + ... on WidgetB { + name + children { + total + nodes { name foo } + } + } + } + } + """) + for { + i <- schema.interpreter + data1 <- i.execute(queryFragments).map(_.data.toString) + data2 <- i.execute(queryInlined).map(_.data.toString) + expected = + """{"widget":{"__typename":"WidgetB","name":"a","children":{"total":1,"nodes":[{"name":"a","foo":"FOO"}]}}}""" + } yield assertTrue(data1 == expected, data2 == expected) } ) } @@ -490,3 +547,72 @@ object NestedInterfaceIssue { caliban.graphQL(RootResolver(queries)) } } + +object i2076 { + sealed trait Widget + object Widget { + implicit val schema: Schema[Any, Widget] = Schema.gen + + case class Args(limit: Option[Int]) + object Args { + implicit val schema: Schema[Any, Args] = Schema.gen + implicit val argBuilder: ArgBuilder[Args] = ArgBuilder.gen + } + + @GQLName("WidgetA") + case class A( + name: String, + children: Args => ZIO[Any, Throwable, A.Connection] + ) extends Widget + object A { + implicit val schema: Schema[Any, A] = Schema.gen + + @GQLName("WidgetAConnection") + case class Connection(total: Int, nodes: Chunk[Child]) + object Connection { + implicit val schema: Schema[Any, Connection] = Schema.gen + } + + @GQLName("WidgetAChild") + case class Child(name: String, foo: String) + object Child { + implicit val schema: Schema[Any, Child] = Schema.gen + } + } + + @GQLName("WidgetB") + case class B( + name: String, + children: Args => ZIO[Any, Throwable, B.Connection] + ) extends Widget + object B { + implicit val schema: Schema[Any, B] = Schema.gen + + @GQLName("WidgetBConnection") + case class Connection(total: Int, nodes: Chunk[Child]) + object Connection { + implicit val schema: Schema[Any, Connection] = Schema.gen + } + + @GQLName("WidgetBChild") + case class Child(name: String, foo: String) + object Child { + implicit val schema: Schema[Any, Child] = Schema.gen + } + } + } + + case class Queries( + widget: Option[Widget] + ) + object Queries { + implicit val schema: Schema[Any, Queries] = Schema.gen + } + + val schema = { + val queries = Queries( + Some(Widget.B("a", _ => ZIO.succeed(Widget.B.Connection(1, Chunk(Widget.B.Child("a", "FOO")))))) + ) + graphQL(RootResolver(queries)) + } +}