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

Fix nullable decoding #1637

Merged
merged 6 commits into from
Jan 21, 2025
Merged

Fix nullable decoding #1637

merged 6 commits into from
Jan 21, 2025

Conversation

lewisjkl
Copy link
Contributor

@lewisjkl lewisjkl commented Jan 16, 2025

Creating as a draft because I think the "solution" I have here may not be the right way to go at this, but wanted to get some thoughts about this in general before continuing. The issue is that if you have, for example, a Smithy structure that looks like:

structure Foo {
  @required
  @nullable
  str: String
}

Smithy4s will decode the JSON payload {} into Foo(Nullable.Null). I would think this is incorrect since the field is required and has no default value (if it had a default value of null, then it'd be working as intended).

I worked on tracking the issue down, and figured out that it is because our Nullable schema is a Bijection to Option. So when the JCodec Schema Visitor goes looking for the default value, it returns Some(Nullable.Null) rather than None for this case. I (at least temporarily) fixed this by modifying the bijection method of the OptionDefaultVisitor, but I'm not sure whether or not this approach is sound.


In this PR, I also investigated the case of:

structure Foo {
  @required
  // @nullable // tested both with and without nullable
  str: RefinedType = null
}

// would have refinement trait on it
string RefinedType

The behavior for this was not as expected, for example in the non-nullable case {} would result in Missing required field error. This is incorrect since in this case it should give an empty string (assuming empty string is a valid part of the domain set of possible values for the refinement type).


PR Checklist (not all items are relevant to all PRs)

  • Added unit-tests (for runtime code)
  • Added bootstrapped code + smoke tests (when the rendering logic is modified)
  • Added build-plugins integration tests (when reflection loading is required at codegen-time)
  • Added alloy compliance tests (when simpleRestJson protocol behaviour is expanded/updated)
  • Updated dynamic module to match generated-code behaviour
  • Added documentation
  • Updated changelog

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

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.

@@ -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.

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

Choose a reason for hiding this comment

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

Looks like this test case was actually asserting the wrong thing. In this case, a null default on a non-nullable field means that the default value is assumed to be 0

@lewisjkl
Copy link
Contributor Author

Before merging this, I think it will be helpful if I add documentation that shows each different combination of required, nullable, and null default and what happens (probably some tests to show this too).

@Baccata Baccata marked this pull request as ready for review January 21, 2025 16:06
@Baccata Baccata merged commit 9a12e32 into series/0.18 Jan 21, 2025
11 checks passed
@Baccata Baccata deleted the fix-nullable-decoding branch January 21, 2025 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants