bwgjoseph / mongoose-vs-ottoman

feature comparison between mongoose and ottoman
0 stars 1 forks source link

support for immutable #45

Closed bwgjoseph closed 3 years ago

bwgjoseph commented 3 years ago

Hi,

Possible to support immutable feature in near future?

Thanks

httpJunkie commented 3 years ago

Supported for P2

AV25242 commented 3 years ago

Created issue https://github.com/couchbaselabs/node-ottoman/issues/373

httpJunkie commented 3 years ago

This was released with alpha.17

bwgjoseph commented 3 years ago

Thank you! Will test this feature soon

bwgjoseph commented 3 years ago

Hi,

I have this test case, and it throws me error

const Immutable2Model = ottoman.model('immutable2', schema, { idKey: '_id' });
        const schemaData = new Immutable2Model({
            name: 'hello',
            operational: true,
        });

        const created = await Immutable2Model.create(schemaData);
        const changeDate = new Date();
        await Immutable2Model.updateById(created._id, {
            createdAt: changeDate,
            createdBy: 'Edwin',
            updatedAt: changeDate,
            updatedBy: 'Edwin',
        });
1) test schema immutable options
    ottoman - ensure immutable fields does not get change using updateById2:
    ImmutableError: Field 'createdAt' is immutable and current cast strategy is set to 'throw'
    at _Model.set [as createdAt] (node_modules\ottoman\src\model\document.ts:368:21)
    at _Model._applyData (node_modules\ottoman\src\model\document.ts:358:16)
    at Z:\mongoose-vs-ottoman\node_modules\ottoman\src\model\create-model.ts:201:15
    at Generator.next (<anonymous>)
    at fulfilled (node_modules\ottoman\lib\model\create-model.js:5:58)

And this is my schema

const defaultDate = new Date();

const baseSchema = new Schema({
    createdAt: {
        type: Date,
        default: () => defaultDate,
        immutable: true,
    },
    updatedAt: {
        type: Date,
        default: () => defaultDate,
    },
    createdBy: {
        type: String,
        default: () => 'Joseph',
        immutable: true,
    },
    updatedBy: {
        type: String,
        default: () => 'Joseph'
    }
});

const schema = new Schema({
    name: {
        type: String,
        required: true,
    },
    operational: {
        type: Boolean,
        default: true
    },
}).add(baseSchema);
gsi-chao commented 3 years ago

If you declare createdBy immutable you can't update this field using any mutation like updateById.

bwgjoseph commented 3 years ago

But in actual usage, the data is passed in by the client and it doesn't make sense to verify whether each field is declared as immutable before calling the API?

In mongoose, if I do this,

const updated = await SModel.findByIdAndUpdate(created._id, {
            createdAt: changeDate,
            createdBy: 'Edwin',
            updatedAt: changeDate,
            updatedBy: 'Edwin',
        }, { new: true }).exec()

It would just ignore the immutable fields, and proceed with the updates.

// created
{
  operational: true,
  _id: 603d1910915dd6378cb535c4,
  name: 'hello',
  createdAt: 2021-03-01T16:40:44.644Z,
  updatedAt: 2021-03-01T16:40:44.644Z,
  createdBy: 'Joseph',
  updatedBy: 'Joseph',
  __v: 0
}

// updated, immutable field does not get changed
{
  operational: true,
  _id: 603d1910915dd6378cb535c4,
  name: 'hello',
  createdAt: 2021-03-01T16:40:44.644Z,
  updatedAt: 2021-03-01T16:40:48.858Z,
  createdBy: 'Joseph',
  updatedBy: 'Edwin',
  __v: 0
}

But if I set the strict mode to throw, it will then throw me the exception

// note the strict option
const updated = await SModel.findByIdAndUpdate(created._id, {
            createdAt: changeDate,
            createdBy: 'Edwin',
            updatedAt: changeDate,
            updatedBy: 'Edwin',
        }, { new: true, strict: 'throw' }).exec()

image

How can I achieve the same with ottoman? Thank you

gsi-chao commented 3 years ago

At the moment to achieve this with ottoman you must declare the schema with the strict property to false. const CardSchema = new Schema(CardSchemaBase, { strict: false }); By default strict is true. If strict is true ottoman will raise an exception.

It would be a good idea to expose, in functions of type mutation, the property strategy and strict Example: const card = await Card.findOneAndUpdate({ cardNumber: { $like: '%5%' } }, cardInfoUpdate, { new: false, strategy: CAST_STRATEGY.THROW, strict: true }); @AV25242 could we create this enhancement task?

AV25242 commented 3 years ago

Ok will do.

AV25242 commented 3 years ago

I have created ticket https://github.com/couchbaselabs/node-ottoman/issues/410

bwgjoseph commented 3 years ago

Thanks.

To further clarify, and if I'm not wrong. In mongoose, the default is true for strict. But this will still allow me to pass in fields that are immutable and able to update as usual.

If I set it to false per query, it will allow me to bypass immutable and allow me to update.

The error will only throw if I explicitly set the mode to throw.

So, the way it work in mongoose is slightly different from what you created in the ticket. With strict as true by default, it should not block me from updating the document unless I explicitly set it to throw?

Source https://mongoosejs.com/docs/api/schematype.html#schematype_SchemaType-immutable

bwgjoseph commented 3 years ago

Tested with alpha.19, save is working as expected, but updateById is not.

I have the same schema as described in https://github.com/bwgjoseph/mongoose-vs-ottoman/issues/45#issuecomment-786803060

Using save [behavior expected]

const ImmutableModel = ottoman.model('immutable', schema, { idKey: '_id' });
const schemaData = new ImmutableModel(opt);
const created = await ImmutableModel.create(schemaData);

const find = await ImmutableModel.find({});
const changeDate = new Date();
created.createdAt = changeDate;
created.createdBy = 'Edwin';
created.updatedAt = changeDate;
created.updatedBy = 'Edwin';

// output is as expected, createdAt and createdBy did not get changed
const updated = await created.save();

Using updateById [behavior not expected]

const ImmutableModel = ottoman.model('immutable', schema, { idKey: '_id' });
const schemaData = new ImmutableModel(opt);
const created = await ImmutableModel.create(schemaData);

const changeDate = new Date();

// throws [ImmutableError: Field 'createdAt' is immutable and current cast strategy is set to 'throw'] error
const updated = await ImmutableModel.updateById(created._id, {
            createdAt: changeDate,
            createdBy: 'Edwin',
            updatedAt: changeDate,
            updatedBy: 'Edwin',
        })

The behavior of updateById should be the same as save unless strict is set to throw. The default should be true, passing in false will ignore immutable (or any other validation in general), and only throw error is set to throw.

I have yet to went through all other methods like findOneAndUpdate, replaceById, updateMany, etc but all of them should have the same behavior.

ariamF11 commented 3 years ago

All mutation functions take by default strategy strict: true except by updateById, in this issue: https://github.com/bwgjoseph/mongoose-vs-ottoman/issues/15 is the description why updateById set default strategy to THROW, that is the one that is applied by default

to avoid this you only have to set updateById option strict=true, something like this:

await Immutable2Model.updateById(created._id, {
    createdAt: changeDate,
    createdBy: 'Edwin',
    updatedAt: changeDate,
    updatedBy: 'Edwin',
}, {strict:true});
bwgjoseph commented 3 years ago

Hi @gsi-ariam, which exact part of #15 indicates why it is set to throw by default. Do let me know, I might be missing something.

ariamF11 commented 3 years ago

https://github.com/bwgjoseph/mongoose-vs-ottoman/issues/15#issuecomment-766933076 https://github.com/couchbaselabs/node-ottoman/pull/371

bwgjoseph commented 3 years ago

I'm still not quite following why is it set to throw by default with that fix

The issue I found in https://github.com/bwgjoseph/mongoose-vs-ottoman/issues/15#issuecomment-762395352 is that when I pass in the wrong data set, it should throw the error with strict: true, isn't it? Because it doesn't conform to the schema definition.

Taking a quote of what was explained in mongoose-docs as an example

// Mongoose will strip out the `name` update, because `name` is immutable
Model.updateOne({}, { $set: { name: 'test2' }, $inc: { age: 1 } });

// If `strict` is set to 'throw', Mongoose will throw an error if you
// update `name`
const err = await Model.updateOne({}, { name: 'test2' }, { strict: 'throw' }).
  then(() => null, err => err);
err.name; // StrictModeError

// If `strict` is `false`, Mongoose allows updating `name` even though
// the property is immutable.
Model.updateOne({}, { name: 'test2' }, { strict: false });

Hence, in the previous example which is this

https://github.com/bwgjoseph/mongoose-vs-ottoman/blob/851bcb736c94063cadcf9412754a731e07efe926/test/ts/updateMany.test.ts#L108-L125

The default mode (of updateById) should be strict: true, and this test would not pass because I am updating to a wrong type (does not conform to the schema), and not because it is set to strict: throw

AV25242 commented 3 years ago

why is updateById default strict is to throw All mutation operation default strict : true

AV25242 commented 3 years ago

A fix for this is in place and should be available with next release

AV25242 commented 3 years ago

Checkout the new alpha.21 release and let us know.