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
Validated newtypes #1454
base: series/0.19
Are you sure you want to change the base?
Validated newtypes #1454
Conversation
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 with |
Working on that 👍 |
@Baccata @kubukoz 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 @length(min: 1)
string httpQuery which when converted from smithy to IR is interpreted as a validated newtype. As a result the hint in
As a workaround I'm excluding the |
I see. This is probably not specific to If the default is "false" (unvalidated newtypes), then you can only go from 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 |
|
||
def apply(a: A): Either[String, Type] | ||
|
||
def unsafeApply(a: A): Type = a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: (for discussion): should we make this throw in case of invalid values? We could also have both variants available, although the "just wrap, not questions asked" one should be so obscure that nobody will accidentally use it.
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.
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). |
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
hmm, so that way users could still apply the meta-trait on a case-by-case basis even if metadata is
Not sure what you mean here - I was thinking of using the |
Well, thing is in order to use the generated metadata, you'd basically have to store a
To be transparent, my concern also has to do with the following aspects :
|
It's already on 0.19 :) |
My bad, haha |
I'm currently exploring this option and have a question about how the I have a My initial attempt was to basically do the same thing we do for 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 |
@Baccata I had a brief discussion with Denis, we're thinking to "just":
That should resolve the double-loading problem, and the configuration continues to live in the Smithy files. Did we get this right? |
That's sensible, actually ! |
protected val validators: List[A => Either[String, A]] | ||
|
||
protected def validateInternal[C](c: C)(value: A)(implicit | ||
ev: RefinementProvider.Simple[C, A] | ||
): Either[String, A] = | ||
ev.make(c).apply(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not liking this. For starters, the validateInternal
re-instantiates the refinement on a per-element basis. Also, the fact that a validation may be composed of several validators is an implementation detail that should not appear in the interface of NewTypeValidated
You could create a smithy4s.Validator
interface that Refinement
would extend, and enrich it with a composition method (andThen).
The renderer would be responsible for rendering something that could look like
val validator : Validator[A] = Validator.of[A].validating(C1).and(C2).and(C3).build()
def apply(a: A) : Either[String, Type] = {
validator(a)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done now, could you please take another look?
Let me know if matches what you had in mind.
package smithy4s | ||
|
||
trait Validator[A, B] { | ||
def validate(value: A): Either[String, B] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: given that a validator can potentially include multiple refinements, should we support EitherNel
/ Either[::[String]
(due to the lack of a better NEL in the stdlib) from day 1, with error accumulation? A fail-fast alternative can also be given (with the current shape), for convenience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this, but decided against it initially due to a lack of a NonEmptyList
. Forcing the caller to deal with a result of Left(Nil)
felt kinda awkward, even though it would technically never happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: commenting on a random spot, but we need a CHANGELOG entry. For Dynamic, I don't think we need to do anything but correct me if I'm wrong please @Baccata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an entry to the CHANGELOG.
modules/codegen/src/smithy4s/codegen/internals/CodegenImpl.scala
Outdated
Show resolved
Hide resolved
case ( | ||
changed, | ||
( | ||
wildcardArg, | ||
shouldGenerateOptics, | ||
shouldRenderValidatedNewtypes | ||
) | ||
) => | ||
val lastOutput = Tracked.lastOutput[Boolean, Seq[File]]( | ||
cacheFactory.make("smithy4sGeneratedSmithyFilesOutput") | ||
) { case (changed, prevResult) => | ||
if (changed || prevResult.isEmpty) { | ||
val file = | ||
(config / smithy4sGeneratedSmithyMetadataFile).value | ||
IO.write( | ||
file, | ||
s"""$$version: "2" | ||
|metadata smithy4sWildcardArgument = "$wildcardArg" | ||
|metadata smithy4sRenderOptics = $shouldGenerateOptics | ||
|metadata smithy4sRenderValidatedNewtypes = $shouldRenderValidatedNewtypes | ||
|""".stripMargin | ||
) | ||
Seq(file) | ||
} else { | ||
prevResult.get | ||
} | ||
} | ||
lastOutput(changed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: I'd extract some of this to avoid the current nesting level, which is becoming a bit unwieldy
Co-authored-by: Jakub Kozłowski <kubukoz@gmail.com>
Add an option to render constrained newtypes over Smithy primitives as "validated" newtypes.
Closes #966
PR Checklist (not all items are relevant to all PRs)