Skip to content

Add more functionality to external validators#2773

Open
asologor wants to merge 1 commit intohapijs:masterfrom
asologor:feature/improve_external_validators
Open

Add more functionality to external validators#2773
asologor wants to merge 1 commit intohapijs:masterfrom
asologor:feature/improve_external_validators

Conversation

@asologor
Copy link

@asologor asologor commented Apr 26, 2022 •
edited
Loading

No breaking changes here. No existing tests were modified.

I know that many people want to see these features in Joi and I think this functionality is a must-have for any backend validator. If you do not merge this PR, consider implementing the same or similar interface yourself.

Added a bunch of helpers for externals:

  • prefs - this one existed before.
  • path - ordered array where each element is the accessor to the value where the error happened.
  • label - label of the value. If you are validating an object's property, it will contain the name of that property.
  • root - the root object or primitive value under validation.
  • context - same as root, but contains only the closest parent object in case of nested objects validation.
  • error - a function with signature function(message). You can use it in a return statement (return error('Oops!')) or you can call it multiple times if you want to push more than one error message in a single external validator.

Added a new setting alwaysExecuteExternals (default is false) which in pair with abortEarly: false will make externals execute even after some synchronous validator failed. Chains of external validators will abort early anyway (e.g. Joi.any().external(...).external(...) - second external won't execute if the first one failed).

Now externals throw the same ValidationError instance as all other validators.

A basic example from the API.md doc:

const data = {
    foo: {
        bar: 'baz'
    }
};

await Joi.object({
    foo: {
        bar: Joi.any().external((value, { prefs, path, label, root, context, error }) => {
            // "prefs" object contains current validation settings
            // value === 'baz'
            // path === ['foo', 'bar']
            // label === 'foo.bar'
            // root === { foo: { bar: 'baz' } }
            // context === { bar: 'baz' }

            if (value !== 'hello') {
                return error(`"${value}" is not a valid value for prop ${label}`);
            }
        })
    }
}).validateAsync(data);

See API.md changes and test suites for more details.

@kammerjaeger
Copy link

kammerjaeger commented Apr 27, 2022

Thanks, this is great, I just wanted to implement this too.
The current implementation is lacking quite a bit for the external methods.

@asologor asologor mentioned this pull request May 30, 2022
Open
This was referenced Jun 27, 2022
Closed
Open
This was referenced Jul 27, 2022
Closed
Open
@jonyeezs
Copy link

jonyeezs commented Jul 28, 2022

Could this add (also part of custom):

message(messages, [local]) - a method to generate an error with an internal 'custom' error code and the provided messages object to use as override. Note that this is much slower than using the preferences messages option but is much simpler to write when performance is not important.

@jonyeezs jonyeezs mentioned this pull request Jul 28, 2022
Closed
@baumerdev
Copy link

baumerdev commented Aug 10, 2022

This is a great improvement over the current .external() implementation.

@jonyeezs I think the current error() helper is acting similar to the messages() function of custom (but with only one message and without local param).

So I'd propose to rename the error() function to message() to be more consistent and to add a new error() helper function that takes a code/type and replaces the default "external" in the created error to be more consistent with

error(code, [local]) - a method to generate error codes using a message code and optional local context.

@max-kahnt-keylight max-kahnt-keylight mentioned this pull request Aug 15, 2022
Open
@Marsup
Copy link
Collaborator

Marsup commented Aug 20, 2022

Is there any real need for multiple calls to error? I feel like this breaks away from the convention in every other place where it's allowed (extensions and custom).
I'd argue that there's no need for alwaysExecuteExternals, abortEarly should probably have behaved with this rule the same way it does with others.

@baumerdev
Copy link

baumerdev commented Aug 20, 2022

I'd argue that there's no need for alwaysExecuteExternals, abortEarly should probably have behaved with this rule the same way it does with others.

With the current (main branch) version .external() ist only called if and after the other validations pass. Even when you set abortEarly to false, external() isn't called if any other validation fails. See docs: "If schema validation failed, no external validation rules are called"

You may change this this behavior to mimic something like .custom() by always executing it if either there is no other previous error or abortEarly is set to false. I would appreciate it and think it's a good idea to not having different behavior for both functions. But I guess that'd be a breaking change for some relying on external() only being called on successfully validated data.

@asologor asologor force-pushed the feature/improve_external_validators branch from 66367f5 to babffbe Compare October 3, 2022 14:47
@asologor
Copy link
Author

asologor commented Oct 3, 2022

Is there any real need for multiple calls to error? I feel like this breaks away from the convention in every other place where it's allowed (extensions and custom).

It might be handy sometimes. A few example cases:

  1. Validating an array
  2. Validating several (or all) fields of a form in a single external valdator

I'd argue that there's no need for alwaysExecuteExternals, abortEarly should probably have behaved with this rule the same way it does with others.

Even if abortEarly = false and some non-external field has an error, externals won't be executed. alwaysExecuteExternals makes externals run regardless of other validator outcomes.

Currently, I don't have time to work on this PR. Maintainers are free to make any changes though.

@kolpitor
Copy link

kolpitor commented Dec 21, 2022

Hi! Someone know when will this PR will be merge? I need this fix ^^'

@Marsup Marsup mentioned this pull request Mar 18, 2023
Merged
@Marsup
Copy link
Collaborator

Marsup commented Mar 20, 2023 •
edited
Loading

First, I'd like to thank @asologor a lot for the efforts put into this PR, it helped me kick off this feature.

I have implemented it in #2931, this is released as of 17.9.0.
I chose to stick to the same API as custom not to confuse people with a similar API but incompatible helpers, it was a bit more work, but I think it's worth it.

For now, alwaysExecuteExternals is not part of it because my mind is not made up yet, I'm not sure whether it should be a global flag, a local flag, or both, or if it should behave accordingly to abortEarly. I don't personally have use for this, so any input the community could provide would help me drive this.

@zachrog
Copy link

zachrog commented May 31, 2023

I would love to see alwaysExecuteExternals implemented so that I could show users all of the errors that exist on a single submission of a form.

@rachelschneiderman
Copy link

rachelschneiderman commented May 31, 2023

The ability to validate all parts of a schema, including properties that rely on externals, at the same time would be hugely impactful to my project. Please consider adopting this feature. Thanks!

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@asologor @kammerjaeger @jonyeezs @baumerdev @Marsup @kolpitor @zachrog @rachelschneiderman