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

fix: handle custom formats with null values #161

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bahmutov
Copy link

If a custom formatted value (string) has null value and the type allows it, the validation should not fail.

Example that is failing on master and this PR is fixing

{
    type: 'object',
    properties: {
      foo: {
        type: ['string', 'null'],
        format: 'as'
      }
    }
  }, {formats: {as:/^a+$/}
// object
{foo: null}

Seems this was due to fmts vs formats usage

@@ -212,15 +212,15 @@ var compile = function(schema, cache, root, reporter, opts) {
}

if (node.format && fmts[node.format]) {
if (type !== 'string' && formats[node.format]) validate('if (%s) {', types.string(name))
if (type !== 'string' && fmts[node.format]) validate('if (%s) {', types.string(name))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fmts[node.format] will always be truthy since it's checked in the outer if

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's good observation, but does not affect the fix. the only difference between formats and fmts is that fmts has user supplied custom formats and built-ins.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that you can remove that part alltogether, since it will always be true:

if (type !== 'string') validate('if (%s) {', types.string(name))

@LinusU
Copy link
Collaborator

LinusU commented May 1, 2018

Hmm, I remember having a discussion about this at my former place of work 🤔

I think that in the end, we worked around this ourself, potentially because we thought that the current behaviour was the corect one 🤔

I guess that after this change the following will pass:

// schema
{
  type: 'object',
  properties: {
    test: { format: /^a+$/ }
  }
}

// input
{ test: 123 }

whereas before it would be invalid?

This should probably be a breaking change...

@johansteffner any comments?

@bahmutov
Copy link
Author

bahmutov commented May 1, 2018

I just added an assertion to make sure non-string values do not pass

t.notOk(validate({foo:123}), 'not as if number')

but your concern is the "missing string" type, which I think the format is not solving (in any way)

@LinusU
Copy link
Collaborator

LinusU commented May 4, 2018

but your concern is the "missing string" type, which I think the format is not solving (in any way)

Before this patch, only passing a format without a type would still run the format on the value coerced into a string. Thus my example would not pass before, but after this patch, it would let it through.

If that is correct this needs to be a breaking change.

@johansteffner
Copy link

@LinusU the solution we ran with was checking that typeof input === 'string' before running the format validator, returning true if the value is not a string.

@LinusU LinusU mentioned this pull request Aug 13, 2018
4 tasks
@LinusU LinusU added this to the 3.0.0 milestone Aug 13, 2018
ChALkeR added a commit to ExodusMovement/schemasafe that referenced this pull request Jun 29, 2020
ChALkeR added a commit to ExodusMovement/schemasafe that referenced this pull request Jun 29, 2020
ChALkeR added a commit to ExodusMovement/schemasafe that referenced this pull request Jun 29, 2020
kklash pushed a commit to ExodusMovement/schemasafe that referenced this pull request Jun 29, 2020
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.

3 participants