disneystreaming / smithy4s

https://disneystreaming.github.io/smithy4s/
Other
351 stars 72 forks source link

Discrepancy of `null` handling when a default is present or not #1581

Open kubukoz opened 2 months ago

kubukoz commented 2 months ago

Compare the following:

structure Person {
  item: String = null
}

structure Person2 {
  item: String
}

structure Person3 {
  item: String = ""
}

Given a payload like {"item": null}, only Person2 will be decoded successfully if you're using generated code.

Noteworthy:

Full repro: https://github.com/kubukoz/demos/tree/smithy4s-nulls-defaults (sbt run shows you all the results)

There's surely a bug here because Dynamic and Codegen don't work the same way, but which should it be? I'm leaning towards thinking that = null should be equivalent to nothing (see #1315), and I'm pretty sure all these cases should accept the input somehow.

kubukoz commented 2 months ago

The difference in generated code:

// Person (null default)
final case class Person(item: String = "")
string.field[Person]("item", _.item).addHints(smithy.api.Default(smithy4s.Document.fromString(""))),

// Person2 (no default)
final case class Person2(item: Option[String] = None)
string.optional[Person2]("item", _.item),

// Person3 (empty default, this is equivalent to Person)
final case class Person3(item: String = "")
string.field[Person3]("item", _.item).addHints(smithy.api.Default(smithy4s.Document.fromString(""))),
Baccata commented 2 months ago

Here's some relevant information :

  1. AWS indeed specifies that defaulting to null should be equivalent to no default.
  2. That being said, the fact that we (smithy4s) have first-class support for alloy#nullable implies that we ought to code-generate the hint as Document.DNull (rather than omitting it), since we diverge from what Michael is saying wrt document shapes.

Looking at dates, it looks like Michael answered after the implementation was merged in smithy4s (we probably made a decision because we were pressed for time).

Considering fixing would be a change to a behaviour that's documented, I think it'd have to go to 0.19 with a very explicit note in the changelog to highlight it.