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

WIP: Advanced validation proof of concept #28

Closed
wants to merge 72 commits into from
Closed

Conversation

brennj
Copy link
Collaborator

@brennj brennj commented Jul 7, 2023

Implements support for advanced validation logic and computed values in JSON Schema Forms based on RFC #39.

The main changes are:

  • Adds a new x-jsf-logic extension that can contain:

    • validations - Custom validation rules defined in JsonLogic format
    • computedValues - Values computed dynamically from other fields
    • if/then/else conditional logic
  • createValidationChecker utility parses schema and extracts rules into a map for efficient validation

  • Properties can define x-jsf-requiredValidations to link required custom validations

  • Computed values can be used in attributes like title, description, minimum etc via x-jsf-computedAttributes

  • yupSchemaWithCustomJSONLogic integrates custom validations into Yup schemas

  • Supports referencing nested fields in objects and arrays

  • Inline rules can be defined directly in computed attributes

  • Extensive test cases added covering wide range of examples

This enables much more advanced dynamic behaviour in forms based on values of other fields. Helps unify validation logic between client and server.

See the RFC internally for full details and other Remote specific examples. Main benefits are:

  • Single source of truth for advanced validations
  • More dynamic/adaptive behaviour
  • External integrations can leverage the logic directly
  • Progress towards fully defining forms in JSON

Copy link
Collaborator

@sandrina-p sandrina-p left a comment

Choose a reason for hiding this comment

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

I reviewed the tests and respective fixtures. Will review the actual code later, probably only on Monday.

require_c: {
rule: {
and: [
{ '!==': [{ var: 'field_b' }, null] },
Copy link
Collaborator

Choose a reason for hiding this comment

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

question/bug: As we mentioned in a past call, this is indeed ugly. But I don't remember exactly why. I even commented these 2 lines !== and the test still passes. What am I missing here?

},
};

export const schemaWithMultipleComputedValueChecks = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting case. Here I'd expect the root field_c to have 'x-jsf-requiredValidations declared even if in the conditional it it's disallowed (the then ... false bit). Which doesn't make sense as it's contradictory.

Perhaps a sanity test showing that impossible case would be useful for future reference.

'*': [{ var: 'field_a' }, 2],
},
},
mod_by_five: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

naming: I noticed multiple places where the naming doesn't make sense and I can foresee this happening in the future. Perhaps we'll need to document guidelines on how to name these rules. Let me give you some examples:

  • Incomplete naming:
    • This one should be b_mod_by_five
    • At schemaWithIfStatementWithComputedValuesAndValidationChecks, the greater_than_b should be a_greater_than_b (or even a_>_b)
  • Desbribe the action, not the outcome
  • At schemaWithGreaterThanChecksForThreeFields the require_c should be a_greater_than_b. Then based on this validation, we apply wtv outcome it's needed (in that example, the c gets required)

There are other cases through the examples, but I think you go the point. Does this make sense to you? :)

);
expect(handleValidation({ field_a: 1, field_b: 1 }).formErrors).toEqual(undefined);
expect(handleValidation({ field_a: 10, field_b: 20 }).formErrors).toEqual(undefined);
expect(handleValidation({ field_a: 10, field_b: 9 }).formErrors).toEqual({
Copy link
Collaborator

Choose a reason for hiding this comment

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

todo: In this type of tests (with conditional fields) we should also test the visibility for field_c is isVisible: false.

tip: You may want to use use the util I created on the other test file called getField()

}
);
expect(handleValidation({ field_a: 20, field_b: 1 }).formErrors).toEqual({
field_b: 'use 10 or more',
Copy link
Collaborator

Choose a reason for hiding this comment

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

incredible

);

//TODO: this should fail because we have a const: 10, it should have an error from yup saying only 10 is accepted.
expect(handleValidation({ field_a: 20, field_b: 1 }).formErrors).toEqual();
Copy link
Collaborator

Choose a reason for hiding this comment

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

just leaving a comment to highlight this bug that you spot

},
field_b: {
type: 'number',
'x-jsf-requiredValidations': ['validation_parent', 'peek_to_nested'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: Why is the peek_to_nested here if it's about field_a.child which has no influence under field_b?

It makes me wonder if a validation at requiredValidations MUST always have something in common with the field itself.

properties: {
child: {
type: 'number',
'x-jsf-requiredValidations': ['child_greater_than_10'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: I tried the following variant where I use peek_to_nested here:

export const twoLevelsOfJSONLogicSchema = {
  properties: {
    field_a: {
      type: 'object',
      'x-jsf-presentation': {
        inputType: 'fieldset',
      },
      properties: {
        child: {
          type: 'number',
          'x-jsf-requiredValidations': ['child_greater_than_10', 'peek_to_nested'],
        },
      },
      required: ['child'],
      'x-jsf-logic': {
        validations: {
          child_greater_than_10: {
            errorMessage: 'Must be smaller than 50!',
            rule: {
              '<': [{ var: 'child' }, 50],
            },
          },
        },
      },
    },
    field_b: {
      type: 'number',
      'x-jsf-requiredValidations': ['validation_parent'],
    },
  },
  'x-jsf-logic': {
    validations: {
      validation_parent: {
        errorMessage: 'Must be greater than 10!',
        rule: {
          '>': [{ var: 'field_b' }, 10],
        },
      },
      peek_to_nested: {
        errorMessage: 'child must be greater than 15!',
        rule: {
          '>': [{ var: 'field_a.child' }, 15],
        },
      },
    },
  },
  required: ['field_a', 'field_b'],
};

And then run the respective test "Validate a field and a nested field together". As expected the test failed:

    JSON Schema invalid! Error: Validation "peek_to_nested" required for "child" doesn't exist.

I think we could have an extra test to ensure this will always fail for sanity check. We already have for root fields, but not for fieldsets.

).toEqual(undefined);
});

it('Validating a single item in an array should work', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

supreme 🔥

@brennj
Copy link
Collaborator Author

brennj commented Aug 24, 2023

Closing in favour of the slew of other PRs that make it more.. digestible :D

  • Part 0 const validation support in json-schema-form
  • Part 1 adds the foundation of JSON logic with x-jsf-logic-validations
  • Part 2 adds the groundwork for x-jsf-logic-computedAttrs
  • Part 3 more advanced string template in a computedAttr
  • Part 4 error handling for badly written schemas regarding x-jsf-logic-computedAttrs
  • Part 5 more error handling, focus towards handling bad validations
  • Part 6 inline rules on x-jsf-logic-computedAttrs
  • Part 7 all of the above support, but when in an if/then/else statement.
  • Part 8 same as part 7, expanding on the test suite
  • Part 9 the same support, but applied to a fieldset/group array item.

@brennj brennj closed this Aug 24, 2023
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.

None yet

2 participants