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

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

Merged
merged 48 commits into from
May 8, 2024
Merged
Show file tree
Hide file tree
Changes from 43 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
3ca2c92
added support for bitset for data types
Mar 15, 2024
00750c2
added support for bitset for all data types
Mar 18, 2024
5da690e
initialize bitset
Mar 18, 2024
874e237
generate setField conditionally
Mar 18, 2024
a189da3
bug fixes
Mar 18, 2024
39e4878
adding unit tests
Mar 18, 2024
f21e6af
cleanup
Mar 18, 2024
c10ceb9
Revert "cleanup"
Mar 18, 2024
caff38c
cleanup
Mar 18, 2024
e178a13
updated flag name, CodeGenCliKt.run.xml
Mar 18, 2024
ecdee83
added additional unit test without field isset
Mar 18, 2024
36d06c0
Added descriptive help for CLI flag
Mar 18, 2024
3bae6d6
refactor builder function
Mar 18, 2024
0ccf925
add parameters to enum constructor
Mar 20, 2024
cd71b3a
fix linting
Mar 21, 2024
909d1fc
Revert "fix linting"
Mar 21, 2024
7a1d567
fix linting, unit tests
Mar 22, 2024
b3513ef
updating linting
Mar 22, 2024
b5c7af4
add field is set to generateJava
Mar 24, 2024
4052436
Revert "add field is set to generateJava"
Mar 25, 2024
7fa7f2f
Revert "updating linting"
Mar 25, 2024
3279171
Revert "fix linting, unit tests"
Mar 25, 2024
544cc66
update generateJava, formatting
Mar 25, 2024
ecdae30
revert xml changes
Mar 25, 2024
f2025ed
linting fix
Mar 25, 2024
e22b4cc
updating isSet and setField to public
Mar 25, 2024
f4833f2
update boolean fields
Mar 29, 2024
7274e31
cleanup
Mar 29, 2024
2b01466
update boolean field generation with tests
Apr 2, 2024
6a515b3
updating unit tests
Apr 3, 2024
e8e60fa
updating unit tests
Apr 3, 2024
5d76a5e
formatting Kotlin
Apr 3, 2024
8513595
Revert "formatting Kotlin"
Apr 3, 2024
0cf86d3
formatting Kotlin
Apr 3, 2024
0e8cc84
updating boolean field and setter names
Apr 3, 2024
bab8c0e
update EntitiesClientApiGenTest and ClientApiGenQueryTest
Apr 17, 2024
659de3d
fix more tests
Apr 17, 2024
aa449b8
bug fix
Apr 19, 2024
f3f16ac
format kotlin
Apr 19, 2024
258cb3b
fix unit tests
Apr 20, 2024
2c4515e
update boolean fields
Apr 25, 2024
dd0935d
cleanup
Apr 25, 2024
9dec043
fix: adding is nullable for input types
Apr 26, 2024
1e7bb05
fix: adding javapoet formatting
Apr 29, 2024
8a83b91
fix: Adding fields for non-nullable without defaults
Apr 29, 2024
3c076cc
cleanup
Apr 29, 2024
228ce44
addressing comments
Apr 29, 2024
9cb3ec6
cleanup
Apr 29, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class CodeGenCli : CliktCommand("Generate Java sources for SCHEMA file(s)") {
)
private val language by option("--language", "-l", help = "Output language").choice("java", "kotlin", ignoreCase = true)
.default("java")
krutikavk marked this conversation as resolved.
Show resolved Hide resolved
private val generateClient by option("--generate-client", "-c", help = "Genereate client api").flag(default = false)
private val generateClient by option("--generate-client", "-c", help = "Generate client api").flag(default = false)
private val generateDataTypes by option(
"--generate-data-types",
help = "Generate data types. Not needed when only generating an API"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,13 @@
package com.netflix.graphql.dgs.codegen.generators.java

import com.netflix.graphql.dgs.codegen.*
import com.netflix.graphql.dgs.codegen.generators.shared.CodeGeneratorUtils.capitalized
import com.netflix.graphql.dgs.codegen.generators.shared.SiteTarget
import com.netflix.graphql.dgs.codegen.generators.shared.applyDirectivesJava
import com.squareup.javapoet.*
import graphql.language.*
import graphql.language.TypeName
import graphql.schema.idl.TypeUtil
import org.slf4j.Logger
import org.slf4j.LoggerFactory
import java.io.Serializable
Expand Down Expand Up @@ -81,13 +83,15 @@ class DataTypeGenerator(config: CodeGenConfig, document: Document) : BaseDataTyp
if (config.generateDataTypes) {
val fieldDefinitions = definition.fieldDefinitions
.filterSkipped()
.map {
.map { fieldDefinition ->
val isNullable = !TypeUtil.isNonNull(fieldDefinition.type)
Field(
it.name,
typeUtils.findReturnType(it.type, useInterfaceType, true),
fieldDefinition.name,
typeUtils.findReturnType(fieldDefinition.type, useInterfaceType, true),
overrideGetter = overrideGetter,
description = it.description,
directives = it.directives
description = fieldDefinition.description,
directives = fieldDefinition.directives,
isNullable = isNullable
)
}
.plus(
Expand Down Expand Up @@ -122,6 +126,7 @@ class InputTypeGenerator(config: CodeGenConfig, document: Document) : BaseDataTy

val name = definition.name
val fieldDefinitions = definition.inputValueDefinitions.map {
val isNullable = !TypeUtil.isNonNull(it.type)
val defaultValue = it.defaultValue?.let { defVal ->
when (defVal) {
is BooleanValue -> CodeBlock.of("\$L", defVal.isValue)
Expand Down Expand Up @@ -155,7 +160,8 @@ class InputTypeGenerator(config: CodeGenConfig, document: Document) : BaseDataTy
type = typeUtils.findReturnType(it.type),
initialValue = defaultValue,
description = it.description,
directives = it.directives
directives = it.directives,
isNullable = isNullable
)
}.plus(extensions.flatMap { it.inputValueDefinitions }.map { Field(it.name, typeUtils.findReturnType(it.type)) })
return generate(name, emptyList(), fieldDefinitions, definition.description, definition.directives)
Expand All @@ -167,7 +173,7 @@ class InputTypeGenerator(config: CodeGenConfig, document: Document) : BaseDataTy
}
}

internal data class Field(val name: String, val type: com.squareup.javapoet.TypeName, val initialValue: CodeBlock? = null, val overrideGetter: Boolean = false, val interfaceType: com.squareup.javapoet.TypeName? = null, val description: Description? = null, val directives: List<Directive> = listOf())
internal data class Field(val name: String, val type: com.squareup.javapoet.TypeName, val initialValue: CodeBlock? = null, val overrideGetter: Boolean = false, val interfaceType: com.squareup.javapoet.TypeName? = null, val description: Description? = null, val directives: List<Directive> = listOf(), val isNullable: Boolean = true)

abstract class BaseDataTypeGenerator(
internal val packageName: String,
Expand Down Expand Up @@ -227,7 +233,7 @@ abstract class BaseDataTypeGenerator(

addEquals(javaType)
addHashcode(javaType)
addBuilder(javaType)
addBuilder(fields, javaType)

val javaFile = JavaFile.builder(packageName, javaType.build()).build()

Expand Down Expand Up @@ -341,7 +347,22 @@ abstract class BaseDataTypeGenerator(
constructorBuilder
.addParameter(parameterBuilder.build())
.addModifiers(Modifier.PUBLIC)
.addStatement("this.\$N = \$N", ReservedKeywordSanitizer.sanitize(it.name), ReservedKeywordSanitizer.sanitize(it.name))
}

fieldDefinitions.forEach {
val constructor = constructorBuilder
.addStatement(
krutikavk marked this conversation as resolved.
Show resolved Hide resolved
"this.\$N = \$N",
ReservedKeywordSanitizer.sanitize(it.name),
ReservedKeywordSanitizer.sanitize(it.name)
)
if (!(!it.isNullable && it.initialValue == null)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably cleaner to write this as:

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

Copy link
Contributor Author

@krutikavk krutikavk Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kilink Fields are generated for all these cases:

  1. Nullable | No default values --> Generate fields
  2. Nullable | With default values --> Generate fields
  3. Non-Nullable | No default values --> Field not needed
  4. 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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

constructorBuilder
.addStatement(
"this.\$N = true",
generateBooleanFieldName(ReservedKeywordSanitizer.sanitize(it.name))
)
}
}

javaType.addMethod(constructorBuilder.build())
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated 👍

if (!(!fieldDefinition.isNullable && fieldDefinition.initialValue == null)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated same as above

addIsDefinedFieldWithGetters(fieldDefinition, javaType)
}
}

private fun addIsDefinedFieldWithGetters(fieldDefinition: Field, javaType: TypeSpec.Builder) {
val fieldName = generateBooleanFieldName(ReservedKeywordSanitizer.sanitize(fieldDefinition.name))
val field = FieldSpec
.builder(com.squareup.javapoet.TypeName.BOOLEAN, fieldName)
.addModifiers(Modifier.PRIVATE)
.initializer("false")
.build()
val getterName = "${fieldName}Defined"

val getter = MethodSpec
.methodBuilder(getterName)
.addModifiers(Modifier.PUBLIC)
.returns(com.squareup.javapoet.TypeName.BOOLEAN)
.addStatement(
"return \$N",
"${generateBooleanFieldName(ReservedKeywordSanitizer.sanitize(fieldDefinition.name))}"
krutikavk marked this conversation as resolved.
Show resolved Hide resolved
).build()
javaType.addField(field)
javaType.addMethod(getter)
}

private fun generateBooleanFieldName(name: String): String {
return "is${name.capitalized()}"
}

private fun addFieldWithGetterAndSetter(returnType: com.squareup.javapoet.TypeName?, fieldDefinition: Field, javaType: TypeSpec.Builder) {
Expand Down Expand Up @@ -390,13 +440,21 @@ abstract class BaseDataTypeGenerator(

val setterName = typeUtils.transformIfDefaultClassMethodExists("set${fieldDefinition.name[0].uppercase()}${fieldDefinition.name.substring(1)}", TypeUtils.Companion.setClass)
val parameterBuilder = ParameterSpec.builder(returnType, ReservedKeywordSanitizer.sanitize(fieldDefinition.name))

val setterMethodBuilder = MethodSpec.methodBuilder(setterName)
.addModifiers(Modifier.PUBLIC)
.addStatement(
"this.\$N = \$N",
ReservedKeywordSanitizer.sanitize(fieldDefinition.name),
ReservedKeywordSanitizer.sanitize(fieldDefinition.name)
)
if (!(!fieldDefinition.isNullable && fieldDefinition.initialValue == null)) {
setterMethodBuilder
.addStatement(
"this.\$N = true",
generateBooleanFieldName(ReservedKeywordSanitizer.sanitize(fieldDefinition.name))
)
}

if (fieldDefinition.directives.isNotEmpty()) {
val (annotations, comments) = applyDirectivesJava(fieldDefinition.directives, config)
Expand Down Expand Up @@ -431,13 +489,13 @@ abstract class BaseDataTypeGenerator(
)
}

private fun addBuilder(javaType: TypeSpec.Builder) {
private fun addBuilder(fields: List<Field>, javaType: TypeSpec.Builder) {
val className = ClassName.get(packageName, javaType.build().name)
val buildMethod = MethodSpec.methodBuilder("build").returns(className).addStatement(
"""
$className result = new $className();
${javaType.build().fieldSpecs.joinToString("\n") { "result.${it.name} = this.${it.name};" }}
return result
$className result = new $className();
${javaType.build().fieldSpecs.joinToString("\n") { "result.${it.name} = this.${it.name};".trimIndent() }}
return result
""".trimIndent()
).addModifiers(Modifier.PUBLIC).build()

Expand All @@ -460,10 +518,20 @@ abstract class BaseDataTypeGenerator(
.addMethod(buildMethod)

javaType.build().fieldSpecs.map {
MethodSpec.methodBuilder(it.name)
val method = MethodSpec.methodBuilder(it.name)
.addJavadoc(it.javadoc)
.returns(builderClassName)
.addStatement("this.${it.name} = ${it.name}")

val fieldName = it.name
val field = fields.find { it.name.contains(fieldName) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (field?.isNullable == true && field.initialValue == null) {
method
.addStatement("this.${generateBooleanFieldName(it.name)} = true")
krutikavk marked this conversation as resolved.
Show resolved Hide resolved
}

method
.addStatement("return this")
.addParameter(ParameterSpec.builder(it.type, it.name).build())
.addModifiers(Modifier.PUBLIC).build()
Expand Down
Loading
Loading