Add Boolean fields to Data classes for supporting sparse update #664

krutikavk · 2024-03-18T06:59:03Z

Description

Issue: #609

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

Generate boolean fields for Java data classes by default, removed additional flag exposed for CodeGen config
Add a boolean for each field in data class called is<Field>Defined
Getter for boolean called getIs<Field>Defined
Setter for each field in the class explicitly sets value of is<Field>Defined
Update boolean field and getter similarly for Builder class
Update Builder.build() function to use setter functions for each field from data class
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:

Schema
Generated data types with this change for schema here

Thanks!

Validation

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

This reverts commit f21e6af.

.run/CodeGenCliKt.run.xml

graphql-dgs-codegen-core/src/main/kotlin/com/netflix/graphql/dgs/codegen/CodeGen.kt

graphql-dgs-codegen-core/src/main/kotlin/com/netflix/graphql/dgs/codegen/CodeGenCli.kt

...en-core/src/main/kotlin/com/netflix/graphql/dgs/codegen/generators/java/DataTypeGenerator.kt

graphql-dgs-codegen-core/src/test/kotlin/com/netflix/graphql/dgs/codegen/CodeGenTest.kt

.run/CodeGenCliKt.run.xml

krutikavk · 2024-03-20T09:30:26Z

Hi @srinivasankavitha Can you take a look at the PR

srinivasankavitha · 2024-03-21T16:53:27Z

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

krutikavk · 2024-03-21T22:47:18Z

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 · 2024-03-21T22:50:31Z

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 · 2024-03-21T22:51:48Z

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 · 2024-03-21T22:55:23Z

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

This reverts commit cd71b3a.

krutikavk · 2024-03-22T00:33:48Z

Linting and some unit tests fixed, please check @srinivasankavitha

krutikavk · 2024-04-26T22:00:54Z

Hi @kilink @srinivasankavitha Apologies for the follow-up, I wanted to check in on the PR review. If there were any concerns or questions, I am happy to address them. Thanks for your patience and understanding!

I see the CI build just failed at stage "Build Examples". Can you share any pointers if this needs to be fixed on the PR and how I can run the CI test locally for validating CI builds?

srinivasankavitha · 2024-04-26T22:08:53Z

Both examples are here: https://github.com/Netflix/dgs-examples-java-2.7
https://github.com/Netflix/dgs-examples-kotlin-2.7
You can run things locally with those examples and applying the codegen plugin from your branch.

krutikavk · 2024-04-26T23:06:36Z

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.?

kilink · 2024-04-28T18:25:39Z

...en-core/src/main/kotlin/com/netflix/graphql/dgs/codegen/generators/java/DataTypeGenerator.kt

+                    ReservedKeywordSanitizer.sanitize(it.name),
+                    ReservedKeywordSanitizer.sanitize(it.name)
+                )
+            if (!(!it.isNullable && it.initialValue == null)) {


It's probably cleaner to write this as:

if (it.isNullable && it.initialValue == null) { // ... }

@kilink Fields are generated for all these cases:

Nullable | No default values --> Generate fields

Nullable | With default values --> Generate fields

Non-Nullable | No default values --> Field not needed

Non-Nullable | With default values --> Generate fields

Fields are generated for both nullable field with default value as well as non-nullable with default value might get assigned default values by initializer without direct request from the client. This is our understanding from existing graphql services leveraging sparse update solution. Having Boolean field for other cases will help identify values set by the client.

With this, I can update the double negative to simplify the logic:

if (it.isNullable && it.initialValue != null))

Is this acceptable?

There's no reason to generate fields for any of the cases except the first one: a nullable field with no defaults. For any case with a default value, it's impossible for DGS to tell if the value was explicitly supplied or not, graphql-java will hand us the variables with the default value already there. So the flag here would always be "true". The flag is pretty useless in that case and misleading.

Ack, updated to generate fields for nullable fields with no default values.

kilink · 2024-04-28T18:28:08Z

...en-core/src/main/kotlin/com/netflix/graphql/dgs/codegen/generators/java/DataTypeGenerator.kt

@@ -360,6 +381,35 @@ abstract class BaseDataTypeGenerator(

    private fun addField(fieldDefinition: Field, javaType: TypeSpec.Builder) {
        addFieldWithGetterAndSetter(fieldDefinition.type, fieldDefinition, javaType)
+        // Generate for all fields except non-nullable without default values


nitpick: I think the comment would be clearer if it was stated as what we want to generate the fields for, e.g.:

Generate for all nullable fields without any defaults

Updated 👍

kilink · 2024-04-28T18:29:37Z

...en-core/src/main/kotlin/com/netflix/graphql/dgs/codegen/generators/java/DataTypeGenerator.kt

@@ -360,6 +381,35 @@ abstract class BaseDataTypeGenerator(

    private fun addField(fieldDefinition: Field, javaType: TypeSpec.Builder) {
        addFieldWithGetterAndSetter(fieldDefinition.type, fieldDefinition, javaType)
+        // Generate for all fields except non-nullable without default values
+        if (!(!fieldDefinition.isNullable && fieldDefinition.initialValue == null)) {


Same comment here as before, I would avoid that "double negative" and write this as:

if (fieldDefinition.isNullable && fieldDefinition.initialValue == null) { // ... }

Updated same as above

...en-core/src/main/kotlin/com/netflix/graphql/dgs/codegen/generators/java/DataTypeGenerator.kt

srinivasankavitha · 2024-04-28T23:05:17Z

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 · 2024-04-29T05:40:29Z

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 · 2024-04-29T19:07:07Z

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

krutikavk · 2024-05-01T17:01:10Z

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

srinivasankavitha · 2024-05-01T17:02:59Z

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

ramapalani · 2024-05-08T17:15:16Z

@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 · 2024-05-08T19:35:27Z

@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 · 2024-05-08T19:57:12Z

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

ramapalani · 2024-05-08T21:03:07Z

Thank you

krutikavk · 2024-05-16T18:43:02Z

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

…te (#664)" This reverts commit 89ed33a.

krutikavk · 2024-05-17T11:59:28Z

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 · 2024-05-17T16:15:58Z

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 · 2024-05-17T20:24:12Z

...en-core/src/main/kotlin/com/netflix/graphql/dgs/codegen/generators/java/DataTypeGenerator.kt

                .addJavadoc(it.javadoc)
                .returns(builderClassName)
                .addStatement("this.${it.name} = ${it.name}")
+
+            val fieldName = it.name
+            val field = fields.find { it.name.contains(fieldName) }


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 · 2024-05-17T21:34:43Z

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:

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
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.
There's another issue which I haven't figured out yet but it's breaking tests at runtime. Will post when I know more.

kkatelia added 9 commits March 15, 2024 15:11

added support for bitset for data types

3ca2c92

added support for bitset for all data types

00750c2

initialize bitset

5da690e

generate setField conditionally

874e237

bug fixes

a189da3

adding unit tests

39e4878

cleanup

f21e6af

Revert "cleanup"

c10ceb9

This reverts commit f21e6af.

cleanup

caff38c

ramapalani reviewed Mar 18, 2024

View reviewed changes

.run/CodeGenCliKt.run.xml Outdated Show resolved Hide resolved

graphql-dgs-codegen-core/src/main/kotlin/com/netflix/graphql/dgs/codegen/CodeGen.kt Outdated Show resolved Hide resolved

eaugeas reviewed Mar 18, 2024

View reviewed changes

graphql-dgs-codegen-core/src/main/kotlin/com/netflix/graphql/dgs/codegen/CodeGenCli.kt Outdated Show resolved Hide resolved

eaugeas reviewed Mar 18, 2024

View reviewed changes

...en-core/src/main/kotlin/com/netflix/graphql/dgs/codegen/generators/java/DataTypeGenerator.kt Outdated Show resolved Hide resolved

eaugeas reviewed Mar 18, 2024

View reviewed changes

...en-core/src/main/kotlin/com/netflix/graphql/dgs/codegen/generators/java/DataTypeGenerator.kt Outdated Show resolved Hide resolved

eaugeas reviewed Mar 18, 2024

View reviewed changes

graphql-dgs-codegen-core/src/test/kotlin/com/netflix/graphql/dgs/codegen/CodeGenTest.kt Outdated Show resolved Hide resolved

kkatelia added 4 commits March 18, 2024 12:38

updated flag name, CodeGenCliKt.run.xml

e178a13

added additional unit test without field isset

ecdee83

Added descriptive help for CLI flag

36d06c0

refactor builder function

3bae6d6

krutikavk commented Mar 18, 2024

View reviewed changes

.run/CodeGenCliKt.run.xml Outdated Show resolved Hide resolved

add parameters to enum constructor

0ccf925

kkatelia added 3 commits March 21, 2024 16:43

fix linting

cd71b3a

Revert "fix linting"

909d1fc

This reverts commit cd71b3a.

fix linting, unit tests

7a1d567

fix: adding is nullable for input types

9dec043

kilink reviewed Apr 28, 2024

View reviewed changes

...en-core/src/main/kotlin/com/netflix/graphql/dgs/codegen/generators/java/DataTypeGenerator.kt Outdated Show resolved Hide resolved

kilink reviewed Apr 28, 2024

View reviewed changes

...en-core/src/main/kotlin/com/netflix/graphql/dgs/codegen/generators/java/DataTypeGenerator.kt Outdated Show resolved Hide resolved

kkatelia added 5 commits April 29, 2024 19:09

fix: adding javapoet formatting

1e7bb05

fix: Adding fields for non-nullable without defaults

8a83b91

cleanup

3c076cc

addressing comments

228ce44

cleanup

9cb3ec6

srinivasankavitha merged commit 89ed33a into Netflix:master May 8, 2024
2 checks passed

srinivasankavitha added a commit that referenced this pull request May 16, 2024

Revert "Add Boolean fields to Data classes for supporting sparse upda…

08eb4ad

…te (#664)" This reverts commit 89ed33a.

srinivasankavitha reviewed May 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Boolean fields to Data classes for supporting sparse update #664

Add Boolean fields to Data classes for supporting sparse update #664

krutikavk commented Mar 18, 2024 •

edited

krutikavk commented Mar 20, 2024

srinivasankavitha commented Mar 21, 2024

krutikavk commented Mar 21, 2024 •

edited

srinivasankavitha commented Mar 21, 2024

krutikavk commented Mar 21, 2024

srinivasankavitha commented Mar 21, 2024 •

edited

krutikavk commented Mar 22, 2024

krutikavk commented Apr 26, 2024

srinivasankavitha commented Apr 26, 2024 •

edited

krutikavk commented Apr 26, 2024

kilink Apr 28, 2024

krutikavk Apr 29, 2024 •

edited

kilink Apr 29, 2024

krutikavk Apr 29, 2024

kilink Apr 28, 2024

krutikavk Apr 29, 2024

kilink Apr 28, 2024

krutikavk Apr 29, 2024

srinivasankavitha commented Apr 28, 2024

krutikavk commented Apr 29, 2024

krutikavk commented Apr 29, 2024

krutikavk commented May 1, 2024

srinivasankavitha commented May 1, 2024

ramapalani commented May 8, 2024

ramapalani commented May 8, 2024

srinivasankavitha commented May 8, 2024

ramapalani commented May 8, 2024

krutikavk commented May 16, 2024

krutikavk commented May 17, 2024 •

edited

srinivasankavitha commented May 17, 2024

srinivasankavitha May 17, 2024

srinivasankavitha commented May 17, 2024

Add Boolean fields to Data classes for supporting sparse update #664

Add Boolean fields to Data classes for supporting sparse update #664

Conversation

krutikavk commented Mar 18, 2024 • edited

Description

Validation

krutikavk commented Mar 20, 2024

srinivasankavitha commented Mar 21, 2024

krutikavk commented Mar 21, 2024 • edited

srinivasankavitha commented Mar 21, 2024

krutikavk commented Mar 21, 2024

srinivasankavitha commented Mar 21, 2024 • edited

krutikavk commented Mar 22, 2024

krutikavk commented Apr 26, 2024

srinivasankavitha commented Apr 26, 2024 • edited

krutikavk commented Apr 26, 2024

Choose a reason for hiding this comment

krutikavk Apr 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srinivasankavitha commented Apr 28, 2024

krutikavk commented Apr 29, 2024

krutikavk commented Apr 29, 2024

krutikavk commented May 1, 2024

srinivasankavitha commented May 1, 2024

ramapalani commented May 8, 2024

ramapalani commented May 8, 2024

srinivasankavitha commented May 8, 2024

ramapalani commented May 8, 2024

krutikavk commented May 16, 2024

krutikavk commented May 17, 2024 • edited

srinivasankavitha commented May 17, 2024

Choose a reason for hiding this comment

srinivasankavitha commented May 17, 2024

krutikavk commented Mar 18, 2024 •

edited

krutikavk commented Mar 21, 2024 •

edited

srinivasankavitha commented Mar 21, 2024 •

edited

srinivasankavitha commented Apr 26, 2024 •

edited

krutikavk Apr 29, 2024 •

edited

krutikavk commented May 17, 2024 •

edited