Support cross-field validation

[lnhrdt]

After reflecting on my comment in #29 I decided to explore contributing a pull request to support cross-field validation.

So far I have only added a failing test that suggests a possible API to add support. I am seeking feedback from the maintainers:

1. would such a feature be considered
2. any feedback on the proposed API and approach
3. any suggestions on implementation

@nlochschmidt and @dhoepelman I would love to hear from you.

Lastly, if I get familiar enough with this library through this process I would be interested in helping maintain it–I'm a big fan of Konform. <3

[dhoepelman]

hey @lnhrdt, thanks! I like the direction of your solution and agree that this is a sorely lacking feature because many validations currently aren't possible. Looking at your strawman PR API-wise it looks doable.

For maintainership, please reply in: https://github.com/konform-kt/konform/discussions/38 (apologies for the 3 week delay, didn't have my notifications set up yet properly)

bikeshedding, what about subject instead of context? I also think the Context class is superfluous or can be made private.

public abstract class ValidationBuilder<T> {
    public val subject: T get() = // ..
}

API would become:

Validation<Register> {
    Register::password {
        addConstraint("cannot equal email") { it != this@Validation.subject.email }
    }
}
// or if user prefers
Validation<Register> {
    val register = subject
    Register::password {
        addConstraint("cannot equal email") { it != register.email }
    }
}

[dhoepelman]

Reading this: https://github.com/konform-kt/konform/issues/29#issuecomment-1204308854

I like this suggestion a lot. we could pass a context object to the validation builder, the first field of which could be subject. API would become:

Validation<Register> { (register) ->
    Register::password {
        addConstraint("cannot equal email") { it != register.email }
    }
}

// In ValidationBuilder
data class Context(val subject: T)

we can extend the context later if we want, it will not break backwards compatibility because the above snippet will keep working if a field is added.

[lnhrdt]

Thank you for chiming in with your thoughts @dhoepelman!

we could pass a context object to the validation builder, the first field of which could be subject.

I agree this is most compelling. I refactored my PR to implement that design. I've tried to make minimal changes to accomplish the design. Here's the latest.

I found that moving the Context class and its management from ValidationBuilder to Validation fit this design better, as Validation has access to value: T via the validate function, while ValidationBuilder does not.

In the most recent implementation, I've enhanced the validate function by applying wrapping and delegation. This allows the function to capture the subject, which is then used to construct the context object. https://github.com/konform-kt/konform/blob/42b7cab6fe7c25f50ac21823fa6a7e9ef23c78b2/src/commonMain/kotlin/io/konform/validation/Validation.kt#L7-L22

In a validator without destructuring, the implementation works. This test passes: https://github.com/konform-kt/konform/blob/42b7cab6fe7c25f50ac21823fa6a7e9ef23c78b2/src/commonTest/kotlin/io/konform/validation/ValidationBuilderTest.kt#L118-L129

However when I add destructuring to a validator's implementation I run into a problem. The subject is evaluated too soon in the Validation class's lifecycle (i.e. before subject has been provided by invoking validate) and the exception in Context is thrown. This test fails: https://github.com/konform-kt/konform/blob/42b7cab6fe7c25f50ac21823fa6a7e9ef23c78b2/src/commonTest/kotlin/io/konform/validation/ValidationBuilderTest.kt#L131-L142

I'm struggling to find a way to implement this in a way that doesn't more fundamentally change the Validation interface, implementations, and lifecycle. So I thought I'd check in here. Do you see any possible improvements to what I've started?

[dhoepelman]

@lnhrdt can you rebase or merge properly? Bit hard to review now

[lnhrdt]

@dhoepelman good catch! I just cleaned up the commit history with a proper rebase.

[dhoepelman]

I took a look at this and the unfortunate thing is that this is using a var, and not in a thread safe way, so this will lead to race conditions and bugs. Here is a test that should specifically surface these kinds of problems.

I'm struggling to find a way to implement this in a way that doesn't more fundamentally change the Validation interface, implementations, and lifecycle.

This might be worth it, especially if we can find a way to implement this in a backwards compatible way by keeping the old API working.

It's a tricky problem to solve, because the builder will not normally have the subject available, so we do need some kind of "getter", but also make sure that the builder doesn't actually have the subject available yet until validation time (throws exception if you try)

I will take some time to look at this problem myself, but not sure about when I will have time.