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

Fix oneOf when multiple inputs have the same default #2403

Closed
wants to merge 1 commit into from

Conversation

guymers
Copy link
Contributor

@guymers guymers commented Sep 16, 2024

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.

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 that the correct one is chosen.
@kyri-petrou
Copy link
Collaborator

kyri-petrou commented Sep 16, 2024

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
Copy link
Contributor Author

guymers commented Sep 16, 2024

Without #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
Copy link
Collaborator

kyri-petrou commented Sep 17, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

guymers commented Sep 17, 2024

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.

@guymers guymers closed this Sep 17, 2024
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

Successfully merging this pull request may close these issues.

2 participants