[FEAT] Non-nullable properties in requestBody can be null and are not coerced into the correct type

[markusenglund]

Information

• Version: 7.x
• Packages:

It's possible to send in null as the value for string, integer and bool properties. Unexpectedly, this will not cause a validation error and the null will not be coerced into the correct type.

Example

export class Test {
    @Property()
    string?: string;

    @Integer()
    integer?: number;

    @Property(Boolean)
    boolean?: boolean;

    @ArrayOf(String)
    stringArray?: string[];
}

Request body:

{
  "string": null,
  "integer": null,
  "boolean": null,
  "stringArray": [null]
}

Inside the controller:

console.log(test) logs:

 {
    string: null,
     integer: null,
     boolean: null,
     stringArray: [ null ]
   }

This is definitely not expected behavior. The problem can be fixed by setting coerceTypes: false in the AJV options, however that's not what we want to do.
The problem seems to stem from the fact that the TS.ED code uses coerceTypes: true but then validates a deepCloned version of the object and returns the original version of the object that doesn't have the corrected types (https://github.com/tsedio/tsed/blob/production/packages/specs/ajv/src/services/AjvService.ts). Is there a reason for that?

Acceptance criteria

• null should be correctly case into the correct schema type.

[Romakita]

Hello @markusenglund
Can read the doc please. There a @Nullable decorator for that.

See you

[github-actions]

🎉 Are you happy?

If you appreciated the support, know that it is free and is carried out on personal time ;)

A support, even a little bit makes a difference for me and continues to bring you answers!

[markusenglund]

I'm well aware of the @Nullable decorator. What I'm trying to express is that the current behavior incorrectly accepts and passes on properties that have the value null even though the @Nullable decorator is not used.

I'm not trying to get the API to accept null - I want it to not accept null since it violates the schema definition.

[Romakita]

Ok understood. The reason is the coerceType is neccessary to accept number a string field and vice versa. I won’t change that because it’s acceptable to use type coercion in this case.

but i don’t want to use the coercion result performed by ajv because it’s the json-mapper to make it that.

your issue is correct but it’s related to ajv. I tried many config with ajv but this is the best config that match the expected behavior actually.

See you

[markusenglund]

I see. Maybe it should be fixed in Json-mapper then? I saw that there are tests that imply that not converting null to the correct type is intended behavior. Is there a reason for that?

Do you agree generally that this is something that should be fixed? There are security implications for letting users send in null in fields that the application developers incorrectly believe are validated as non-nullable. It's especially important in fields that are required.

[Romakita]

For this is the validator responsability to avoid null on field. Maybe ajv had an option to not coerce null value.

[Romakita]

I’ll try to take a time to find the best option ^^

[Romakita]

I saw that there are tests that imply that not converting null to the correct type is intended behavior. Is there a reason for that

This test is correct. null is value that can be used on String (combined with enum), when you haven't information over the property. Some teams use empty string, but other team use null.

It's the same thing when a property expose a price from a Booking Engine:

• price with value => the price known because the given criteria is enough to compute a price
• price with null => the price cannot be determine based the given criteria. In this case, we can't return 0 value, because this value meaning the price is known and it's free ^^

This is why I don't use the coercion result from ajv:
https://ajv.js.org/coercion.html

The solution

Adding a new option to let ajvService to returns the coerced value. Json-mapper won't see the difference, because the type will be correctly mapped by Ajv.

Example:

@Configuration({
   ajv: {
      returnsCoercedValue: true // let ajv Service to return the coerced value 
   }
})

Without Nullable decorator:

class Model {
   @Property()
   propString: string;  // null become ''
   
   @Property()
   propNumber: number;  // null become 0

   @Property()
   propBool: boolean;  // null become false
}

But if Nullable decorator is used:

class Model {
   @Nullable(String)
   propString: string | null;  // keep null value
   
   @Nullable(Number)
   propNumber: number | null; 

   @Nullable(Boolean)
   propBool: boolean | null;
}

Acceptance criteria

• Test cover the different coerced value from null when the option returnsCoercedValue is used and the prop isn't decorated by Nullable
• The null value is kept when ajv.returnsCoercedValue: true and the property is decorated by Nullable
• The null value is kept when ajv.returnsCoercedValue: false (default mapper behavior)
• The documentation on model.md describe this new options

[Romakita]

It's an enhancement and not a bug. PR are welcome :)

[Romakita]

same issue #2504

[Romakita]

The first problem is over AjvService. It try to coerce null to the expected type. I think has the right behavior regarding this discussion.

Let AjvService coerce the expected value should resolve this issue and should make the use of Nullable mandatory if we actually want to have null as a possible value.
Adding an option to the json-mapper is therefore not necessary, but rather on the AJV options themselves.

[github-actions]

🎉 Are you happy?

If you appreciated the support, know that it is free and is carried out on personal time ;)

A support, even a little bit makes a difference for me and continues to bring you answers!

[Romakita]

🎉 This issue has been resolved in version 7.43.0 🎉

The release is available on:

• v7.43.0
• GitHub release

Your semantic-release bot 📦🚀