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

SUGGESTION: guidance for write-side validation of constraints #966

Open
caoilte opened this issue May 17, 2023 · 13 comments
Open

SUGGESTION: guidance for write-side validation of constraints #966

caoilte opened this issue May 17, 2023 · 13 comments

Comments

@caoilte
Copy link

caoilte commented May 17, 2023

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.

@kubukoz
Copy link
Member

kubukoz commented May 19, 2023

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.

@Baccata
Copy link
Contributor

Baccata commented May 31, 2023

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 ?

@lewisjkl
Copy link
Contributor

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?

@Baccata
Copy link
Contributor

Baccata commented May 31, 2023

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

@yisraelU
Copy link
Contributor

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

@lewisjkl
Copy link
Contributor

@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.

@yisraelU
Copy link
Contributor

@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 )

@kubukoz
Copy link
Member

kubukoz commented Jul 25, 2023

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
Copy link
Member

kubukoz commented Feb 21, 2024

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

@lewisjkl
Copy link
Contributor

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.

@yisraelU
Copy link
Contributor

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

@Baccata
Copy link
Contributor

Baccata commented Feb 22, 2024

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

@kubukoz
Copy link
Member

kubukoz commented Feb 28, 2024

@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.

@denisrosca denisrosca mentioned this issue Mar 19, 2024
9 tasks
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

No branches or pull requests

5 participants