Skip to content

feat(core): Add validate function based rule condition #2441

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lucas-koehler
Copy link
Contributor

Add a new rule condition ValidateFunctionCondition using a given validate function to evaluate the condition result. This allows using arbitrary custom logic to evaluate condition results.
This facilitates not using schema-conditions to be able to only use one pre-compiled AJV for the data schema at a later stage.

Add a new rule condition `ValidateFunctionCondition` using a given `validate` function
to evaluate the condition result. This allows using arbitrary custom logic to evaluate
condition results.
This facilitates not using schema-conditions to be able to only use one pre-compiled AJV
for the data schema at a later stage.
Copy link

netlify bot commented Apr 23, 2025

Deploy Preview for jsonforms-examples ready!

Name Link
🔨 Latest commit 0632025
🔍 Latest deploy log https://app.netlify.com/sites/jsonforms-examples/deploys/680a12284bda5900088349ad
😎 Deploy Preview https://deploy-preview-2441--jsonforms-examples.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@coveralls
Copy link

coveralls commented Apr 23, 2025

Coverage Status

coverage: 82.569% (+0.05%) from 82.517%
when pulling 0632025 on lk/validation-functions
into 5218263 on master.

@lucas-koehler lucas-koehler requested a review from sdirix April 23, 2025 11:00
*
* @param data The data as resolved via the scope.
* @returns `true` if the condition is fulfilled */
validate: (data: unknown) => boolean;
Copy link
Member

@sdirix sdirix Apr 23, 2025

Choose a reason for hiding this comment

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

I think we should hand in much more data here, i.e. whatever we have available at call time.

For example

Suggested change
validate: (data: unknown) => boolean;
validate: (rule: {
data: unknown;
fullData: unknown;
schema: JSONSchema,
rootSchema: JSONSchema,
uischema: UISchemaElement,
path: string; // or string[]
// etc.
}) => boolean;

Not sure about the naming, just as an idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally, I agree with your sentiment. However, we do not have that much available without breaking the public API:
The evaluateCondition method has these parameters:

data: any, // This is the full data
condition: Condition,
path: string,
ajv: Ajv

Tracing upwards in the call history to the first publicly exposed methods using this - evalVisibility and evalEnablement - they have these parameters available (the same is true for isVisible and isEnabled):

uischema: UISchemaElement, // ui schema element containing the rule
data: any, // full data
path: string = undefined, // Optional instance path for nested controls
ajv: Ajv

Thus, without breaking the API of there we could additionally hand in:

  • the UISchemaElement containing the condition
  • the full data
  • the optional instance path for nested controls

I omitted handing in the full data because this can be covered by using the root scope #. Thinking about it again, this is then only the nested root data.
Thus, I suggest handing in the three listed properties in addition to the scoped data. I'd like to avoid breaking the API. WDYT?

@lucas-koehler
Copy link
Contributor Author

@sdirix I now added the additional context information being available without breaking API as described here: #2441 (comment)

@lucas-koehler lucas-koehler requested a review from sdirix April 24, 2025 10:28
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.

3 participants