Fix handling of non-Option optionals in field filtering (#1662)

* Fix handling of non-Option optionals

* Flip boolean

* Fix nullable support

* Add new test for the fix

* Add new suite

* unused import

* add changelog

* Move nullability handling to option schemas

* Move to a smaller match

* Add more cases of funky optionals in the json tests

* add comment

* Fix behavior for optional nullables

* Add tests for the nullable fix

Author: kubukoz

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.31
+
+* Fix an issue with `FieldFilter`'s handling of optional fields that aren't represented by an actual `Option` (e.g. bijections) in [#1662](https://github.com/disneystreaming/smithy4s/pull/1662)
+
 # 0.18.30
 
 * Add utilities for Service.Builder in [#1644](https://github.com/disneystreaming/smithy4s/pull/1644)

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

@@ -0,0 +1,44 @@
+package smithy4s
+
+import munit.FunSuite
+import smithy4s.schema.FieldFilter
+import smithy4s.schema.Field
+import munit.Location
+
+class FieldFilterSpec extends FunSuite {
+
+  test("SkipUnsetOptions should remove Nones") {
+    check(Schema.string.option, None, false)
+  }
+
+  test("SkipUnsetOptions should remove Nones bijected to something else") {
+    case class OptionalLike[+A](underlying: Option[A])
+
+    check(
+      Schema.string.option.biject(OptionalLike(_))(_.underlying),
+      OptionalLike(None),
+      false
+    )
+  }
+
+  test("SkipUnsetOptions should keep null nullables") {
+    check(Schema.string.nullable, Nullable.Null, true)
+  }
+
+  test("SkipUnsetOptions should skip a None if it contains a nullable") {
+    check(Schema.string.nullable.option, None, false)
+  }
+
+  test("SkipUnsetOptions should keep optional null nullables when present") {
+    check(Schema.string.nullable.option, Some(Nullable.Null), true)
+  }
+
+  private def check[A](schema: Schema[A], value: A, expectedToKeep: Boolean)(
+      implicit loc: Location
+  ) = {
+    val filter =
+      FieldFilter.SkipUnsetOptions.compile(Field("label", schema, identity[A]))
+    assertEquals(filter(value), expectedToKeep)
+  }
+
+}

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

@@ -16,6 +16,13 @@
 
 package smithy4s.schema
 
+import smithy4s.~>
+import alloy.Nullable
+import smithy4s.schema.Schema.OptionSchema
+import smithy4s.schema.Schema.BijectionSchema
+import smithy4s.schema.Schema.RefinementSchema
+import smithy4s.schema.Schema.LazySchema
+
 trait FieldFilter { self =>
   def compile[S, A](
       field: Field[S, A]
@@ -158,12 +165,38 @@ object FieldFilter {
 
   val SkipNonRequiredDefaultValues: FieldFilter = skipNonRequiredDefaultValues
 
-  private case object skipUnsetOptions extends FieldFilter.SkipNonRequired {
-    def compileNonRequired[S, A](field: Field[S, A]): Predicate[A] = { a =>
-      a != None
+  private object IsNoneVisitor extends (Schema ~> Predicate) {
+
+    def apply[A](schema: Schema[A]): Predicate[A] = schema match {
+      // nullables are technically never None, so we fall through
+      case OptionSchema(underlying) =>
+        if (underlying.hints.has(Nullable) && !underlying.isOption)
+          Function.const(false)
+        else
+          _ == None
+
+      case BijectionSchema(underlying, bijection) =>
+        this(underlying).compose(bijection.from)
+
+      case RefinementSchema(underlying, refinement) =>
+        this(underlying).compose(refinement.from)
+
+      // technically, realistically this case is probably not reachable
+      // because a recursive schema be wrapped in an option first, and wouldn't be traversed by this visitor.
+      // could possibly be reached from a recursive list/map
+      case LazySchema(suspend) =>
+        val underlying = suspend.map(this(_))
+        v => underlying.value(v)
+
+      case _ => Function.const(false)
     }
   }
 
+  private case object skipUnsetOptions extends FieldFilter.SkipNonRequired {
+    def compileNonRequired[S, A](field: Field[S, A]): Predicate[A] =
+      IsNoneVisitor(field.schema).andThen(!_)
+  }
+
   val SkipUnsetOptions: FieldFilter = skipUnsetOptions
 
   val Default = SkipUnsetOptions && SkipNonRequiredDefaultValues

modules/json/test/src/smithy4s/json/JsonCodecApiTests.scala

@@ -22,6 +22,9 @@ import smithy4s.Blob
 import smithy4s.schema.Schema
 import smithy4s.HintMask
 import smithy4s.schema.FieldFilter
+import smithy.api.Length
+import smithy4s.RefinementProvider
+import smithy4s.Nullable
 
 class JsonCodecApiTests extends FunSuite {
 
@@ -82,26 +85,6 @@ class JsonCodecApiTests extends FunSuite {
     assertEquals(encoded, Blob("""{"a":null}"""))
   }
 
-  test(
-    "explicit nulls should be used when set"
-  ) {
-    val schemaWithJsonName = Schema
-      .struct[Option[String]]
-      .apply(
-        Schema.string
-          .optional[Option[String]]("a", identity)
-      )(identity)
-
-    val capi = Json.payloadCodecs.withJsoniterCodecCompiler(
-      Json.jsoniter.withFieldFilter(FieldFilter.EncodeAll)
-    )
-
-    val codec = capi.encoders.fromSchema(schemaWithJsonName)
-    val encoded = codec.encode(None)
-
-    assertEquals(encoded, Blob("""{"a":null}"""))
-  }
-
   test(
     "explicit nulls should be parsable regardless of fieldFilter setting"
   ) {
@@ -125,4 +108,82 @@ class JsonCodecApiTests extends FunSuite {
     }
   }
 
+  test(
+    "schemas backed by an OptionSchema should be treated same as OptionSchema itself"
+  ) {
+    case class OptionalLike[+A](underlying: Option[A])
+    case class Options(
+        justOption: Option[String],
+        bijectedOption: OptionalLike[String],
+        refinedOption: Option[String],
+        recursive: Option[Options],
+        optionalNullable: Option[Nullable[String]]
+    )
+
+    lazy val schema: Schema[Options] = Schema.recursive {
+      Schema
+        .struct[Options]
+        .apply(
+          Schema.string.optional[Options]("justOption", _.justOption),
+          Schema.string.option
+            .biject(OptionalLike(_))(_.underlying)
+            .field[Options]("bijectedOption", _.bijectedOption),
+          Schema.string.option
+            .validated(Length(max = Some(10)))(
+              RefinementProvider.lengthConstraint(_.fold(0)(_.length))
+            )
+            .field[Options]("refinedOption", _.justOption),
+          schema.optional[Options]("recursive", _.recursive),
+          Schema.string.nullable
+            .optional[Options]("optionalNullable", _.optionalNullable)
+        )(Options.apply)
+    }
+
+    val capi = Json.payloadCodecs.withJsoniterCodecCompiler(
+      Json.jsoniter.withFieldFilter(FieldFilter.SkipUnsetOptions)
+    )
+
+    val encoder = capi.encoders.fromSchema(schema)
+
+    assertEquals(
+      encoder
+        .encode(
+          Options(
+            justOption = None,
+            bijectedOption = OptionalLike(None),
+            refinedOption = None,
+            recursive = None,
+            optionalNullable = None
+          )
+        )
+        .toUTF8String,
+      Blob("{}").toUTF8String
+    )
+
+    assertEquals(
+      encoder
+        .encode(
+          Options(
+            justOption = Some("a"),
+            bijectedOption = OptionalLike(Some("a")),
+            refinedOption = Some("a"),
+            recursive = Some(
+              Options(
+                justOption = None,
+                bijectedOption = OptionalLike(None),
+                refinedOption = None,
+                recursive = None,
+                optionalNullable = None
+              )
+            ),
+            optionalNullable = Some(Nullable.Null)
+          )
+        )
+        .toUTF8String,
+      Blob(
+        """{"justOption":"a","bijectedOption":"a","refinedOption":"a","recursive":{},"optionalNullable":null}"""
+      ).toUTF8String
+    )
+  }
+
 }