Add a MappedAbstractType that will transform the input value
#401
Conversation
- Since Scala 2.x does not have union types, one can represent them a number of different ways, a common way is to use a boxed type like Either, or Coproduct from various libraries. To use these boxed types is impossible in Sangria because it basically expects the type to unboxed, and inheriting from `Any` is the only way to achieve that. - This introduces a concept to map a type `T` to Any such that you can unbox after the definition, and get a value what the native type defs expect.
| @@ -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) | |||
There was a problem hiding this comment.
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)
}
}There was a problem hiding this comment.
Yeah definitely better, and good point about after isUndefined...
| @@ -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] { | |||
| override def contraMap(value: T): Any = func(value) | |||
| }.asInstanceOf[OutputType[T] with MappedAbstractType[T]] | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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] { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
- Also fix some code reuse with AbstractTypes
|
Thoughts on what you want to do about overriding UnionType? I could also potentially type alias |
|
I think it's fine for now. I like the idea with a type alias, but I think I would rather prefer to save for later as a migration strategy in case we add a support for scala 3 union types in future. In the current for it will affects only users who explicitly use Thanks for working on it 👍 |
number of different ways, a common way is to use a boxed type like Either,
or Coproduct from various libraries. To use these boxed types is impossible
in Sangria because it basically expects the type to unboxed, and
inheriting from
Anyis the only way to achieve that.Tto Any such that you can unboxafter the definition, and get a value what the native type defs expect.