ajv

ajv copied to clipboard

What issue does this pull request resolve? Currently when using nullable and coerceTypes alongside each other coerceTypes is run before nullable is processed, removing the ability to pass null in the data and retaining the actual value instead of defaulting to the predefined coercion rules.

APIs often rely on the ability to allow callers to pass null values to handle specific use cases other than just being undefined. While coerceTypes is very helpful to parse to the correct data types along the way, using it for use cases like these becomes quite impossible.

As described here #1078 it has been recommended before to use nullable to prevent type coercion which is weird since it doesn't really work together.

What changes did you make? Added an additional coerceNull option to be used alongside coerceTypes. It defaults to true whenever coerceTypes is enabled so existing behaviour is unaltered. A test was added under spec/coercion.spec.ts to ensure the behaviour works as intended.

Is there anything that requires more attention while reviewing? As this is my first PR to the repository I'm quite unfamiliar where things belong. So sorry in advance if this could have been implemented in a more logical way somewhere else.

@epoberezkin can you approve the workflow again? It seems linting got borked during the refactor.

@segersniels I am now thinking and I am not sure I understand the scenario when this option is needed.

Without any changes, if you have nullable in the schema (or type: [anytype, “null”]) null would not be coerced without any changes.

I’ve checked the tests you added and I also do not see the scenario when it doesn’t work like this without an additional option.

Can you please clarify?

Sorry for not asking this question sooner… We should have started it from the issue though, not from PR. Catching up now :)

If you check the test that I added without the coerceNull enabled you will see that the data will be coerced instead of retaining its null value with nullable enabled. @epoberezkin

I see the tests that do coerce null in the absence of nullable: true in the schema - which is ok, but if nullable: true is added to the schema null will not be coerced.

I am not sure there should be two ways to control null coercion - one via the schema, and another via the option.

I see the tests that do coerce null in the absence of nullable: true in the schema - which is ok, but if nullable: true is added to the schema null will not be coerced.

Exactly. It would be nice if we had the option to preserve null values even when nullable is not defined (or false). This would cause the data to be invalid and informing that you passed something invalid. While if you don't do this you just automatically enforce eg. an empty string which then also needs to be handled further by the API depending on it.

It would make more sense IMHO if we had the option to fully disable touching null for people that want their data to be invalid in this use case.

Eg. you are trying to add an order to a webshop and the API is expecting amount to never be null, so you set it to nullable: false. However the client implementing the API fucks up and accidentally sends null somehow and the API then proceeds to coerce the null to 0. The data will not be invalid and the API will proceed unless you explicitly start adding cases to catch stuff like this. Ignore the fact that a proper API should probably already be handling edge cases like this, it doesn't take away the fact that this adds extra overhead while setting coerceNull: false would already catch it way earlier in the flow.

@epoberezkin Any progress on this? Would love to get this merged upstream if you're okay with the reasoning behind the PR.

@epoberezkin Any progress on this? Would love to get this merged upstream if you're okay with the reasoning behind the PR.

Could you please rename the branch? I can't manage it via UI as it is also called master...

The same argument can be applied to not using coercion at all - if you don't want to coerce null, and you don't want to allow null, why do you want any coercion at all?

If the idea is to allow strings instead of numbers it would just seem bad API design - this is something better handled in the client I think...

The same argument can be applied to not using coercion at all - if you don't want to coerce null, and you don't want to allow null, why do you want any coercion at all?

If the idea is to allow strings instead of numbers it would just seem bad API design - this is something better handled in the client I think...

I don't think you're understanding the use case. When you explicitly tell your API to not accept null you'd expect a validation error to be thrown. Instead null gets coerced and the validation succeeds. Which is odd IMHO and can imagine to a lot of other people as well. This option would just allow you to explicitly control the null coercion behavior without having to completely disable coercion.

I don't think you're understanding the use case.

I know - I am trying to understand :)

This option would just allow you to explicitly control the null coercion behavior without having to completely disable coercion.

that’s exactly what I don’t understand - what’s the application use case where you need it? I mean - if you want your client to be precise - don’t use coercion at all. If you want client to be sloppy - ok, do use coercion, but why would not allow null coerced in this case? Can you provide a specific api/application example where it is helpful?

Just to be clear, I want to establish that it is genuinely useful application development pattern that would be supported by this feature, and that there is no better way to achieve the same objective in the application - I absolutely do not want to add all possible features to support all possible application development patters, the feature surface is pretty huge already… Does it make sense?

Ajv already runs the tests against 128 instances (I think), quite slowly, and it’s nowhere near enough to cover all possible combinations of options - the number of these combinations is about 900m, not even accounting for the options that allow to pass schemas/keywords/formats, affect standalone code generation, and not accounting for the difference between false and undefined… There are lots of possible subtle edge cases when some options are combined, it’s only surprising why so few were found…

You are proposing the option that doubles this number - so I just want to establish that what you want to achieve on the application level really requires this option and cannot be achieved more effectively without any ajv changes.

Can you provide a specific api/application example where it is helpful?

Returning back to the simple webshop use case where a client (eg. browser) wants to POST an order to an API. An overly explicit API request body validation in this case could look a little something like this:

const schema = {
    type: 'object',
    properties: {
        userId: {
            type: 'number',
            nullable: false,
        },
        itemId: {
            type: 'number',
            nullable: false,
        },
        amount: {
            type: 'number',
            nullable: false,
        },
    },
    allRequired: true,
};

After going through the validation and coercion the API then continues to implement something super simple like the following (let's just ignore any API patterns for the simplicity of this example):

const placeOrder = async (userId: number, itemId: number, amount: number) => {
    // Check if provided userId is a valid user
    const user = await UserModel.findOne({
        where: {
            id: userId,
        },
    });

    if (!user) {
        throw new Error(`No user was found for userId: ${userId}`);
    }

    // Do the same for all passed parameters...
};

A super basic example where the API wants to ensure that the parameters are all valid and make sense. Here's where things start to go wrong. A frontend developer fucks up the implementation and for some reason something fucks up (eg. fetching something from the Redux store or w/e) and ends up sending null as one of the properties in the POST request.

You would expect to see this error handled as soon as possible during the AJV request validation. But since we have coerceTypes enabled, even if we have nullable explicitly set to false, the passed null gets coerced to 0 and will get passed to w/e code the API is executing. Let's say I'm a user of AJV that doesn't know the logic of how nullable and coerceTypes work and don't know that in this case it gets coerced to 0. The API will then proceed to throw the following error: Error: No user was found for userId: 0.

Do you see where things become confusing? You would expect the API (indirectly AJV) to throw the error as soon as possible, during request validation, notifying the client that the API doesn't accept null as a value and shouldn't bother trying to execute any code after it.

Sorry for the long and badly explained example. It's basically just a matter of having AJV to be more strict and providing full control over the request validation. There's probably a few other examples that could explain the use case more but can't think of any at the moment.

Could you please rename the branch? I can't manage it via UI as it is also called master...

I'm afraid it's not possible to rename the branch once the PR is created. If you want I can create a new PR starting from a different branch. Why does it need to be renamed though?

Why does it need to be renamed though?

GitHub makes it difficult to manage PRs from master to master, I can only do it via command line...

I would like to add my belated support for the PR

@epoberezkin Any chance you would consider this (or a similar) PR at the current date? Happy to create a fresh one.

I'd like to add my statement of support. I want the ability to coerce types in my schema validation (particularly strings to numbers) but coercing nulls into numbers is very bad for my use case and it's common for a null value to arrive on attributes where I'm expecting a number value. There's no apparent way to supply an approved list of coercion rules, so having the ability to turn off coercion from null into other values would be a huge help.