Add Boolean fields to Data classes for supporting sparse update

[krutikavk]

Description

Issue: #609

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

1. Generate boolean fields for Java data classes by default, removed additional flag exposed for CodeGen config
2. Add a boolean for each field in data class called is<Field>Defined
3. Getter for boolean called getIs<Field>Defined
4. Setter for each field in the class explicitly sets value of is<Field>Defined
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.

Example of data classes created:

1. Schema
2. Generated data types with this change for schema here

Thanks!

Validation

Sample schema and codegen with bitset: https://github.com/krutikavk/dgs-codegen-run

[krutikavk]

Hi @srinivasankavitha Can you take a look at the PR

[srinivasankavitha]

Hi @srinivasankavitha Can you take a look at the PR
Thanks for the PR! Will do later this week.

[krutikavk]

Thanks for the PR! Will do later this week.

Thanks @srinivasankavitha. I am also working on updating Kotlin codegen.
What is the difference between generators/kotlin and generators/kotlin2--should I be updating both? The flag that exposes Kotlin2 data type generation generateKotlinNullableClasses is not exposed as a flag for codegen CLI

[srinivasankavitha]

yeah that flag is specific to kotlin2. Yes, it would be great of you can add to both since we try to maintain feature parity as much as possible.

[krutikavk]

yeah that flag is specific to kotlin2. Yes, it would be great of you can add to both since we try to maintain feature parity as much as possible.

Sounds great, thank you 👍

[srinivasankavitha]

Also, you can run ./gradlew formatKotlin to fix linting errors that are failing the build.

[krutikavk]

Linting and some unit tests fixed, please check @srinivasankavitha

[srinivasankavitha]

Thanks @srinivasankavitha I ran through the examples and dropped in a quick fix adding conditional boolean field generation for input type values as well.

The build failure message is as below:

ERROR:Unable to test /home/runner/work/dgs-codegen/dgs-codegen/build/examples/dgs-examples-java-2.7, command '/home/runner/work/dgs-codegen/dgs-codegen/gradlew -p /home/runner/work/dgs-codegen/dgs-codegen/build/examples/dgs-examples-java-2.7 -c /home/runner/work/dgs-codegen/dgs-codegen/build/examples/dgs-examples-java-2.7/settings.gradle.kts clean check' failed! Command '['/home/runner/work/dgs-codegen/dgs-codegen/gradlew', '-p', '/home/runner/work/dgs-codegen/dgs-codegen/build/examples/dgs-examples-java-2.7', '-c', '/home/runner/work/dgs-codegen/dgs-codegen/build/examples/dgs-examples-java-2.7/settings.gradle.kts', 'clean', 'check']' returned non-zero exit status 1.

I am wondering if this fix will fix the test as well--I a m unsure what test is failing atm ^ as sent above. Can you please retry the build and guide if I need to do anything else to fix like updating expected values etc.?

This seems to have fixed the build now, right? I see the CI build is green.

[krutikavk]

Thanks @srinivasankavitha I ran through the examples and dropped in a quick fix adding conditional boolean field generation for input type values as well.
The build failure message is as below:
ERROR:Unable to test /home/runner/work/dgs-codegen/dgs-codegen/build/examples/dgs-examples-java-2.7, command '/home/runner/work/dgs-codegen/dgs-codegen/gradlew -p /home/runner/work/dgs-codegen/dgs-codegen/build/examples/dgs-examples-java-2.7 -c /home/runner/work/dgs-codegen/dgs-codegen/build/examples/dgs-examples-java-2.7/settings.gradle.kts clean check' failed! Command '['/home/runner/work/dgs-codegen/dgs-codegen/gradlew', '-p', '/home/runner/work/dgs-codegen/dgs-codegen/build/examples/dgs-examples-java-2.7', '-c', '/home/runner/work/dgs-codegen/dgs-codegen/build/examples/dgs-examples-java-2.7/settings.gradle.kts', 'clean', 'check']' returned non-zero exit status 1.
I am wondering if this fix will fix the test as well--I a m unsure what test is failing atm ^ as sent above. Can you please retry the build and guide if I need to do anything else to fix like updating expected values etc.?

This seems to have fixed the build now, right? I see the CI build is green.

@srinivasankavitha Yes I was just able to validate this locally as well, this fixed the build. Please help validate and merge the PR.

[krutikavk]

@kilink Thanks for reviewing, I have updated as per your feedback. Please check if the changes are good

[krutikavk]

Hi @kilink @srinivasankavitha Checking in if you need anything from me before the PR can be merged

[srinivasankavitha]

Hi @krutikavk - we are out this week. We will take a look next week.

[ramapalani]

@srinivasankavitha following up on this. Will you be able to review this PR? And let us know if you need any further changes to the PR

[ramapalani]

@srinivasankavitha Thanks for approving.
What is the next step for this PR? Will you be able to merge this or do you require additional approvals?

[srinivasankavitha]

Just merged the PR. Will do a release later this week or early next week.

[ramapalani]

Thank you

[krutikavk]

Hi @srinivasankavitha Checking in when you are able to create a new release tag for this feature

[krutikavk]

Hi @srinivasankavitha @kilink I see this change has been recently reverted in commit 08eb4ad. Will this be updated/released soon? Do update if there are any outstanding issues with the feature.

[srinivasankavitha]

Hi @krutikavk - yes, I did a release yesterday and it broke a bunch of our projects and had to rollback. I'm yet to investigate and get a good idea of the failure scenarios (there are multiple). Will post an update, so we can fix forward. At teh very least, we might need to feature flag it and disable by default since the addition of the new field is breaking tests for users that are checking strings using toString().

[srinivasankavitha]

This should be

val field = fields.find { iter-> ReservedKeywordSanitizer.sanitize(iter.name) == fieldName }

Otherwise it matches other fields that are similarly named and is incorrect.
e.g. something and isSomethingThere..will generate isSomething that conflicts with isSomethingThere field.

Also this is problematic if the field isSomething is also there as part of the schema.

[srinivasankavitha]

Hi @krutikavk - yes, I did a release yesterday and it broke a bunch of our projects and had to rollback. I'm yet to investigate and get a good idea of the failure scenarios (there are multiple). Will post an update, so we can fix forward. At the very least, we might need to feature flag it and disable by default since the addition of the new field is breaking tests for users that are checking strings using toString().

Ok so couple things that need fixing:

1. The check needs to be fixed per my comment here along with test coverage: https://github.com/Netflix/dgs-codegen/pull/664/files#diff-9e24a3fdc10ed2f83c56a842ef173a41cf511d9d3cc154d736a9825e6bd3e699R527

2. The "==" method for the generated classes should exclude checking the additional isSomething fields since it fails the check when something is set to null explicitly vs it just being null. The objects are technically equivalent wrt values at that point. We'll need tests for that as well.

3. There's another issue which I haven't figured out yet but it's breaking tests at runtime. Will post when I know more.

[krutikavk]

@srinivasankavitha Following up on whether you were able to figure out the issue with breaking tests at runtime. Once all necessary changes are identified, I can work on updating this on the feature. Let us know the best path to move forward

[srinivasankavitha]

Ok. I have a few urgent priorities to work on before I can get back into investigating this issue. In the meantime, feel free to look into the ones I've already identified when you get a chance. I'll post an update on the last when I get some time to look into it further.

[srinivasankavitha]

Ok, so spent quite a bit of time debugging (3) and it turns out it is the same problem as (2). The equality should not include these new isSomething fields because that fails equality checks in data loaders etc. So that shows up as a failure in tests in because there is no data. So same problem basically.

So I think if we fix the 2 issues already pointed out, should be good.

We should still introduce a feature flag just to disable it in case of issues where the schema has fields explicitly named 'somethingandisSomething. In this scenario the generated isSomething` will clash with the existing schema field and cause issues. However, this flag should only be used to disable selectively and not advertised as such.

[krutikavk]

ACK @srinivasankavitha thanks for the detailed feedback. I ll update the PR this week with the following changes:

1. Add an internal feature flag when fields named something and isSomething are already present in the schema. In such cases, generating Boolean for this field should be disabled.
2. Equals method should exclude checks for generated Boolean fields.
3. Add appropriate unit tests.

Let me know in case there is anything else that I'd need since these test cases were not covered as a part of CI build.

[srinivasankavitha]

ACK @srinivasankavitha thanks for the detailed feedback. I ll update the PR this week with the following changes:

1. Add an internal feature flag when fields named something and isSomething are already present in the schema. In such cases, generating Boolean for this field should be disabled.
2. Equals method should exclude checks for generated Boolean fields.
3. Add appropriate unit tests.

Let me know in case there is anything else that I'd need since these test cases were not covered as a part of CI build.

Thanks @krutikavk. For (1), we just need a flag to disable. By default we still want to have the feature be enabled. Perhaps we can even change the name from iSSomething to isSomethingSet to make it even more explicit and reduce the likelihood of clashes.

The rest sounds accurate, thanks.

[krutikavk]

@srinivasankavitha @kilink I have opened a separate PR for all changes for this PR: #697.
Please help review the change.

[srinivasankavitha]

Thanks @krutikavk. Heads up that I won't get to this till next week.

[krutikavk]

Thanks @srinivasankavitha Are failed scenarios from old PR also a part of CI build now? Any way for us to do a pre-emptive check if the issue is resolved with the updated PR #697 ?