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.

Sign up for GitHub

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

Fix nullable decoding #1637

Merged
merged 6 commits into from
Jan 21, 2025
Merged

Fix nullable decoding #1637

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e65f39c
fix issue with nullable required fields decoding
lewisjkl Jan 16, 2025
e5ece71
update for document
lewisjkl Jan 16, 2025
e7cd523
handle combo of refinements with nullable
lewisjkl Jan 17, 2025
d53e5de
update changelog
lewisjkl Jan 17, 2025
081bb3e
update document spec with refinement cases
lewisjkl Jan 17, 2025
a8a6bcd
add tests and docs for decoding of different field types
lewisjkl Jan 19, 2025
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
43 changes: 43 additions & 0 deletions modules/bootstrapped/test/src/smithy4s/DocumentSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -953,6 +953,49 @@ class DocumentSpec() extends FunSuite {
assertEquals(doc, expected)
}

test(
"Required nullable field with NO default should NOT infer null when value missing"
) {
case class Foo(str: Nullable[String])
implicit val fieldSchema: Schema[Foo] =
Schema
.struct[Foo](
Schema.string.nullable
.required[Foo]("str", _.str)
)(Foo.apply)

val result = Document.decode[Foo](Document.DObject(Map.empty))
expect.same(
result,
Left(
smithy4s.codecs.PayloadError(
smithy4s.codecs.PayloadPath.parse(".str"),
"str",
"Required field not found"
)
)
)
}

test(
"Required nullable field WITH null default SHOULD infer null when value missing"
) {
case class Foo(str: Nullable[String])
implicit val fieldSchema: Schema[Foo] =
Schema
.struct[Foo](
Schema.string.nullable
.required[Foo]("str", _.str)
.addHints(smithy.api.Default(Document.DNull))
)(Foo.apply)

val result = Document.decode[Foo](Document.DObject(Map.empty))
expect.same(
result,
Right(Foo(Nullable.Null))
)
}

private def inside[A, B](
a: A
)(assertPF: PartialFunction[A, Unit])(implicit loc: munit.Location) = {
Expand Down
5 changes: 3 additions & 2 deletions modules/core/src/smithy4s/schema/Schema.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,8 +354,9 @@ object Schema {
private object OptionDefaultVisitor extends SchemaVisitor.Default[Option] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I was not able to create any test cases that indicated an implementation for refinements would do anything here. Every case I could think of would still bypass calling the refinement case. The DefaultValueSchemaVisitor did need to be updated however.

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)
Copy link
Contributor Author

@lewisjkl lewisjkl Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

this.apply(schema) will return Some(None), which when doing .map(bijection.to) becomes Some(Nullable.Null). Basically these concepts are not treated the same since Option.empty is the absence of value while Nullable.Null is the inclusion of an explicit Null.

}
}

def operation(id: ShapeId): OperationSchema[Unit, Nothing, Unit, Nothing, Nothing] =
Expand Down
43 changes: 43 additions & 0 deletions modules/json/test/src/smithy4s/json/SchemaVisitorJCodecTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -836,6 +836,49 @@ class SchemaVisitorJCodecTests() extends FunSuite {
expect.same(result, json)
}

test(
"Required nullable field with NO default should NOT infer null when value missing"
) {
case class Foo(str: Nullable[String])
implicit val fieldSchema: Schema[Foo] =
Schema
.struct[Foo](
Schema.string.nullable
.required[Foo]("str", _.str)
)(Foo.apply)

val result = util.Try(readFromString[Foo]("{}")).toEither
expect.same(
result,
Left(
PayloadError(
PayloadPath.parse(".str"),
"str",
"Missing required field"
)
)
)
}

test(
"Required nullable field WITH null default SHOULD infer null when value missing"
) {
case class Foo(str: Nullable[String])
implicit val fieldSchema: Schema[Foo] =
Schema
.struct[Foo](
Schema.string.nullable
.required[Foo]("str", _.str)
.addHints(smithy.api.Default(Document.DNull))
)(Foo.apply)

val result = readFromString[Foo]("{}")
expect.same(
result,
Foo(Nullable.Null)
)
}

case class Patchable(a: Option[Nullable[Int]])

object Patchable {
Expand Down
Loading