Skip to content

Validators for collections#1879

Open
msosnicki wants to merge 9 commits intodisneystreaming:series/0.18from
msosnicki:validated-newtypes-collections
Open

Validators for collections#1879
msosnicki wants to merge 9 commits intodisneystreaming:series/0.18from
msosnicki:validated-newtypes-collections

Conversation

@msosnicki
Copy link
Contributor

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)
  • Added alloy compliance tests (when simpleRestJson protocol behaviour is expanded/updated)
  • Updated dynamic module to match generated-code behaviour
  • Added documentation
  • Updated changelog

@msosnicki msosnicki marked this pull request as draft January 29, 2026 14:57
memberRefinements = memberRefinements
)

override def toSchema(a: Schema[A]): Schema[B] = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct in the current implementation. See for example how ValidatedString changed - the underlyingType contains schema with all refinements attached to it. We also have validator with the same schema information duplicated. Then we have implicit val schema = validator.toSchema(underlyingSchema).
So the resulting schema will have refinements applied twice.
I removed that duplication, but that means that the Validator.toSchema` is just applying the outermost bijection, which in my opinion defeats it's purpose.


}

@deprecated
Copy link
Contributor Author

@msosnicki msosnicki Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo: add details for all deprecations + update changelog

def validate(value: A): Either[String, B]

// todo: deprecate
def toSchema(a: Schema[A]): Schema[B]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this method is a bit problematic, especially the more complex scenario there is. For example, see the code generated for ValidatedRefinedListConstrainedMember.

The underlyingType contains the full schema besides bijection, and I haven't changed that.

So doing the schema = validator.toSchema(underlyingSchema) where the validator contains all the schema bindings kind of duplicated. There was a bug present before (see here) that was wrapping schemas in refinements multiple times and after fixing it the validator is just applying the outermost bijection.

package smithy4s

sealed trait Validator[A, B] {
sealed trait Validator[A, B] { self =>
Copy link
Contributor Author

@msosnicki msosnicki Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried not to change the current API too much, but given the issues highlighted in the other comments, I think that having a simpler interface, just:

trait Validator[A] {
  def validate(a: A): Either[String, A]
}

and having a schema visitor Schema ~> Validator that works for any type would be much better. at least at first glance. We would get rid of the duplication of schema bindings that are basically the same in the Schema and Validator members, and this would open up possibility to validate any type, not just the ones that the @validatedNewtype is restricted to.
Not sure how it would fit the 0.18 though

@msosnicki msosnicki marked this pull request as ready for review January 29, 2026 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant