feat: IsPort decorator should accept number value

[skobaken]

I was trying to...

class Config {
  @IsPort()
  port: number;
}
const c = new Config();
c.port = 3000;
console.log(validateSync(c)) // error occurred

The problem:
validateSync returns an error in the above snippet.
The error doesn't appear when port type is string.
Is this behavior intended?

class-validator/src/decorator/string/IsPort.ts

Line 11 in 60f72a8

return typeof value === 'string' && isPortValidator(value);

[mzhang28]

It seems like the validator library where isPortValidator is imported from also exhibits the same behavior. I've reported this there as well (validatorjs/validator.js#1562) but the extra typeof check should be removed since validator does its own type-checking anyway.

[mzhang28]

Never mind, it seems that the validator library only likes string types. Maybe this library should perform the conversion ahead of time in the case that it's a number? The wrapper doesn't necessarily need to inherit the restrictions of the underlying library.

[vlapo]

Sure we use validatorjs as main validator library, but we also have own non-string validators e.g. IsInt and more. I think we could accept also numeric input for IsPort.

@NoNameProvided what do you think?

[NoNameProvided]

Technically the current behavior is correct because a port can be described as a pipe and that will be a string. However, I think that is such a rare use-case that we can change it to accept numbers only.

So we would check if the received value is a number and convert it to a string and then pass it to the library. (So we are not accepting both numbers and strings.)

The code can look something like:

typeof value=== 'number' && isPortValidator(value.toString()); 

[vlapo]

@NoNameProvided Why is this breaking change? We can accept both number and string and change is minimal. It is only new functionality not breaking change.

Code should be:

export function isPort(value: unknown): boolean {
  return (typeof value === 'string' || typeof value === 'number') && isPortValidator(value.toString());
}

What do you mean by port can be described as a pipe?

[NoNameProvided]

What do you mean by port can be described as a pipe?

When you expect a port most cases you either get a port number: 8080 or you can get a pipe: \\\\.\\pipe\\Pipe.

Why is this breaking change? We can accept both numbers and strings and change is minimal.

Yes, but that is exactly what I want to avoid. We should not do preprocessing of the values before validating them. That behavior will lead to more issues in the long-term for the users. It's better to be strict and catch errors early than implicitly convert. I would even say, that class-validator should never change the received value before processing it, but as you can see there are exceptions, but even then I would only allow a single value type.

[mzhang28]

The pipe example you gave would not be validated correctly since the first thing the underlying validator library does is check that the string represents a number and that it's within the 65535 range.

While I agree that unnecessary conversion can cause problems, sticking strictly to what the validator library accepts as input would mean that things like port or numbers must exist in the class as strings, and then converted into Number form by users separately.

If you believe that's a behavior that makes sense for this library, I won't prod further but I don't see why a class validator needs to act as a simple thin wrapper around a string validator library.

[NoNameProvided]

I am down for discussion, but still afraid of multiple types. How about accepting only numbers then, does that makes more sense? I am not totally firm on not allowing multiple types, but I would like to see some more feedback on this, before going down this path.

[linkvt]

Hi @NoNameProvided , I also noticed this and was really surprised why I am not able to use IsPort() for a number, not sure if this makes even sense for a string in any case, where the user could just use the number type.
It seems you are the main maintainer right now, so I guess it is mostly on you to decide this, right?

Allowing e.g. IsPort only on numbers would be breaking for quite a lot of validations, not sure if this would be intended even if it is not really necessary?
Are you also really sure that it would make it much more error prone when using multiple types? I would need to check all decorators but IMO the set of validations with multiple types is really limited - or can be limited by you. E.g. allow string and number on IsPort(), allow string and URL on IsUrl() etc...