From 72f0b181947cdbd4d9c6ec1a80f9482566681d87 Mon Sep 17 00:00:00 2001 From: Kyri Petrou Date: Wed, 6 Dec 2023 11:41:37 +0100 Subject: [PATCH 1/3] Use a mutable map in ObjectFieldResolver and reduce branches in object step reduction --- build.sbt | 6 +++- .../scala/caliban/execution/Executor.scala | 30 ++++++++++--------- .../caliban/schema/ObjectFieldResolver.scala | 12 ++++---- core/src/main/scala/caliban/schema/Step.scala | 14 ++++----- 4 files changed, 34 insertions(+), 28 deletions(-) diff --git a/build.sbt b/build.sbt index 759a7d6b21..36a027692f 100644 --- a/build.sbt +++ b/build.sbt @@ -1,3 +1,4 @@ +import com.typesafe.tools.mima.core.{ IncompatibleMethTypeProblem, IncompatibleResultTypeProblem, ProblemFilters } import org.scalajs.linker.interface.ModuleSplitStyle import sbtcrossproject.CrossPlugin.autoImport.{ crossProject, CrossType } @@ -671,7 +672,10 @@ lazy val enableMimaSettingsJVM = Def.settings( mimaFailOnProblem := enforceMimaCompatibility, mimaPreviousArtifacts := previousStableVersion.value.map(organization.value %% moduleName.value % _).toSet, - mimaBinaryIssueFilters ++= Seq() + mimaBinaryIssueFilters ++= Seq( + ProblemFilters.exclude[IncompatibleMethTypeProblem]("caliban.schema.Step#ObjectStep*"), + ProblemFilters.exclude[IncompatibleResultTypeProblem]("caliban.schema.Step#ObjectStep*") + ) ) lazy val enableMimaSettingsJS = diff --git a/core/src/main/scala/caliban/execution/Executor.scala b/core/src/main/scala/caliban/execution/Executor.scala index b36b19149c..dfe69d74cb 100644 --- a/core/src/main/scala/caliban/execution/Executor.scala +++ b/core/src/main/scala/caliban/execution/Executor.scala @@ -33,7 +33,9 @@ object Executor { queryExecution: QueryExecution = QueryExecution.Parallel, featureSet: Set[Feature] = Set.empty )(implicit trace: Trace): URIO[R, GraphQLResponse[CalibanError]] = { - val wrapPureValues = fieldWrappers.exists(_.wrapPureValues) + val wrapPureValues = fieldWrappers.exists(_.wrapPureValues) + val isDeferredEnabled = featureSet(Feature.Defer) + type ExecutionQuery[+A] = ZQuery[R, ExecutionError, A] val execution = request.operationType match { @@ -57,32 +59,32 @@ object Executor { path: List[Either[String, Int]] ): ReducedStep[R] = { - def reduceObjectStep(objectName: String, fields: Map[String, Step[R]]) = { + def reduceObjectStep(objectName: String, fields: collection.Map[String, Step[R]]): ReducedStep[R] = { val filteredFields = mergeFields(currentField, objectName) val (deferred, eager) = filteredFields.partitionMap { case f @ Field("__typename", _, _, _, _, _, _, directives, _, _, _) => Right((f.aliasedName, PureStep(StringValue(objectName)), fieldInfo(f, path, directives))) case f @ Field(name, _, _, _, _, _, args, directives, _, _, fragment) => - val aliasedName = f.aliasedName - val field = fields - .get(name) - .fold(NullStep: ReducedStep[R])(reduceStep(_, f, args, Left(aliasedName) :: path)) - - val info = fieldInfo(f, path, directives) + val field = fields.get(name) match { + case Some(step) => reduceStep(step, f, args, Left(f.aliasedName) :: path) + case _ => NullStep + } + val entry = (f.aliasedName, field, fieldInfo(f, path, directives)) - fragment.collectFirst { + fragment match { // The defer spec provides some latitude on how we handle responses. Since it is more performant to return // pure fields rather than spin up the defer machinery we return pure fields immediately to the caller. - case IsDeferred(label) if featureSet(Feature.Defer) && !field.isPure => - (label, (aliasedName, field, info)) - }.toLeft((aliasedName, field, info)) + case Some(IsDeferred(label)) if isDeferredEnabled && !field.isPure => Left(label, entry) + case _ => Right(entry) + } } + val eagerReduced = reduceObject(eager, wrapPureValues) deferred match { - case Nil => reduceObject(eager, wrapPureValues) + case Nil => eagerReduced case d => DeferStep( - reduceObject(eager, wrapPureValues), + eagerReduced, d.groupBy(_._1).toList.map { case (label, labelAndFields) => val (_, fields) = labelAndFields.unzip reduceObject(fields, wrapPureValues) -> label diff --git a/core/src/main/scala/caliban/schema/ObjectFieldResolver.scala b/core/src/main/scala/caliban/schema/ObjectFieldResolver.scala index 1aa5de7622..18057dbfe4 100644 --- a/core/src/main/scala/caliban/schema/ObjectFieldResolver.scala +++ b/core/src/main/scala/caliban/schema/ObjectFieldResolver.scala @@ -3,7 +3,7 @@ package caliban.schema import caliban.execution.Field import caliban.schema.Step.{ MetadataFunctionStep, ObjectStep } -import scala.collection.immutable.HashMap +import scala.collection.mutable final private class ObjectFieldResolver[R, A]( objectName: String, @@ -22,15 +22,15 @@ final private class ObjectFieldResolver[R, A]( value: A, field: Field ): Step[R] = { - val fieldsBuilder = HashMap.newBuilder[String, Step[R]] - var remaining = field.distinctFieldNames + val fieldsBuilder = new mutable.HashMap[String, Step[R]]() + + var remaining = field.distinctFieldNames while (!remaining.isEmpty) { val name = remaining.head val resolve = fieldsMap.get(name) - if (resolve eq null) () - else fieldsBuilder += name -> resolve(value) + if (resolve ne null) fieldsBuilder.update(name, resolve(value)) remaining = remaining.tail } - ObjectStep(objectName, fieldsBuilder.result()) + ObjectStep(objectName, fieldsBuilder) } } diff --git a/core/src/main/scala/caliban/schema/Step.scala b/core/src/main/scala/caliban/schema/Step.scala index ef0304799d..aa5cbbef15 100644 --- a/core/src/main/scala/caliban/schema/Step.scala +++ b/core/src/main/scala/caliban/schema/Step.scala @@ -4,18 +4,18 @@ import caliban.CalibanError.ExecutionError import caliban.Value.NullValue import caliban.execution.{ Field, FieldInfo } import caliban.{ InputValue, ResponseValue } -import zio.stream.ZStream import zio.query.ZQuery +import zio.stream.ZStream sealed trait Step[-R] object Step { - case class ListStep[-R](steps: List[Step[R]]) extends Step[R] - case class FunctionStep[-R](step: Map[String, InputValue] => Step[R]) extends Step[R] - case class MetadataFunctionStep[-R](step: Field => Step[R]) extends Step[R] - case class ObjectStep[-R](name: String, fields: Map[String, Step[R]]) extends Step[R] - case class QueryStep[-R](query: ZQuery[R, Throwable, Step[R]]) extends Step[R] - case class StreamStep[-R](inner: ZStream[R, Throwable, Step[R]]) extends Step[R] + case class ListStep[-R](steps: List[Step[R]]) extends Step[R] + case class FunctionStep[-R](step: Map[String, InputValue] => Step[R]) extends Step[R] + case class MetadataFunctionStep[-R](step: Field => Step[R]) extends Step[R] + case class ObjectStep[-R](name: String, fields: collection.Map[String, Step[R]]) extends Step[R] + case class QueryStep[-R](query: ZQuery[R, Throwable, Step[R]]) extends Step[R] + case class StreamStep[-R](inner: ZStream[R, Throwable, Step[R]]) extends Step[R] // PureStep is both a Step and a ReducedStep so it is defined outside this object // This is to avoid boxing/unboxing pure values during step reduction From fbd66d2aabcac5bdff2e6ae131b65997021aa9d3 Mon Sep 17 00:00:00 2001 From: Kyri Petrou Date: Wed, 6 Dec 2023 11:49:34 +0100 Subject: [PATCH 2/3] Fix test --- .../test/scala/caliban/interop/fs2/Fs2InteropSchemaSpec.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/interop/cats/src/test/scala/caliban/interop/fs2/Fs2InteropSchemaSpec.scala b/interop/cats/src/test/scala/caliban/interop/fs2/Fs2InteropSchemaSpec.scala index 2ee59483b4..1d0f708e79 100644 --- a/interop/cats/src/test/scala/caliban/interop/fs2/Fs2InteropSchemaSpec.scala +++ b/interop/cats/src/test/scala/caliban/interop/fs2/Fs2InteropSchemaSpec.scala @@ -120,7 +120,7 @@ object Fs2InteropSchemaSpec extends ZIOSpecDefault { isSubtype[ObjectStep[Any]]( hasField( "fields", - _.fields, + _.fields.toMap, hasKey( "bar", isSubtype[StreamStep[Any]]( From f1fa6b59257de65b751acc8236b6334688560fec Mon Sep 17 00:00:00 2001 From: Kyri Petrou Date: Wed, 6 Dec 2023 12:06:02 +0100 Subject: [PATCH 3/3] Fix Scala 2.12 compiling --- core/src/main/scala/caliban/execution/Executor.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/scala/caliban/execution/Executor.scala b/core/src/main/scala/caliban/execution/Executor.scala index dfe69d74cb..94deb78b80 100644 --- a/core/src/main/scala/caliban/execution/Executor.scala +++ b/core/src/main/scala/caliban/execution/Executor.scala @@ -74,7 +74,7 @@ object Executor { fragment match { // The defer spec provides some latitude on how we handle responses. Since it is more performant to return // pure fields rather than spin up the defer machinery we return pure fields immediately to the caller. - case Some(IsDeferred(label)) if isDeferredEnabled && !field.isPure => Left(label, entry) + case Some(IsDeferred(label)) if isDeferredEnabled && !field.isPure => Left((label, entry)) case _ => Right(entry) } }