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

Add more functionality to external validators #2773

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

asologor
Copy link

@asologor asologor commented Apr 26, 2022

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

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

@jonyeezs
Copy link

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.

@baumerdev
Copy link

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.

@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

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

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

@Marsup
Copy link
Collaborator

Marsup commented Mar 20, 2023

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.

@zRogersFlexion
Copy link

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

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants