Validated newtypes

[denisrosca]

Add an option to render constrained newtypes over Smithy primitives as "validated" newtypes.

Closes #966

• Add validated newtypes
• Make validated newtypes rendering opt-in via metadata
• Handle primitives

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)
• Updated dynamic module to match generated-code behaviour
• Added documentation
• Updated changelog

[Baccata]

Hey @denisrosca , thanks for looking into this.

However : validated newtypes should not be rendered by default, for the simple reason that it'd break a lot of user code and that's not really acceptable now that smithy4s has received a reasonable amount of adoption. So it should be an opt-in, controlled by means of smithy metadata (with a key prefixed withsmithy4s)

[denisrosca]

Hey @denisrosca , thanks for looking into this.

However : validated newtypes should not be rendered by default, for the simple reason that it'd break a lot of user code and that's not really acceptable now that smithy4s has received a reasonable amount of adoption. So it should be an opt-in, controlled by means of smithy metadata (with a key prefixed withsmithy4s)

Working on that 👍
Just wanted to create the PR as early as possible to get some 👀 on the newtype encoding.

[denisrosca]

@Baccata @kubukoz
I've hit a bit of a problem after adding the metadata conditional.

The main issue is that now the same smithy source file can be rendered in different ways depending if the metadata flag is set to true or false.

Consider this smithy definition:

namespace newtypes.validated

use smithy4s.meta#unwrap
use alloy#simpleRestJson

@length(min: 1, max: 10)
string ValidatedCity

@length(min: 1, max: 10)
string ValidatedName

@unwrap
@length(min: 1, max: 10)
string ValidatedCountry

structure Person {
  @httpLabel
  @required
  name: ValidatedName

  @httpQuery("town")
  town: ValidatedCity

  @httpQuery("country")
  country: ValidatedCountry
}

which gets rendered to:

final case class Person(name: ValidatedName, town: Option[ValidatedCity] = None, country: Option[String] = None)

object Person extends ShapeTag.Companion[Person] {
  val id: ShapeId = ShapeId("newtypes.validated", "Person")

  val hints: Hints = Hints.empty

  implicit val schema: Schema[Person] = struct(
    ValidatedName.schema.required[Person]("name", _.name).addHints(smithy.api.HttpLabel()),
    ValidatedCity.schema.optional[Person]("town", _.town).addHints(smithy.api.HttpQuery.unsafeApply("town")),
    ValidatedCountry.underlyingSchema.optional[Person]("country", _.country).addHints(smithy.api.HttpQuery.unsafeApply("country")),
  ){
    Person.apply
  }.withId(id).addHints(hints)
}

This fails to compile because smithy.api#httpQuery is defined like this:

@length(min: 1)
string httpQuery

which when converted from smithy to IR is interpreted as a validated newtype. As a result the hint in Person.schema is rendered via a call to smithy.api.HttpQuery.unsafeApply which leads to a compilation error:

[info] [error] /private/var/folders/_z/c52c1q4n0fj6617nkht2pgcr0000gn/T/sbt_59b16f00/target/scala-2.13/src_managed/main/scala/newtypes/validated/Person.scala:18:89: value unsafeApply is not a member of object smithy.api.HttpQuery
[info] [error]     ValidatedCity.schema.optional[Person]("town", _.town).addHints(smithy.api.HttpQuery.unsafeApply("town")),

As a workaround I'm excluding the smithy.api namespace from the validated newtypes feature, but that doesn't seem like a bullet proof solution.

[kubukoz]

I see. This is probably not specific to smithy.api, but a symptom of a more general problem: we should make sure that we refer to newtypes in the same way they were generated, even across codegen runs (e.g. smithy4s runs its own, then you have a library on top, then you have the user code).

If the default is "false" (unvalidated newtypes), then you can only go from false (nothing) to true (by setting the metadata key). Perhaps keeping the "previous value" in smithy4sGenerated metadata would be the way?

Shapes from a known previously-generated namespace would be referred to in the style they were generated, and the shapes in the currently-being-generated compilation unit would use the metadata key.

Regardless of whether we do it this way or another, I think smithy.api could get the validated treatment from day 1.

[Baccata]

If anything, this is yet another argument for rendering dynamic bindings instead of static ones. As much as I hate to say it, the arguments in favour of dynamic bindings are starting to largely outweigh the ones in favour of static binding.

Regardless of whether we do it this way or another, I think smithy.api could get the validated treatment from day 1.

I disagree (somewhat heavily). I appreciate that validated newtypes may be a desirable feature for users, but it's also not something I want to deal when writing unit tests in smithy4s that involve hints, and arguably the maintainers of smithy4s are the ones who are most expose to the shapes coming from the "smithy.api" namespace ... unless you could macro-ify the validation when dealing with literals.

Putting aside the "should we just give up and render dynamic bindings", what we could do is create a meta-trait that would trigger the render of newtypes as validated. The metadata flag would indicate that a model preprocessor should be run to add the meta trait to all relevant shapes. The meta-trait would be stored in the resource of published artifacts, alongside the generated code, and could be inspected by downstream code-gen runs (unlike smithy4s-prefixed metadata, which get eluded).

[kubukoz]

it's also not something I want to deal when writing unit tests in smithy4s that involve hints

We discussed this briefly with Denis, I suggested to try and see how much stuff breaks: if it's more than a handful of locations let's keep them as-is, but if it's just a few I'd say unsafeApply is perfectly fine. Let's be pragmatic ;)

The metadata flag would indicate that a model preprocessor should be run to add the meta trait to all relevant shapes

hmm, so that way users could still apply the meta-trait on a case-by-case basis even if metadata is false / undefined. That'd be cool, I think.

The meta-trait would be stored in the resource of published artifacts, alongside the generated code, and could be inspected by downstream code-gen runs (unlike smithy4s-prefixed metadata, which get eluded).

Not sure what you mean here - I was thinking of using the smithy4sGenerated metadata, which does get inspected by downstream codegen runs.

[Baccata]

Not sure what you mean here - I was thinking of using the smithy4sGenerated metadata, which does get inspected by downstream codegen runs.

Well, thing is in order to use the generated metadata, you'd basically have to store a Map[ShapeId, Something], which is somewhat redundant where you could have a @something trait instead, which you could inspect from the shapes itself without having to thread the map where it needs to be inspected. It's arguably more idiomatic to smithy to use traits to store metadata related to specific shapes.

but if it's just a few I'd say unsafeApply is perfectly fine

To be transparent, my concern also has to do with the following aspects :

1. binary compatibility. If you want smithy.api and alloy shapes to be validated newtypes, it'd unavoidably means moving this PR to 0.19, as it'll be breaking bincompat of generated traits.
2. convenience of using hints in annotations. This concern is tied to my prototype https://github.com/Baccata/smithy4s-deriving. The inconvenience of validated types in hints would then be felt by end users.

[kubukoz]

moving this PR to 0.19

It's already on 0.19 :)

[Baccata]

My bad, haha

[denisrosca]

Putting aside the "should we just give up and render dynamic bindings", what we could do is create a meta-trait that would trigger the render of newtypes as validated. The metadata flag would indicate that a model preprocessor should be run to add the meta trait to all relevant shapes. The meta-trait would be stored in the resource of published artifacts, alongside the generated code, and could be inspected by downstream code-gen runs (unlike smithy4s-prefixed metadata, which get eluded).

I'm currently exploring this option and have a question about how the metadata flag would be passed to the generator.

I have a ValidatedNewtypesTransformer model transformer that, when enabled, adds a new @validateNewtypeConstraints (name is still up for debate) trait to the relevant shapes.

My initial attempt was to basically do the same thing we do for smithy4sRenderOptics (i.e. have a setting in the build plugin that gets written to generated-metadata.smithy), but that presents a bit of a problem.
The issue is that the decision if ValidatedNewtypesTransformer should be executed needs to happen before loading the smithy model, but that information is in generated-metadata.smithy which is read by the model loader.

What do you think about making this setting a full-blown generator argument instead of passing it via the generated metadata file?

[Baccata]

What do you think about making this setting a full-blown generator argument instead of passing it via the generated metadata file?

You make a good point, it'd be silly to load the model twice. However, I'd like to avoid multiplying codegen arguments which need to be forwarded to 3 different "front-ends" (SBT/Mill/cli), in favour of allowing some level of configuration via the reading of a smithy-build.json file. Interestingly, there was a discussion this week about this.

[kubukoz]

@Baccata I had a brief discussion with Denis, we're thinking to "just":

• make the transformer run unconditionally (like e.g. AwsConstraintsRemover), but
• make it check the metadata key before applying any changes

That should resolve the double-loading problem, and the configuration continues to live in the Smithy files. Did we get this right?

[Baccata]

That's sensible, actually !

[Baccata]

this class and ValidatorBuilder have exactly the same data structure. There's no reason to separate the Validator with its Builder at all then, as you could very well just have a def alsoValidate[C](constraint: C)(implicit ...) : ValidatorBuilder[A, B] directly on the Validator interface.

[denisrosca]

Reworked this a bit and got rid of the multiple "builders" and added, as suggested, a new alsoValidate method.

[Baccata]

If this is intended to be some configuration, this is not the right place for it. That'd be here :

See

smithy4s/modules/codegen/src/smithy4s/codegen/internals/Renderer.scala

Lines 43 to 46 in 89e88c9

	case class Config(
	errorsAsScala3Unions: Boolean,
	wildcardArgument: String,
	renderOptics: Boolean

[denisrosca]

This is not meant as "configuration", but more of a record of how namespaces where previously generated.

I'm using this in the ValidatedNewtypesTransformer to make sure that namespaces that were previously generated using this new feature have the proper @validateNewtype trait applied.

Does that make sense?

[Baccata]

I see. Yeah I guess that works 👍. The approach I would have preferred would have been to generate an apply @validatedNewType statement in a bespoke file that would get packaged with the artifact, but I think your approach is sound too, and arguably requires less work.

[Baccata]

You're gonna hate me, but the schema of NewtypeValidated should not be a bijection, it should be a refinement (or a series of refinements)

[Baccata]

To be more precise : the schema should not call upon asBijectionUnsafe.

I think the easiest way to achieve this would be to add a def toSchema(a: Schema[A]) : Schema[B] to the Validator[A, B] interface, because the ValidatorImpl keeps everything that you'll need to formulate the correct schema : a list of refinements and a bijection to apply at the end.

[Baccata]

I think ValidatedNewtype sounds better (as in, it's correct English)