yoitsro / joigoose

Joi validation for your Mongoose models without the hassle of maintaining two schemas
MIT License
178 stars 19 forks source link

Joi.date and Joi.in raise error #53

Open maltere opened 3 years ago

maltere commented 3 years ago

My issue arises with Joi.date in combination with Joi.in and I think it is a bug, but correct me if I am doing something wrong.

const schema = Joi.object().keys({
    dates: Joi.array().items(Joi.date()),
    assignedOn: Joi.date().allow(Joi.in('dates'), null).default(null)
});

It seems to me that the mongoose is not validating the given object correctly

Test Script

This is my full Test Script I wrote to further evaluate this issue:

const Joi = require('joi');
const mongoose = require('mongoose');

const connect = connectionString => {
    console.log(`Connect to MongoDB: ${ connectionString }`);
    const connection = mongoose.createConnection(`mongodb://${ connectionString }`, {
        useNewUrlParser: true,
        useCreateIndex: true,
        useUnifiedTopology: true
    });

    connection.then(() => {
        console.log('Connected correctly to MongoDB.');
    });

    connection.catch(err => {
        console.log('Connection error to MongoDB');
        console.error(error);
    });

    return connection
}

const connection = connect('localhost/tmp');

const Joigoose = require('joigoose')(connection);

const test = async function () {
    console.log('Start Test')

    const schema = Joi.object().keys({
        dates: Joi.array().items(Joi.date()),
        assignedOn: Joi.date().allow(Joi.in('dates'), null).default(null)
    });

    const obj = {
        "dates": [
            "2021-02-09 16:00"
        ],
        "assignedOn": "2021-02-09 16:00"
    };

    // No Error
    const joiResult = schema.validate(obj);
    console.log('joiResult:', joiResult);

    console.log(
        'Test if `assignedOn` is included in `dates`:',
        joiResult.value.dates.includes(joiResult.value.assignedOn)
    );

    const Model = connection.model('Model', Joigoose.convert(schema));

    const instance = new Model(joiResult.value);

    try {
        // Error
        await instance.save()
        console.log('Instance saved.');
        console.log(instance)
    } catch (error) {
        console.error(error);
    }
}

Promise.all([connection]).then(test).finally(() => process.exit(0));

Output

Connect to MongoDB: localhost/tmp
Connected correctly to MongoDB.
Start Test
joiResult: {
  value: {
    dates: [ 2021-02-09T15:00:00.000Z ],
    assignedOn: 2021-02-09T15:00:00.000Z
  }
}
Test if `assignedOn` is included in `dates`: true
Error: Model validation failed: assignedOn: Validator failed for path `assignedOn` with value `Tue Feb 09 2021 16:00:00 GMT+0100 (Central European Standard Time)`
    at ValidationError.inspect (PATH_TO_MY_PROJECT/node_modules/mongoose/lib/error/validation.js:47:26)
    at formatValue (node:internal/util/inspect:735:19)
    at inspect (node:internal/util/inspect:309:10)
    at formatWithOptionsInternal (node:internal/util/inspect:1967:40)
    at formatWithOptions (node:internal/util/inspect:1849:10)
    at console.value (node:internal/console/constructor:330:14)
    at console.warn (node:internal/console/constructor:363:61)
    at test (PATH_TO_MY_PROJECT/test.js:62:17)
    at processTicksAndRejections (node:internal/process/task_queues:93:5) {
  errors: {
    assignedOn: ValidatorError: Validator failed for path `assignedOn` with value `Tue Feb 09 2021 16:00:00 GMT+0100 (Central European Standard Time)`
        at validate (PATH_TO_MY_PROJECT/node_modules/mongoose/lib/schematype.js:1256:13)
        at PATH_TO_MY_PROJECT/node_modules/mongoose/lib/schematype.js:1231:24
        at processTicksAndRejections (node:internal/process/task_queues:93:5) {
      properties: [Object],
      kind: 'user defined',
      path: 'assignedOn',
      value: 2021-02-09T15:00:00.000Z,
      reason: undefined,
      [Symbol(mongoose:validatorError)]: true
    }
  },
  _message: 'Model validation failed'
}

System Information:

Do you need more information from me? I appreciate any help with this.

yoitsro commented 3 years ago

Hey @maltere. I think the issue is that you're not telling Joigoose to convert the values before trying to pass it into Mongo.

So Joigoose interprets assignedOn as a Date: https://github.com/yoitsro/joigoose/blob/master/lib/index.js#L138.

But because the convert flag isn't passed into the options for Joigoose, under the hood Joi accepts the input as valid, but performs no conversion. That string value is then passed to Mongoose, but Mongoose is expecting a Date type.

See here for how to pass additional options to the instance of Joi which is used in Joigoose: https://github.com/yoitsro/joigoose#with-joi-options-which-apply-to-all-validators

I'll leave this open for now in case this doesn't fix it, but please close it if this works.

maltere commented 3 years ago

Thank you @yoitsro for your response!

So I tried multiple things according to your answer within my given Test Script:

1. Adding only the convert flag

This means the line were I require joigoose changes to the following:

const Joigoose = require('joigoose')(connection, { convert: true });

2. Also changing the Input to the Mongoose Model

So Joigoose interprets assignedOn as a Date: https://github.com/yoitsro/joigoose/blob/master/lib/index.js#L138.

My input to the new Model was actually already the converted Object by Joi. So Mongoose can actually already expect a Date object, because they are.

But to be sure this does not produce the error in joigoose I changed the script to create the Model with the original Object.

const instance = new Model(obj);

instead of

const instance = new Model(joiResult.value);

However, both attempts produce the same output and error as given above.

maltere commented 3 years ago

A working example would be, changing the defined schema to the following:

const schema = Joi.object().keys({
    dates: Joi.array().items(Joi.date()),
    assignedOn: Joi.date().allow(null).default(null)
});

Therefore I suggested the problem lays within the use of Joi.in together with Joi.date.

The output to the mentioned modification would be:

Connect to MongoDB: localhost/tmp
Connected correctly to MongoDB.
Start Test
joiResult: {
  value: {
    dates: [ 2021-02-09T15:00:00.000Z ],
    assignedOn: 2021-02-09T15:00:00.000Z
  }
}
Test if `assignedOn` is included in `dates`: false
Instance saved.
{
  dates: [ 2021-02-09T15:00:00.000Z ],
  assignedOn: 2021-02-09T15:00:00.000Z,
  _id: 5ffdd431abd0d0ed8af1fd8a,
  __v: 0
}
yoitsro commented 3 years ago

Great debugging. Seems like we need to add a test suite for Joi.in() (and the associated fix!).

How would you feel about maybe giving this a shot?

maltere commented 3 years ago

I added a test case provoking the error.

After I took a deeper look into the Joigoose.convert and Joigoose.mongooseValidateWrapper functions I wanted to provide some thoughts.

  1. mongooseValidateWrapper does not get the whole schema. Instead it is provided with only the lowest Base Schemas which are then validated. Also the value parameter is therefore not the whole object.
  2. This provides an issue with the Joi.in function which is basically an extended Joi.ref object. For it to work it needs to be able to access the concerning values. A quick example on this would be:
/*
 * We reference "dates" so Joi is expecting a field on the same level as "assignedOn" called "dates".
 */
const schema = Joi.object().keys({
    dates: Joi.array().items(Joi.date()),
    assignedOn: Joi.date().allow(Joi.in('dates'), null).default(null)
});

const obj = {
    "dates": [
        "2021-02-09 16:00"
    ],
    "assignedOn": "2021-02-09 16:00"
};

/*
 * For validation we need to provide the whole object, 
 * because for the validation of "assignedOn" we need to access the values of "dates" 
 */
console.log(schema.validate(obj));

I am willing to help fixing this issue, but currently I do not know enough about mongoose or joigoose to fix this issue. Maybe you can help me out here. Do you have an idea on how to solve this issue?

yoitsro commented 3 years ago

Thanks for taking a look into this. A known caveat of Joigoose is that there is no peer based validation right now.

I think this could be fixed.

There are two parts to Joigoose:

  1. The conversion stage - to convert the Joi schema into a Mongoose schema
  2. The validation stage - taking the input and validating it against the Joi schema

As it stands, the conversion stage works relatively well, but the validation stage leaves a lot to be desired.

I wonder if the two stages could be decoupled and pretty much just use Joi's validation to perform validation on the input. That was the intention when I started out, but things escalated for one reason or another.

I won't have time to look at this in the near future, but I'd happily review a PR. Sorry I can't be of more help at the moment.