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

[kubukoz]

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:

• Dynamic will accept the input in all cases, but only Person3 will have a field present with the default value. The others will simply not have that field.

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]

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]

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.

[kubukoz]

this relates to #867, I think. Just noting because there was no link between them

[ghostbuster91]

I stared looking into this and I have some questions.

Is the below correct? Should the presence of empty @default affect the target scala type? For the first case we would like to have:

final case class Person(item: Option[String] = None)
string.field[Person]("item", _.item).addHints(smithy.api.Default(smithy4s.Document.DNull))),

Then, for the required nullable we would have:

final case class Person(item: Nullable[String] = Nullable.Null)
string.field[Person]("item", _.item).addHints(smithy.api.Default(smithy4s.Document.DNull)))

and for optional nullable:

final case class Person(item: Option[Nullable[String]] = None)
string.field[Person]("item", _.item).addHints(smithy.api.Default(smithy4s.Document.DNull)))

all of the above would accept the {"item": null} payload.

Regarding @kubukoz question "There's surely a bug here because Dynamic and Codegen don't work the same way, but which should it be?"

I think that given the above encodings all of the original smithy definitions should accept that input and they should result in following runtime values:

smithy	scala	payload	value
@default String	Option[String]	{"item": null}	None
String	Option[String]	{"item": null}	None
@default("") String	String	{"item": null}	""

[kubukoz]

We had a brief sync on this, and the current thinking between me & @ghostbuster91 is that:

• only @nullable should be affected by @default(null)
• everything else should treat it as equivalent to "no default" as per spec

which means the "empty strings" functionality should go away. @Baccata would that be satisfactory?

[Baccata]

We had a brief sync on this, and the current thinking between me & @ghostbuster91 is that [...]. @Baccata would that be satisfactory?

Yes, I think we can probably roll with that

[Baccata]

To elaborate on my expectation for the resolution :

• @default(null); @nullable should result in the rendering of the smithy.api.Default(Document.DNull) hint, and the corresponding Scala-parameter should be set to Nullable.Null at the case-class level.
• @default(null) should result in the rendering of smithy.api.Default(Document.DNull), but no default parameter should be rendered at the case-class level. In the absence of @required, the field would be rendered as an optional field.
• The smithy4s-core logic should be amended to treat DNull as the presence of a None value when @nullable is present, but the absence of a value when @nullable is absent.

I do not want for @default(null) to be eluded from the rendered hints : it is useful for our @nullable usecase, and may be useful for other usecases down the line (we can never predict these things). Moreover, rendering the hint means we're keeping a faithful representation of the smithy model.

[kubukoz]

#1667, #1696 - this is resolved in 0.19

[kubukoz]

actually...

structure Person3 {
  item: String = ""
}

an item: null still fails to decode here. Codegen only, 0.18 and 0.19 alike. I think that's very wrong and should be fixed.