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.

Sign up for GitHub

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 incorrect wrapping of pure values by some FieldWrappers #2162

Merged
merged 4 commits into from
Mar 15, 2024
Merged

Fix incorrect wrapping of pure values by some FieldWrappers #2162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
34afa57
Propagate whether fields are pure post-step reduction
kyri-petrou Mar 15, 2024
42f5554
Optimize
kyri-petrou Mar 15, 2024
d759119
Add comment back
kyri-petrou Mar 15, 2024
d4ee4c4
Remove argument from method
kyri-petrou Mar 15, 2024
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
50 changes: 26 additions & 24 deletions core/src/main/scala/caliban/execution/Executor.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,30 +159,33 @@ object Executor {
} else (Nil, filteredFields.map(reduceField))
}

val eagerReduced = reduceObject(eager, wrapPureValues)
val eagerReduced = reduceObject(eager)
deferred match {
case Nil => eagerReduced
case d =>
DeferStep(
eagerReduced,
d.groupBy(_._1).toList.map { case (label, labelAndFields) =>
val (_, fields) = labelAndFields.unzip
reduceObject(fields, wrapPureValues) -> label
reduceObject(fields) -> label
},
path
)
}
}

def reduceListStep(steps: List[Step[R]]): ReducedStep[R] = {
def reduceMixed(head: ReducedStep[R], remaining0: List[Step[R]]): ReducedStep[R] = {

def reduceToListStep(head: ReducedStep[R], remaining0: List[Step[R]]): ReducedStep[R] = {
var i = 1
val nil = Nil
val lb = ListBuffer.empty[ReducedStep[R]]
var isPure = wrapPureValues && head.isPure
lb addOne head
var remaining = remaining0
while (remaining ne nil) {
val step = reduceStep(remaining.head, currentField, arguments, PathValue.Index(i) :: path)
if (isPure && !step.isPure) isPure = false
lb addOne step
i += 1
remaining = remaining.tail
Expand All @@ -192,11 +195,12 @@ object Executor {
Types.listOf(currentField.fieldType) match {
case Some(tpe) => tpe.isNullable
case None => false
}
},
isPure
)
}

def reducePures(head: PureStep, remaining0: List[Step[R]]): ReducedStep[R] = {
def reduceToPureStep(head: PureStep, remaining0: List[Step[R]]): ReducedStep[R] = {
var i = 1
val nil = Nil
val lb = ListBuffer.empty[ResponseValue]
Expand All @@ -213,13 +217,14 @@ object Executor {

if (steps.isEmpty) PureStep(ListValue(Nil))
else {
val step = reduceStep(steps.head, currentField, arguments, PathValue.Index(0) :: path)
if (step.isPure) {
reduceStep(steps.head, currentField, arguments, PathValue.Index(0) :: path) match {
// In 99.99% of the cases, if the head is pure, all the other elements will be pure as well but we catch that error just in case
// NOTE: Our entire test suite passes without catching the error
try reducePures(step.asInstanceOf[PureStep], steps.tail)
catch { case _: ClassCastException => reduceMixed(step, steps.tail) }
} else reduceMixed(step, steps.tail)
case step: PureStep =>
try reduceToPureStep(step, steps.tail)
catch { case _: ClassCastException => reduceToListStep(step, steps.tail) }
case step => reduceToListStep(step, steps.tail)
}
}
}

Expand Down Expand Up @@ -322,23 +327,20 @@ object Executor {
): FieldInfo =
FieldInfo(aliasedName, field, path, fieldDirectives, field.parentType)

private def reduceObject(
items: List[(String, ReducedStep[R], FieldInfo)],
wrapPureValues: Boolean
): ReducedStep[R] = {
private def reduceObject(items: List[(String, ReducedStep[R], FieldInfo)]): ReducedStep[R] = {
var hasPures = false
var hasQueries = false
val nil = Nil
var hasQueries = wrapPureValues
var remaining = items
while ((remaining ne nil) && !(hasPures && hasQueries)) {
val isPure = remaining.head._2.isPure
if (!hasPures && isPure) hasPures = true
else if (!hasQueries && !isPure) hasQueries = true
if (isPure && !hasPures) hasPures = true
else if (!isPure && !hasQueries) hasQueries = true
else ()
remaining = remaining.tail
}

if (hasQueries) ReducedStep.ObjectStep(items, hasPures)
if (hasQueries || wrapPureValues) ReducedStep.ObjectStep(items, hasPures, !hasQueries)
else
PureStep(
ObjectValue(items.asInstanceOf[List[(String, PureStep, FieldInfo)]].map { case (k, v, _) => (k, v.value) })
Expand Down Expand Up @@ -475,11 +477,11 @@ object Executor {

def loop(step: ReducedStep[R], isTopLevelField: Boolean = false): ExecutionQuery[ResponseValue] =
step match {
case PureStep(value) => ZQuery.succeed(value)
case ReducedStep.QueryStep(step) => step.flatMap(loop(_))
case ReducedStep.ObjectStep(steps, hasPureFields) => makeObjectQuery(steps, hasPureFields, isTopLevelField)
case ReducedStep.ListStep(steps, areItemsNullable) => makeListQuery(steps, areItemsNullable)
case ReducedStep.StreamStep(stream) =>
case PureStep(value) => ZQuery.succeed(value)
case ReducedStep.QueryStep(step) => step.flatMap(loop(_))
case ReducedStep.ObjectStep(steps, hasPureFields, _) => makeObjectQuery(steps, hasPureFields, isTopLevelField)
case ReducedStep.ListStep(steps, areItemsNullable, _) => makeListQuery(steps, areItemsNullable)
case ReducedStep.StreamStep(stream) =>
ZQuery
.environmentWith[R](env =>
ResponseValue.StreamValue(
Expand All @@ -488,7 +490,7 @@ object Executor {
}.provideEnvironment(env)
)
)
case ReducedStep.DeferStep(obj, nextSteps, path) =>
case ReducedStep.DeferStep(obj, nextSteps, path) =>
val deferredSteps = nextSteps.map { case (step, label) =>
Deferred(path, step, label)
}
Expand Down
37 changes: 28 additions & 9 deletions core/src/main/scala/caliban/schema/Step.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,20 +78,37 @@ object Step {
}

sealed abstract class ReducedStep[-R] { self =>
final def isPure: Boolean = self.isInstanceOf[PureStep]
def isPure: Boolean
}

object ReducedStep {
case class ListStep[-R](steps: List[ReducedStep[R]], areItemsNullable: Boolean) extends ReducedStep[R]
case class ObjectStep[-R](fields: List[(String, ReducedStep[R], FieldInfo)], hasPureFields: Boolean)
extends ReducedStep[R]
case class QueryStep[-R](query: ZQuery[R, ExecutionError, ReducedStep[R]]) extends ReducedStep[R]
case class StreamStep[-R](inner: ZStream[R, ExecutionError, ReducedStep[R]]) extends ReducedStep[R]
case class DeferStep[-R](
final case class ListStep[-R](
steps: List[ReducedStep[R]],
areItemsNullable: Boolean,
isPure: Boolean
) extends ReducedStep[R]

final case class ObjectStep[-R](
fields: List[(String, ReducedStep[R], FieldInfo)],
hasPureFields: Boolean,
isPure: Boolean
) extends ReducedStep[R]

final case class QueryStep[-R](query: ZQuery[R, ExecutionError, ReducedStep[R]]) extends ReducedStep[R] {
final val isPure = false
}

final case class StreamStep[-R](inner: ZStream[R, ExecutionError, ReducedStep[R]]) extends ReducedStep[R] {
final val isPure = false
}

final case class DeferStep[-R](
obj: ReducedStep[R],
deferred: List[(ReducedStep[R], Option[String])],
path: List[PathValue]
) extends ReducedStep[R]
) extends ReducedStep[R] {
final val isPure = false
}

// 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
Expand All @@ -105,4 +122,6 @@ object ReducedStep {
*
* @param value the response value to return for that step
*/
case class PureStep(value: ResponseValue) extends ReducedStep[Any] with Step[Any]
final case class PureStep(value: ResponseValue) extends ReducedStep[Any] with Step[Any] {
final val isPure = true
}
30 changes: 30 additions & 0 deletions core/src/test/scala/caliban/wrappers/WrappersSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,36 @@ object WrappersSpec extends ZIOSpecDefault {
counter <- ref.get
} yield assertTrue(counter == 2)
},
// i2161
test("wrapPureValues true and false") {
case class Obj1(a1: List[Obj2])
case class Obj2(a2: Int)
case class Test(a0: Obj1, b: UIO[Int])
for {
ref1 <- Ref.make[Int](0)
wrapper1 = new FieldWrapper[Any](true) {
def wrap[R1 <: Any](
query: ZQuery[R1, ExecutionError, ResponseValue],
info: FieldInfo
): ZQuery[R1, ExecutionError, ResponseValue] =
ZQuery.fromZIO(ref1.update(_ + 1)) *> query
}
ref2 <- Ref.make[Int](0)
wrapper2 = new FieldWrapper[Any](false) {
def wrap[R1 <: Any](
query: ZQuery[R1, ExecutionError, ResponseValue],
info: FieldInfo
): ZQuery[R1, ExecutionError, ResponseValue] =
ZQuery.fromZIO(ref2.update(_ + 1)) *> query
}
interpreter <-
(graphQL(RootResolver(Test(Obj1(List(Obj2(1))), ZIO.succeed(2)))) @@ wrapper1 @@ wrapper2).interpreter.orDie
query = gqldoc("""{ a0 { a1 { a2 } } b }""")
_ <- interpreter.execute(query)
counter1 <- ref1.get
counter2 <- ref2.get
} yield assertTrue(counter1 == 4, counter2 == 1)
},
test("Max fields") {
case class A(b: B)
case class B(c: Int)
Expand Down