Fix nullable decoding (#1637)

* fix issues with nullable required fields decoding
* handle combo of refinements with nullable
* update changelog
* update document spec with refinement cases
* add tests and docs for decoding of different field types

CHANGELOG.md

@@ -5,6 +5,10 @@ When adding entries, please treat them as if they could end up in a release any
 
 Thank you!
 
+# 0.18.29
+
+* Fix for decoding of required nullable fields and some combinations of refinements with nullable fields (see [#1637](https://github.com/disneystreaming/smithy4s/pull/1637))
+
 # 0.18.28
 
 * Better support for timestamps before Linux Epoch and trimming the Timestamp nanosecond part (see [#1623](https://github.com/disneystreaming/smithy4s/pull/1623))

modules/bootstrapped/test/src/smithy4s/DocumentSpec.scala

@@ -953,6 +953,199 @@ class DocumentSpec() extends FunSuite {
     assertEquals(doc, expected)
   }
 
+  test("combinations of required, nullable, and null default") {
+    testFieldCombination(true, true, true)
+    testFieldCombination(false, true, true)
+    testFieldCombination(false, false, true)
+    testFieldCombination(false, false, false)
+    testFieldCombination(true, false, false)
+    testFieldCombination(true, true, false)
+    testFieldCombination(true, false, true)
+    testFieldCombination(false, true, false)
+  }
+
+  private def testFieldCombination(
+      required: Boolean,
+      nullable: Boolean,
+      nullDefault: Boolean
+  )(implicit loc: munit.Location): Unit = {
+    val toDecode = Document.DObject(Map.empty)
+    val hints =
+      if (nullDefault) Hints(smithy.api.Default(Document.DNull))
+      else Hints.empty
+    // scalafmt: { maxColumn: 120 }
+    if (!required && nullDefault) nonRequiredWithDefault(nullable, hints, toDecode)
+    else if (required && nullable) requiredNullable(nullDefault, hints, toDecode)
+    else if (!required && nullable && !nullDefault) nonRequiredNullable(hints, toDecode)
+    else if (required) requiredNonNullable(nullDefault, hints, toDecode)
+    else if (!nullDefault) nonRequiredNonNullable(hints, toDecode)
+  }
+
+  def nonRequiredWithDefault(
+      nullable: Boolean,
+      hints: Hints,
+      toDecode: Document
+  )(implicit loc: munit.Location): Unit = {
+    if (nullable) {
+      case class Foo(f: Nullable[String])
+      implicit val schema: Schema[Foo] =
+        Schema.struct(Schema.string.nullable.field[Foo]("f", _.f).addHints(hints))(
+          Foo.apply
+        )
+      val result = Document.decode[Foo](toDecode)
+      // required = false, nullable = true, nullDefault = true
+      expect.same(result.toOption.get, Foo(Nullable.Null))
+    } else {
+      case class Foo(f: String)
+      implicit val schema: Schema[Foo] =
+        Schema.struct(Schema.string.field[Foo]("f", _.f).addHints(hints))(
+          Foo.apply
+        )
+      val result = Document.decode[Foo](toDecode)
+      // required = false, nullable = false, nullDefault = true
+      expect.same(result.toOption.get, Foo(""))
+    }
+  }
+
+  def requiredNullable(
+      nullDefault: Boolean,
+      hints: Hints,
+      toDecode: Document
+  )(implicit loc: munit.Location): Unit = {
+    case class Foo(f: Nullable[String])
+    implicit val schema: Schema[Foo] =
+      Schema.struct(
+        Schema.string.nullable.required[Foo]("f", _.f).addHints(hints)
+      )(
+        Foo.apply
+      )
+    val result = Document.decode[Foo](toDecode)
+    if (nullDefault)
+      // required = true, nullable = true, nullDefault = true
+      expect.same(result.toOption.get, Foo(Nullable.Null))
+    else
+      // required = true, nullable = true, nullDefault = false
+      expect(result.isLeft)
+  }
+
+  def nonRequiredNullable(
+      hints: Hints,
+      toDecode: Document
+  )(implicit loc: munit.Location): Unit = {
+    case class Foo(f: Option[Nullable[String]])
+    implicit val schema =
+      Schema.struct(
+        Schema.string.nullable.optional[Foo]("f", _.f).addHints(hints)
+      )(
+        Foo.apply
+      )
+    val result = Document.decode[Foo](toDecode)
+    // required = false, nullable = true, nullDefault = false
+    expect.same(result.toOption.get, Foo(None))
+  }
+
+  def requiredNonNullable(
+      nullDefault: Boolean,
+      hints: Hints,
+      toDecode: Document
+  )(implicit loc: munit.Location): Unit = {
+    case class Foo(f: String)
+    implicit val schema: Schema[Foo] =
+      Schema.struct(Schema.string.required[Foo]("f", _.f).addHints(hints))(
+        Foo.apply
+      )
+    val result = Document.decode[Foo](toDecode)
+    // required = true, nullable = false, nullDefault = true
+    if (nullDefault) expect.same(result.toOption.get, Foo(""))
+    // required = true, nullable = false, nullDefault = false
+    else expect(result.isLeft)
+  }
+
+  def nonRequiredNonNullable(
+      hints: Hints,
+      toDecode: Document
+  )(implicit loc: munit.Location): Unit = {
+    case class Foo(f: Option[String])
+    implicit val schema =
+      Schema.struct(Schema.string.optional[Foo]("f", _.f).addHints(hints))(
+        Foo.apply
+      )
+    val result = Document.decode[Foo](toDecode)
+    // required = false, nullable = false, nullDefault = false
+    expect.same(result.toOption.get, Foo(None))
+  }
+
+  test(
+    "Required refined field with null default"
+  ) {
+    case class Test()
+    object Test extends ShapeTag.Companion[Test] {
+      def id: ShapeId = ShapeId("test", "Test")
+      def schema: Schema[Test] = Schema.constant(Test())
+    }
+    case class Foo(str: String)
+    case class Bar(foo: Foo)
+    implicit val provider: RefinementProvider[Test, String, Foo] =
+      Refinement.drivenBy[Test](str => Right(Foo.apply(str)), _.str)
+    val fieldSchema: smithy4s.schema.Field[Bar, Foo] =
+      Schema.string
+        .refined[Foo](
+          Test()
+        )
+        .required[Bar]("foo", _.foo)
+        .addHints(smithy.api.Default(Document.DNull))
+    implicit val schema: Schema[Bar] =
+      Schema.struct[Bar](fieldSchema)(Bar.apply)
+
+    expect.same(
+      Document.decode[Bar](
+        Document.DObject(Map("foo" -> Document.fromString("test")))
+      ),
+      Right(Bar(Foo("test")))
+    )
+    expect.same(
+      Document.decode[Bar](Document.DObject(Map.empty)),
+      // Empty string here because null default is implied to be empty string
+      // for a non-nullable string field
+      Right(Bar(Foo("")))
+    )
+  }
+
+  test(
+    "Nullable required refined field with null default"
+  ) {
+    case class Test()
+    object Test extends ShapeTag.Companion[Test] {
+      def id: ShapeId = ShapeId("test", "Test")
+      def schema: Schema[Test] = Schema.constant(Test())
+    }
+    case class Foo(str: String)
+    case class Bar(foo: Nullable[Foo])
+    implicit val provider: RefinementProvider[Test, String, Foo] =
+      Refinement.drivenBy[Test](str => Right(Foo.apply(str)), _.str)
+    val fieldSchema: smithy4s.schema.Field[Bar, Nullable[Foo]] =
+      Schema.string
+        .refined[Foo](
+          Test()
+        )
+        .nullable
+        .required[Bar]("foo", _.foo)
+        .addHints(smithy.api.Default(Document.DNull))
+    implicit val schema: Schema[Bar] =
+      Schema.struct[Bar](fieldSchema)(Bar.apply)
+
+    expect.same(
+      Document.decode[Bar](
+        Document.DObject(Map("foo" -> Document.fromString("test")))
+      ),
+      Right(Bar(Nullable.value(Foo("test"))))
+    )
+    expect.same(
+      Document.decode[Bar](Document.DObject(Map.empty)),
+      Right(Bar(Nullable.Null))
+    )
+  }
+
   private def inside[A, B](
       a: A
   )(assertPF: PartialFunction[A, Unit])(implicit loc: munit.Location) = {

modules/bootstrapped/test/src/smithy4s/schema/DefaultValueSpec.scala

@@ -133,7 +133,7 @@ final class DefaultValueSpec extends FunSuite {
   test("refined") {
     val b: Schema[Int] =
       Schema.int.refined(smithy.api.Range(None, Option(BigDecimal(1))))
-    testCaseOpt(b, None)
+    testCaseOpt(b, Some(0))
   }
 
   test("recursive") {

modules/core/src/smithy4s/schema/DefaultValueSchemaVisitor.scala

@@ -89,7 +89,8 @@ private[schema] object DefaultValueSchemaVisitor extends SchemaVisitor[Option] {
   def refine[A, B](
       schema: Schema[A],
       refinement: Refinement[A, B]
-  ): Option[B] = None
+  ): Option[B] =
+    schema.compile(this).flatMap(refinement.apply(_).toOption)
 
   def lazily[A](suspend: Lazy[Schema[A]]): Option[A] =
     suspend.map(_.compile(this)).value

modules/core/src/smithy4s/schema/Schema.scala

@@ -354,8 +354,9 @@ object Schema {
   private object OptionDefaultVisitor extends SchemaVisitor.Default[Option] {
     def default[A] : Option[A] = None
     override def option[A](schema: Schema[A]) : Option[Option[A]] = Some(None)
-    override def biject[A, B](schema: Schema[A], bijection: Bijection[A, B]): Option[B] =
-      this.apply(schema).map(bijection.to)
+    override def biject[A, B](schema: Schema[A], bijection: Bijection[A, B]): Option[B] = {
+      if (schema.hints.has[alloy.Nullable]) None else this.apply(schema).map(bijection.to)
+    }
   }
 
   def operation(id: ShapeId): OperationSchema[Unit, Nothing, Unit, Nothing, Nothing] =

modules/docs/markdown/04-codegen/03-default-values.md

@@ -42,3 +42,26 @@ Here the default for the field `one` will be assumed to be an empty string (`""`
 | resource    | N/A                   |
 
 Not every shape type has a corresponding zero value. For example, there is no reasonable zero value for a structure or a union type. As such, they will not have a zero value set even if they are marked with a null default trait.
+
+## Decoding and defaults
+
+The following table shows different scenarios for decoding of a structure named `Foo` with a single field `s`. The type of the field will differ depending on the scenario. However, the input for each scenario is the same: an empty JSON Object (`{}`). We are using JSON to show this behavior (based on the smithy4s json module), but the same is true of how smithy4s decodes `Document` with `Document.DObject(Map.empty)` as an input.
+
+| Required | Nullable | Null Default | Scala Representation               | Input: {}                    |
+|----------|----------|--------------|------------------------------------|------------------------------|
+| false    | true     | true         | `Foo(s: Nullable[String])`         | Foo(Null)                    |
+| false    | true     | false        | `Foo(s: Option[Nullable[String]])` | Foo(None)                    |
+| false    | false    | true         | `Foo(s: String)`                   | Foo("")                      |
+| false    | false    | false        | `Foo(s: Option[String])`           | Foo(None)                    |
+| true     | false    | false        | `Foo(s: String)`                   | Missing required field error |
+| true     | false    | true         | `Foo(s: String)`                   | Foo("")                      |
+| true     | true     | false        | `Foo(s: Nullable[String])`         | Missing required field error |
+| true     | true     | true         | `Foo(s: Nullable[String])`         | Foo(Null)                    |
+
+#### Key for Table Above
+
+* Required - True if the field is required, false if not (using `smithy.api#required` trait)
+* Nullable - True if the field is nullable, false if not (using `alloy#nullable` trait)
+* Null Default - True if the field has a default value of null, false if it has no default (using `smithy.api#default` trait)
+* Scala Representation - Shows what type is generated for this schema by smithy4s
+* Input: {} - Shows the result of what smithy4s will return when decoding the input of an empty JSON object (`{}`)