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 missing value for BasicVal #29

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

waitingcheung
Copy link
Contributor

The following error occurs when the value is missing for min_length and max_length. I have added a test case to reproduce the problem and the corresponding fix.

                if (value.length > max_len) {
                         ^

TypeError: Cannot read property 'length' of undefined
    at Object.check (/path/fieldval.js:1460:26)
    at Function.FieldVal.use_check (/path/fieldval.js:649:50)
    at Function.FieldVal.use_checks (/path/fieldval.js:703:38)
    at Object.check (/path/fieldval.js:1802:49)
    at Function.FieldVal.use_check (/path/fieldval.js:649:50)
    at check_done (/path/fieldval.js:578:46)
    at Function.FieldVal.use_check (/path/fieldval.js:589:17)
    at Function.FieldVal.use_checks (/path/fieldval.js:703:38)
    at FieldVal.get_async (/path/fieldval.js:274:39)
    at FieldVal.get (/path/fieldval.js:244:31)

@waitingcheung
Copy link
Contributor Author

@MarcusLongmuir Would you mind letting me know how this pull request looks to you? Thank you.

@MarcusLongmuir
Copy link
Contributor

MarcusLongmuir commented Mar 31, 2017

@waitingcheung: Apologies for the late reply. I missed the notifications.

The original intention with FieldVal was that you would chain your checks such that you would do validator.get("name", BasicVal.string(true), BasicVal.min_length(3));, however it's not clear that this is required to avoid throwing exceptions as you've shown above.

I think it would be cleaner to replace the typeof strings with calls to FieldVal's .string() check as that is what is assumed you've put before these min_length and max_length checks, but if you haven't then you get that behaviour for free.

However, another issue is that min_length and max_length in their current form can be used on arrays. This change would restrict it to just strings.

@waitingcheung
Copy link
Contributor Author

@MarcusLongmuir Thank you for your explanations.

What I intended to show in my test case is that FieldVal.get() returns undefined for invalid field names. When we chain it with other functions such as BasicVal.min_length and those functions access the length property of undefined, we get the TypeError I showed you because undefined does not have such a property.

For example, if we chain it like this without .string() as you suggested, we can reproduce the TypeError.

validator.get("invalid field name", BasicVal.min_length(3));

What about throwing an error when FieldVal.get() takes an invalid field name as its argument?

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.

None yet

2 participants