ghostdogpr / caliban

Functional GraphQL library for Scala
https://ghostdogpr.github.io/caliban/
Apache License 2.0
947 stars 248 forks source link

Fix `oneOf` when multiple inputs have the same default #2403

Closed guymers closed 2 months ago

guymers commented 2 months ago

When determining which subtype ArgBuilder to use, use the field instead of attempting to build each one. This ensures that if more than one subtype has the same default then the correct one is chosen.

kyri-petrou commented 2 months ago

After looking into this, I think there is a more fundamental issue with the ArgBuilder that is the root cause of this bug. Minimized repro with a test that should be passing but it's failing on current main

      test("case class with optional fields fails when input is `null`") {
        case class Foo(a: Option[String])
        case class Bar(foo: Foo) derives ArgBuilder.Auto

        val out = ArgBuilder[Bar].build(NullValue)
        assertTrue(out.isLeft)
      }

This is usually caught during validation, but in cases that Validation is disabled then null will be treated the same way as {} which I believe is wrong. If we fix this issue then the oneOf inputs will work as expected as well

guymers commented 2 months ago

Without https://github.com/ghostdogpr/caliban/pull/2323 the current oneOf code works.

Is there anything wrong with getting the exact ArgBuilder based on the field in the oneOf input though?

kyri-petrou commented 2 months ago

Is there anything wrong with getting the exact ArgBuilder based on the field in the oneOf input though

There isn't something wrong with this concept (in fact, that would be the ideal approach).

There was something that didn't feel right to me and took me a while to identify (that's why I didn't mention it my previous comment). The current approach works for both derived and user-defined ArgBuilders, but with the changes introduced in this PR, user-defined ones won't work unless they also override def firstField (which is not backwards compatible and overall not great for UX).

e.g., if we were to change the definition of the ArgBuilder in the oneOf inputs test in ExecutionSpec to use the custom one below, it will pass with the series/2.x branch but fail in the PR branch:

        implicit val fooStringAb: ArgBuilder[Foo.FooString] = new ArgBuilder[Foo.FooString] {
          def build(input: InputValue): Either[ExecutionError, Foo.FooString] =
            input match {
              case ObjectValue(fields) if fields.contains("stringValue") =>
                fields("stringValue") match {
                  case StringValue(value) => Right(Foo.FooString(value))
                  case _                  => Left(ExecutionError("Expected stringValue to be a String"))
                }
              case _                                                     =>
                Left(ExecutionError("Expected stringValue to be present"))
            }
        }
kyri-petrou commented 2 months ago

By the way I'll add a test for the above case since to make sure we don't accidentally introduce this regression in the future

guymers commented 2 months ago

Ahh yes you could manually implement ArgBuilder.

Someone could do something like this though:

case class Arg1(s: String) extends Foo
object Arg1 {
  implicit val argBuilder: ArgBuilder[Arg1] = {
    case ObjectValue(fields) =>
      fields.get("str") match {
        case Some(StringValue(value)) => Right(Arg1(value))
      }
    }
  }
}
case class Arg2(str: String) extends Foo
object Arg2 {
  implicit val argBuilder: ArgBuilder[Arg2] = ArgBuilder.gen
}

and never get a Arg2.