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

Support cross-field validation #98

Closed
wants to merge 2 commits into from

Conversation

lnhrdt
Copy link

@lnhrdt lnhrdt commented Apr 18, 2024

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

dhoepelman commented May 10, 2024

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: #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
Copy link
Collaborator

Reading this: #29 (comment)

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

lnhrdt commented May 12, 2024

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.

public operator fun <T> invoke(init: ValidationBuilder<T>.(Context<T>) -> Unit): Validation<T> {
val context = Context<T>()
val builder = ValidationBuilderImpl<T>()
init(builder, context)
val validation = builder.build()
return object : Validation<T> by validation {
override fun validate(value: T): ValidationResult<T> {
context.subject = value
return validation.validate(value)
}
override fun invoke(value: T): ValidationResult<T> {
return validate(value)
}
}
}

In a validator without destructuring, the implementation works. This test passes:

@Test
fun validatingFieldsWithContext() {
val fieldValidation =
Validation<Register> { context ->
Register::password {
addConstraint("cannot equal email") { it != context.subject.email }
}
}
Register(email = "sillyuser@test.com", password = "sillyuser@test.com")
.let { assertEquals(1, countErrors(fieldValidation(it), Register::password)) }
}

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:

@Test
fun validatingFieldsWithDestructuredContext() {
val fieldValidation =
Validation<Register> { (register) ->
Register::password {
addConstraint("cannot equal email") { it != register.email }
}
}
Register(email = "sillyuser@test.com", password = "sillyuser@test.com")
.let { assertEquals(1, countErrors(fieldValidation(it), Register::password)) }
}

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

dhoepelman commented May 13, 2024

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

- Moving Context from ValidationBuilder to Validation
- This commit includes a failing test, destructuring doesn't work yet.
@lnhrdt
Copy link
Author

lnhrdt commented May 13, 2024

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

@dhoepelman
Copy link
Collaborator

dhoepelman commented May 14, 2024

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.

@dhoepelman dhoepelman marked this pull request as draft September 9, 2024 10:02
@dhoepelman
Copy link
Collaborator

Going to go in a different direction, probably building on #65, so closing this. Thanks for the inspiration!

@dhoepelman dhoepelman closed this Sep 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.

2 participants