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

Do not skip custom validators if value is null and allowNull is true #9143

Closed
papb opened this issue Mar 5, 2018 · 9 comments
Closed

Do not skip custom validators if value is null and allowNull is true #9143

papb opened this issue Mar 5, 2018 · 9 comments

Comments

@papb
Copy link
Member

@papb papb commented Mar 5, 2018

In 2013, the pull request #601 was created to resolve some issues such as #270.

@tremby said (source)

I'd like to see this implemented, but I don't think it should even be an option. If allowNull is true, clearly you want to allow null. If it's not null, only then should the other validations happen.

Five years later, I would like to respectfully disagree. It should be an (opt-out, of course) option. Below I will present a situation where the mandatory aspect of this change is preventing me from doing what I want.

My use case is that I want to allow null only in some cases. Therefore, neither "always" (accomplished with allowNull: true) nor "never" (accomplished by allowNull: false) suit me, and since PR #601 made this a definitive check, any attempt of providing custom validators don't work.

myField: {
  validate: {
    custom(value) {
      if (value === null && !foobar(this.otherField1, this.otherField2)) {
        throw new Error("myField can't be null when otherField1 and otherField2 satisfy property X");
      }
    }
  }
}

When I attempt to create the instance giving null to that field, my custom validator above does not execute at all, no matter what value allowNull is set to.

Therefore this should be an option instead of a forced behaviour.

@awdnf
Copy link

@awdnf awdnf commented Mar 20, 2018

+1

1 similar comment
@damianijr
Copy link

@damianijr damianijr commented Mar 20, 2018

+1

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Apr 7, 2018

I think atleast no validate state should not be tied to allowNull, validators defined on attribute must run

@dylandechant
Copy link

@dylandechant dylandechant commented Oct 31, 2018

this also fits a use case i have

@papb
Copy link
Member Author

@papb papb commented Jan 2, 2019

After further thought on this issue and further investigations on sequelize's code:

Current sequelize behavior is as follows

If the value of an attribute is null, no validators defined on that attribute run at all. Also, in that case, if allowNull is false, a ValidationError is thrown.

How I think it should be

If the value of an attribute is null and allowNull is false, the behavior shouldn't change, i.e., no validators defined on that attribute should run at all and a ValidationError should be thrown. To me this makes sense because running any other validators would be a waste anyway, because the validation is already going to fail.

But if the value is null and allowNull is true:

  • No built-in validators defined on that attribute should run, because none of the build-in validators make sense with a null value (so for this part, the behavior won't change).
  • Custom validators defined on that attribute should run.

Because of the last bullet point, this change would consist of a breaking change, because a few people's code could start throwing ValidationErrors where they previously didn't. Regardless, in my humble opinion is this change is deserved, because the current behavior of unexpectedly ignoring user defined validators is very confusing and restrictive (as I can't define my conditional validation as I wanted in the original post).

I will open a PR implementing the above changes soon (including documentation changes, of course).

@papb papb changed the title Feature request: change feature made in PR #601 from mantadory to optional (opt-out, i.e. active by default) Do not skip custom validators if value is null and allowNull is true Jan 2, 2019
@dalepo
Copy link

@dalepo dalepo commented Jul 9, 2019

I know I'm late for this, but I think this PR does not make any sense. If you have a special case to validate your own model, then treat it as a special case with a model validation.
This broke most of the models from our project, we have around ~150 models. Because of this, we probably did a mistake on upgrading to v5.

@papb
Copy link
Member Author

@papb papb commented Jul 10, 2019

Hi @dalepo, I'm sorry to hear that...

Would you be willing to update your validators or will you end up rolling back to v4?

As I said in the upgrade guide:

To avoid problems when upgrading, please check all your custom validators defined per attribute, where allowNull is true, and make sure all these validators behave correctly when the value is null.

In other words, you can upgrade by adding a if (value === null) return; in the first line of each of your validators... 150 is a lot though, I feel your pain... Good luck.

@dalepo
Copy link

@dalepo dalepo commented Jul 10, 2019

Hi @papb we probably are gonna roll back.

Adding if (value === null) return; would be a good solution, but sadly these functions are used in other modules and tests.
That or probably do a wrapper for each function.

@papb
Copy link
Member Author

@papb papb commented Jul 10, 2019

Of course you know your situation more than I do, but why would the if (value === null) return; break anything? Do your other modules and tests pass null explicitly to the validator? And return something other than undefined?

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

Successfully merging a pull request may close this issue.

6 participants