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

Bug report: Poor empty value handling for default format validator? #99

Closed
mondwan opened this Issue Feb 18, 2016 · 18 comments

Comments

Projects
None yet
6 participants
@mondwan
Contributor

mondwan commented Feb 18, 2016

Description

Given I need to validate value is not empty, for example

  • '': Invalid
  • ' ': Valid

Problems

Option presence explicitly states whitespaces only is not a valid string. So I head to format.

However, default format validator always allows empty value (empty defined above) at the beginner which ignores my regex object.

Reproduce the problem

Here is the fiddle to demonstrate the problem: https://jsfiddle.net/8okbtbuz/

Given I have a regex ^.{1,64}$' which ensures there is at least one character, validator.single() always treat the empty value is a valid one.

Problematic codes spotted

https://github.com/ansman/validate.js/blob/master/validate.js#L948

It always return success for empty value. This is not good.....

@ansman

This comment has been minimized.

Show comment
Hide comment
@ansman

ansman Feb 20, 2016

Owner

This is by design, you have to use both presence and format to achieve what you want.

Owner

ansman commented Feb 20, 2016

This is by design, you have to use both presence and format to achieve what you want.

@mondwan

This comment has been minimized.

Show comment
Hide comment
@mondwan

mondwan Feb 21, 2016

Contributor

Nope. Both of them does not work for me

By code,

https://github.com/ansman/validate.js/blob/master/validate.js#L759

For option presence, It always return false for empty string. (a string contains spaces only)

Code example,

var a = ['', ' ', '  '];

a.forEach(function (v) {
    console.log('value |%s|', v);

  // Presence on
    console.log('presesence on' , validate.single(v, {
    presence: true,
    format: {
        pattern: "^.{1,64}$",
      message: 'haha',
    },
  }));

  // presence off
    console.log('presence off', validate.single(v, {
    presence: false,
    format: {
        pattern: "^.{1,64}$",
      message: 'haha',
    },
  }));
 });

Output

value ||
presesence on ["can't be blank"]
presence off undefined
value | |
presesence on ["can't be blank"]
presence off undefined
value |  |
presesence on ["can't be blank"]
presence off undefined

Fiddler

https://jsfiddle.net/8okbtbuz/1/

Expected result

'' -> invalid, empty string
' ' -> valid, string with at least one space
Contributor

mondwan commented Feb 21, 2016

Nope. Both of them does not work for me

By code,

https://github.com/ansman/validate.js/blob/master/validate.js#L759

For option presence, It always return false for empty string. (a string contains spaces only)

Code example,

var a = ['', ' ', '  '];

a.forEach(function (v) {
    console.log('value |%s|', v);

  // Presence on
    console.log('presesence on' , validate.single(v, {
    presence: true,
    format: {
        pattern: "^.{1,64}$",
      message: 'haha',
    },
  }));

  // presence off
    console.log('presence off', validate.single(v, {
    presence: false,
    format: {
        pattern: "^.{1,64}$",
      message: 'haha',
    },
  }));
 });

Output

value ||
presesence on ["can't be blank"]
presence off undefined
value | |
presesence on ["can't be blank"]
presence off undefined
value |  |
presesence on ["can't be blank"]
presence off undefined

Fiddler

https://jsfiddle.net/8okbtbuz/1/

Expected result

'' -> invalid, empty string
' ' -> valid, string with at least one space
@ansman

This comment has been minimized.

Show comment
Hide comment
@ansman

ansman Feb 22, 2016

Owner

Ah, I see your dilemma. I'll draft a solution.

Owner

ansman commented Feb 22, 2016

Ah, I see your dilemma. I'll draft a solution.

@ansman

This comment has been minimized.

Show comment
Hide comment
@ansman

ansman Aug 15, 2016

Owner

@mondwan, @jpbufe3, @sckogi I've drafted a solution in ecde117, how does it look?
I see two main use cases, one is user inputted data such a form on a web page. It's easy to put an extra space somewhere which doesn't really qualify as having entered something.

The other is a server which should have strict parsing in which a whitespace string is very much defined.

Owner

ansman commented Aug 15, 2016

@mondwan, @jpbufe3, @sckogi I've drafted a solution in ecde117, how does it look?
I see two main use cases, one is user inputted data such a form on a web page. It's easy to put an extra space somewhere which doesn't really qualify as having entered something.

The other is a server which should have strict parsing in which a whitespace string is very much defined.

@mondwan

This comment has been minimized.

Show comment
Hide comment
@mondwan

mondwan Aug 16, 2016

Contributor

Hello @ansman , glad to see this patch. In my opinions, changes on format suits my need and the introduction of disallowEmpty on presence is quite good.

However, I have not checked out the changes on others as there are no real usages on my side. I cannot judge whether they are good or not

2 suggestions at last:

  • Comments like here should be removed or updated. Otherwise, it cause confusion.
  • disallowEmpty should be allowEmpty? This provides better backward compatibility?

Thanks for your patch :D

Contributor

mondwan commented Aug 16, 2016

Hello @ansman , glad to see this patch. In my opinions, changes on format suits my need and the introduction of disallowEmpty on presence is quite good.

However, I have not checked out the changes on others as there are no real usages on my side. I cannot judge whether they are good or not

2 suggestions at last:

  • Comments like here should be removed or updated. Otherwise, it cause confusion.
  • disallowEmpty should be allowEmpty? This provides better backward compatibility?

Thanks for your patch :D

@sckogi

This comment has been minimized.

Show comment
Hide comment
@sckogi

sckogi Aug 16, 2016

Hi @ansman, I agree with @mondwan about allowEmpty. And it seems in spec is missing some cases like datetime([], {}) and so.

Thanks ;D !

sckogi commented Aug 16, 2016

Hi @ansman, I agree with @mondwan about allowEmpty. And it seems in spec is missing some cases like datetime([], {}) and so.

Thanks ;D !

ansman pushed a commit that referenced this issue Aug 24, 2016

@ansman

This comment has been minimized.

Show comment
Hide comment
@ansman
Owner

ansman commented Aug 24, 2016

@mondwan

This comment has been minimized.

Show comment
Hide comment
@mondwan

mondwan Oct 17, 2016

Contributor

Hello @ansman , may I know the date for releasing this patch?

Contributor

mondwan commented Oct 17, 2016

Hello @ansman , may I know the date for releasing this patch?

@ansman

This comment has been minimized.

Show comment
Hide comment
@ansman

ansman Nov 5, 2016

Owner

Released in 0.11.0

Owner

ansman commented Nov 5, 2016

Released in 0.11.0

@mondwan

This comment has been minimized.

Show comment
Hide comment
@mondwan

mondwan Nov 7, 2016

Contributor

Hello @ansman , after using the 0.11.0 in my production environment, I believe value '' should be included in isDefined().

In line 275

# It should rewrite this
return obj !== null && obj !== undefined;
# to this
return obj !== null && obj !== undefined && obj !== '';

Here is a JSfiddler to illustrate the reasons:

Goal of my program

  • Allow string if value is ''
  • Allow strings for example ' ', ' ' if they obey regex ^.{1,5}$

Current result

# Case1
value ||
allowEmpty true ["Invalid from format"]
allowEmpty false ["Invalid from presence", "Invalid from format"]

# Case2
value | |
allowEmpty true undefined
allowEmpty false ["Invalid from presence"]

# Case3
value |  |
allowEmpty true undefined
allowEmpty false ["Invalid from presence"]

# Case 4
value |012345|
allowEmpty true ["Invalid from format"]
allowEmpty false ["Invalid from format"]

Summary

Version before 0.11.1 failure for #Case 1, 2, 3 and version 0.11.1 failures only on #Case 1.

In version 0.11.1, rule format will always be executed even string is "not defined" which is ''. Therefore, #Case 1 is invalid. For #Case2, 3, they are valid as values "are defined".

Below is the expected result I hope to get.

Expected result

# Case1
value ||
allowEmpty true undefined
allowEmpty false ["Invalid from presence", "Invalid from format"]

# Case2
value | |
allowEmpty true undefined
allowEmpty false ["Invalid from presence"]

# Case3
value |  |
allowEmpty true undefined
allowEmpty false ["Invalid from presence"]

# Case 4
value |012345|
allowEmpty true ["Invalid from format"]
allowEmpty false ["Invalid from format"]
Contributor

mondwan commented Nov 7, 2016

Hello @ansman , after using the 0.11.0 in my production environment, I believe value '' should be included in isDefined().

In line 275

# It should rewrite this
return obj !== null && obj !== undefined;
# to this
return obj !== null && obj !== undefined && obj !== '';

Here is a JSfiddler to illustrate the reasons:

Goal of my program

  • Allow string if value is ''
  • Allow strings for example ' ', ' ' if they obey regex ^.{1,5}$

Current result

# Case1
value ||
allowEmpty true ["Invalid from format"]
allowEmpty false ["Invalid from presence", "Invalid from format"]

# Case2
value | |
allowEmpty true undefined
allowEmpty false ["Invalid from presence"]

# Case3
value |  |
allowEmpty true undefined
allowEmpty false ["Invalid from presence"]

# Case 4
value |012345|
allowEmpty true ["Invalid from format"]
allowEmpty false ["Invalid from format"]

Summary

Version before 0.11.1 failure for #Case 1, 2, 3 and version 0.11.1 failures only on #Case 1.

In version 0.11.1, rule format will always be executed even string is "not defined" which is ''. Therefore, #Case 1 is invalid. For #Case2, 3, they are valid as values "are defined".

Below is the expected result I hope to get.

Expected result

# Case1
value ||
allowEmpty true undefined
allowEmpty false ["Invalid from presence", "Invalid from format"]

# Case2
value | |
allowEmpty true undefined
allowEmpty false ["Invalid from presence"]

# Case3
value |  |
allowEmpty true undefined
allowEmpty false ["Invalid from presence"]

# Case 4
value |012345|
allowEmpty true ["Invalid from format"]
allowEmpty false ["Invalid from format"]
@alolis

This comment has been minimized.

Show comment
Hide comment
@alolis

alolis Nov 7, 2016

I am trying to figure out if this should now return error or not:

validate.single("", {presence: false, { length : { minimum : 5 } }});

I am expecting to NOT return error since presence: false but I am getting a length error. Is there something I am missing here? I am using 0.11.1

alolis commented Nov 7, 2016

I am trying to figure out if this should now return error or not:

validate.single("", {presence: false, { length : { minimum : 5 } }});

I am expecting to NOT return error since presence: false but I am getting a length error. Is there something I am missing here? I am using 0.11.1

@mondwan

This comment has been minimized.

Show comment
Hide comment
@mondwan

mondwan Nov 7, 2016

Contributor
Contributor

mondwan commented Nov 7, 2016

@alolis

This comment has been minimized.

Show comment
Hide comment
@alolis

alolis Nov 7, 2016

@mondwan , thanks for the answers for starters. Redefining isDefined and include "" as an undefined value, might cause other issues though in different cases where it does make sense.

My case for the validator is a form which has optional fields but if the user does enter a value, it should be validated (specifically it's an optional "New Password" field)

I guess i need to re-define isUndefined or just use a regex validator. The presence: {value: false, allowEmpty: true} would have been a nice solution to this, without messing around with anything else.

alolis commented Nov 7, 2016

@mondwan , thanks for the answers for starters. Redefining isDefined and include "" as an undefined value, might cause other issues though in different cases where it does make sense.

My case for the validator is a form which has optional fields but if the user does enter a value, it should be validated (specifically it's an optional "New Password" field)

I guess i need to re-define isUndefined or just use a regex validator. The presence: {value: false, allowEmpty: true} would have been a nice solution to this, without messing around with anything else.

@mondwan

This comment has been minimized.

Show comment
Hide comment
@mondwan

mondwan Nov 7, 2016

Contributor
Contributor

mondwan commented Nov 7, 2016

@alolis

This comment has been minimized.

Show comment
Hide comment
@alolis

alolis Nov 7, 2016

However, if you are trying to solve this by regex, I guess regex like this ^.{0,5}$ should play well on
existing mechanism.

That's what I did, but I really think it should be fixed via the presence validator.

alolis commented Nov 7, 2016

However, if you are trying to solve this by regex, I guess regex like this ^.{0,5}$ should play well on
existing mechanism.

That's what I did, but I really think it should be fixed via the presence validator.

@obsidianart

This comment has been minimized.

Show comment
Hide comment
@obsidianart

obsidianart Nov 8, 2016

agree with alolis (I have a similar scenario) it would be nice to specify value:false. For the time being I'll downgrade validatejs. I didn't expect a breaking change in a minor release either...

obsidianart commented Nov 8, 2016

agree with alolis (I have a similar scenario) it would be nice to specify value:false. For the time being I'll downgrade validatejs. I didn't expect a breaking change in a minor release either...

@mondwan

This comment has been minimized.

Show comment
Hide comment
@mondwan

mondwan Nov 22, 2016

Contributor

Hello @ansman , any comment for this issue?

Contributor

mondwan commented Nov 22, 2016

Hello @ansman , any comment for this issue?

@airtonix

This comment has been minimized.

Show comment
Hide comment
@airtonix

airtonix Apr 16, 2018

easiest way to handle this is by using the regexp or operator:

before:


		format: {
			pattern: '[0-9]{6,10}',
			flags: 'ig',
			message: '^Valid Account numbers are between 6 and 10 digits'
		},

code_2018-04-17_09-25-46

after

		format: {
			pattern: '^([\s]+)?$|[0-9]{6,10}',
			flags: 'ig',
			message: '^Leave blank or provide a valid account number containing between 6 and 10 digits'
		},

code_2018-04-17_09-22-27

airtonix commented Apr 16, 2018

easiest way to handle this is by using the regexp or operator:

before:


		format: {
			pattern: '[0-9]{6,10}',
			flags: 'ig',
			message: '^Valid Account numbers are between 6 and 10 digits'
		},

code_2018-04-17_09-25-46

after

		format: {
			pattern: '^([\s]+)?$|[0-9]{6,10}',
			flags: 'ig',
			message: '^Leave blank or provide a valid account number containing between 6 and 10 digits'
		},

code_2018-04-17_09-22-27

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