smithy4s icon indicating copy to clipboard operation
smithy4s copied to clipboard

SUGGESTION: guidance for write-side validation of constraints

Open caoilte opened this issue 2 years ago • 13 comments

disclaimer: We're only using smithy4s as an IDL for events so this description is written from that point of view and may need translation in your head to make sense from a more service oriented perspective.

It would be really nice if there was an on rails way to check that smithy case classes have been constructed such that they pass all the constraints defined on the corresponding IDL. I only just realised it wasn't happening when our smithy defined events started failing validation on rehydration. My team will probably solve the problem by writing scalacheck tests that do a round trip on all our event structures (ie check that they can be turned into JSON and back again without erroring). It would be nice if there was an idiomatic way to check at runtime rather than relying on test exhaustivity but perhaps it isn't practical. Nevertheless @kubukoz suggested raising this issue as a placeholder to discuss.

caoilte avatar May 17 '23 09:05 caoilte

This one may be tough. The first thing that came to mind was changing the RefinementSchema type to account for this, but now I'm thinking we would have to make it impossible to construct invalid values in the first place - so, newtypes marked with constraint traits / refinements would have to get smart constructors, which would be the ones that perform the check.

This would be a breaking change, and we'd probably want to offer an unsafe variant just to make it a bit more convenient for cases like testing. Other than that, I don't see anything obvious that would make this non-viable.

kubukoz avatar May 19 '23 00:05 kubukoz

Hi @caoilte, in the current stateit'd be possible to write something like :

trait Validator[A] {
   def validate(a: A) : Either[List[String], A] 
}
object ValidatorSchemaVisitor extends SchemaVisitor[Validator] {
   ...
}  

which would let you derive a validator for any generated type.

but now I'm thinking we would have to make it impossible to construct invalid values in the first place - so, newtypes marked with constraint traits / refinements would have to get smart constructors, which would be the ones that perform the check.

I think I'd welcome this change. @daddykotex or @lewisjkl, any of you want to tackle this ? Or someone on your team, @kubukoz ?

Baccata avatar May 31 '23 09:05 Baccata

I'm happy to take a look at this. A few clarification questions:

  • would all types with refinements get different constructors, or just newtypes?
  • would the new constructor be in addition to the current one, or instead of?

lewisjkl avatar May 31 '23 12:05 lewisjkl

would all types with refinements get different constructors, or just newtypes?

probs just newtypes.

would the new constructor be in addition to the current one, or instead of?

instead of, but the current constructor would be rebranded as unsafe

Baccata avatar May 31 '23 12:05 Baccata

@lewisjkl are you still looking at this? I have a bit of bandwidth and can take this on

yisraelU avatar Jul 25 '23 15:07 yisraelU

@lewisjkl are you still looking at this?

I have a bit of bandwidth and can take this on

Awesome, yeah I totally dropped the ball on this. Appreciate you taking it on.

lewisjkl avatar Jul 25 '23 16:07 lewisjkl

@Baccata are we looking to introduce a SmartNewType or modify the existing Newtype class and change all existing calls to apply to unsafe (will be a lot of generated code changes )

yisraelU avatar Jul 25 '23 16:07 yisraelU

I'm thinking we shouldn't make such a change required for users updating to 0.18.

Can we have a deprecation cycle?

  • Make this the default with opt-out via smithy metadata
  • drop the setting in 0.19 altogether

@yisraelU sorry but this doesn't answer your question 😅

kubukoz avatar Jul 25 '23 22:07 kubukoz

My team is interested in having this sooner than later. @yisraelU @lewisjkl either of you working on it?

kubukoz avatar Feb 21 '24 23:02 kubukoz

My team is interested in having this sooner than later. @yisraelU @lewisjkl either of you working on it?

I haven't been working on this, no. But if I may have some bandwidth open up next week to take a look. But if you have capacity on your side then go for it.

lewisjkl avatar Feb 21 '24 23:02 lewisjkl

@kubukoz I have done some work in it , but waiting on some guidance

yisraelU avatar Feb 21 '24 23:02 yisraelU

I don't have bandwidth to offer guidance on this, besides maybe looking at previous art: https://github.com/monix/newtypes/blob/main/core/shared/src/main/scala/monix/newtypes/NewValidated.scala

Baccata avatar Feb 22 '24 08:02 Baccata

@denisrosca is looking into this. We identified a couple axes of the problem:

  • is the newtype unwrapped or not?
  • is it a collection?
  • is it a constraint trait or a user-defined refinement?
  • are constraints applied on a member or directly on a newtype?

and for now we'll tackle just the most obvious case, "wrapped" (default) newtypes with constraint traits applied directly.

It appears that user-defined refinements do not need any work to be done, because their construction is by design constrained by what the user does in their refinement provider.

kubukoz avatar Feb 28 '24 15:02 kubukoz