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

Separate nullableFieldStyle Configuration for Input types and response type? #6175

Open
Mohammad-Dwairi opened this issue Oct 2, 2024 · 5 comments
Labels
☕ Java ⚙️ Codegen 🧊 Icebox Features that would be nice but probably won't happen in the near future ✨ Type: Feature

Comments

@Mohammad-Dwairi
Copy link

Mohammad-Dwairi commented Oct 2, 2024

Question

I'm generating Java models and trying to configure nullable fields in the response to be wrapped in Optional using nullableFieldStyle.set("javaOptional").

However, this setting seems to be applied to both input types and response type in the schema. Is there a way to restrict this configuration so that it only affects nullable fields in the response models type X? maybe by having separate configuration options for input types and response types?

Configuration Sample

service("xyz") {
    generateKotlinModels.set(false)
    nullableFieldStyle.set("javaOptional")
    generateOptionalOperationVariables.set(false)
}

Bug?

Using the configurations shown above, the Optional wrapper is applied to nullable fields inside input types but not applied to top-level nullable input variables. This inconsistency causes the generated builder to treat top-level nullable input variables as plain types instead of wrapping them in Optional.

I'm not entirely sure which behavior is expected in this case: whether the Optional wrapper should be applied uniformly to all nullable fields or if the difference between top-level variables and fields inside input types is intentional, or if it shouldn't wrap any input variable in Optional since generateOptionalOperationVariables is set to false (I think this makes most sense).

I believe there is a small conflict between nullableFieldStyle and generateOptionalOperationVariables options, since applying nullableFieldStyle option will indirectly apply Optional wrappers to the input types fields.

nullableFieldStyle generateOptionalOperationVariables Effect on Operation Variables Effect on Response Fields
Applied true All nullable operation variables are wrapped in Optional Nullable fields are wrapped in Optional
Applied false Only nullable fields in input types are wrapped in Optional Nullable fields are wrapped in Optional
Not Applied true Nullable Operation variables are wrapped in Optional internally, but not in the input builders. Expected? Nothing is wrapped with Optional
Not Applied false Nothing is wrapped in Optional Nothing is wrapped in Optional

Example

Input type

input CheckOutDateFilter {
    from: String # nullable
    to: String # nullable
}

Sample Query

query PropertyReservations(
    $propertyId: String!,
    $checkOutDate: CheckOutDateFilter # nullable
    $cursor: String # nullable
) {
    ....
}

Generated Builder

CheckOutDateFilter checkOutDateFilter = CheckOutDateFilter
                .builder()
                .from(Optional.empty()) // The builder expects this param as Optional<String>
                .to(Optional.empty()) // The builder expects this param as Optional<String>
                .build();

PropertyReservationsQuery reservationsQuery = PropertyReservationsQuery
                .builder()
                .propertyId(propertyId)
                .cursor(null) // The builder expects this param as String, not Optional<String> although it's nullable
                .checkOutDate(checkOutDateFilter) // The builder expects this param as CheckOutDateFilter, not Optional<CheckOutDateFilter> although it's nullable
                .build();

The top-level nullable input variables (cursor and checkOutDate) are not wrapped in Optional, whereas the fields inside the input type CheckOutDateFilter are wrapped in Optional.

Apollo Version

v 4.0.0

@Mohammad-Dwairi Mohammad-Dwairi changed the title Separate nullableFieldStyle Configuration for Input types and response type? Separate nullableFieldStyle Configuration for Input types and response type? Oct 2, 2024
@BoD
Copy link
Contributor

BoD commented Oct 2, 2024

About your 1st question on separating the handling of nulls in output vs input types: we don't currently have a way to do that. Can you tell us a bit more about why you'd like to treat them differently?


About generateOptionalOperationVariables: originally this option was meant to simplify the instantiation of operations by removing the need to wrap values in Optional when passing them to the constructor.

But operation builders makes this unnecessary:

  • non nullable operation variables are generated as methods accepting a T (you must call it and pass a non-null value)
  • nullable operation variables as well (you may not call it, or call it with null, or call it with a non-null value)

Now when using nullableFieldStyle.set("javaOptional"), nullable fields in input types and operation variables become Optional.

So this becomes:

  • non nullable operation variables are generated as methods accepting a T (you must call it and pass a non-null value)
  • nullable operation variables are generated as methods accepting an Optional<T> (you may not call it, or call it with empty(), or call it with a non-empty value)

But as you noticed, with generateOptionalOperationVariables.set(false), they are generated as previously (no Optional) - I am actually not sure why and I think this is indeed a bug.

I would keep generateOptionalOperationVariables to true (the default) and use the operations builders - unless there's a reason for setting it to false, that I'm missing?.

Let me know if that makes sense!

@Mohammad-Dwairi
Copy link
Author

About your 1st question on separating the handling of nulls in output vs input types: we don't currently have a way to do that. Can you tell us a bit more about why you'd like to treat them differently?

We have some cases where the inputs for the operations are mostly static, so having to wrap them in Optional every time looks unnecessary and complicates the operation instantiation, especially for complex operations that accepts many inputs. But I see it's very useful when dealing with the output types to avoid accessing null fields in the response, so I thought having the flexibility to configure them separately is something nice to have.

I would keep generateOptionalOperationVariables to true (the default) and use the operations builders - unless there's a reason for setting it to false, that I'm missing?

Since you've mentioned that we can't treat input types differently at the moment, then I don't have a reason to set generateOptionalOperationVariables to false. I was trying this option hoping it will disable all Optional wrappers in the input types, while still getting nullable fields wrapped in Optional in the output types using nullableFieldStyle.set("javaOptional").


QQ: We don't have a way to differentiate between input types and output types when generating the Optional wrappers, is this is because of a technical limitation? or is it just unavailable at the moment but it could be added if you see a value for having it?

Thanks!

@BoD
Copy link
Contributor

BoD commented Oct 3, 2024

Thanks a lot for the feedback - that makes sense.

There is no technical limitation, but having this separation would need to add another option to the codegen which already has a lot of complexity - so ideally we'd like to avoid it for now. The ask makes sense however so let's keep this ticket open and see if it gains some traction.

In the meantime I was wondering if using annotations (jetbrainsAnnotations, androidAnnotations or jsr305Annotations) could be a good compromise for you? They are not as "strongly typed" as Optionals, but are definitely less verbose to use in operations/input types.

@Mohammad-Dwairi
Copy link
Author

Thanks a lot! I'm not sure about the annotations, can we apply them to the nullable fields in the generated models?

@BoD
Copy link
Contributor

BoD commented Oct 4, 2024

can we apply them to the nullable fields in the generated models?

Yes the annotations will be applied in input and output types.

@BoD BoD added the 🧊 Icebox Features that would be nice but probably won't happen in the near future label Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☕ Java ⚙️ Codegen 🧊 Icebox Features that would be nice but probably won't happen in the near future ✨ Type: Feature
Projects
None yet
Development

No branches or pull requests

2 participants