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

Support configuration of explicit defaults in more encoders #1290

Closed
1 of 3 tasks
kubukoz opened this issue Nov 7, 2023 · 2 comments · Fixed by #1315
Closed
1 of 3 tasks

Support configuration of explicit defaults in more encoders #1290

kubukoz opened this issue Nov 7, 2023 · 2 comments · Fixed by #1315
Labels
good first issue Good for newcomers

Comments

@kubukoz
Copy link
Member

kubukoz commented Nov 7, 2023

In smithy4s, JSON writers and the SimpleRestJson builder have a configuration option that controls how default values are handled on the write side: https://disneystreaming.github.io/smithy4s/docs/protocols/simple-rest-json/overview#explicit-null-encoding

Note: the documentation is a bit stale, as it only mentions optional fields and nulls. In reality, the behavior of the @default trait is also controlled by this option: when disabled, a value that matches the default will simply be omitted from the payload. The documentation should be updated to reflect this.

Also, the combination of optional + @default makes the field essentially required - with the explicit defaults setting on, you won't see a null in this case, but rather the default value.

All this is only limited to JSON - this feature should also be supported in other encoders.

Here are the ones I know of where it'd make sense:

  • SchemaVisitorMetadataWriter: this is used to write HTTP headers, query parameters. Ends up being surfaced via Metadata.Encoder.
  • SchemaVisitorPathEncoder: writes HTTP path segments. This already always writes default values, otherwise URIs might be invalid.
  • DocumentEncoderSchemaVisitor: encodes values to documents.

We can also take a look at the others: look for usages of foreachUnlessDefault and getUnlessDefault methods, as not-writing default values seems to be their primary purpose.

For each of the encoders/writers that are missing such a configuration option, add one.

  • this needs to be controllable from a public API, e.g. Metadata.Encoder.withExplicitDefaults(true)
  • the change needs to be backwards compatible, and implemented in a future-aware way, i.e. following https://docs.scala-lang.org/overviews/core/binary-compatibility-for-library-authors.html (e.g. don't expose the copy method of the configuration)
  • the change needs to be documented
  • (possibly contentious point) the existing SimpleRestJsonBuilder configuration should affect these new options within the scope of said builder. I don't have a strong stance on this but it seems to be the most consistent approach.

Feel free to pick up any part of this - the ticket can be implemented incrementally. Just let us know which one you're taking on!

@kubukoz kubukoz added the good first issue Good for newcomers label Nov 7, 2023
@Pooja1672
Copy link

Hi @kubukoz , I would like to work on this issue. Can you please assign this issue to me?

@kubukoz
Copy link
Member Author

kubukoz commented Nov 8, 2023

Thank you! Please ping me if you need any hints.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants