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.

Sign up for GitHub

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

Jump to bottom

FIX string parsing issue of number format #371

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

FIX string parsing issue of number format #371

wants to merge 1 commit into from

Conversation

furkankose
Copy link

While I was using convict in a project, I realized that number format validation does not work properly. When you set a string value (other than the values that can be parse to number) to a number field, convict should throw a validation error but it does not. That's why, I added an isNumber function to fix this issue.

By the way, I am not totally sure about the scope of number format. That's why I estimated NaN and Infinity values as truthy values because of their data types. If NaN and Infinity values are not valid options for number format, I can update the function by taking that into consideration.

You can reproduce the issue by using the code below

const convict = require('convict')

const config = convict({
  number: {
    format: Number,
    default: 53,
  },
})

config.load({number: 'convict'})
config.validate({strict: true})

console.log('number: ', config.get('number')) // number: NaN

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.8%) to 91.599% when pulling fb7711d on furkankose:master into e3770a6 on mozilla:master.

@A-312
Copy link
Contributor

A-312 commented May 4, 2020

Be careful the behavior will change in the next version of convict

@A-312
Copy link
Contributor

A-312 commented May 4, 2020

Madarche works on my PR, my code was like that https://github.com/A-312/node-blueconfig/blob/dc67c0de4dccbc2d04039f9a43f5cb91d08de340/packages/blueconfig/lib/index.js#L475

@furkankose
Copy link
Author

Be careful the behavior will change in the next version of convict

Oops, I was not aware of this. Should I close the PR in this situation?

@A-312
Copy link
Contributor

A-312 commented May 4, 2020 •
edited
Loading

@madarche Maybe you should keep a PR open "Convict next version".

@kon14
Copy link

kon14 commented Jul 4, 2022

Any updates on this?

@BenceSzalai
Copy link

Shouldn't the same changes be applied to int type as well? parseInt is also susceptible of returning NaN for random strings.

case 'int': v = parseInt(v, 10); break`

@madarche
Copy link
Collaborator

madarche commented May 8, 2023 •
edited
Loading

Yes, both Number and int are affected by the same problem.

Here are the definitions of what the int and nat formats are expected to be and those definitions have no problems:
https://github.com/mozilla/node-convict/blob/master/packages/convict/src/main.js#L56-L61

So a fix should try to leverage those definitions instead of adding a new way, a new function, to validate what a number is.

Also a fix would really be warmly welcomed if it could contain a matching test and not lower the test coverage.

Thank you

@BenceSzalai
Copy link

BenceSzalai commented May 8, 2023 •
edited
Loading

Ah, thanks for the response. I think i can look into this, doesn't seem to be a huge thing.

It is off-topic, but don't know a better place to ask: generally speaking I find this repo a bit confusing, as there are issues and open PRs with little activity for years suggesting it may be abandoned, also mentions of "next version" / v7 further discouraging involvement, and then I got this unexpectedly quick response. So not sure what should I think of this project. Is there a status page/roadmap or similar to educate myself before engaging with the project?

@madarche
Copy link
Collaborator

You have said it all @BenceSzalai: very little activity almost abandoned, but because the project is stable, used and useful, it's being maintained to a minimum to keep the applications running and the users safe. Also small additions that improve this package without making it more complex are welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@furkankose @coveralls @A-312 @kon14 @BenceSzalai @madarche