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.

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

Add a MappedAbstractType that will transform the input value #401

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
11 changes: 11 additions & 0 deletions src/main/scala/sangria/execution/Resolver.scala
Original file line number Diff line number Diff line change
Expand Up @@ -901,6 +901,17 @@ class Resolver[Ctx](
resolveActionsPar(path, obj, actions, userCtx, fields.namesOrdered)
case Failure(error) ⇒ Result(ErrorRegistry(path, error), None)
}
case abst: MappedAbstractType[Any @unchecked] ⇒
val newValue = abst.contraMap(value)
Copy link
Member

@OlegIlyenko OlegIlyenko Aug 28, 2018

Choose a reason for hiding this comment

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

I think it would be safer to extract value after isUndefinedValue to avoid accidental NPEs. It checks for a null (even though null is not supposed to be used, None should be used to represent it, but code still should handle it gracefully).

Maybe something like this? (this will also avoid duplication)

case abst: AbstractType 
  if (isUndefinedValue(value))
    Result(ErrorRegistry.empty, None)
  else {
    val actualValue =
      abst match {
        case mapped: MappedAbstractType[Any @unchecked]  mapped.contraMap(value) 
        case _  value
      }
    
    abst.typeOf(actualValue, schema) match {
      case Some(obj)  resolveValue(path, astFields, obj, field, actualValue, userCtx)
      case None  Result(ErrorRegistry(path,
        UndefinedConcreteTypeError(path, abst, schema.possibleTypes.getOrElse(abst.name, Vector.empty), actualValue, exceptionHandler, sourceMapper, astFields.head.location.toList)), None)
    }
  }

Copy link
Contributor Author

@dispalt dispalt Aug 28, 2018

Choose a reason for hiding this comment

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

Yeah definitely better, and good point about after isUndefined...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in f589aea

if (isUndefinedValue(value))
Result(ErrorRegistry.empty, None)
else
abst.typeOf(newValue, schema) match {
case Some(obj) ⇒ resolveValue(path, astFields, obj, field, newValue, userCtx)
case None ⇒ Result(ErrorRegistry(path,
UndefinedConcreteTypeError(path, abst, schema.possibleTypes.getOrElse(abst.name, Vector.empty), newValue, exceptionHandler, sourceMapper, astFields.head.location.toList)), None)
}

case abst: AbstractType ⇒
if (isUndefinedValue(value))
Result(ErrorRegistry.empty, None)
Expand Down
9 changes: 8 additions & 1 deletion src/main/scala/sangria/schema/Schema.scala
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ sealed trait AbstractType extends Type with Named {
schema.possibleTypes get name flatMap (_.find(_ isInstanceOf value).asInstanceOf[Option[ObjectType[Ctx, _]]])
}

sealed trait MappedAbstractType[T] extends Type with AbstractType with OutputType[T] {
def contraMap(value: T): Any
}

sealed trait NullableType
sealed trait UnmodifiedType

Expand Down Expand Up @@ -295,6 +299,9 @@ case class UnionType[Ctx](
astNodes: Vector[ast.AstNode] = Vector.empty) extends OutputType[Any] with CompositeType[Any] with AbstractType with NullableType with UnmodifiedType with HasAstInfo {
def rename(newName: String) = copy(name = newName).asInstanceOf[this.type]
def toAst: ast.TypeDefinition = SchemaRenderer.renderType(this)
def map[T](func: T => Any): OutputType[T] with MappedAbstractType[T] = new UnionType[Ctx](name, description, types, astDirectives, astNodes) with MappedAbstractType[T] {
Copy link
Member

Choose a reason for hiding this comment

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

I also a bit concerned about the name... I wonder whether "map" might be confusing for some people since its role here is a bit different from collections and other places where map often shows up.

But maybe it's fine, I'm not 100% sure. WDYT? Maybe something more specific (like mapValue) would better describe its purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mapValue is definitely better, since this stuff doesn't really follow laws or anything and especially since map here doesn't really change the type param of UnionType. Your call on it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in f589aea

override def contraMap(value: T): Any = func(value)
}.asInstanceOf[OutputType[T] with MappedAbstractType[T]]
Copy link
Member

@OlegIlyenko OlegIlyenko Aug 28, 2018

Choose a reason for hiding this comment

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

Not 100% sure how I feel about this part. On one hand, I can understand practical reasons behind it, but on the other hand I'm not a big fan of such structures especially for case classes.

Though it provides quite non intrusive way to enable this feature. I think this is nice, especially considering that it is not yet clear how scala 3 union types will affect the design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah so you nailed my thought process here, I did not way to break the UnionType contract. Honestly, I just wasn't sure what you'd think, so i'd be looking for your advice on this. I also dislike doing this to case classes.

}

case class Field[Ctx, Val](
Expand Down Expand Up @@ -760,7 +767,7 @@ case class Schema[Ctx, Val](

def renderPretty: String = toAst.renderPretty
def renderPretty(filter: SchemaFilter): String = toAst(filter).renderPretty

def renderCompact: String = toAst.renderCompact
def renderCompact(filter: SchemaFilter): String = toAst(filter).renderCompact

Expand Down
51 changes: 50 additions & 1 deletion src/test/scala/sangria/execution/UnionInterfaceSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@ class UnionInterfaceSpec extends WordSpec with Matchers with FutureResultSupport

case class Dog(name: Option[String], barks: Option[Boolean]) extends Named
case class Cat(name: Option[String], meows: Option[Boolean]) extends Named
case class Person(name: Option[String], pets: Option[List[Option[Any]]], friends: Option[List[Option[Named]]]) extends Named
case class Person(name: Option[String], pets: Option[List[Option[Any]]], friends: Option[List[Option[Named]]]) extends Named {
def eitherPets: Option[List[Option[Either[Dog, Cat]]]] = pets.map(_.map(_.map {
case d: Dog => Left(d)
case c: Cat => Right(c)
}))
}

val NamedType = InterfaceType("Named", fields[Unit, Named](
Field("name", OptionType(StringType), resolve = _.value.name)))
Expand All @@ -26,8 +31,11 @@ class UnionInterfaceSpec extends WordSpec with Matchers with FutureResultSupport

val PetType = UnionType[Unit]("Pet", types = DogType :: CatType :: Nil)

val Pet2Type = UnionType[Unit]("Pet2", types = DogType :: CatType :: Nil).map[Either[Dog, Cat]](_.fold(dog => dog: Any, cat => cat: Any))

val PersonType = ObjectType("Person", interfaces[Unit, Person](NamedType), fields[Unit, Person](
Field("pets", OptionType(ListType(OptionType(PetType))), resolve = _.value.pets),
Field("pets2", OptionType(ListType(OptionType(Pet2Type))), resolve = _.value.eitherPets),
Field("favouritePet", PetType, resolve = _.value.pets.flatMap(_.headOption.flatMap(identity)).get),
Field("favouritePetList", ListType(PetType), resolve = _.value.pets.getOrElse(Nil).flatMap(x ⇒ x).toSeq),
Field("favouritePetOpt", OptionType(PetType), resolve = _.value.pets.flatMap(_.headOption.flatMap(identity))),
Expand Down Expand Up @@ -131,6 +139,47 @@ class UnionInterfaceSpec extends WordSpec with Matchers with FutureResultSupport
validateQuery = false
)

"executes using mapped union types" in check(
bob,
"""
{
__typename
name
favouritePet {name}
favouritePetOpt {name}
pets {
__typename
name
barks
meows
}
pets2 {
__typename
name
barks
meows
}
}
""",
Map(
"data" → Map(
"__typename" → "Person",
"name" → "Bob",
"favouritePet" → Map("name" → "Garfield"),
"favouritePetOpt" → Map("name" → "Garfield"),
"pets" → List(
Map("__typename" → "Cat", "name" → "Garfield", "meows" → false),
Map("__typename" → "Dog", "name" → "Odie", "barks" → true)
),
"pets2" → List(
Map("__typename" → "Cat", "name" → "Garfield", "meows" → false),
Map("__typename" → "Dog", "name" → "Odie", "barks" → true)
)
)
) ,
validateQuery = false
)

"executes union types with inline fragments" in check(
bob,
"""
Expand Down