RFC: Input Object Builders, should non optional arguments be in the constructor?

[BoD]

In v4, a codegen option generateInputBuilders has been added to generate builders for Input Objects.

For this input object:

input UserInput {
  firstName: String!
  lastName: String!
  email: String
} 

the generated class will look like this:

data class UserInput(
  val firstName: String,
  val lastName: String,
  val email: Optional<String?> = Optional.Absent,
) {
  class Builder {
    private var firstName: String? = null
    private var lastName: String? = null
    private var email: Optional<String?> = Optional.Absent

    fun firstName(firstName: String): Builder {
      this.firstName = firstName
      return this
    }

    fun lastName(lastName: String): Builder {
      this.lastName = lastName
      return this
    }

    fun email(email: String?): Builder {
      this.email = Optional.Present(email)
      return this
    }

    fun build(): UserInput = UserInput(
      firstName = firstName ?: error("missing value for firstName"),
      lastName = lastName ?: error("missing value for lastName"),
      email = email,
    )
  }
}

The check for non optional arguments is done at runtime in the build() method.

Usage:

UserInput.Builder()
  .firstName("John") // Could forget
  .lastName("Doe") // Could forget
  .email("[email protected]")
  .build()

Putting them in the builder's constructor would make this a build time check:

data class UserInput(
  val firstName: String,
  val lastName: String,
  val email: Optional<String?> = Optional.Absent,
) {
  class Builder(
    private val firstName: String,
    private val lastName: String,
  ) {
    private var email: Optional<String?> = Optional.Absent

    fun email(email: String?): Builder {
      this.email = Optional.Present(email)
      return this
    }

    fun build(): UserInput = UserInput(
      firstName = firstName,
      lastName = lastName,
      email = email,
    )
  }
}

Usage:

UserInput.Builder(
  firstName = "John", // Can't forget to supply it
  lastName = "Doe", // Can't forget to supply it
)
  .email("[email protected]")
  .build()

However:

• this makes the builder's API inconsistent (some arguments are setters / some are constructor arguments)
• for Input types that only have non optional arguments, the builder's constructor will be the same as the input type's constructor itself, but with the disadvantage of more verbosity of the builder pattern
• this is prone to the issue of the arguments that can change order with a new version of a schema (issue Arguments order for generated Kotlin/Java code #4659).

Other considerations:

• making this change later will probably be difficult
• this Kotlin issue discusses a feature request to enforce named arguments
• in case of @oneOf input objects, we would generate only builder constructors for each input field.

[StylianosGakis]

Taking a step back, I wanted to ask a question which I perhaps am lacking some context for.

Why does the builder exist in the first place for this? Is there any historical context? Perhaps easier to use from Java? Is it necessary for some non-trivial examples?

Personally I would be super happy just with

data class UserInput(
  val firstName: String,
  val lastName: String,
  val email: Optional<String?> = Optional.Absent,
)

and I could just use that constructor directly, then that would leave no room for errors. That way I can also express email being completely absent, or being present but null, due to the usage of Optional.

With all that said, if I was using the builder myself, I would again appreciate making mistakes impossible, rather than having a more "standard" builder API, which then gives me runtime crashes if I forget to add something. But I think if the context as to why this option was added was explained in this issue too, it would help understand who is using it and why, to make a better judgement on this trade-off.

[BoD]

Thanks @StylianosGakis you are right that some context about the builders themselves is helpful! They were introduced here which provides some info, and the TLDR is:

• for input values, GraphQL conflates nullable and optional (wrote a bit about this here)
• so all fields of a nullable type need to be wrapped in Optional
• this has proven to be a bit confusing (e.g. generateOptionalOperationVariables setting is ignored. #4747), and also it's kind of verbose/tedious to use: Optional.present(myValue)
• the builder pattern improves this: if you want the field to be absent, don't call the method. If you want it present, call it with your value (possibly null, and no need to wrap).

Edit: forgot to mention builders also help with the issue mentioned here (albeit moving non-optional arguments to the builder's constructor brings it back)

[StylianosGakis]

Right so for folks that might prefer the builder approach to get one of the three options (absent, present + null, present + non-null).
I can't help but think that the builder approach in fact would make me less comfortable than the simple constructor + Optional<>.
With optional, at least I know that I got those 3 options, and I have to consciously make the right decision, and I don't have an option to call it wrong by accident. With the builder, me not calling one parameter might mean that I forgot about it, due to doing it in haste, my autocompletion not bringing the right function high enough for me to notice it etc.

So while I do not see myself enabling this flag, if I did, I would definitely prefer the "can't call this wrong" approach, aka the one where the necessary parameters are part of the builder constructor. That's my feedback here 😊

[BoD]

With the builder, me not calling one parameter might mean that I forgot about it

Note that with the constructor since we have default values, the risk to forget one of the parameters is also there to some degree.

if I did, I would definitely prefer the "can't call this wrong" approach, aka the one where the necessary parameters are part of the builder constructor

Thanks for the feedback! 🙏