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

Bailing between rules on different properties / objects #424

Closed
RomkeVdMeulen opened this issue Mar 21, 2017 · 7 comments
Closed

Bailing between rules on different properties / objects #424

RomkeVdMeulen opened this issue Mar 21, 2017 · 7 comments

Comments

@RomkeVdMeulen
Copy link
Contributor

#301 discussed how bailing between rules on a single property should work, and the API used for this is quite clear. But now I have a different scenario I would like to discuss:

ValidationRules
	.ensure("myProp")
		.required()
	.ensureObject()
		.satisfies(() => expensiveBackendValidation())
	.on(myOptionsObject);

I want to bail on the backend validation of the entire object if any of the property validations fail. Ideally I'd like to write something like:

ValidationRules
	.ensure("myProp")
		.required()
	.then()
	.ensureObject()
		.satisfies(() => expensiveBackendValidation())
	.on(myOptionsObject);

though I admit that's a bit ambiguous (I might have written the same code if I'd intended to add another rule to myProp but forgot).

Should there be some way of ordering validation of different properties / objects and bailing early? And if so, what should that look like?

@RomkeVdMeulen
Copy link
Contributor Author

For now I've come up with this solution:

constructor() {
	this.rules = ValidationRules
		.ensure("myProp")
			.required()
				.tag("client")
		.ensureObject()
			.satisfies(() => expensiveBackendValidation())
				.tag("server")
		.on(this.myOptionsObject)
		.rules;
}

// ...

async save() {
	if (this.validationController.validating) {
		return;
	}

	const clientSideRules = ValidationRules.taggedRules(this.rules, "client");
	if (!(await this.validationController.validate({object: this.myOptionsObject, rules: clientSideRules})).valid) {
		return;
	}
	const serverSideRules = ValidationRules.taggedRules(this.rules, "server");
	if (!(await this.validationController.validate({object: this.myOptionsObject, rules: serverSideRules})).valid) {
		return;
	}

	this.saveTheObject();
}

This seems pretty flexible, and I like the clarity of what's going on here. However it does involve a lot of boilerplate code and I'm not sure what would happen if I add additional rules and forget to tag them.

@jdanyow
Copy link
Contributor

jdanyow commented Mar 25, 2017

@RomkeVdMeulen can you give this a try and let me know if it works for you? If it does, I'll make the API nicer.

change this:

ValidationRules
  .ensure('myProp')
    .required()
  .ensureObject()
    // .then()  // <--- currently we don't have this... :(
    .satisfies(() => expensiveBackendValidation())
  .on(myOptionsObject);

to this:

const ensureObject = ValidationRules
  .ensure('myProp')
    .required()
  .ensureObject();

// do the equivalent of .then()
ensureObject.sequence++;

// continue with rules...
ensureObject
    .satisfies(() => expensiveBackendValidation())
  .on(myOptionsObject);

or this:

ValidationRules.ensureObject()
  .satisfies(() => true)
  .then()
  .satisfies(() => expensiveBackendValidation())
  .on(myOptionsObject);

@RomkeVdMeulen
Copy link
Contributor Author

I tried your first suggestion:

const rules = ValidationRules
	.ensure((options: MyClass) => options.myProp)
		.required()
	.ensureObject();

(<any> rules).sequence++;

rules
	.satisfies(() => this.backendValidation())
	.on(this.options);

which worked great! However, if I add another rule to the property with a higher sequence:

const rules = ValidationRules
	.ensure((options: MyClass) => options.myProp)
		.required()
		.then()
		.satisfies(val => val !== "some_value")
	.ensureObject();

(<any> rules).sequence++;

rules
	.satisfies(() => this.backendValidation())
	.on(this.options);

then this doesn't work as intended: the backend validation now has the same priority as the custom satisfies call, while what I want to happen is if either property rule is invalid the backend call is pre-empted. Of course, if I change sequence++ to sequence += 2 then it works like I want it to.

@RomkeVdMeulen
Copy link
Contributor Author

All of this may be hard to clearly express in the API. For example, imagine this scenario:

const rules = ValidationRules
	.ensure("myProp")
		.required()
		.then()
		.satisfies(val => val !== "some_value")
	.ensure("otherProp")
		.required()
		.then()
		.satisfies(val => val !== "another_value")
		.then()
		.satisfies(val => !isNaN(parseInt(val, 10)))
	.someNewApiCall() // to be decided
	.ensureObject()
		.satisfies(() => this.backendValidation())
	.on(this.options);

In this case each property should be progressively validated according to its logic, and the object validation should only occur when all those validations check out.

If you're willing to forgo the flexibility of being able to use the new method between every ensure(Object), you could consider calling it finally and only allowing it to be used once, putting all subsequent validation logic in a special slot that gets called last. I imagine this would cover most use-cases where people want to delay expensive (server-side) validation until all cheap (local) validation has checked out.

@RomkeVdMeulen
Copy link
Contributor Author

#279 may also be relevant to this

@jdanyow
Copy link
Contributor

jdanyow commented Mar 27, 2017

Another thing you can do is create separate rulesets. One set that is cheap to execute which you use in your UI as you do today. Another set containing expensive rules that you validate only upon submit, and only when the "cheap" rules pass. This gives you complete control and saves you from trying to express the conditions in the fluent API

@RomkeVdMeulen
Copy link
Contributor Author

Good point. I was doing the same thing with tagging in my earlier example, but storing references to two rulesets might be even simpler.

This is probably the best solution in terms of flexibility and being explicit, but it does create a bit of boilerplate validating each set in order. But since I don't have a good idea how the API should support this, I guess I'll just close this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants