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

Context.arg Wraps Optional Arguments inside another Option #945

Closed
SeanWhoCodes opened this issue Dec 27, 2022 · 4 comments
Closed

Context.arg Wraps Optional Arguments inside another Option #945

SeanWhoCodes opened this issue Dec 27, 2022 · 4 comments

Comments

@SeanWhoCodes
Copy link
Contributor

SeanWhoCodes commented Dec 27, 2022

Overview

Given an argument MyArg of type Argument[Option[MyInputObject]], context.arg(MyArg) doesn't return a value of type Option[MyInputObject] within the 'resolve' function, but wraps the argument value inside another Option and returns Option[Option[MyInputObject]] instead.

Environment

This issue was verified in the following environment:

  • sangria: 3.4.1
  • sangria-circe: 1.3.2
  • Scala: 3.2.1
  • JDK: 17.0.5

Reproduction

Example Code

Below is an example that demonstrates the issue.

import io.circe.*
import sangria.ast.Document
import sangria.execution.Executor
import sangria.macros.*
import sangria.marshalling.*
import sangria.marshalling.circe.circeDecoderFromInput
import sangria.schema.*

import scala.concurrent.Await
import scala.concurrent.ExecutionContext.Implicits.global
import scala.concurrent.duration.DurationInt
import scala.util.{Failure, Success}

object NestedOptionArgIssue {
  case class EchoMessageInput(message: String)

  given Decoder[EchoMessageInput] = (cursor: HCursor) =>
    for {
      message <- cursor.get[String]("message")
    } yield EchoMessageInput(message)

  given FromInput[EchoMessageInput] = circeDecoderFromInput[EchoMessageInput]

  val EchoMessageInputObjectType: InputObjectType[EchoMessageInput] =
    InputObjectType[EchoMessageInput](
      name = "EchoMessageInput",
      fields = List(
        InputField(name = "message", fieldType = StringType),
      ),
    )

  val MaybeEchoMessageInputArg: Argument[Option[EchoMessageInput]] =
    Argument[Option[EchoMessageInput]](
      name = "maybeEchoMessageInput",
      argumentType = OptionInputType(EchoMessageInputObjectType),
    )

  class GraphQLContext {
    def echoMessage(maybeEchoMessageInput: Option[EchoMessageInput]): Option[String] =
      maybeEchoMessageInput.map(input => s"Echoing message: ${input.message}")
  }

  val QueryType: ObjectType[GraphQLContext, Unit] =
    ObjectType[GraphQLContext, Unit](
      name = "Query",
      description = "GraphQL queries.",
      fields = fields[GraphQLContext, Unit](
        Field(
          name = "echoMessage",
          fieldType = OptionType(StringType),
          arguments = MaybeEchoMessageInputArg +: Nil,
          resolve = context => {
            val maybeInput = context.arg(MaybeEchoMessageInputArg) // 👈 Due to some reason, 'Context.arg' wraps the argument inside another Option.
            System.err.println(s"Surprisingly 'maybeInput' has a nested type 'Option[Option[EchoMessageInput]]': ${maybeInput.toString}")

            val flattenedInput = maybeInput.asInstanceOf[Option[Option[EchoMessageInput]]].flatten // 👈 So the casting is necessary to flatten the nested type.
            context.ctx.echoMessage(flattenedInput)
          },
        ),
      ),
    )

  val schema: Schema[GraphQLContext, Unit] = Schema[GraphQLContext, Unit](query = QueryType)

  private val queryWithArg: Document =
    graphql"""
      query {
        echoMessage(maybeEchoMessageInput: { message: "Hello World!" })
      }
      """

  def main(args: Array[String]): Unit = {
    val execute = Executor.execute(schema, queryWithArg, new GraphQLContext)

    Await.ready(execute, 10.seconds).value match {
      case Some(Success(result))    => println(s"Execution Result: $result")
      case Some(Failure(throwable)) => System.err.println(s"Execution Error: ${throwable.getMessage}")
      case None                     => ???
    }
  }
}

Behaviours

The following line from the example above is supposed to be Option[EchoMessageInput], but the log shows that the actual value is Some(Some(EchoMessageInput(Hello World!))), indicating Context.arg somehow wraps the argument value inside another Option.

val maybeInput = context.arg(MaybeEchoMessageInputArg)

Is there any misconfiguration in my example above that causes this undesired behaviour? Thank you.

@yanns
Copy link
Contributor

yanns commented Jan 13, 2023

Thanks for the detailed report. Sorry I missed the notification somehow.
At first sight, I thought it was similar to #434
But I'm not totally sure.

Could you open a PR with a failing test in https://github.com/sangria-graphql/sangria/blob/main/modules/core/src/test/scala/sangria/schema/ArgsSpec.scala ?

SeanWhoCodes added a commit to SeanWhoCodes/sangria that referenced this issue Jan 20, 2023
- See sangria-graphql#945
- Unit tests added to demonstrate the possible issue with Args
  that wraps optional arguments inside another Option;
@SeanWhoCodes
Copy link
Contributor Author

@yanns - No worries and I also just came back from my holiday.

As suggested, I raised #952 to demonstrate the unexpected behaviours related to arbitrary input objects and Args. From what I can see, the tests indicate the following:

  • build with custom input argument: arguments of custom input types work fine as long as they are not optional;
  • build with optional custom input argument and defined input: optional arguments of custom input types wrap the optional results inside another Option (e.g. Some(Some(CustomInput("Hello Sangria"))));

The findings above match what I observed previously, although circeDecoderFromInput from sangria-circe was used in my original code snippet, but I'm using argonautDecoderFromInput from sangria-argonaut in the tests to be consistent with other existing ones.

@yanns
Copy link
Contributor

yanns commented Feb 7, 2023

Thanks a lot for the PR!

By using @@ InputObjectResult, I could make it work: 4d81bbf

Is it something that also fix your case?

@SeanWhoCodes
Copy link
Contributor Author

@yanns - Amazing! @@ InputObjectResult does fix the problem!

It makes sense to me now that we need the @@ tags in the following argument scenarios:

  • Optional Object types: Argument[Option[EchoMessageInput @@ InputObjectResult]]
  • Optional Scala types: Argument[Option[Int @@ CoercedScalaResult]]

Thanks so much for your help!

@yanns yanns closed this as completed Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants