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.