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

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

Open
kubukoz opened this issue Sep 10, 2024 · 2 comments
Open

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

kubukoz opened this issue Sep 10, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@kubukoz
Copy link
Member

kubukoz commented Sep 10, 2024

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
Copy link
Member Author

kubukoz commented Sep 10, 2024

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(""))),

@kubukoz kubukoz added the bug Something isn't working label Sep 10, 2024
@Baccata
Copy link
Contributor

Baccata commented Sep 11, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants