Add Boolean fields to Data classes for supporting sparse update

[krutikavk]

Description Issue: https://github.com/Netflix/dgs-codegen/issues/609

Add Boolean fields to all data classes. Here is a gist of changes:

1. Generate boolean fields for Java data classes for all nullable fields without any default value by default
2. Add a boolean for each field in data class called is<Field>Set
3. Getter for boolean called getIs<Field>Set
4. Setter for each field in the class explicitly sets value of is<Field>Set
5. Update boolean field and getter similarly for Builder class
6. Update Builder.build() function to use setter functions for each field from data class
7. Please note since this feature is enabled as default, most tests needed an update to account for additional fields added to data classes.

In addition, following changes are made as per feedback from #664

1. Add a flag generateIsSetFields with default value true. This can be disabled when a class has existing fields named is<Field>Set.
2. Equals method should exclude checks for generated Boolean fields.
3. Add appropriate unit tests.

[krutikavk]

@srinivasankavitha @kilink

[krutikavk]

Hi @srinivasankavitha @kilink Gentle reminder to check if you're able to validate the PR

[srinivasankavitha]

Sorry for the delay, I've been having busy on call weeks. Will get to it over the new fe days. I did take a quick look, but I think I need to spend more time to validate the changes, hence have not approved it.

[krutikavk]

Sorry for the delay, I've been having busy on call weeks. Will get to it over the new fe days. I did take a quick look, but I think I need to spend more time to validate the changes, hence have not approved it.

Thanks @srinivasankavitha Any updates yet? PLease let me know if there are any further changes in the PR

[srinivasankavitha]

Can you add a test case for the bug fix around the field names being similar? e.g. if schema has something and isSomethingThere fields, previously it would generate isSomething that conflicts with the existing schema isSomethingThere field. Now we generate isSomethingSet but still can cause issues.

Also, add test cases where Reserved Keywords are used to ensure that doesn't break.

Finally add a test case to validate that the setters are not generated when the config setting is false.

[srinivasankavitha]

I ran into another repo for which this feature breaks the build. This is going to take some time for me to get back to investigating since I'm mostly looking into this in my downtime. Will post an update once I know more.

[krutikavk]

Thanks for the feedback @srinivasankavitha.

1. I added an enhancement to detect presence of clashing fields with name <Field> and is<Field>Set. If present, it will disable boolean field generation.
2. In addition, flags --generate-isset-fields/--skip-generate-isset-fields can also be used to disable boolean field generation. By default, this flag is set so boolean fields will be generated by default provided condition 1 is false.
3. Added new test cases to test combination of scenarios 1 and 2.
4. Added test case for reserved keyword
5. Added additional test cases as applicable.

Please review and let me know if this is good.

[srinivasankavitha]

Thanks. We need a few changes to make things work since I tested with more projects:

1. Let's not add any enhancements, please remove the code that disables the feature in scenarios where the clash occurs. I;d rather have the reason for failure be explicit.
2. We need to add @JsonIgnore annotation on the isSomethingSet getters so it doesn't show up when doing json printing - this causes test failures for us
3. Can you rename the getter for the isSomethingSet field from isSomethingSetDefined to isSomethingSet().

[paulbakker]

@krutikavk For context, I've been helping to debug the issues that this feature is causing on existing project.

What is the reason the is* fields are generated on all types, and not just input types? Since the "sparse" update feature implies that this is about input to datafetchers, I think it would be sufficient to only add this to input types?

[krutikavk]

@srinivasankavitha

Thanks. We need a few changes to make things work since I tested with more projects:

1. Let's not add any enhancements, please remove the code that disables the feature in scenarios where the clash occurs. I;d rather have the reason for failure be explicit.

In that case, users will have to use the disable flag to explicitly disable boolean field generation when field clashes exist. Any field clashes will result in failure--can you clarify if this is fine?

2. We need to add @JsonIgnore annotation on the isSomethingSet getters so it doesn't show up when doing json printing - this causes test failures for us

ACK

3. Can you rename the getter for the isSomethingSet field from isSomethingSetDefined to isSomethingSet().

ACK

[srinivasankavitha]

Yes - confirming that in case of field clashes, users can explicitly disable. It would be better to make the failure scenarios more obvious for this feature.

[paulbakker]

In that case, users will have to use the disable flag to explicitly disable boolean field generation when field clashes exist. Any field clashes will result in failure--can you clarify if this is fine?

The idea of disabling when an issue is spotted is ok, but it should disable just for that field. Automatically disabling at the project level would be very confusing to users. At this point I think it's better to just leave it explicit instead of adding more changes like @srinivasankavitha said.

[krutikavk]

@krutikavk For context, I've been helping to debug the issues that this feature is causing on existing project.

What is the reason the is* fields are generated on all types, and not just input types? Since the "sparse" update feature implies that this is about input to datafetchers, I think it would be sufficient to only add this to input types?

Yes verified this, the field can be applied to input data types only for purposes of sparse update. Do you suggest I implement this?

Also, ack on other feedback to let users explicitly disable this feature for field name clashes, will update in the next iteration. I ll come up with an implementation soon as I can, do let me know if you have come across any other issues with the feature in the meanwhile.

[srinivasankavitha]

Also, ack on other feedback to let users explicitly disable this feature for field name clashes, will update in the next iteration. We don't need any additional changes for this since you already added a config option for disabling.

[paulbakker]

Yes verified this, the field can be applied to input data types only for purposes of sparse update. Do you suggest I implement this?

Yes, I think that makes sense and it also decreases the scope of where this change could potentially impact existing code.

[krutikavk]

Thanks for confirming @paulbakker @srinivasankavitha. I am working on updating the changes atm. Is there a way to identify type of class as DataTypeGenerator or InputTypeGenerator from BaseTypeGenerator? I may be able to reuse the current changes if there's a simpler way to identify this.

[krutikavk]

@paulbakker @srinivasankavitha I am working on the change to add Boolean data types only for Input types by adding Boolean fields for appropriate data fields to fieldDefinitions.

1. With this approach, I see [addFieldWithGetterAndSetter](https://github.com/Netflix/dgs-codegen/blob/955556960144e43369a4658408dbd717103e1017/graphql-dgs-codegen-core/src/main/kotlin/com/netflix/graphql/dgs/codegen/generators/java/DataTypeGenerator.kt#L404C17-L404C44) will generate getters and setter for Boolean fields and the names might not be customizable.
2. addEquals will need additional logic to exclude using Boolean fields generated for comparison.

Please let me know if this approach is acceptable or you have any other suggestions. The previous version of code you reviewed would be significantly changed with this approach. I will have a version up today.

Thanks

[paulbakker]

@krutikavk Yes, this should only happen on input types, so these changes need to happen. The current base class doesn't differentiate between input types vs normal types, so that needs to be added. A isInputType argument to the generate method could work.

I need to point out that the next set of changes really needs to be implemented and tested very well, so please be careful with the changes you're making and test with many different scenarios. The PR is taking too much time on our end for testing and validating, and we can't justify continuing to spend time on this. Because of the impact of the changes, we also can't just merge and release.

[krutikavk]

Hi @paulbakker @srinivasankavitha! Checking in to update I am working on testing Boolean fields on different scenarios. It will be helpful if you can share some of your use-cases that you have tested this on as some of these are not covered by existing unit tests or test suite.

[paulbakker]

@krutikavk yesterday I pushed a framework PR that has a completely different approach for the problem. Would like your thoughts as well: https://github.com/Netflix/dgs-framework/pull/1987

[srinivasankavitha]

@paulbakker - I think the PR you have is a really nice solution to the problem this codegen PR is trying to address without the complexity and risk of the changes involved in this PR. I would be in favor of pursuing that as the solution instead of adding this feature to codegen.

[krutikavk]

Thanks @paulbakker! Checking the PR

[krutikavk]

@paulbakker I checked the PR, this looks to be a good solution that should work for sparse update without breaking any existing use-cases. When is the change expected to be released? Once we are able to confirm sparse update use-case, I can close this PR as not needed anymore. Thanks!

[paulbakker]

That's great @krutikavk!

I don't think there's any reason we have to wait with a release, so we can probably do that tomorrow if that helps. I'll check with @srinivasankavitha.

[paulbakker]

Just released 9.1.0, which has the new feature! Let me know if this works for you. https://github.com/Netflix/dgs-framework/releases/tag/v9.1.0

[krutikavk]

Just released 9.1.0, which has the new feature! Let me know if this works for you. https://github.com/Netflix/dgs-framework/releases/tag/v9.1.0

Hi @paulbakker I tested this feature with sparse update for input types. I see isArgumentSet returns Boolean based on coerced input arguments. For input type fields that have default value, isArgumentSet always returns true.

Here's a small POC testing this feature: https://github.com/krutikavk/sparseupdate-poc/pull/1

Can you please help check on this and share if you have any suggestions for identifying isArgumentSet for default values.

[paulbakker]

For input type fields that have default value, isArgumentSet always returns true.

I think that's the correct behavior. The point of default values is that they are always set. If fields should be truly optional, they should be modeled like that in the schema, so I think this is a schema design question.

[paulbakker]

You would have the same issue with Codegen btw, because default values are set before a datafetcher is even invoked, so the generated types would also indicate them as being explicitly set.

[paulbakker]

Assuming we won't go forward with codegen changes, I'm closing this PR. Thanks for pushing the feature @krutikavk, it has helped a lot figuring out what to do about this!

[krutikavk]

Thanks you @paulbakker and @srinivasankavitha for your continued support and guidance on this!