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

feat: @IsOptional should works only for undefined values #491

Open
vsternbach opened this issue Dec 19, 2019 · 18 comments · May be fixed by #2443
Open

feat: @IsOptional should works only for undefined values #491

vsternbach opened this issue Dec 19, 2019 · 18 comments · May be fixed by #2443
Labels
type: feature Issues related to new features.

Comments

@vsternbach
Copy link

This code:

import { IsNotEmpty, IsOptional, IsString, validate } from 'class-validator';
import { plainToClass } from 'class-transformer';

const PATCH = 'patch';
const POST = 'post';

export class Test {
  @IsOptional({ groups: [PATCH] })
  @IsNotEmpty({ always: true })
  @IsString()
  name: string;
}

async function getValidationErrors(obj, group) {
  return await validate(plainToClass(Test, obj), { groups: [group] });
}

describe('Test', () => {
  it('should fail on post without name', async () => {
    const errors = await getValidationErrors({}, POST);
    expect(errors).not.toEqual([]);
  });
  it('should fail on post when name is undefined', async () => {
    const errors = await getValidationErrors({ name: undefined }, POST);
    expect(errors).not.toEqual([]);
  });
  it('should fail on post when name is null', async () => {
    const errors = await getValidationErrors({ name: null }, POST);
    expect(errors).not.toEqual([]);
  });
  it('should fail on post when name is empty', async () => {
    const errors = await getValidationErrors({ name: '' }, POST);
    expect(errors).not.toEqual([]);
  });
  it('should succeed on patch without name property', async () => {
    const errors = await getValidationErrors({}, PATCH);
    expect(errors).toEqual([]);
  });
  it('should fail on patch when name is undefined', async () => {
    const errors = await getValidationErrors({ name: undefined }, PATCH);
    expect(errors).not.toEqual([]);
  });
  it('should fail on patch when name is null', async () => {
    const errors = await getValidationErrors({ name: null }, PATCH);
    expect(errors).not.toEqual([]);
  });
  it('should fail on patch when name is empty', async () => {
    const errors = await getValidationErrors({ name: '' }, PATCH);
    expect(errors).not.toEqual([]);
  });
});

gives the following results:
Screen Shot 2019-12-19 at 18 15 43
Am I missing something here?

Looking at the code of @IsOptional I see that it checks that the property is not undefined or null, that's why the test for empty string is not failing, but in theory shouldn't it check that this property exists on the object?

@vlapo
Copy link
Contributor

vlapo commented Jan 10, 2020

I do not understand your problem. I think your test cases are not good. You define property name as optional in case PATCH is in groups.

it('should fail on patch when name is undefined', async () => {
    const errors = await getValidationErrors({ name: undefined }, PATCH);
    expect(errors).not.toEqual([]);
  });
  it('should fail on patch when name is null', async () => {
    const errors = await getValidationErrors({ name: null }, PATCH);
    expect(errors).not.toEqual([]);
  });

So Test should succeed on patch when name is undefined/null.
From docs:

@IsOptional() Checks if given value is empty (=== null, === undefined) and if so, ignores all the validators on the property.

@Kiyozz
Copy link

Kiyozz commented Mar 26, 2020

I use nestjs to validate a DTO from my controller


I have this

class MyDTO {
  @IsOptional()
  @IsNotEmpty()
  locale?: string
}

I send this body json

{
  "locale": null
}

The expected behaviour:

  • ERROR 400, locale should not be empty

The actual behaviour:

  • CODE 200, locale null is ignored.

If I have this DTO

class MyDTO {
  @IsNotEmpty()
  locale?: string
}

Then null value throws CODE 400 which is expected.


@IsOptional() should ignores only when the key is not passed in the request (aka. undefined) or an option to not ignores null value.

@vlapo vlapo changed the title @IsOptional with @IsNotEmpty is not working feat: @IsOptional should works only for undefined values Mar 26, 2020
@vlapo vlapo added the type: feature Issues related to new features. label Mar 26, 2020
@vlapo
Copy link
Contributor

vlapo commented Mar 26, 2020

I understand now. But as it is a breaking change I want to have more feedback from community.

@jrafaels
Copy link

I don't know if is the same issue, but i need to validate if a variable is notEmpty, notNull and notUndefined. Is that possible?

@sam3d
Copy link

sam3d commented May 5, 2020

@vlapo How about something like an extra config option? The implementation I've got in my fork doesn't break backwards compatibility (as the option defaults to true, which is the default behaviour):

@isOptional({ nullable: false }) // object.hasOwnProperty(property);
@isOptional({ nullable: true }) // value !== undefined && value !== null
@isOptional() // value !== undefined && value !== null

The commit on the fork that adds this behaviour: se-internal@5ac9fa0

@fhp
Copy link

fhp commented May 27, 2020

We would love this as well, ether with a nullable flag, or as a separate set of decorators (although naming them can be difficult).
@sam3d, your fork looks good to me, would you mind creating a PR?

@sam3d
Copy link

sam3d commented May 28, 2020

Thanks @fhp! Yeah we had originally tried to create a new decorator and must've spent over an hour trying to come up with a good name. Eventually, we decided it made more sense to borrow the design pattern from typeorm's @ManyToOne() decorator (a nullable property that defaults to true).

I'll definitely put it into a PR at some point in the next couple of days! Just waiting until I have a moment to add tests. Confirmation from @vlapo or another maintainer that at least the design proposal is accepted would be amazing so I know I'm not doing it for nothing 😄

@omidh28
Copy link

omidh28 commented Jul 7, 2020

Any update for this? really want to see this feature in next release
@sam3d

@NoNameProvided
Copy link
Member

I agree about the strange behavior of this.

I like the API proposed by @sam3d. We can use the current behavior as default and add a warning in the changelog about eventually being flipped.

I will take a crack at this at the weekend.

@omidh28
Copy link

omidh28 commented Aug 14, 2020

@NoNameProvided Any update? 10 days has been passed

@NickKelly1
Copy link
Contributor

How about keeping @IsOptional as-is and adding new decorators @IsNullable and @IsUndefinable.
Any support for this?

@ruscon
Copy link
Contributor

ruscon commented Sep 16, 2020

/**
 * Skips validation if the target is null
 *
 * @example
 * ```typescript
 * class TestModel {
 *     @IsNullable({ always: true })
 *     big: string | null;
 * }
 * ```
 */
export function IsNullable(options?: ValidationOptions): PropertyDecorator {
    return function IsNullableDecorator(prototype: object, propertyKey: string | symbol): void {
        ValidateIf((obj): boolean => null !== obj[propertyKey], options)(prototype, propertyKey);
    };
}
/**
 * Skips validation if the target is undefined
 *
 * @example
 * ```typescript
 * class TestModel {
 *     @IsUndefinable({ always: true })
 *     big?: string;
 * }
 * ```
 */
export function IsUndefinable(options?: ValidationOptions): PropertyDecorator {
    return function IsUndefinableDecorator(prototype: object, propertyKey: string | symbol): void {
        ValidateIf((obj): boolean => undefined !== obj[propertyKey], options)(prototype, propertyKey);
    };
}

@hfhchan-plb
Copy link

For other parts of TypeScript, "optional" means either the property is not set, or it is set to undefined. In no other places does "optional" imply "nullable", whereas "nullable" itself can sometimes imply "optional" (c.f. NonNullable utility type). The naming of IsOptional is really confusing.

I suggest the following changes to align with the terminology of TypeScript:

  1. @IsOptional() taking a new strict parameter, defaulting to false which allows null for back-compat, and true for denying null. The strict parameter should default to true at some later version and possibly throw if it is set to false.
  2. @IsNullable with a strict parameter defaulting to true which only allows null, and false for also allowing undefined.

@mwanago
Copy link

mwanago commented Mar 14, 2023

I think

@IsOptional({ nullable: false })

looks great, as suggested by @sam3d
@vlapo Are there any plans on implementing that?

@mwanago
Copy link

mwanago commented Mar 14, 2023

Meanwhile, I suggest creating two separate decorators:

import { ValidateIf } from 'class-validator';

export function CanBeUndefined() {
  return ValidateIf((object, value) => value !== undefined);
}

export function CanBeNull() {
  return ValidateIf((object, value) => value !== null);
}

@pigulla
Copy link
Contributor

pigulla commented Mar 15, 2023

In case anyone is interested, I've had the exact same issue and implemented this (alongside quite a few more decorators) in class-validator-extended, specifically Optional() and Nullable().

@oscarmeanwell
Copy link

This is still an issue, please can this be updated? Some fantastic solutions have been presented. I am happy to make a PR

@fr1sk
Copy link

fr1sk commented Jul 28, 2023

any update on this one? And what about PartialType which is making all props optional? Can we somehow pass this nullable param to PartialType as well? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature Issues related to new features.