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

when and concat break preceding oneOf #473

Closed
Philipp91 opened this issue Feb 24, 2019 · 4 comments
Closed

when and concat break preceding oneOf #473

Philipp91 opened this issue Feb 24, 2019 · 4 comments

Comments

@Philipp91
Copy link
Contributor

// Regular oneOf validation, nothing interesting.
await Yup.string().oneOf(['a'])..isValid('a'); // true
await Yup.string().oneOf(['a'])..isValid('b'); // false

// Followed by a when() that does not trigger, everything stays correct and the same.
await Yup.string().oneOf(['a']).when('x', {is: () => false, then: Yup.string()}).isValid('a'); // true
await Yup.string().oneOf(['a']).when('x', {is: () => false, then: Yup.string()}).isValid('b'); // false

// Followed by a when() that triggers, incorrectly marks as invalid.
await Yup.string().oneOf(['a']).when('x', {is: () => true, then: Yup.string()}).isValid('a'); // false

The last one with validate() throws "ValidationError: this must be one of the following values: ". There are no values listed. Seems like this.schema._whitelist is empty.

@Philipp91
Copy link
Contributor Author

Debugged it a bit further, this should be equivalent to:

await Yup.string().max(3).concat(Yup.string().oneOf(['a'])).isValid('a'); // true
await Yup.string().oneOf(['a']).concat(Yup.string().max(3)).isValid('a'); // false

@Philipp91 Philipp91 changed the title when breaks preceding oneOf when breaks preceding concat/oneOf Feb 24, 2019
@Philipp91 Philipp91 changed the title when breaks preceding concat/oneOf when and concat break preceding oneOf Feb 24, 2019
@Philipp91
Copy link
Contributor Author

Perhaps this is already fixed by #444.

@Philipp91
Copy link
Contributor Author

It's not fixed at head. Here's a failing test case to reproduce:

  it('concat should preserve oneOf', async function() {
    let inst = string().oneOf(['a']).concat(string().default('hi'));

    await inst.isValid('a').should.become(true);
  });

Probably the concat function (or the prependDeep() that it uses) need to be made aware of RefSet (currently it just overwrites one with the other) and merge two RefSets such that either one is forced to be empty (and the other one is returned), or the inner one (IIUC that's the target in prependDeep) takes precendence in case of conflicts, i.e. whitelistOut = (whitelistSource UNION whitelistTarget) SETMINUS blacklistTarget and blacklistOut = (blacklistSource UNION blacklistTarget) SETMINUS whitelistTarget. This may require RefSet to move to its own file, or to expose a RefSet.merge() or RefSet.prependDeep() function that can be called from outside.

Let me know if you want me to send a PR.

Philipp91 added a commit to Philipp91/yup that referenced this issue Mar 7, 2019
Philipp91 added a commit to Philipp91/yup that referenced this issue Mar 7, 2019
@gregie156
Copy link

gregie156 commented Apr 8, 2019

@Philipp91 Your fix looks solid. Are you going to push it into the main Yup repo?

edit: Oh, I see you opened a PR and they are dragging their feet approving it.

gregie156 added a commit to gregie156/yup that referenced this issue Apr 8, 2019
fix: jquense#473 make concat compatible with (not)oneOf
@jquense jquense closed this as completed in 8d21cc9 May 6, 2019
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

2 participants