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

Validate port numbers #1346

Closed
sholladay opened this issue Oct 31, 2017 · 7 comments
Closed

Validate port numbers #1346

sholladay opened this issue Oct 31, 2017 · 7 comments
Assignees
Labels
feature New functionality or improvement
Milestone

Comments

@sholladay
Copy link

sholladay commented Oct 31, 2017

I have this in a few places...

joi.validate(option, {
    port : joi.number().integer().min(0).max(65535)
});

Given that joi is most commonly used for servers (related: string().hostname()) and port numbers are a standardized and commonly used thing, I would prefer for this to be built in.

joi.validate(option, {
    port : joi.number().port()
});

Beyond the readability and succinctness of it, I like that this would free my apps from having the 65535 constant, which people sometimes ask me about.

Thoughts?

@WesTyler
Copy link
Contributor

WesTyler commented Nov 1, 2017

My first thought is that this would make a better extension than addition to the core api. I honestly don't know why I think that, but there you have it. :P

@Marsup
Copy link
Collaborator

Marsup commented Nov 1, 2017

I don't think we have anything resembling the wrapping of other existing primitives, not sure I feel good about it either.

@sholladay
Copy link
Author

sholladay commented Nov 1, 2017

By "wrapping other primitives", do you mean higher-level abstractions? joi has plenty of these. In fact, number.positive(), used in this very example, is one of them.

@robertkeizer
Copy link

The hostname specification is defined ( Let's just use RFC 1035 link ) as an abstraction of certain characters and formatting.

An example being that there is a maximum length of 255 octects ( let's just call them characters because they're limited to that space ).

This is reflected ( good ) in Joi:

Joi.validate( { hostname: "fooasldfkjasldkfjasldkfjlasdjflsadjflasjdflkasjdflasjdlfsdlkjfalsdfjalsfjlasdkfjlasdjfalsfjlasfjlaskdfjlasdkfjlaskdjflaskdjflaskdjflsakjfdlaskfjlsakdfjlasdkjflasdkfjalskdfjlsakdfjlaskdjflaskdjflsakdfjbaronetwofoobarasdfkjadlfajlfkajsdflkdajsfldasjflkasjdflaksjflasjflsakjflaskdjflaskdjflasdjflaskdfjlaskdfjlaskfjlasjdflkasdjflaskjdflaskdjflasdkjfalsdjflaskjdflaskjdflasjdflakdsfj" }, s, function( err, result ){ if( err ){ console.log( "invalid" ); } else{ console.log( "valid" ); } } );

// ( output is 'invalid' )

Why would port numbers not also be included, if they are also limited by an RFC ( and industry standard )?

I'm all for keeping the core fairly small, but if you're including hostname, I feel port would also be appropriate.

@Marsup
Copy link
Collaborator

Marsup commented Nov 25, 2017

Can someone submit a PR then?

@mrveera
Copy link

mrveera commented Jan 18, 2018

Hey @Marsup, I added this feature and I got it work, but before submitting PR I already added some other code to my master branch. If it's cleared I can submit PR for this feature also. Otherwise, If you agree I'll add this feature code to other branch and I'll submit PR.

@sholladay
Copy link
Author

That would be fantastic, @veera83372. Thanks for taking the time.

mrveera added a commit to mrveera/joi that referenced this issue Feb 3, 2018
@Marsup Marsup self-assigned this Mar 11, 2018
@Marsup Marsup added this to the 13.2.0 milestone Mar 11, 2018
@Marsup Marsup closed this as completed Mar 11, 2018
@hueniverse hueniverse added feature New functionality or improvement and removed request labels Sep 19, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New functionality or improvement
Projects
None yet
Development

No branches or pull requests

6 participants