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

generateOptionalOperationVariables setting is ignored. #4747

Closed
Orbyt opened this issue Mar 9, 2023 · 10 comments
Closed

generateOptionalOperationVariables setting is ignored. #4747

Orbyt opened this issue Mar 9, 2023 · 10 comments

Comments

@Orbyt
Copy link

Orbyt commented Mar 9, 2023

Version

3.7.4

Summary

The generateOptionalOperationVariables option seems to be ignored.

Configuration:

apollo {
    mapScalar("...")
    packageName.set("com.example.example")
    generateOptionalOperationVariables.set(false)
}

A generated class for a mutation input:

public data class SignupMemberInput(
  public val acceptedTos: Boolean,
  public val cellPhone: Optional<String?> = Optional.Absent,
  public val chosenName: Optional<String?> = Optional.Absent,
  public val clientMutationId: Optional<String?> = Optional.Absent,
  public val dob: String,
  public val email: String
)

According to the documentation, the relevant variables should be nullable.

Steps to reproduce the behavior

Use Apollo 3.7.4 with the generateOptionalOperationVariables.set(false) setting and generate a mutation input with optional values.

@BoD
Copy link
Contributor

BoD commented Mar 9, 2023

Hi!

The option is only taken into account for operation variables - the rationale being that when you write an operation to accept an argument, it is usually because you want to pass a value for it, but that is not as obvious for input fields.

@Orbyt
Copy link
Author

Orbyt commented Mar 9, 2023

operation variables

Would you mind elaborating? Are you suggesting that the setting only applies to "top-level" variables and not variables in an input model?

@BoD
Copy link
Contributor

BoD commented Mar 9, 2023

Sure thing! This means that if you write an operation that accept a variable that is nullable, e.g:

query MyQuery($myVariable: String) {
  ...
}

Then, by default, this will be generated with an Optional:

data class MyQuery(val myVariable: Optional<String?>) {...}

But you can use the generateOptionalOperationVariables option to generate it without it:

data class MyQuery(val myVariable: String?) {...}

However if your schema defines input fields that have nullable fields, e.g.

input SignupMemberInput {
  cellPhone: String
}

these will always be generated with Optional<String?> independently of the option, as you noticed.

@BoD
Copy link
Contributor

BoD commented Mar 9, 2023

About supporting this also on input objects there may be a valid use-case, would you mind telling us a bit more about your schema and what you are trying to do?

@Orbyt
Copy link
Author

Orbyt commented Mar 9, 2023

Ok, thank you for the example! It's appreciated. I am migrating an application to Apollo 3 and many, if not most, of the queries and mutations in the application use input model objects with nested fields instead of independent "top-level" variables like the ones you referenced. Is there a way to migrate those queries and mutations to Apollo 3 without having to rewrite every input argument to use Optional.Present()? For example, this is the SignupMemberInput with Apollo 2:

val mutation = SignupMemberMutation.builder().input(
    SignupMemberInput.builder().apply {
        dob(Utils.changeDateFormat(user.dobMMDDYYYY, "MM/dd/yyyy", "yyyy-MM-dd"))
        firstName(user.firstName)
        lastName(user.lastName)
        ssLast4(user.ssnLastFour)
        email(user.email)
        cellPhone(user.phone)
        password(user.password)
        acceptedTos(true)
        formIds(formIds)
        medium(ConsentMediumEnum.ANDROID)
    }.build()
).build()

After reading the documentation, I'm fairly certain that would need to be rewritten in Apollo 3 to:

val input = SignupMemberInput(
    dob = Utils.changeDateFormat(user.dobMMDDYYYY, "MM/dd/yyyy", "yyyy-MM-dd"),
    firstName = Optional.Present(user.firstName),
    lastName = user.lastName,
    ssLast4 = Optional.Present(user.ssnLastFour),
    email = user.email,
    cellPhone = Optional.Present(user.phone),
    password = user.password,
    acceptedTos = true,
    formIds = Optional.Present(formIds),
    medium = Optional.Present(ConsentMediumEnum.android)
)
val mutation = SignupMemberMutation(input)

Is that correct? Is there a way to configure Apollo to generate nested fields in input objects as simple nullables?

About supporting this also on input objects there may be a valid use-case, would you mind telling us a bit more about your schema and what you are trying to do?

I am attempting to upgrade an application to Apollo 3 using the migration guide. The application has about 6,000 lines of queries, so rewriting every input constructor to use Optional.Present (assuming that is what is required and assuming my example is correct) would constitute a significant change.

@BoD
Copy link
Contributor

BoD commented Mar 9, 2023

Thanks a lot for the context, that helps a ton!

As a reminder, the reason the Optional are generated is because how in GraphQL nullable input fields can mean 2 things: can be sent as null, or can be omitted (wrote about this topic in a recent blog post).

That being said, I can see that when converting your own classes to the input models generated by Apollo, the Optionals can add some friction, including in your case, where you're migrating existing code.

This is not a great solution, but you could generate Java models (with generateKotlinModels.set(false) and generateModelBuilders.set(true)) instead of Kotlin models, as they would be closer to what you had in v2 and help the migration. But of course this should really be a temporary measure as Kotlin models are a better choice in a Kotlin project obviously.

For the future, we're thinking of possible ideas to help there:

  • a migration helper in the IDE plugin (currently under development) that could cover this case
  • support for an option similar to generateOptionalOperationVariables but for input objects, however we are trying to limit the number of codegen options which is already substantial
  • the generated models could have a "smart" constructor to accept values not wrapped in Optional, with the help of a compiler plugin (logged here)

@Orbyt
Copy link
Author

Orbyt commented Mar 10, 2023

@BoD Thank you. I'll investigate the feasibility of using those configuration options for Java models, though I understand Kotlin models are probably preferable.

Regarding the options you listed, a way of obscuring the concept of Optional entirely, or at least to the extent possible, might provide a better developer experience. I believe a similar concept is the goal of the third option you noted.

Regardless, I will assume the Optional.Present pattern I posted in my example is "correct". Please let me know otherwise.

Thank you for linking that blog post! I read it and thought it was helpful.

@BoD
Copy link
Contributor

BoD commented Mar 10, 2023

I will assume the Optional.Present pattern I posted in my example is "correct". Please let me know otherwise.

That's correct 👍

@Orbyt
Copy link
Author

Orbyt commented Mar 14, 2023

@BoD Thank you. I'm going to close this issue as the original question has been addressed.

This is not a great solution, but you could generate Java models (with generateKotlinModels.set(false) and generateModelBuilders.set(true)) instead of Kotlin models, as they would be closer to what you had in v2 and help the migration.

I have attempted to use those two configuration options. While they do force Java models to be generated, the generated models seem to be quite different than the ones generated via Apollo v2. For example:

try {                                                                               
    val response = client.get().query(query).execute()                              
    if (response.hasErrors()) {                                                                                                                            
        onError(response.errors?.first()?.message ?: genericErrorMsg, false)        
    } else {                                                                        
        val data = response.data!!                                                  
        data.coverageCheck                                                          
                                                                                    
        if (data.coverageCheck() != null) {                                         
            onSuccess(                                                              
                data.coverageCheck()!!.coverageStatus(),                            
                data.coverageCheck()!!.accountStatus(),                             
                data.coverageCheck()!!.coveredBy()?.name(),                         
                data.coverageCheck()!!.coveredBy()?.id(),                           
                data.coverageCheck()!!.coveredBy()?.mfaRequired(),                  
                data.coverageCheck()!!.signupToken()                                
            )                                                                       
        } else {                                                                    
            onError(genericErrorMsg, false)                                         
        }                                                                           
    }                                                                               
} catch (exception: ApolloException) {                                                                                               
    onError("ApolloException ${exception.message}", true)                           
    return                                                                          
}                                                                                   

data.coverageCheck() can no longer be used as it seems the generated Java models do not have traditional getter and setter methods like they did in Apollo v2. I did not find anything in the relevant documentation that suggested there was a configuration option for restoring the getters and setters.

I have also attempted to perform an upgrade to Apollo v3 using the new compatibility modes (useVersion2Compat()). However, it seems Java model generation is not supported when using useVersion2Compat():

Java codegen does not support codegenModels=compat

I suspect many of the queries and mutations I referenced earlier will need to be rewritten, regardless of whether or not Java models are used.

@martinbonnin
Copy link
Contributor

4.0.0-alpha.3-SNAPSHOT has a generateInputBuilders Gradle option to bring back builders.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants