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

Caliban Client: Allow query to not specify every subtype for the Union Type #446

Closed
anotherhale opened this issue Jun 12, 2020 · 23 comments · Fixed by #1099
Closed

Caliban Client: Allow query to not specify every subtype for the Union Type #446

anotherhale opened this issue Jun 12, 2020 · 23 comments · Fixed by #1099
Labels
client GraphQL Client module enhancement New feature or request

Comments

@anotherhale
Copy link

I would like to be able to write queries for a Union Type and not have to specify every subtype.

Currently when you query a Union Type you must provide all of the subtype implementations or you will get a compile error. If I remove the Pilot.shipName.map(Role.Pilot) from the role query:

    val character = {
      import caliban.client.Client.Character._
      (name ~
        nicknames ~
        origin ~
        role(
          Captain.shipName.map(Role.Captain),
          Engineer.shipName.map(Role.Engineer),
          Mechanic.shipName.map(Role.Mechanic),
        )).mapN(Character)
    }

I get the following compile time error:

Error:(27, 13) not enough arguments for method role: (onCaptain: caliban.client.SelectionBuilder[caliban.client.Client.Captain,A], onEngineer: caliban.client.SelectionBuilder[caliban.client.Client.Engineer,A], onMechanic: caliban.client.SelectionBuilder[caliban.client.Client.Mechanic,A], onPilot: caliban.client.SelectionBuilder[caliban.client.Client.Pilot,A])caliban.client.SelectionBuilder[caliban.client.Client.Character,Option[A]].
Unspecified value parameter onPilot.
        role(

I discussed this with @ghostdogpr and we determined that this is doable by returning the __typename for the case classes that are not specified and returning an Option[A] instead of the A.

@ghostdogpr ghostdogpr added the client GraphQL Client module label Jun 12, 2020
@ghostdogpr
Copy link
Owner

ghostdogpr commented Jun 13, 2020

The related code is https://github.com/ghostdogpr/caliban/blob/master/codegen/src/main/scala/caliban/codegen/ClientWriter.scala#L240. I think it would be nice if we kept the existing method (no need to get an Option when you're willing to provide all cases) and add a new one with that behavior, where each parameter would be optional.
I'm guessing the new method should return a ChoiceOf(OptionOf(...)) and each union member not provided would be a choice returning None. Let me know if you need help.

@ghostdogpr ghostdogpr changed the title Allow query to not specify every subtype for the Union Type Caliban Client: Allow query to not specify every subtype for the Union Type Jun 13, 2020
@ghostdogpr ghostdogpr added the enhancement New feature or request label Jun 18, 2020
@anotherhale
Copy link
Author

anotherhale commented Jun 23, 2020

Actually if I could get some help with this that would be great.

The new reference to the above
https://github.com/ghostdogpr/caliban/blob/master/tools/src/main/scala/caliban/tools/ClientWriter.scala#L243

First to make sure we are on the same page this is how I envision this working. For union type queries you should be able to specify only the results that you want. The following examples are based on the Caliban example data:

(You can use this query with graphiQL to see the result)

{
  characters {
    name
    role {
      ... on Engineer {
        shipName
      }
    }
  }
}

This would result in the following GraphQL response:

{
  "data": {
    "characters": [
      {
        "name": "Naomi Nagata",
        "role": {
          "shipName": "Rocinante"
        }
      },
      {
        "name": "Amos Burton",
        "role": {}
      },
      {
        "name": "Alex Kamal",
        "role": {}
      },
      {
        "name": "Chrisjen Avasarala",
        "role": null
      },
      {
        "name": "Josephus Miller",
        "role": null
      },
      {
        "name": "Roberta Draper",
        "role": null
      }
    ]
  }

Because the query only specified the shipName property the results that returned an empty role result or a null result should be decoded as None:

List(
  Character(Naomi Nagata, None),
  Character(Amos Burton,Some(Mechanic(Some(Rocinante)))),
  Character(Alex Kamal,None),
  Character(Chrisjen Avasarala,None),
  Character(Josephus Miller,None),
  Character(Roberta Draper,None)
)

The Client Generated code for this query could look like this:

  type Character
  object Character {
    def name: SelectionBuilder[Character, String] = Field("name", Scalar())
    def role[A](
        onCaptain: Option[SelectionBuilder[Captain, A]] = None,
        onEngineer: Option[SelectionBuilder[Engineer, A]] = None,
        onMechanic: Option[SelectionBuilder[Mechanic, A]] = None,
        onPilot: Option[SelectionBuilder[Pilot, A]] = None
    ): SelectionBuilder[Character, Option[Option[A]]] =
      Field(
        "role",
        OptionOf(
          ChoiceOf(
            Map(
              "Captain" -> OptionOf(
                Obj(onCaptain)
              ),
              "Engineer" -> OptionOf(
                Obj(onEngineer)
              ),
              "Mechanic" -> OptionOf(
                Obj(onMechanic)
              ),
              "Pilot"    -> OptionOf(
                Obj(onPilot)
              )
            )
          )
        )
      )
  }

I don't have access to my computer that I was working this on to remember some of the challenges with this approach. I do remember that the overridden methods fromGraphQL and toSelectionSet did not match the def signature. I can work on this if you help point me in the right direction or we architect it out some more.

@ghostdogpr
Copy link
Owner

How about the following implementation:

def roleOption[A](
  onCaptain: Option[SelectionBuilder[Captain, A]] = None,
  onEngineer: Option[SelectionBuilder[Engineer, A]] = None,
  onMechanic: Option[SelectionBuilder[Mechanic, A]] = None,
  onPilot: Option[SelectionBuilder[Pilot, A]] = None
): SelectionBuilder[Character, Option[Option[A]]] =
  Field(
    "role",
    OptionOf(
      ChoiceOf(
        Map(
          "Captain"  -> onCaptain.fold[FieldBuilder[Option[A]]](NullField)(a => OptionOf(Obj(a))),
          "Engineer" -> onEngineer.fold[FieldBuilder[Option[A]]](NullField)(a => OptionOf(Obj(a))),
          "Mechanic" -> onMechanic.fold[FieldBuilder[Option[A]]](NullField)(a => OptionOf(Obj(a))),
          "Pilot"    -> onPilot.fold[FieldBuilder[Option[A]]](NullField)(a => OptionOf(Obj(a)))
        )
      )
    )
  )

This introduces NullField like this:

case object NullField extends FieldBuilder[Option[Nothing]] {
  override def fromGraphQL(value: Value): Either[DecodingError, Option[Nothing]] = Right(None)
  override def toSelectionSet: List[Selection]                                   = Nil
}

One more thing to do is in ChoiceOf#toSelectionSet to remove the members that have an empty selection when forming the GraphQL query.

@anotherhale
Copy link
Author

anotherhale commented Jul 1, 2020

@ghostdogpr Yes that works. I filtered the builderMap in the toSelectionSet with the following code (I added the isNullField helper function:

  case object NullField extends FieldBuilder[Option[Nothing]] {
    override def fromGraphQL(value: Value): Either[DecodingError, Option[Nothing]] = Right(None)
    override def toSelectionSet: List[Selection]                                   = Nil
  }
  
  def isNullField(p : Any): Boolean = p match {
    case  NullField => true
    case _ => false
  }

  case class ChoiceOf[A](builderMap: Map[String, FieldBuilder[A]]) extends FieldBuilder[A] {
    override def fromGraphQL(value: Value): Either[DecodingError, A] =
      value match {
        case ObjectValue(fields) =>
          for {
            typeNameValue <- fields.find(_._1 == "__typename").map(_._2).toRight(DecodingError("__typename is missing"))
            typeName <- typeNameValue match {
              case StringValue(value) => Right(value)
              case _                  => Left(DecodingError("__typename is not a String"))
            }
            fieldType <- builderMap.get(typeName).toRight(DecodingError(s"type $typeName is unknown"))
            result    <- fieldType.fromGraphQL(value)
          } yield result
        case _ => Left(DecodingError(s"Field $value is not an object"))
      }
    
    override def toSelectionSet: List[Selection] = {
      val filteredBuilderMap = builderMap.filter((f) => !isNullField(f._2) );
      Selection.Field(None, "__typename", Nil, Nil, Nil, 0) ::
        filteredBuilderMap.map { case (k, v) => { Selection.InlineFragment(k, v.toSelectionSet) }}.toList
    }
  }

If this looks good to you I will try to update the code generator to generate the proper client code.

For the ClientExampleApp I also updated the query to demonstrate named parameters for Role works well.

    val character = {
      import caliban.client.Client.Character._
      (name ~
        nicknames ~
        origin ~
        role(
          onEngineer = Some(Engineer.shipName.map(Role.Engineer)),
          onPilot = Some(Pilot.shipName.map(Role.Pilot))
        )
      ).mapN(Character)
    }

@ghostdogpr
Copy link
Owner

@anotherhale that looks good! About the code generation, should it be an additional function or replace the existing one? I feel that the existing one will be nicer when you want to specify all cases because you don't need to wrap the selection in Some(...) so I tend to prefer having both. What do you think?

@anotherhale
Copy link
Author

anotherhale commented Jul 2, 2020

I agree it would be good to have a different function. I do feel like ChoiceOf would be a better name for this function and the function that requires all of the union types would be AllOf. I understand that is a change in API though, so if you want to keep ChoiceOf the same and maybe name the new function AnyOf or something similar that works too.

@ghostdogpr
Copy link
Owner

@anotherhale I think both functions can use ChoiceOf no?

@anotherhale
Copy link
Author

Yes. I see what you mean now. The code would just generate both a role function and a roleOption function?

type Character
  object Character {
    def name: SelectionBuilder[Character, String]            = Field("name", Scalar())
    def nicknames: SelectionBuilder[Character, List[String]] = Field("nicknames", ListOf(Scalar()))
    def origin: SelectionBuilder[Character, Origin]          = Field("origin", Scalar())
    def role[A](
      onCaptain: SelectionBuilder[Captain, A],
      onEngineer: SelectionBuilder[Engineer, A],
      onMechanic: SelectionBuilder[Mechanic, A],
      onPilot: SelectionBuilder[Pilot, A]
    ): SelectionBuilder[Character, Option[A]] =
      Field(
        "role",
        OptionOf(
          ChoiceOf(
            Map(
              "Captain"  -> Obj(onCaptain),
              "Engineer" -> Obj(onEngineer),
              "Mechanic" -> Obj(onMechanic),
              "Pilot"    -> Obj(onPilot)
            )
          )
        )
      )
    def roleOption[A](
                 onCaptain: Option[SelectionBuilder[Captain, A]] = None,
                 onEngineer: Option[SelectionBuilder[Engineer, A]] = None,
                 onMechanic: Option[SelectionBuilder[Mechanic, A]] = None,
                 onPilot: Option[SelectionBuilder[Pilot, A]] = None
               ): SelectionBuilder[Character, Option[Option[A]]] =
      Field(
        "role",
        OptionOf(
          ChoiceOf(
            Map(
              "Captain"  -> onCaptain.fold[FieldBuilder[Option[A]]](NullField)(a => OptionOf(Obj(a))),
              "Engineer" -> onEngineer.fold[FieldBuilder[Option[A]]](NullField)(a => OptionOf(Obj(a))),
              "Mechanic" -> onMechanic.fold[FieldBuilder[Option[A]]](NullField)(a => OptionOf(Obj(a))),
              "Pilot"    -> onPilot.fold[FieldBuilder[Option[A]]](NullField)(a => OptionOf(Obj(a)))
            )
          )
        )
      )
  }

And then the query would could use either one like so:
For the role:

    val character = {
      import caliban.client.Client.Character._
      (name ~
        nicknames ~
        origin ~
        role(
          Captain.shipName.map(Role.Captain),
          Engineer.shipName.map(Role.Engineer),
          Mechanic.shipName.map(Role.Mechanic),
          Pilot.shipName.map(Role.Pilot)
        )).mapN(Character)
    }

Or the roleOption:

    val character = {
      import caliban.client.Client.Character._
      (name ~
        nicknames ~
        origin ~
        roleOption(
          onEngineer = Some(Engineer.shipName.map(Role.Engineer))
        )).mapN(Character)
    }

@ghostdogpr
Copy link
Owner

Yep, that's what I was suggesting 👍

@anotherhale
Copy link
Author

anotherhale commented Jul 6, 2020

This is the change that I made to the ClientWriter to generate the roleOption type.

      } else if (unionTypes.nonEmpty) {
        (
          s"[$typeLetter]",
          s"(${unionTypes.map(t => s"""on${t.name}: Option[SelectionBuilder[${t.name}, $typeLetter]] = None""").mkString(", ")})",
          s"Option[${writeType(field.ofType).replace(fieldType, typeLetter)}]",
          writeTypeBuilder(
            field.ofType,
            s"ChoiceOf(Map(${unionTypes.map(t => s""""${t.name}" -> on${t.name}.fold[FieldBuilder[Option[A]]](NullField)(a => OptionOf(Obj(a)))""").mkString(", ")}))"
          )
        )

https://github.com/ghostdogpr/caliban/blob/master/tools/src/main/scala/caliban/tools/ClientWriter.scala#L250-L260

I did not see how to generate both without creating another TypeDefinition or passing a flag to generate this optional type instead. Do you have any ideas?

@ghostdogpr
Copy link
Owner

Yeah I see the problem. Maybe add an intermediate function before writeField is called, and there you call writeField twice with different parameters?

@anotherhale
Copy link
Author

Sorry been busy with work and life. I will give this a try this week.

@anotherhale
Copy link
Author

I have PR that I can push soon. I just need to get it approved for release by my company first.

@ghostdogpr
Copy link
Owner

@anotherhale great! 😄

@askmrsinh
Copy link

@anotherhale
Hi, are you still waiting on the approval?

@anotherhale
Copy link
Author

Sorry finally making some progress. The wheels of bureaucracy turn slowly... I will post the PR once approved.

@askmrsinh
Copy link

@anotherhale
I am afraid that the wheels of bureaucracy might have fallen off.
Would you mind if I take a shot at this?

@anotherhale
Copy link
Author

I will try to see if I can expedite it.

@anotherhale
Copy link
Author

I have it all completed with docs and examples too. Everyone is on vacation for the holidays - probably won't see anything until January.

@anotherhale
Copy link
Author

I just got approval on releasing this. I will open a PR as soon as I merge in latest changes and resolve any conflicts.

@anotherhale
Copy link
Author

anotherhale commented Feb 24, 2021

Sorry this took so long. This request to contribute to open source was a pioneer effort in my organization and now they have put in place policies and procedures to make contributing back to the open source community much easier and quicker. 👍

@ghostdogpr
Copy link
Owner

Great to hear 👏

@askmrsinh
Copy link

This is great news. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client GraphQL Client module enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants