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

Implement ObjectStep using a Function1 #2031

Merged
merged 3 commits into from
Dec 9, 2023
Merged

Implement ObjectStep using a Function1 #2031

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
88a77fe
Implement ObjectStep using a Function1
kyri-petrou Dec 8, 2023
faebd9c
Add collection compat import for Scala 2.12
kyri-petrou Dec 8, 2023
d9ebb32
Resolve unknown fields to `NullStep`
kyri-petrou Dec 9, 2023
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
2 changes: 1 addition & 1 deletion core/src/main/scala-2/caliban/schema/SchemaDerivation.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ trait CommonSchemaDerivation[R] {

def join[T](ctx: ReadOnlyCaseClass[Typeclass, T]): Typeclass[T] = new Typeclass[T] {
private lazy val objectResolver =
new ObjectFieldResolver[R, T](
ObjectFieldResolver[R, T](
getName(ctx),
ctx.parameters.map { p =>
getName(p) -> { (v: T) => p.typeclass.resolve(p.dereference(v)) }
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/scala-3/caliban/schema/ObjectSchema.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ final private class ObjectSchema[R, A](
def fs = fields.map { (name, _, schema, i) =>
name -> { (v: A) => schema.resolve(v.asInstanceOf[Product].productElement(i)) }
}
new ObjectFieldResolver(getName(anns, info), fs)
ObjectFieldResolver(getName(anns, info), fs)
}

def toType(isInput: Boolean, isSubscription: Boolean): __Type = {
Expand Down
4 changes: 2 additions & 2 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 @@ -59,13 +59,13 @@ object Executor {
path: List[Either[String, Int]]
): ReducedStep[R] = {

def reduceObjectStep(objectName: String, fields: collection.Map[String, Step[R]]): ReducedStep[R] = {
def reduceObjectStep(objectName: String, getFieldStep: String => Option[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 field = fields.get(name) match {
val field = getFieldStep(name) match {
case Some(step) => reduceStep(step, f, args, Left(f.aliasedName) :: path)
case _ => NullStep
}
Expand Down
2 changes: 0 additions & 2 deletions core/src/main/scala/caliban/execution/Field.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,6 @@ case class Field(

private[caliban] val aliasedName: String = alias.getOrElse(name)

private[caliban] lazy val distinctFieldNames: List[String] = fields.map(_.name).distinct

def combine(other: Field): Field =
self.copy(
fields = self.fields ::: other.fields,
Expand Down
45 changes: 17 additions & 28 deletions core/src/main/scala/caliban/schema/ObjectFieldResolver.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,25 @@
package caliban.schema

import caliban.execution.Field
import caliban.schema.Step.{ MetadataFunctionStep, ObjectStep }
import caliban.schema.Step.ObjectStep

import scala.collection.compat._
import scala.collection.mutable

final private class ObjectFieldResolver[R, A](
objectName: String,
fields: Iterable[(String, A => Step[R])]
final private class ObjectFieldResolver[R, A] private (
name: String,
fields: mutable.HashMap[String, A => Step[R]]
) {

private val fieldsMap: java.util.HashMap[String, A => Step[R]] = {
val map = new java.util.HashMap[String, A => Step[R]]()
fields.foreach { case (name, resolve) => map.put(name, resolve) }
map
}

def resolve(value: A): Step[R] = MetadataFunctionStep(resolveForField(value, _))

private def resolveForField(
value: A,
field: Field
): Step[R] = {
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 ne null) fieldsBuilder.update(name, resolve(value))
remaining = remaining.tail
def resolve(value: A): Step[R] = ObjectStep(
name,
fields.get(_) match {
case Some(f) => Some(f(value))
case None => None
}
ObjectStep(objectName, fieldsBuilder)
}
)
}

private object ObjectFieldResolver {
def apply[R, A](objectName: String, fields: Iterable[(String, A => Step[R])]): ObjectFieldResolver[R, A] =
// NOTE: mutable.HashMap is about twice as fast than immutable.HashMap for .get
new ObjectFieldResolver(objectName, mutable.HashMap.from(fields))
}
6 changes: 4 additions & 2 deletions core/src/main/scala/caliban/schema/Schema.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,8 @@ trait GenericSchema[R] extends SchemaDerivation[R] with TemporalSchema {
directives: List[Directive] = List.empty
): Schema[R1, A] =
new Schema[R1, A] {
override def toType(isInput: Boolean, isSubscription: Boolean): __Type =
override def toType(isInput: Boolean, isSubscription: Boolean): __Type = {
val _ = resolver
ghostdogpr marked this conversation as resolved.
Show resolved Hide resolved
if (isInput) {
makeInputObject(
Some(customizeInputTypeName(name)),
Expand All @@ -187,9 +188,10 @@ trait GenericSchema[R] extends SchemaDerivation[R] with TemporalSchema {
directives = Some(directives)
)
} else makeObject(Some(name), description, fields(isInput, isSubscription).map(_._1), directives)
}

private lazy val resolver =
new ObjectFieldResolver[R1, A](name, fields(false, false).map(f => (f._1.name, f._2)))
ObjectFieldResolver[R1, A](name, fields(false, false).map(f => (f._1.name, f._2)))

override def resolve(value: A): Step[R1] = resolver.resolve(value)
}
Expand Down
19 changes: 12 additions & 7 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 @@ -10,12 +10,16 @@ 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: 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]
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 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 ObjectStep[-R](name: String, fields: String => Option[Step[R]]) extends Step[R]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking we could potentially remove Option and return NullStep directly to save the allocation. But you won't be able to use orElse, even though it can be worked around. Any thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was thinking that as well but then hit the .orElse point. One way would be to check if the step is a NullValue but that could be a legit case of a field returning None.

Another would be using a PartialFunction instead, although I don't think it'd be as performant. I can give it a go

Copy link
Collaborator Author

@kyri-petrou kyri-petrou Dec 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah PartialFunction seems too messy. Returning NullStep and then merging fields as below works, but has the caveat I mentioned above

case (ObjectStep(name, fields1), ObjectStep(_, fields2)) =>
      ObjectStep(
        name,
        s =>
          fields2(s) match {
            case NullStep => fields1(s)
            case step     => step
          }
      )

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's okay since merging arbitrarily keeps one of them anyway (we're not supposed to have the same field twice in theory)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think you're right. The chances that this is a legit usecase are extremely low anyways. Changed fields to String => Step[R] instead

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Squeezing the performance to the last bit 😄

object ObjectStep {
def apply[R](name: String, fields: Map[String, Step[R]]): ObjectStep[R] = new ObjectStep[R](name, fields.get)
}

// 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 @@ -35,7 +39,8 @@ object Step {
case (FunctionStep(l), r) => FunctionStep(args => mergeRootSteps(l(args), r))
case (l, FunctionStep(r)) => FunctionStep(args => mergeRootSteps(l, r(args)))
// fields2 override fields1 in case of conflict
case (ObjectStep(name, fields1), ObjectStep(_, fields2)) => ObjectStep(name, fields1 ++ fields2)
case (ObjectStep(name, fields1), ObjectStep(_, fields2)) =>
ObjectStep(name, (s: String) => fields2(s).orElse(fields1(s)))
// if only step1 is an object, keep it
case (ObjectStep(_, _), _) => step1
// otherwise keep step2
Expand Down
26 changes: 11 additions & 15 deletions interop/cats/src/test/scala/caliban/interop/fs2/Fs2InteropSchemaSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@ import caliban.introspection.adt.{ __Type, __TypeKind }
import caliban.parsing.adt.Definition.TypeSystemDefinition.TypeDefinition
import caliban.parsing.adt.Definition.TypeSystemDefinition.TypeDefinition.{ FieldDefinition, ObjectTypeDefinition }
import caliban.parsing.adt.Type._
import caliban.schema.Schema.auto._
import caliban.schema.Step.{ MetadataFunctionStep, ObjectStep, StreamStep }
import caliban.schema.{ PureStep, Schema, Step }
import cats.effect.std.Dispatcher
import cats.effect.unsafe.implicits._
import cats.effect.{ IO, LiftIO, Resource }
import cats.effect.{ IO, LiftIO }
import fs2.Stream
import zio.interop.catz._
import zio.test.Assertion._
Expand Down Expand Up @@ -120,19 +119,16 @@ object Fs2InteropSchemaSpec extends ZIOSpecDefault {
isSubtype[ObjectStep[Any]](
hasField(
"fields",
_.fields.toMap,
hasKey(
"bar",
isSubtype[StreamStep[Any]](
hasField(
"inner",
step =>
Unsafe.unsafe { implicit unsafe =>
runtime.unsafe.run(step.inner.runCollect)
},
isSubtype[Exit.Success[Chunk[Step[Any]]]](
equalTo(Exit.Success(expectedChunk))
)
_.fields("bar").get,
isSubtype[StreamStep[Any]](
hasField(
"inner",
step =>
Unsafe.unsafe { implicit unsafe =>
runtime.unsafe.run(step.inner.runCollect)
},
isSubtype[Exit.Success[Chunk[Step[Any]]]](
equalTo(Exit.Success(expectedChunk))
)
)
)
Expand Down