Merge pull request #1169 from jchyb/fix-issue-906

Fix issue with wrong Action[Ctx, _] conversion being summoned
sangria-graphql · Jan 14, 2025 · da8bb1e · da8bb1e
2 parents c659711 + 0463c69
commit da8bb1e
Show file tree

Hide file tree

Showing 5 changed files with 128 additions and 58 deletions.
diff --git a/build.sbt b/build.sbt
@@ -254,6 +254,11 @@ lazy val derivation = project
     name := "sangria-derivation",
     Test / testOptions += Tests.Argument(TestFrameworks.ScalaTest, "-oF"),
     mimaPreviousArtifacts := Set("org.sangria-graphql" %% "sangria-derivation" % "4.0.0"),
+    mimaBinaryIssueFilters ++= Seq(
+      // internal method
+      ProblemFilters.exclude[DirectMissingMethodProblem](
+        "sangria.macros.derive.DeriveMacroSupport.unsafeSelectByName")
+    ),
     // Macros
     libraryDependencies ++= (if (isScala3.value) Seq.empty
                              else Seq("org.scala-lang" % "scala-reflect" % scalaVersion.value)),

diff --git a/modules/derivation/src/main/scala-3/sangria/macros/derive/DeriveMacroSupport.scala b/modules/derivation/src/main/scala-3/sangria/macros/derive/DeriveMacroSupport.scala
@@ -5,7 +5,7 @@ import sangria.schema.{InputType, OutputType}
 import scala.quoted._
 import scala.reflect.ClassTag
 
-trait DeriveMacroSupport {
+private[derive] trait DeriveMacroSupport {
 
   def reportErrors(using Quotes)(errors: Seq[(PositionPointer, String)]) = {
     import quotes.reflect.report
@@ -109,11 +109,12 @@ trait DeriveMacroSupport {
     }
   }
 
-  protected def unsafeSelectByName[S](using quotes: Quotes)(using
-      Type[S])(memberExpr: Expr[_], name: String): Expr[S] = {
+  protected def unsafeSelectByName[T: Type](using quotes: Quotes)(
+      memberExpr: Expr[_],
+      name: String
+  ): Expr[T] = {
     import quotes.reflect._
-    val a = Select.unique(memberExpr.asTerm, name)
-    a.etaExpand(Symbol.spliceOwner).asExprOf[S]
+    Select.unique(memberExpr.asTerm, name).asExprOf[T]
   }
 
   protected def getClassTag[T](using Type[T], Quotes): Expr[ClassTag[T]] = {

diff --git a/modules/derivation/src/main/scala-3/sangria/macros/derive/DeriveObjectTypeMacro.scala b/modules/derivation/src/main/scala-3/sangria/macros/derive/DeriveObjectTypeMacro.scala
@@ -129,34 +129,13 @@ class DeriveObjectTypeMacro(using globalQuotes: Quotes) extends DeriveMacroSuppo
                         ctxValType match {
                           case Some((tpe, fn)) =>
                             given Type[CtxVal] = tpe
-                            Expr.summon[t => Action[Ctx, t]] match
-                              case Some(conversion) =>
-                                '{
-                                  $conversion(${
-                                    unsafeSelectByName[t]('{ $fn(c.ctx) }, field.method.name)
-                                  })
-                                }
-                              case None =>
-                                reportSummoningErrors(
-                                  Seq(
-                                    s"Implicit conversion not found: ${TypeRepr.of[t => Action[Ctx, t]].show}"),
-                                  Seq(None)
-                                )
+                            wrappedInActionConversion[Ctx, t](
+                              unsafeSelectByName[t]('{ $fn(c.ctx) }, field.method.name)
+                            )
                           case None =>
-                            Expr.summon[t => Action[Ctx, t]] match
-                              case Some(conversion) =>
-                                '{
-                                  $conversion(${
-                                    unsafeSelectByName[t]('{ c.value }, field.method.name)
-                                      .asExprOf[t]
-                                  })
-                                }
-                              case None =>
-                                reportSummoningErrors(
-                                  Seq(
-                                    s"Implicit conversion not found: ${TypeRepr.of[t => Action[Ctx, t]].show}"),
-                                  Seq(None)
-                                )
+                            wrappedInActionConversion[Ctx, t](
+                              unsafeSelectByName[t]('{ c.value }, field.method.name)
+                            )
                         }
                       }
                   }
@@ -181,7 +160,7 @@ class DeriveObjectTypeMacro(using globalQuotes: Quotes) extends DeriveMacroSuppo
                   case Some(lookup) => '{ $lookup.graphqlType }
                   case None =>
                     reportSummoningErrors(
-                      Seq(s"GraphQlOutputType not found: ${TypeRepr.of[t => Action[Ctx, t]].show}"),
+                      Seq(s"GraphQlOutputType not found: ${TypeRepr.of[t => Action[Ctx, _]].show}"),
                       Seq(None)
                     )
           }
@@ -319,40 +298,34 @@ class DeriveObjectTypeMacro(using globalQuotes: Quotes) extends DeriveMacroSuppo
               case d: DefDef =>
                 d.returnTpt.tpe.asType match
                   case '[t] =>
-                    Expr.summon[t => Action[Ctx, t]] match
-                      case Some(conversion) =>
-                        '{
-                          $conversion(${
-                            args
-                              .foldLeft[Select | Apply](method) { (m, argList) =>
-                                Apply(m, argList)
-                              }
-                              .asExprOf[t]
-                          })
+                    wrappedInActionConversion[Ctx, t](
+                      args
+                        .foldLeft[Select | Apply](method) { (m, argList) =>
+                          Apply(m, argList)
                         }
-                      case None =>
-                        reportSummoningErrors(
-                          Seq(
-                            s"Implicit conversion not found: ${TypeRepr.of[t => Action[Ctx, t]].show}"),
-                          Seq(None)
-                        )
+                        .asExprOf[t]
+                    )
           else
             globalQuotes.reflect.Ref(member.method).tpe.widen.asType match {
               case '[t] =>
-                Expr.summon[t => Action[Ctx, t]] match
-                  case Some(conversion) =>
-                    '{ $conversion(${ method.asExprOf[t] }) }
-                  case None =>
-                    reportSummoningErrors(
-                      Seq(
-                        s"Implicit conversion not found: ${TypeRepr.of[t => Action[Ctx, t]].show}"),
-                      Seq(None)
-                    )
+                wrappedInActionConversion[Ctx, t](method.asExprOf[t])
             }
         }
       }
   }
 
+  private def wrappedInActionConversion[Ctx: Type, T: Type](value: Expr[T]) = {
+    import globalQuotes.reflect._
+    Expr.summon[T => Action[Ctx, _]] match
+      case Some(conversion) =>
+        '{ $conversion(${ value }) }
+      case None =>
+        reportSummoningErrors(
+          Seq(s"Implicit conversion not found: ${TypeRepr.of[T => Action[Ctx, _]].show}"),
+          Seq(None)
+        )
+  }
+
   private def createArg(config: Seq[MacroDeriveObjectSetting], member: KnownMember)(
       arg: globalQuotes.reflect.Symbol) =
     import globalQuotes.reflect._

diff --git a/modules/derivation/src/test/scala/sangria/macros/derive/DeriveObjectTypeMacroSpec.scala b/modules/derivation/src/test/scala/sangria/macros/derive/DeriveObjectTypeMacroSpec.scala
@@ -836,5 +836,32 @@ class DeriveObjectTypeMacroSpec extends AnyWordSpec with Matchers with FutureRes
 
       "deriveObjectType[Unit, TestContainer[TestSubject]]()" should compile
     }
+
+    "handle Future result values correctly (issue #906)" in {
+      import Issue906Context._
+      import sangria.parser.QueryParser
+
+      val query = QueryParser
+        .parse(
+          s"""
+             query SomeTest {
+                 mySample {
+                   myFutureString
+                 }
+             }
+            """
+        )
+        .fold(throw _, identity)
+
+      Executor.execute(MySample.schema, query, MyRepository.sample).await should be(
+        Map(
+          "data" -> Map(
+            "mySample" -> Map(
+              "myFutureString" -> "Hello World"
+            )
+          )
+        )
+      )
+    }
   }
 }
diff --git a/modules/derivation/src/test/scala/sangria/macros/derive/Issue906Context.scala b/modules/derivation/src/test/scala/sangria/macros/derive/Issue906Context.scala
@@ -0,0 +1,64 @@
+package sangria.macros.derive
+
+import sangria.schema._
+
+import scala.concurrent.Future
+
+object Issue906Context {
+
+  @GraphQLName("SampleCaseClass")
+  @GraphQLDescription("A sample case class")
+  final case class SampleCaseClass(
+      name: String
+  ) {
+    @GraphQLField
+    @GraphQLName("myString")
+    def graphqlMyString: String = "Hello World"
+
+    @GraphQLField
+    @GraphQLName("myFutureString")
+    def graphqlMyFutureString: Future[String] = Future.successful("Hello World")
+
+  }
+
+  object SampleCaseClass {
+    val sample: SampleCaseClass = SampleCaseClass("My test")
+  }
+
+  trait MyRepository {
+    def getSampleCaseClass: SampleCaseClass
+  }
+
+  object MyRepository {
+    def sample: MyRepository = new MyRepository {
+      override def getSampleCaseClass: SampleCaseClass = SampleCaseClass.sample
+    }
+  }
+
+  object MySample {
+    val schema: Schema[MyRepository, Unit] = {
+
+      implicit val graphqlSampleCaseClass: ObjectType[MyRepository, SampleCaseClass] =
+        deriveObjectType[MyRepository, SampleCaseClass]()
+
+      val queries: ObjectType[MyRepository, Unit] = ObjectType(
+        "Query",
+        fields(
+          Field(
+            "mySample",
+            graphqlSampleCaseClass,
+            Some("test"),
+            arguments = Nil,
+            resolve = (ctx: Context[MyRepository, Unit]) => SampleCaseClass.sample
+          )
+        )
+      )
+
+      Schema(
+        query = queries,
+        mutation = None,
+        additionalTypes = Nil
+      )
+    }
+  }
+}