feat(validation): link two or more properties' validation #2015

XmasCarola · 2024-07-28T19:24:59Z

Pull Request

📖 Description

Possibility to connect properties if they are related to each other, so that the validation of one property acts as a trigger for the other(s). This is useful when two variables are interconnected, for example start and end date that are supposed to function as comparison values. Providing multiple checks with subsequent highlights can point out all the values that need to be modified to properly fill out a form.

🎫 Issues

👩‍💻 Reviewer Notes

📑 Test Plan

⏭ Next Steps

* possibility to connect properties if they are related to each other

codecov · 2024-07-28T19:32:56Z

Codecov Report

Attention: Patch coverage is 96.96970% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.99%. Comparing base (d4ab396) to head (ff51484).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
packages/validation/src/serialization.ts	89.65%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2015      +/-   ##
==========================================
+ Coverage   88.97%   88.99%   +0.02%     
==========================================
  Files         280      280              
  Lines       23232    23317      +85     
  Branches     5350     5379      +29     
==========================================
+ Hits        20670    20752      +82     
- Misses       2562     2565       +3

Flag	Coverage Δ
	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sayan751

Hi @XmasCarola, thanks for the contribution. This is an interesting change. Left a couple of suggestions.

Coming to bike-shading now :D
The method name linkProperties seems a bit ambiguous to me. The purpose is not immediately clear whether the validation of the current property is dependent on the other properties, or is it the other way around. As your test shows, it is the other way around. From that perspective, a .dependsOn method will be more intuitive.

As an example from your tests

 const rules = validationRules
  .on(defineRuleOnClass ? Person : obj)

    .ensure('name')
    .required()

    .ensure('age')
    .dependsOn('name')
    .required()

    .ensure('address.line1')
    .dependsOn('name')
    .required()
    .withMessage('Address is required.')

It don't have to be dependsOn, any other intuitive name should also work.

Sayan751 · 2024-07-29T06:14:34Z

packages/validation/src/validator.ts

+      if (propertyRule !== void 0 && propertyRule.linkedProperties.length > 0) {
+        const additionalRules = propertyRule.linkedProperties.map(lp => rules.find(r => r.property.name === lp)).filter(Boolean) as PropertyRule[];
+        return (await Promise.all([propertyRule?.validate(object, propertyTag, scope), ...additionalRules.map(async (rule) => rule.validate(object, propertyTag, scope))])).flat();
+      }
      return (await rules.find((r) => r.property.name === propertyName)?.validate(object, propertyTag, scope)) ?? [];


propertyRule should be reused here.

Sayan751 · 2024-07-29T06:18:06Z

packages/validation/src/validator.ts

+      const propertyRule: PropertyRule | undefined = rules.find((r) => r.property.name === propertyName);
+      if (propertyRule !== void 0 && propertyRule.linkedProperties.length > 0) {
+        const additionalRules = propertyRule.linkedProperties.map(lp => rules.find(r => r.property.name === lp)).filter(Boolean) as PropertyRule[];
+        return (await Promise.all([propertyRule?.validate(object, propertyTag, scope), ...additionalRules.map(async (rule) => rule.validate(object, propertyTag, scope))])).flat();


Why the .filter is needed? Are we expecting the rules to be falsy?

There could be undefined values in case .find() does not detect a match.

Sayan751 · 2024-07-29T06:27:54Z

docs/user-docs/aurelia-packages/validation/defining-rules.md

+  .ensure('name')
+    .linkProperties(['age', 'address']);
+```
+


Maybe add a bit more details. To me at least, after reading the docs, the purpose was not clear.

Sayan751 · 2024-07-29T06:31:26Z

packages/validation/src/rule-provider.ts

@@ -123,6 +123,7 @@ export class PropertyRule<TObject extends IValidateable = IValidateable, TValue
  private latestRule?: IValidationRule;
  /** @internal */
  public readonly l: IServiceLocator;
+  public linkedProperties: PropertyKey[] = [];


I think that it would be great if these linked properties can be serialized; refer model based validation.

To that end, the accept method(s) and the rule hydrators needs to be refactored.

Sayan751 · 2024-07-29T06:44:53Z

packages/validation/src/rule-provider.ts

+   * Links the PropertyRule to other key values.
+   * @param properties - Array of property keys to link.
+   */
+  public linkProperties(properties: PropertyKey[]) {


I think that we should also support lambda expressions as input along with PropertyKey, like we also do for the .ensure.

Sayan751 · 2024-07-29T17:59:44Z

packages/validation/src/validator.ts

@@ -57,6 +57,11 @@ export class StandardValidator implements IValidator {
    const scope = Scope.create({ [rootObjectSymbol]: object });

    if (propertyName !== void 0) {
+      const propertyRule: PropertyRule | undefined = rules.find((r) => r.property.name === propertyName);


When validating multiple properties, we might want to do that when the validation instruction involves validating a single property or a single binding. We might want to avoid repetitive executions of the same rules when the validation is triggered for the entire object or for all bindings.

Sayan751 · 2024-07-29T18:00:07Z

Maybe slightly differently, we can mark a set of properties as one group. When one of those properties is validated, all the properties in the set are validated.

For example:

validationRules
  .on(defineRuleOnClass ? Person : obj)
  .group('name', 'age', 'address.line1') // <-- validate all 3 properties when one of those are validated.

    .ensure('name')
    .required()

    .ensure('age')
    .required()

    .ensure('address.line1')
    .required()
    .withMessage('Address is required.')

@bigopon What do you think about that?

bigopon · 2024-07-30T05:09:56Z

... When one of those properties is validated, all the properties in the set are validated.

If we had a group of

[radio input]: 1 way / 2 way flight
[date input]: departure date
[date input]: return date

I think I want to express something like this

  .ensureGroup('direction', 'departureDate', 'returnDate', (direction, departureDate, returnDate) => {
    if (direction === '1 way') {
      if (returnDate != null) { throw new Error('1 way flight has no return date'); }
      if (startDate < Date.now()) { throw new Error('No time travel possible'); }
    } else {
      if (startDate > returnDate) { throw new Error('not possible to go back in time'); }
      if (!hasFlight(returnDate)) { throw new Error('no flights on the date back, boat?'); }
    }
  });

and the trigger point will probably be after the radio and a date (either departure or return) has been selected. I wouldn't want to trigger any date independently I think.

Sayan751 · 2024-07-30T07:14:40Z

@bigopon I like your example a lot. Only instead of throwing error, it needs to return false. Or we can utilize the newly introduced state rule in background for multiple error messages.

XmasCarola · 2024-07-30T09:13:20Z

Hi guys, thank you for your input! :)

bigopon · 2024-08-09T14:03:26Z

Just an idea, maybe a serializable version of the ensureGroup above would be something like

.ensureGroup(
  ['direction', 'departureDate', 'returnDate'],
  `$direction === '1-way' && $returnDate == null && $departureDate > Date.now()
    || $returnDate > $departureDate
  `
)

XmasCarola · 2024-08-23T18:16:59Z

Hi guys, I've made some additional integrations to the code. After analyzing the structure of ensureGroup(), I realized that what @bigopon is aiming for can be achieved with the current setup. As a result, I designed a test to emulate his request (you can refer to "can validate linked properties w/ custom logic" inside validator.spec.ts). Let me know your thoughts. Thanks!

bigopon · 2024-09-01T14:46:47Z

packages/validation/src/validator.ts

+      const propertyRule: PropertyRule | undefined = rules.find((r) => r.property.name === propertyName);
+      if (propertyRule !== void 0 && propertyRule.linkedProperties.length > 0) {
+        const additionalRules = propertyRule.linkedProperties.map(lp => rules.find(r => r.property.name === lp)).filter(Boolean) as PropertyRule[];
+        return (await Promise.all([propertyRule.validate(object, propertyTag, scope), ...additionalRules.map(async (rule) => rule.validate(object, propertyTag, scope))])).flat();


is the .flat() necessary?

I think that's needed to flatten the validation results, because there might be multiple validation results from a single property. This depends on how the rules are defined (if .then is used).

bigopon · 2024-09-01T14:53:13Z

packages/validation/src/validator.ts

@@ -57,7 +57,12 @@ export class StandardValidator implements IValidator {
    const scope = Scope.create({ [rootObjectSymbol]: object });

    if (propertyName !== void 0) {
-      return (await rules.find((r) => r.property.name === propertyName)?.validate(object, propertyTag, scope)) ?? [];
+      const propertyRule: PropertyRule | undefined = rules.find((r) => r.property.name === propertyName);
+      if (propertyRule !== void 0 && propertyRule.linkedProperties.length > 0) {


I think with this addition, what we have is the ability to trigger a set of rules together with another rule. I also can't answer whether the depended rules will run first or in parallel with the dependent rule. For those reasons, I think the .dependsOn() is inappropriate. Maybe let's try to come up with a more accurate name?
What about:

afterRules(...)

withRules(...)

whenRules(...)

with(...)

useRules(...)

withGroup(...)

After revisiting the code, I also agree that the .dependsOn might not be a correct name for this. If we have the following:

rules .on(object) .ensure('prop1') .rule1() .dependsOn(['prop2', 'prop3'])

then the rules associated with prop2 and prop3 are executed when the prop1 is validated. So, in other words, the validation of prop2 and prop3 are triggered by the validation of prop1, making prop2 and prop3 dependent on prop1 and not the other way around. I am unsure if that is the goal? If yes, then .dependsOn for that might be a confusing name.

I was under the impression that the goal is to validate a set of properties as a group. Therefore, I am inclined towards the .ensureGroup idea from @bigopon. @XmasCarola Do you think, implementing that satisfies your use-case as well?

This is not a blocking comment, but on this note, we should also evaluate the UX aspect for this. My preference is that when one property is changed and therefore validated, only the rules associated with that property are executed, and the errors are shown accordingly.
However, with the introduction of this change, many properties will potentially be validated when the value of one property is changed, showing validation errors for potentially many properties. IMO, that increases the cognitive load for the end user.

The reason for my PR is that validating multiple properties simultaneously is sometimes necessary. It happened to me to encounter interdependent properties, such as booleans that need adjustment when one changes, or dates, like start and end date. Implementing date comparisons using Aurelia 1.0's validation plugin was challenging and led me to search online, where I found some related issues => aurelia/validation#279 (see the comment at the bottom). When two dates must follow a sequence, it’s helpful to indicate that adjusting either value can resolve the error.

Other frameworks validate all 'touched' fields whenever one is modified, which is another way to address this problem. However, your main solution - @Sayan751 - offers developers more flexibility, as some find it frustrating when parts of a form are validated unnecessarily. We could have related grouped properties with a specific validation order, as you both suggested. ;)

@XmasCarola I too have experience such need for validating multiple properties as a group. Hence, there is no question about the legitimate need of this feature. The point on the UX is more or less an emphasis on seeing it in action and evaluating how it is affecting the UX for a legitimate use-case. I appreciate you working on it!

bigopon · 2024-09-01T14:54:02Z

btw, the benchmark jobs can be fixed by simply updating this branch with latest master @XmasCarola

Sayan751 · 2024-09-01T17:42:14Z

packages/validation/src/rule-provider.ts

+   * Links the PropertyRule to other key values.
+   * @param properties - Array of properties to link (accepts strings, object keys as well as property accessor functions).
+   */
+  public dependsOn(properties: string[] | PropertyAccessor[]) {


I think that the intellisense might not work as expected with this. We might need something like PropertyAccessor<TObject, TValue>, like it is done for .ensure.

Sayan751 · 2024-09-01T17:51:37Z

packages/validation/src/validator.ts

+      const propertyRule: PropertyRule | undefined = rules.find((r) => r.property.name === propertyName);
+      if (propertyRule !== void 0 && propertyRule.linkedProperties.length > 0) {
+        const additionalRules = propertyRule.linkedProperties.map(lp => rules.find(r => r.property.name === lp)).filter(Boolean) as PropertyRule[];
+        return (await Promise.all([propertyRule.validate(object, propertyTag, scope), ...additionalRules.map(async (rule) => rule.validate(object, propertyTag, scope))])).flat();


I think that's needed to flatten the validation results, because there might be multiple validation results from a single property. This depends on how the rules are defined (if .then is used).

Sayan751 · 2024-09-01T19:46:18Z

packages/validation/src/validator.ts

@@ -57,7 +57,12 @@ export class StandardValidator implements IValidator {
    const scope = Scope.create({ [rootObjectSymbol]: object });

    if (propertyName !== void 0) {
-      return (await rules.find((r) => r.property.name === propertyName)?.validate(object, propertyTag, scope)) ?? [];
+      const propertyRule: PropertyRule | undefined = rules.find((r) => r.property.name === propertyName);
+      if (propertyRule !== void 0 && propertyRule.linkedProperties.length > 0) {


After revisiting the code, I also agree that the .dependsOn might not be a correct name for this. If we have the following:

rules .on(object) .ensure('prop1') .rule1() .dependsOn(['prop2', 'prop3'])

then the rules associated with prop2 and prop3 are executed when the prop1 is validated. So, in other words, the validation of prop2 and prop3 are triggered by the validation of prop1, making prop2 and prop3 dependent on prop1 and not the other way around. I am unsure if that is the goal? If yes, then .dependsOn for that might be a confusing name.

I was under the impression that the goal is to validate a set of properties as a group. Therefore, I am inclined towards the .ensureGroup idea from @bigopon. @XmasCarola Do you think, implementing that satisfies your use-case as well?

bigopon · 2024-09-02T01:20:55Z

... I also can't answer whether the depended rules will run first or in parallel with the dependent rule. ...

One thing that is possible if we declare all properties of a group together, is the ability to explicitly declare the sequence. Similar to the way Gulp enable sequence in it task running. For example:

// all at once
.group(['direction', 'returnDate', 'departureDate'], ([direction, returnDate, departureDate]) => ...)

// direction then departure and return dates at once
.group('direction', ['departureDate', 'returnDate'], (direction, [departureDate, returnDate]) => ...)

// direction then departure date then return date
.group('direction', 'departureDate', 'returnDate', (direction, departureDate, returnDate) => ...)

We can also allow .group to only run after all individual properties with validation have validated before validating the whole as a group

Sayan751 · 2024-09-02T17:56:24Z

@XmasCarola @bigopon I think that if we agree on the .ensureGroup thing, then I propose to keep it simple for the first version. Then, depending on how the devs are using it, we can always make it more complicated (read fine-tuning :)). My proposal is as follows:

.ensureGroup(
  ['property', or => accessor],
  (value1, value2, object) => boolean | Promise<boolean>
)

The rule is executed when any one of the property value is changed. The order of the rule execution is determined by how the rule is registered. I mean that that is the default order of executing the rules. We should maintain that here as well, without treating this rule too specially.

My mental model is as follows: when we encounter an .ensureGroup, we attach the same rule for all the given properties. That way when one of those property changes, the rule will automatically be executed, with a small tweak that using the property accessors or expression, the current value of all properties will be fetched and supplied to the lambda for the evaluation of the rule. When generating an error from this rule, the error needs to be generated for all the properties (needs to be replicated for alll properties), so that the information can be shown in the UI via the subscribers (this makes me a bit uncomfortable; maybe we need smart out-the-box subscribers that can show only one error for a single UI component; however, this might be a secondary issue, as showing errors is very much context-specific, and one might need custom subscriber anyway).

bigopon · 2024-09-03T13:05:33Z

... the error needs to be generated for all the properties ...

I don't feel like this is an acceptable default at all. Adding a group validation and suddenly I won't be able to figure out where it went wrong in my tests is an undesirable scenario to be in.

In a group validation, issues may not always be with a single property. Like in my example, sometimes it's the direction, sometimes it's the departure, sometimes it's the return date. Do we want to always notify all those direction departure and return dates when only one of those fails?
If we don't want that, and want to be precise where to notify, where should be have that information? To me, maybe the lambda that is responsible for validation should also be responsible for signaling: maybe it should return more than just a boolean:

.ensureGroup(
  ['property', or => accessor],
  (value1, value2, object) =>
    | { valid: true }
    | Promise<{ valid: true }>
+   | { valid: false; property: string }
+   | Promise<{ valid: false; property: string }>
)

Sayan751 · 2024-09-03T18:21:26Z

@bigopon Like the idea. I would suggest an extension of that to the following:

.ensureGroup(
  ['property', or => accessor],
  (value1, value2, object) =>
+    | { valid: boolean, { property: string, valid: boolean, messageOrMessageKey?: string }[] }
+    | Promise<{ valid: boolean, { property: string, valid: boolean, messageOrMessageKey?: string }[] }>
)

If no message or message key is provided in the result, then the usual workflow of resolving the message should take over the task. We need to remember that it should work with i18n as well.

XmasCarola · 2024-10-05T17:33:26Z

@Sayan751 @bigopon Sorry guys, I committed by mistake on master.

…la/aurelia into validation-linked-props

…oup - update test coverage

…oup - remove duplicated rules

…oup - clean code

…oup - remove prettier format

Remove latest rule reassignment

feat(validation): link two or more properties' validation

84ebba7

* possibility to connect properties if they are related to each other

Sayan751 reviewed Jul 29, 2024

View reviewed changes

Sayan751 requested a review from bigopon July 29, 2024 06:48

Sayan751 reviewed Jul 29, 2024

View reviewed changes

XmasCarola force-pushed the validation-linked-props branch 2 times, most recently from 1353ece to 84ebba7 Compare August 23, 2024 17:54

Merge branch 'master' into validation-linked-props

6b8044a

XmasCarola force-pushed the validation-linked-props branch 2 times, most recently from 79b0f79 to 6b8044a Compare August 23, 2024 20:43

feat(validation): link two or more properties' validation revised

7eba6cb

bigopon reviewed Sep 1, 2024

View reviewed changes

Sayan751 reviewed Sep 1, 2024

View reviewed changes

XmasCarola added 3 commits October 5, 2024 19:01

Merge branch 'master' into validation-linked-props

4fa084a

Merge branch 'master' into validation-linked-props

2956f7e

feat(validation): link two or more properties' validation w/ ensureGroup

e3af6bd

XmasCarola added 2 commits October 6, 2024 14:55

feat(validation): link two or more properties' validation w/ ensureGroup

7710641

Merge branch 'validation-linked-props' of https://github.com/XmasCaro…

c98613d

…la/aurelia into validation-linked-props

XmasCarola added 5 commits October 6, 2024 22:11

feat(validation): link two or more properties' validation w/ ensureGr…

e78d50e

…oup - update test coverage

feat(validation): link two or more properties' validation w/ ensureGr…

32d63c9

…oup - remove duplicated rules

feat(validation): link two or more properties' validation w/ ensureGr…

3744687

…oup - clean code

feat(validation): link two or more properties' validation w/ ensureGr…

da9ed3f

…oup - remove prettier format

feat(validation): link two or more properties' validation w/ ensureGroup

ff51484

Remove latest rule reassignment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(validation): link two or more properties' validation #2015

feat(validation): link two or more properties' validation #2015

XmasCarola commented Jul 28, 2024

codecov bot commented Jul 28, 2024 •

edited

Loading

Sayan751 left a comment

Sayan751 Jul 29, 2024

Sayan751 Jul 29, 2024

XmasCarola Aug 23, 2024

Sayan751 Jul 29, 2024

Sayan751 Jul 29, 2024

Sayan751 Jul 29, 2024

Sayan751 Jul 29, 2024

Sayan751 commented Jul 29, 2024

bigopon commented Jul 30, 2024

Sayan751 commented Jul 30, 2024

XmasCarola commented Jul 30, 2024 •

edited

Loading

bigopon commented Aug 9, 2024

XmasCarola commented Aug 23, 2024

bigopon Sep 1, 2024

Sayan751 Sep 1, 2024

bigopon Sep 1, 2024

Sayan751 Sep 1, 2024

Sayan751 Sep 1, 2024

XmasCarola Sep 2, 2024

Sayan751 Sep 2, 2024

bigopon commented Sep 1, 2024

Sayan751 Sep 1, 2024

Sayan751 Sep 1, 2024

Sayan751 Sep 1, 2024

bigopon commented Sep 2, 2024

Sayan751 commented Sep 2, 2024

bigopon commented Sep 3, 2024

Sayan751 commented Sep 3, 2024

XmasCarola commented Oct 5, 2024

feat(validation): link two or more properties' validation #2015

Are you sure you want to change the base?

feat(validation): link two or more properties' validation #2015

Conversation

XmasCarola commented Jul 28, 2024

Pull Request

📖 Description

🎫 Issues

👩‍💻 Reviewer Notes

📑 Test Plan

⏭ Next Steps

codecov bot commented Jul 28, 2024 • edited Loading

Codecov Report

Sayan751 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sayan751 commented Jul 29, 2024

bigopon commented Jul 30, 2024

Sayan751 commented Jul 30, 2024

XmasCarola commented Jul 30, 2024 • edited Loading

bigopon commented Aug 9, 2024

XmasCarola commented Aug 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigopon commented Sep 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigopon commented Sep 2, 2024

Sayan751 commented Sep 2, 2024

bigopon commented Sep 3, 2024

Sayan751 commented Sep 3, 2024

XmasCarola commented Oct 5, 2024

codecov bot commented Jul 28, 2024 •

edited

Loading

XmasCarola commented Jul 30, 2024 •

edited

Loading