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

Validation chain order #851

Closed
andrey-hohlov opened this issue Apr 22, 2020 · 23 comments
Closed

Validation chain order #851

andrey-hohlov opened this issue Apr 22, 2020 · 23 comments

Comments

@andrey-hohlov
Copy link

andrey-hohlov commented Apr 22, 2020

Hi.
Thank you for yup. We love yup much more than joi)

But we stacked with a problem which raised many times:
#113
#251
#256
#499
#503
#602

Simple example:

We have an object with field user. This field is required, should be valid MongoDB id and a user with this id should exist in the database.

const schema = yup.object({
  user: yup.string()
    .required()
    .test(someMongoIdValidator)
    .test({
      message: () => 'user is not exists',
      test: async (id) => {
        const user = await User.findById(id);
        return !!user;
      },
    }),
});

No sense to check if mongo id is valid for empty string and of course no sense to make database request (yes, it's possible to return false if id is empty).
We have cases with more reference fields and with arrays of references.

So, we just want to know - are there any plans to implement an option to keep the validation order?
Is this consistent with the core idea of yup? Maybe it's already planned and help is needed.

Or we just have to manage it by multiple schemas and something like "validation waterfall" - apply next schema with only valid fields from the previous schema and merge results at the end.

@andriinebylovych
Copy link

Have a similar use case(this is just one of the examples) - validation of date of birth:
The date is inputted as a string:
1 first there should be the date
2 second it should be 8 characters length
3 third it should be a valid date
4 fourth - the user should be more than 18 years old.

The fourth validator depends on the third one(on the code above I had to repeat the code from validator 3).

dateOfBirth: Yup.string()
            .required('Required')
            .test(
                'length',
                'Date should be in format mm/dd/yyyy, ex: 02121998',
                (date: string) => date.length === 8
            )
            .test('notDate', 'Wrong date', (date: string) => {
                let dateInDate: Date;
                try {
                    dateInDate = turnDateInUSFormattedStringToDate(date);
                    return isDateValid(dateInDate);
                } catch (error) {
                    return false;
                }
            })
            .test(
                'moreThan18Years',
                'Should be more than 18 years old',
                (date: string) => {
                    let dateInDate: Date;
                    try {
                        dateInDate = turnDateInUSFormattedStringToDate(date);
                        if (!isDateValid(dateInDate)) {
                            return false;
                        }
                        const age = calculateAge(dateInDate);
                        return age >= 18;
                    } catch (error) {
                        return false;
                    }
                }
            )

So chain order is very important.

@andrey-hohlov
Copy link
Author

andrey-hohlov commented Apr 22, 2020

@AndriiGitHub there is some workaround for your case:

You need to have only one test which returns true if dateOfBirth is not defined (required validator will make the job).
Next, it checks the date length, the date is a valid date and the date is <= today - 18 years.
After each check, you can throw an error with custom messages with this.createError method.
https://github.com/jquense/yup#mixedtestname-string-message-string--function-test-function-schema

But why do you have to write own length validator while yup has one?
it's the way to one big .test() with all handcrafted validations inside. That's why chain order is needed.

@jquense
Copy link
Owner

jquense commented Apr 22, 2020

Hey folks. I tend think that depending on validation order is generally a bad pattern, since it tightly couples all validations together and is slower. It's also unclear how things like schema.concat should work if order is important but sorting would be awkward. Overall with cases like @andriinebylovych s date example that is better solved by using a single test and customizing the message as needed using this.createError

That said, I'm open to a nicer API for doing that a bit more simply. I've just not had the time or need to explore that space for such an API. If someone wants to propose a casual RFC for an approach where the complexity is commiserate with the margin benefit of the feature i'd be more than happy to help get it in.

@ryall
Copy link

ryall commented May 5, 2020

I think @codepunkt suggestion was elegant, with .sequence(): #256 (comment)

That way it has to be explicitly enabled per rule group:

const schema = yup.object({
  user: yup
    .sequence() // Enabled only for schema.user
    .string()
    .required()
    .test(someMongoIdValidator)
    .test({
      message: () => 'user does not exist',
      test: async (id) => {
        const user = await User.findById(id);
        return !!user;
      },
    }),
  password: yup
    .string()
    .required()
    .min(8)
    .test(someSpecialPasswordTest),
});

In the above case, user and password would validate in parallel, as would the specific rules for password, but the specific rules for user would execute sequentially. If .sequence() was also added to the object schema, then user and password would also validate sequentially.

Alternative names could be sync(), synchronous() or sequenced().

@Vincz
Copy link

Vincz commented May 13, 2020

This option would be awesome. Simple and efficient and it would cover 99% of the uses case related to validation chain.

@Dashue
Copy link

Dashue commented May 24, 2020

Another one here that would find sequence beneficial to solve if phone,username or email is already taken. For now I'm using #256 (comment)

Update: Realized this didn't work as I had async validation on more than one field. Once it hit a valid case for hitting the test async function it kept doing it when editing other fields. Using formik.

@jquense
Copy link
Owner

jquense commented May 26, 2020

Hey folks, there isn't a need to voice your approval of the feature generally. I'm fine with adding something that does this, what we need is a proposal for an API that works well.

sequence is generally ok but its use seems either limited or confusing.

  • sequence().string().required().min() I think reads awkwardly, and doesn't allow any parallelism in the schema, but does remove ambiguity around which parts are sequential.

  • string.sequence().required().min() is better to read but more confusing, what parts are sequential? what about tests before the sequence()

I don't have a solid proposal but something like

yup.string()
  .required()
  .with(
    string().test(firstThis).test(thenThis)
   )

Where you pass an inner schema might be clearer? or maybe just easier to to implement.
Ultimately i think this is a good example where a more functional API generally would be useful, as proposed: https://twitter.com/monasticpanic/status/1219635860368449536

you could do string.test(serial(firstThis, thenThat), required)

@ryall
Copy link

ryall commented May 27, 2020

Ultimately i think this is a good example where a more functional API generally would be useful, as proposed: https://twitter.com/monasticpanic/status/1219635860368449536

you could do string.test(serial(firstThis, thenThat), required)

Yeah, agreed, the Twitter post seems like a cleaner solution and seems easier to read than the current implementation. It clearly separates tests from other operations such as transformations, simplifies the implementation (I suspect) because all tests are encapsulated in a single function and promotes a more modular approach to writing custom tests, rather than in-lining them into the schema.

It should even be possible to do more advanced operations with that syntax, such as:

string.test(
  sequenced(
    firstThis, 
    thenThat, 
    parallel(butThese, canRun, inParallel), 
    thenFinally
  ), 
  required, // also runs in parallel with the 'sequenced' block as that's the default
)

Plus you can have a shortcut for when you want the default to be sequenced using string.testSequenced(...)

@andrey-hohlov
Copy link
Author

Ultimately i think this is a good example where a more functional API generally would be useful, as proposed: https://twitter.com/monasticpanic/status/1219635860368449536

Looks good, especially for cases where we need only validation without any transformations.

parallel and series (as one more name option) - like Gulp have.

const schema = yup.object({
  user: yup.string()
    .series(
      required,
      test(someMongoIdValidator)
      test({
        message: () => 'user is not exists',
        test: async (id) => {
          const user = await User.findById(id);
          return !!user;
        },
      }),
    )
});

Very good

@ekawatani
Copy link

ekawatani commented Jun 12, 2020

Or perhaps, if we want to maintain the current syntax, we could let the series function return the parent schema to allow chaining. This would be also useful when schemas are built dynamically and they are passed around.

const schema = yup.object({
  user: yup.string()
    .series(schema => schema
      .required()
      .test(someMongoIdValidator)
      .test({
        message: () => 'user is not exists',
        test: async (id) => {
          const user = await User.findById(id);
          return !!user;
        },
      }),
    )
});

@devevignesh
Copy link

Hey guys, I'm trying to chain the validation order but it's not working. Please check #952

@vipcxj
Copy link

vipcxj commented Jul 1, 2020

I don't think Yup is a good tool for solving validation once for all. Instead of spending a lot of time google for the things that Yup might not be able to do, it's better to use Yup in a large validation function to validate individual values as needed, then you have full control of the logical of validation. For example, I want to validate userName and force it not empty and has not been used. And when validating whether it has been used, I will show a spin beside the input. Obviously, it is very strange that a spin show and then the error is the user name is required. extremely, the network request for checking the user name may faild, however test only has true or false. In reality, there are too many things that Yup can't handle, but if I use it in a big validate function, It will save me the time of writing duplicate verify code.

@muriloalvespereira
Copy link

Hi guys, I am a beginner, I started coding 3 months ago and I am facing this issue bellow, my code is working but I would like to fetch just once and not in each type. Could you help me? Thank you so much

email: Yup.string()
    .email("Email should be valid and contain @")
    .required("Email is required")
    .test({
      message: () => "Email already exists",
      test: async (values) => {
        if (values) {
          try {
            let response = await fetch("http:/localhost:3005/users/check", {
              method: "POST",
              headers: {
                "Content-Type": "application/json",
              },
              body: JSON.stringify({ email: values }),
            });
            if (response.ok) {
              return true;
            } else {
              return false;
            }
          } catch (error) {
            console.log(error);
          }
        }
      },
    }),

@ryall
Copy link

ryall commented Aug 14, 2021

Hi guys, I am a beginner, I started coding 3 months ago and I am facing this issue bellow, my code is working but I would like to fetch just once and not in each type. Could you help me? Thank you so much

email: Yup.string()
    .email("Email should be valid and contain @")
    .required("Email is required")
    .test({
      message: () => "Email already exists",
      test: async (values) => {
        if (values) {
          try {
            let response = await fetch("http:/localhost:3005/users/check", {
              method: "POST",
              headers: {
                "Content-Type": "application/json",
              },
              body: JSON.stringify({ email: values }),
            });
            if (response.ok) {
              return true;
            } else {
              return false;
            }
          } catch (error) {
            console.log(error);
          }
        }
      },
    }),

You should ask this on https://stackoverflow.com - GitHub's issues are generally for bugs and features, not for coding assistance

@muriloalvespereira
Copy link

Hi guys, I am a beginner, I started coding 3 months ago and I am facing this issue bellow, my code is working but I would like to fetch just once and not in each type. Could you help me? Thank you so much

email: Yup.string()
    .email("Email should be valid and contain @")
    .required("Email is required")
    .test({
      message: () => "Email already exists",
      test: async (values) => {
        if (values) {
          try {
            let response = await fetch("http:/localhost:3005/users/check", {
              method: "POST",
              headers: {
                "Content-Type": "application/json",
              },
              body: JSON.stringify({ email: values }),
            });
            if (response.ok) {
              return true;
            } else {
              return false;
            }
          } catch (error) {
            console.log(error);
          }
        }
      },
    }),

You should ask this on https://stackoverflow.com - GitHub's issues are generally for bugs and features, not for coding assistance

I went to stackoverflow and I could find one question saying this is not possible :)

@gersondinis
Copy link

gersondinis commented Sep 30, 2021

This works as a chain workaround. You can add 'await' lines as you want.

firstName: Yup
        .string()
        .test(async (v, context) => {
            try {
                await Yup.string().min(3, 'Min 3 chars msg').max(5, 'Max 5 chars msg').validate(v);
                await new Promise((resolve, reject) => setTimeout(() => {
                    reject(Error('Promise error msg'))
                }, 1000));
            } catch ({message}) {
                return context.createError({message});
            }
            return true;
        })

More complex scenarios may include debounce.

@Qiming-Liu
Copy link

This works as a chain workaround. You can add 'await' lines as you want.

firstName: Yup
        .string()
        .test(async (v, context) => {
            try {
                await Yup.string().min(3, 'Min 3 chars msg').max(5, 'Max 5 chars msg').validate(v);
                await new Promise((resolve, reject) => setTimeout(() => {
                    reject(Error('Promise error msg'))
                }, 1000));
            } catch ({message}) {
                return context.createError({message});
            }
            return true;
        })

More complex scenarios may include debounce.

Genius, This should be documented as an official use case.

@Qiming-Liu
Copy link

@gersondinis @jquense @ryall
So, as above, a wrapper method called sequence can be like this:

Yup.addMethod(Yup.string, 'sequence', function (funcList) {
  return this.test(async (value, context) => {
    try {
      for (const func of funcList) {
        await func().validate(value);
      }
    } catch ({ message }) {
      return context.createError({ message });
    }
    return true;
  });
});

Then we can do this:

username: Yup.string().sequence([
        () => Yup.string().max(20).required('Username is required'), // check format
        () => Yup.string().unique('Username is already taken', uniqueUsername),  // check uniqe via api
      ]),

unique is my custom Yup method and uniqueUsername is an axiosInstance

@hellojt9040
Copy link

@jquense , I believe that dependency of some validation, on others, are valid use-cases.
Like in signup form user email existence should be validated on the fly but if the entered value is not a valid email id (random text: 'abc'), the whole point of calling API has no point.

Could you please add something like a configurator parameter for dependency on other validation?

Example: The last parameter array will contain all the validators' names, which needs to be completed before this test function runs

const validationSchema = yup.object({
       email: yup
            .string()
            .email()
            .test("unique_email", "Email already registered", async (email, values) => {
                const response = await debouncedApi(email);
                return response;
              }, ['email'])
    });

Formik has a special configuration for yup. We also want to continue with yup with all kinds of scenarios.

@Empire024
Copy link

@Qiming-Liu

This works wonderfully, however, I am running into issues debouncing the unique method. I have tried using the lodash debounce function, as well as writing my own. When I finally got it to work, after meeting all previous validations in a sequence, they wouldn't validate as true until the debounced test function finally ran. I believe this to be a common use-case, is there a possibly a way to do it correctly?

@Qiming-Liu
Copy link

Qiming-Liu commented Apr 4, 2022

@Empire024
Sorry, in my case, I didn't use the debounced function, so I don't know if this will help you. I wrote an api on the back-end that simply returns whether the value has the same value in the database. true for unique, false for not unique.

So the unique function is like this:

Yup.addMethod(Yup.string, 'unique', function (message, axiosInstance) {
  return this.test('unique', message, async (value) => {
    const { data } = await axiosInstance(value);
    return data; // You may need to change this line to match your api 
  });
});

Example of use:

username: Yup.string().sequence([
        () => Yup.string().max(20).required('Username is required'), // check format
        () => Yup.string().unique('Username is already taken', uniqueUsername),  // check uniqe via api
      ]),

PS: Of course, for security reasons, in order to prevent the client from traversing all the database data through this api, a limit needs to be set to limit the number of requests. Each time the user triggers input onchange, a unique request is sent if the format is ok. I admit it's probably not a good use case cause that would be a lot requests but it's great for user experience.

@bjoluc
Copy link

bjoluc commented May 24, 2022

For TypeScript users, here's the utility function I ended up using:

// Based on https://github.com/jquense/yup/issues/851#issuecomment-931295671
export function yupSequentialStringSchema(schemas: yup.StringSchema[]) {
  return yup.string().test(async (value, context) => {
    try {
      for (const schema of schemas) {
        // eslint-disable-next-line no-await-in-loop
        await schema.validate(value);
      }
    } catch (error: unknown) {
      const message = (error as yup.ValidationError).message;
      return context.createError({message});
    }

    return true;
  });
}

const schema = yup.object({
  email: yupSequentialStringSchema([
    yup.string().required(),
    yup.string().email(),
    // ...
  ]),
});

@minorgod
Copy link

minorgod commented Aug 12, 2022

Another option for typescript users if you still want to use yup.addMethod(). This drove me insane so I'm providing a fairly complete example to help y'all.

Add this to a global.d.ts file:

// global.d.ts
declare module 'yup' {
    interface StringSchema<TType, TContext, TDefault, TFlags> {
        sequence: (funcList: Array<() => yup.Schema>) => yup.Schema
    }
}

Add a 'sequence' method to yup StringSchema...

yup.addMethod(yup.StringSchema, 'sequence', function (funcList: Array<() => yup.Schema>) {
    return this.test(async (value, context) => {
        try {
            for (const func of funcList) {
                await func().validate(value)
            }
        } catch (error: any) {
            return context.createError({ message: error.message, type: error.type })
        }
        return true
    })
})

An async function that checks a backend to see if an email is unique:

const uniqueEmail = async (params: EmailValidatonParamsType) => {
    // some async functionality such as an Axois request
    const response = await Promise.resolve({
        data: { exists: true }
    })
    return !response.data.exists
}

Now, suppose you have a login form with an option to either create new account or login to existing account. You can use the sequence method we defined above to prevent making the ajax calls to check for an existing accounts when your user has entered an invalid email address. EG:

const MIN_PASSWORD_LENGTH = 1024 // (humor)

export const loginFormSchema = yup
    .object()
    .shape({
        // isNewUser is some form control where the user selects if they are new or returning
        isNewUser: yup.string().required(),
       // The email form field
        email: yup.string().when('isNewUser', {
            is: 'true',
            then: (schema) =>
                schema.sequence([
                    // validate that the email is a valid format before you call the api to check for uniqueness
                    () =>  schema.email('Your email address appears to be invalid.').required('Please enter an email address.'),
                    // Now do the async validations. If the above validations failed, this won't run. 
                    () =>
                        schema.ensure().test(
                            'check-email-taken', // this will be the error type when vaildation fails,
                            'Email exists already!', // the error message
                            async (value, context) => {
                                try {
                                    // our async validation happens here 
                                    return await uniqueEmail({ email: value })
                                } catch (e: any) {
                                    // various things could cause the async function above to throw an error, so you should
                                    // handle it here or you'll get a generic validation error message
                                    return context.createError({
                                        path: 'email',
                                        message:
                                            'Communication error.',
                                        type: 'communication-error',
                                    })
                                }
                            },
                        ),
                ]),
            // If the user choose to log in instead of creating new account we can just do our non-async validations
            otherwise: (schema) =>
                schema.email('Your email address appears to be invalid.').required('Please enter an email address.'),
        }),
        // The password form field
        // We define our password validations to run only if they are trying to log in as an existing user. 
        password: yup.string().when('isNewUser', {
            is: 'false',
            then: () =>
                yup
                    .string()
                    .min(MIN_PASSWORD_LENGTH, `Password must be at least ${MIN_PASSWORD_LENGTH} characters`)
        }),
    })
    .required()

Hope this saves someone some aggravation. Also, you might want to add some kind of debouncing to the async call depending on your situation. I leave that up to you.

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

No branches or pull requests