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

Compose validation builders #63

Closed
wants to merge 13 commits into from

Conversation

gmulders
Copy link

This branch features a list of changes:

Composition of ValidationBuilders

Currently there is the use of something called PropKey to register builders. A PropKey contains a KProperty and an enum value that explains what type of Validation to build. This is not really extensible.

I refactored it to use the Composition pattern. A ValidationBuilder can be composed with another that adds an extra level of features.

This was done so that in the future we can add different algebraic operators (think oneOf, lazy, eager, ...)

Validation on Java functions

It also enables us to create validations on Java functions, because that is abstracted away in a MappedValidationBuilder that works allows us to switch from a validation over a type T to a type V where (T) -> V.

Custom error types

Now the errors are always strings. This is great in 99% of the cases, but in some cases I'd like to add some context to the error. Thus the error type is now generic.

enum class Errors { ONE, TWO, THREE }
val validation = Validation<String, Register, Errors> {
    Register::referredBy {
        pattern(staticHint(TWO), ".+@.+")
    }
}

The static function returns a HintBuilder;

typealias HintArguments = List<Any>
typealias HintBuilder<C, T, E> = C.(T, HintArguments) -> E

Note the default error type is String, this means that almost no breaking changes were introduced.

Required as Constraint

It was not possible to add a hint to the required validation (since it was no Constraint).

The composition pattern makes it easy to rebuild this. So now we can add a hint after a required block:

SomeObject::optionalValue required with {
    // validate non-null value
} hint stringHint("whoops!")

or

SomeObject::optionalValue required with(stringHint("whoops!")) {
    // validate non-null value
}

Note the with function creates an object that combines a hint with an init function for a ValidationBuilder. This is the most noticeable breaking change.

Improved type safety

To catch type errors on compile time instead runtime, a number of functions have improved type safety

  • maxItems
  • minItems
  • uniqueItems

Breaking changes

  • Introduction of with function for required builder
  • Extra type arguments needed when adding extension functions to ValidationBuilder<A, B, C>
  • addConstraint has different signature
  • More?

README

The readme is updated to reflect the latest changes, but needs some changes to elaborate on:

  • Context
  • Custom error types

Extra notes on the changes

I know there are a lot of changes in this branch and understand that it is a lot to process in one go. I also believe that the essence of Konform is still intact, proven by the strong backwards compatibility.

Please consider merging this branch.

@nlochschmidt
Copy link
Member

Thanks again for the contribution.

I think it makes sense to first figure out what to do with #61 as this PR includes the same changes.

Initial impression is that this simplifies a couple of things in the builder, which I am very glad about and really hope to see integrated. Also if this could help to solve #14 that would be much appreciated

I also think the addition of an error type E is great, that said as with #61 I don't see it composing well yet. It looks rather like all the errors need to be the same in one Validation and I don't see an example in the tests dealing with different error types in different constraints within the same validation.

Maybe I am missing something?

}

interface ConstraintBuilder<C, T, E> {
infix fun hint(hint: HintBuilder<C, T, E>) : ConstraintBuilder<C, T, E>
Copy link
Member

Choose a reason for hiding this comment

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

It should be possible to define an extension function to allow the old hint "error message" in addition to hint stringHint("error message". Maybe stringHint could be internal.

infix fun <C,T> ConstraintBuilder<C,T,String>.hint(hint: String) = hint(stringHint(hint))

Copy link
Author

Choose a reason for hiding this comment

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

Nice one!

I added this extension function and one on ValidationBuilder:
fun <C, T> ValidationBuilder<C, T, String>.addConstraint(hint: String, vararg values: Any, test: C.(T) -> Boolean): ConstraintBuilder<C, T, String>

I would keep stringHint public. It provides the "templated" approach that was previously in the NodeValidation. I can imagine that it is very useful when creating your own constraint extensions.

internal abstract fun <R> onEachArray(prop: KProperty1<T, Array<R>>, init: ValidationBuilder<R>.() -> Unit)
infix fun <R> KProperty1<T, Iterable<R>>.onEach(init: ValidationBuilder<C, R, E>.() -> Unit) = onEachIterable(this.name, this, init)
@JvmName("onEachIterable")
infix fun <R> KFunction1<T, Iterable<R>>.onEach(init: ValidationBuilder<C, R, E>.() -> Unit) = onEachIterable(this.name, this, init)
Copy link
Member

Choose a reason for hiding this comment

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

This and the other extension functions on KFunction1 are not tested as far as I can see.

Copy link
Author

Choose a reason for hiding this comment

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

You are right. I added some tests in a jvmTest module.

@gmulders
Copy link
Author

🤔 I think you are right about not being able to compose different types of errors yet. I'll check if there is some graceful solution for this.

Just as in the other merge request; composition is only needed if one decides to mix different error types. If one sticks to the default (String) nothing changes.

The way around this (used in this branch) is to include the HintBuilder as an argument for the constraint function and to overload the constraint function with a function where stringHint(...) is called. So when using your own error type, you always need to call the constraint functions with your own HintBuilder. See test validatingRequiredFieldsWithCustomErrorType as an example of this.

As for #14, this is indeed solved in this branch, as we can now build validations on KFunction1<A, B>.

- add hint extension fn to ConstraintBuilder to support `hint "error message"` if E is String
- add addConstraint to ValidationBuilder to support `addConstraint("string", .., ..)` if E is String
@gmulders
Copy link
Author

So I thought about the mapping between two error types and I think that it is not possible but not needed.

Let me elaborate: since it is a custom error type we don't know what it is dependent of; e.g. an enum is dependent on nothing, but a custom error type could be dependent on

  • value on test,
  • parameters for the test and
  • context

If the error type where we want to map away from is String, and the custom error type is dependent on the context, then there is no way to easily map one to the other.

However, I think this is not needed. The constraint functions in Konform are all written in a way that they support the HintBuilder (and overloaded for the default String case). Anyone building its own constraint functions should either do it the same way or use their own custom type.

So although we can add a map function argument to the run function, to map the errors from the From type to the To type, it will miss the value on test and the parameters for the test.

Besides, there is a better alternative for using pre-build validations; constraint functions.

@nlochschmidt nlochschmidt changed the base branch from master to main March 29, 2024 11:47
@dhoepelman
Copy link
Collaborator

This PR has a couple of problems:

  • Very large with many breaking changing to the API
  • Incompatible with the current version

I haven't looked in depth at the problems this PR tries to solve, but this is a bit more than I am willing to spend time on at the moment. Feel free to resubmit the PR(s) in smaller batches against the latest version or bring it up in an issue, so I can take a look again.

@dhoepelman dhoepelman closed this May 10, 2024
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.

3 participants