Repository: cjbooms/fabrikt
Pull request #208 (2023-05-08 08:18:27)

Fix nullability and default value issues for JSON merge patch
Introduced in #189 , the "JSON Merge Patch" pattern implemented via `JsonNullable` still has some rough edges like bugs with nullability of the inner type parameters as well as with default values of properties. Both should be fixed with this PR.

larsduelfer: Would be awesome if this can be integrated soon since we are relying on this fix! Thanks in advance!
cjbooms: 👍 

Patch:
diff --git a/src/main/kotlin/com/cjbooms/fabrikt/generators/OasDefault.kt b/src/main/kotlin/com/cjbooms/fabrikt/generators/OasDefault.kt
index f4eaa34..76f25ac 100644
--- a/src/main/kotlin/com/cjbooms/fabrikt/generators/OasDefault.kt
+++ b/src/main/kotlin/com/cjbooms/fabrikt/generators/OasDefault.kt
@@ -2,49 +2,54 @@ package com.cjbooms.fabrikt.generators
 
 import com.cjbooms.fabrikt.model.KotlinTypeInfo
 import com.cjbooms.fabrikt.util.toUpperCase
+import com.squareup.kotlinpoet.ClassName
 import com.squareup.kotlinpoet.CodeBlock
-import com.squareup.kotlinpoet.ParameterSpec
 import com.squareup.kotlinpoet.TypeName
 import java.net.URI
 import java.util.Base64
 
 sealed class OasDefault {
 
-    abstract fun setDefault(paramSpecBuilder: ParameterSpec.Builder): ParameterSpec.Builder
+    abstract fun getDefault(): CodeBlock
 
     data class StringValue(val strValue: String) : OasDefault() {
-        override fun setDefault(paramSpecBuilder: ParameterSpec.Builder): ParameterSpec.Builder {
-            return paramSpecBuilder.defaultValue("%S", strValue)
-        }
+        override fun getDefault(): CodeBlock =
+            CodeBlock.of("%S", strValue)
     }
 
     data class NumericValue(val numericValue: Number) : OasDefault() {
-        override fun setDefault(paramSpecBuilder: ParameterSpec.Builder): ParameterSpec.Builder {
-            return paramSpecBuilder.defaultValue("%L", numericValue)
-        }
+        override fun getDefault(): CodeBlock =
+            CodeBlock.of("%L", numericValue)
     }
 
     data class UriValue(val strValue: String) : OasDefault() {
-        override fun setDefault(paramSpecBuilder: ParameterSpec.Builder): ParameterSpec.Builder {
-            return paramSpecBuilder.defaultValue("%T(\"$strValue\")", URI::class)
-        }
+        override fun getDefault(): CodeBlock =
+            CodeBlock.of("%T(\"$strValue\")", URI::class)
     }
+
     data class ByteValue(val base64String: String) : OasDefault() {
-        override fun setDefault(paramSpecBuilder: ParameterSpec.Builder): ParameterSpec.Builder {
-            return paramSpecBuilder.defaultValue(CodeBlock.of("%T.getDecoder().decode(\"$base64String\")", Base64::class))
-        }
+        override fun getDefault(): CodeBlock =
+            CodeBlock.of("%T.getDecoder().decode(\"$base64String\")", Base64::class)
     }
 
     data class BooleanValue(val boolValue: Boolean) : OasDefault() {
-        override fun setDefault(paramSpecBuilder: ParameterSpec.Builder): ParameterSpec.Builder {
-            return paramSpecBuilder.defaultValue("%L", boolValue)
-        }
+        override fun getDefault(): CodeBlock =
+            CodeBlock.of("%L", boolValue)
     }
 
     data class EnumValue(val type: TypeName, val enumValue: String) : OasDefault() {
-        override fun setDefault(paramSpecBuilder: ParameterSpec.Builder): ParameterSpec.Builder {
-            return paramSpecBuilder.defaultValue("%T.${enumValue.toUpperCase()}", type)
-        }
+        override fun getDefault(): CodeBlock =
+            CodeBlock.of("%T.${enumValue.toUpperCase()}", type)
+    }
+
+    data class JsonNullableValue(val inner: OasDefault) : OasDefault() {
+        override fun getDefault(): CodeBlock =
+            CodeBlock.of(
+                "%T.of(${inner.getDefault()})", ClassName(
+                    "org.openapitools.jackson.nullable",
+                    "JsonNullable",
+                )
+            )
     }
 
     companion object {
@@ -56,12 +61,14 @@ sealed class OasDefault {
                             if (type != null && typeInfo.entries.contains(default)) OasDefault.EnumValue(type, default)
                             else null
                         }
+
                         is KotlinTypeInfo.Text -> OasDefault.StringValue(default)
                         is KotlinTypeInfo.Uri -> OasDefault.UriValue(default)
                         is KotlinTypeInfo.ByteArray -> OasDefault.ByteValue(default)
                         else -> null
                     }
                 }
+
                 is Number -> OasDefault.NumericValue(default)
                 is Boolean -> OasDefault.BooleanValue(default)
                 else -> null
diff --git a/src/main/kotlin/com/cjbooms/fabrikt/generators/PropertyUtils.kt b/src/main/kotlin/com/cjbooms/fabrikt/generators/PropertyUtils.kt
index 6519412..fa3f665 100644
--- a/src/main/kotlin/com/cjbooms/fabrikt/generators/PropertyUtils.kt
+++ b/src/main/kotlin/com/cjbooms/fabrikt/generators/PropertyUtils.kt
@@ -39,7 +39,7 @@ object PropertyUtils {
                 ClassName(
                     "org.openapitools.jackson.nullable",
                     "JsonNullable",
-                ).parameterizedBy(type.copy(nullable = false))
+                ).parameterizedBy(type.copy(nullable = type.isNullable))
             } else {
                 type
             }
@@ -124,9 +124,14 @@ object PropertyUtils {
                 property.initializer(name)
                 val constructorParameter: ParameterSpec.Builder = ParameterSpec.builder(name, wrappedType)
                 val oasDefault = getDefaultValue(this, parameterizedType)
+                val wrappedDefault =
+                    if (classSettings.isMergePatchPattern)
+                        oasDefault?.let { OasDefault.JsonNullableValue(it) }
+                    else
+                        oasDefault
                 if (!isRequired) {
-                    if (oasDefault != null) {
-                        oasDefault.setDefault(constructorParameter)
+                    if (wrappedDefault != null) {
+                        constructorParameter.defaultValue(wrappedDefault.getDefault())
                     } else {
                         val undefinedDefault = if (classSettings.isMergePatchPattern) {
                             "JsonNullable.undefined()"
diff --git a/src/main/kotlin/com/cjbooms/fabrikt/generators/client/ClientGeneratorUtils.kt b/src/main/kotlin/com/cjbooms/fabrikt/generators/client/ClientGeneratorUtils.kt
index a11af64..b26137a 100644
--- a/src/main/kotlin/com/cjbooms/fabrikt/generators/client/ClientGeneratorUtils.kt
+++ b/src/main/kotlin/com/cjbooms/fabrikt/generators/client/ClientGeneratorUtils.kt
@@ -87,7 +87,8 @@ object ClientGeneratorUtils {
         val specs = parameters.map {
             val builder = it.toParameterSpecBuilder()
             if (it is RequestParameter) {
-                if (it.defaultValue != null) OasDefault.from(it.typeInfo, it.type, it.defaultValue)?.setDefault(builder)
+                if (it.defaultValue != null) OasDefault.from(it.typeInfo, it.type, it.defaultValue)
+                    ?.let { builder.defaultValue(it.getDefault()) }
                 else if (!it.isRequired) builder.defaultValue("null")
             }
             builder.build()


Test patch:
diff --git a/src/test/resources/examples/jsonMergePatch/api.yaml b/src/test/resources/examples/jsonMergePatch/api.yaml
index 240c95d..6c13d93 100644
--- a/src/test/resources/examples/jsonMergePatch/api.yaml
+++ b/src/test/resources/examples/jsonMergePatch/api.yaml
@@ -74,3 +74,43 @@ components:
       properties:
         p:
           type: string
+    NullabilityCheck:
+      type: object
+      x-json-merge-patch: true
+      properties:
+        not-null-no-default-not-required:
+          type: string
+          nullable: false
+          required: false
+        nullable-no-default-not-required:
+          type: string
+          nullable: true
+          required: false
+        not-null-with-default-not-required:
+          type: string
+          nullable: false
+          required: false
+          default: ""
+        nullable-with-default-not-required:
+          type: string
+          nullable: true
+          required: false
+          default: ""
+        not-null-no-default-required:
+          type: string
+          nullable: false
+          required: true
+        nullable-no-default-required:
+          type: string
+          nullable: true
+          required: true
+        not-null-with-default-required:
+          type: string
+          nullable: false
+          required: true
+          default: ""
+        nullable-with-default-required:
+          type: string
+          nullable: true
+          required: true
+          default: ""
\ No newline at end of file
diff --git a/src/test/resources/examples/jsonMergePatch/models/Models.kt b/src/test/resources/examples/jsonMergePatch/models/Models.kt
index 3311386..d21d8d5 100644
--- a/src/test/resources/examples/jsonMergePatch/models/Models.kt
+++ b/src/test/resources/examples/jsonMergePatch/models/Models.kt
@@ -3,12 +3,13 @@ package examples.jsonMergePatch.models
 import com.fasterxml.jackson.annotation.JsonProperty
 import org.openapitools.jackson.nullable.JsonNullable
 import javax.validation.Valid
+import javax.validation.constraints.NotNull
 import kotlin.String
 
 data class InnerMergePatch(
     @param:JsonProperty("p")
     @get:JsonProperty("p")
-    val p: JsonNullable<String> = JsonNullable.undefined()
+    val p: JsonNullable<String?> = JsonNullable.undefined()
 )
 
 data class InnerNotMergePatch(
@@ -27,7 +28,7 @@ data class InnerOnlyMergePatchInline(
 data class InnerOnlyMergePatchInlineInner(
     @param:JsonProperty("p")
     @get:JsonProperty("p")
-    val p: JsonNullable<String> = JsonNullable.undefined()
+    val p: JsonNullable<String?> = JsonNullable.undefined()
 )
 
 data class InnerOnlyMergePatchRef(
@@ -41,20 +42,20 @@ data class NestedMergePatchInline(
     @param:JsonProperty("inner")
     @get:JsonProperty("inner")
     @get:Valid
-    val inner: JsonNullable<NestedMergePatchInlineInner> = JsonNullable.undefined()
+    val inner: JsonNullable<NestedMergePatchInlineInner?> = JsonNullable.undefined()
 )
 
 data class NestedMergePatchInlineInner(
     @param:JsonProperty("p")
     @get:JsonProperty("p")
-    val p: JsonNullable<String> = JsonNullable.undefined()
+    val p: JsonNullable<String?> = JsonNullable.undefined()
 )
 
 data class NestedMergePatchRef(
     @param:JsonProperty("inner")
     @get:JsonProperty("inner")
     @get:Valid
-    val inner: JsonNullable<InnerMergePatch> = JsonNullable.undefined()
+    val inner: JsonNullable<InnerMergePatch?> = JsonNullable.undefined()
 )
 
 data class NoMergePatchInline(
@@ -77,11 +78,42 @@ data class NoMergePatchRef(
     val inner: InnerNotMergePatch? = null
 )
 
+data class NullabilityCheck(
+    @param:JsonProperty("not-null-no-default-not-required")
+    @get:JsonProperty("not-null-no-default-not-required")
+    val notNullNoDefaultNotRequired: JsonNullable<String?> = JsonNullable.undefined(),
+    @param:JsonProperty("nullable-no-default-not-required")
+    @get:JsonProperty("nullable-no-default-not-required")
+    val nullableNoDefaultNotRequired: JsonNullable<String?> = JsonNullable.undefined(),
+    @param:JsonProperty("not-null-with-default-not-required")
+    @get:JsonProperty("not-null-with-default-not-required")
+    @get:NotNull
+    val notNullWithDefaultNotRequired: JsonNullable<String> = JsonNullable.of(""),
+    @param:JsonProperty("nullable-with-default-not-required")
+    @get:JsonProperty("nullable-with-default-not-required")
+    @get:NotNull
+    val nullableWithDefaultNotRequired: JsonNullable<String> = JsonNullable.of(""),
+    @param:JsonProperty("not-null-no-default-required")
+    @get:JsonProperty("not-null-no-default-required")
+    val notNullNoDefaultRequired: JsonNullable<String?> = JsonNullable.undefined(),
+    @param:JsonProperty("nullable-no-default-required")
+    @get:JsonProperty("nullable-no-default-required")
+    val nullableNoDefaultRequired: JsonNullable<String?> = JsonNullable.undefined(),
+    @param:JsonProperty("not-null-with-default-required")
+    @get:JsonProperty("not-null-with-default-required")
+    @get:NotNull
+    val notNullWithDefaultRequired: JsonNullable<String> = JsonNullable.of(""),
+    @param:JsonProperty("nullable-with-default-required")
+    @get:JsonProperty("nullable-with-default-required")
+    @get:NotNull
+    val nullableWithDefaultRequired: JsonNullable<String> = JsonNullable.of("")
+)
+
 data class TopLevelLevelMergePatchInline(
     @param:JsonProperty("inner")
     @get:JsonProperty("inner")
     @get:Valid
-    val inner: JsonNullable<TopLevelLevelMergePatchInlineInner> = JsonNullable.undefined()
+    val inner: JsonNullable<TopLevelLevelMergePatchInlineInner?> = JsonNullable.undefined()
 )
 
 data class TopLevelLevelMergePatchInlineInner(
@@ -94,5 +126,5 @@ data class TopLevelLevelMergePatchRef(
     @param:JsonProperty("inner")
     @get:JsonProperty("inner")
     @get:Valid
-    val inner: JsonNullable<InnerNotMergePatch> = JsonNullable.undefined()
+    val inner: JsonNullable<InnerNotMergePatch?> = JsonNullable.undefined()
 )