feat: IsPort decorator should accept number value

[skobaken7]

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? https://github.com/typestack/class-validator/blob/60f72a829da5b7669b852b96e4764ed525d772c3/src/decorator/string/IsPort.ts#L11

[iptq]

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

[iptq]

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.

[iptq]

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...

[jbree]

Just ran into this issue today when IsPort didn't behave as I had assumed. I see the point about not accepting multiple types, especially if that's something this library doesn't already do. Could be a huge breaking change.

How about an alternative? Add an IsPortNumber decorator. The name clearly precludes non-numerical ports which may or may not be supported by the underlying string validating library. This would provide support for those of us who want to use numbers, without any breaking changes.

[hungtcs]

I added such a custom decorator to my project:

import { buildMessage, isPort, ValidateBy, ValidationOptions } from 'class-validator';

export const IS_PORT_NUMBER = 'isPortNumber';

export function isPortNumber(value: unknown): boolean {
  return typeof value === 'number' && isPort(`${ value }`);
}

export function IsPortNumber(validationOptions?: ValidationOptions): PropertyDecorator {
  return ValidateBy(
    {
      name: IS_PORT_NUMBER,
      constraints: [],
      validator: {
        validate: (value): boolean => {
          return isPortNumber(value);
        },
        defaultMessage: buildMessage(eachPrefix => eachPrefix + '$property must be a valid port number', validationOptions),
      },
    },
    validationOptions
  );
}

[NoNameProvided]

How about an alternative? Add an IsPortNumber decorator.

No. That is duck-taping instead of deciding on a firm solution.

To cycle back to the original discussion: I don't want to introduce multiple expected types. It's a slippery slope and soon we would have multiple requests to add support for accepting random convertible types for other decorators because users don't normalize their incoming data.

There are two solutions:

• we should move forward with updating the current @IsPort decorator
• we don't change the current behavior

If we move forward with the update we should update the logic to accept a number in the 0-65535 range. This means we replicate the isPort validator from validator.js with the only difference of expecting a number.

At this point I am not even sure the decorator is needed, because this is easily replicated with the following:

class MyPayload {
  @IsNumber()
  @Min(0)
  @Max(65535)
  port: number;
}

On the other hand, we have no easy way to replicate the current behavior because we don't have min/max validators for numbers represented as strings.

Another thing to note is that after changing the type the first request would be to add an option to the decorator to limit the allowed range. This is a different breaking change because currently, we expect the validation options in the first parameter.

So I start to lean toward the opinion that this is good as is, because if someone needs port as numbers
it is already easily achievable.

[braaar]

Well reasoned, @NoNameProvided. I think the Min and Max approach to validating port numbers is quite elegant in its simplicity and transparency.

Some people may still want to use IsPortNumber as a shorthand, in which case I think it's reasonable to expect people to use custom decorators.

[hungtcs]

For me, the advantage of using the IsPortNumber decorator is that it can have corresponding error information. I prefer the existing IsPort decorator to support the number type, maybe add a new parameter like this

@IsPort({ number: true })
public port: number;

[NoNameProvided]

Closing this as wontfix.

The problem can be solved with existing decorators (see my comment) or via implementing a custom decorator if someone wants this specific behavior.

[github-actions[bot]]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.