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

Date coercion and typechecking inconsistent on bulkWrite #14400

Closed
2 tasks done
drew-vanta opened this issue Mar 1, 2024 · 3 comments
Closed
2 tasks done

Date coercion and typechecking inconsistent on bulkWrite #14400

drew-vanta opened this issue Mar 1, 2024 · 3 comments
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. typescript Types or Types-test related issue / Pull Request
Milestone

Comments

@drew-vanta
Copy link

drew-vanta commented Mar 1, 2024

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Mongoose version

8.2.0

Node.js version

18.19.0

MongoDB server version

6.0

Typescript version (if applicable)

5.3.3

Description

Different methods of using bulkWrite to update a single Date property are inconsistent in their typechecking and coercion to Dates in the DB when passing an ISOString.

The closest mention here is #11773, but I also wanted to flag that this issue appears with .bulkWrite as well and how it interacts with typechecking, since there is some degree of validation depending on what level of the list one is working with.

Steps to Reproduce

I ran the following code to compare various cases. Some cases type check, when trying to use updateOne within bulkWrite, others don't. In addition, there is at least one case of using updateOne within bulkWrite that saves the Date as a string as opposed to a Date.

import { connect, model, Schema, set } from "mongoose";
import assert from "assert";

// This script passes whether this is set or not.
set("runValidators", true);

const testSchema = new Schema({
  dateProperty: { type: Date },
});

const TestModel = model<{
  dateProperty: Date;
}>("Test", testSchema);

const assertBizarreBehaviors = async (cases: Array<() => Promise<void>>) => {
  for (const testCase of cases) {
    try {
      if ((await TestModel.find({}).exec()).length > 0) {
        await TestModel.collection.drop();
      }

      await testCase();
      await TestModel.collection.drop();
      console.log("test passed");
    } catch (e) {
      console.error("Test failed", e);
    }
  }
};

if (require.main === module) {
  void (async () => {
    await connect("mongodb://localhost:27017/test");
    console.log("connected to mongoose");
    await assertBizarreBehaviors([
      // create/non-lean: type checks, coerces to date
      async () => {
        const date1 = new Date("2020-01-01");
        await TestModel.create({
          dateProperty: date1.toISOString(),
        });

        const doc = await TestModel.findOne();
        assert.ok(doc != null);
        assert.deepStrictEqual(doc.dateProperty, date1);
      },
      // create/lean: type checks, coerces to date
      async () => {
        const date2 = new Date("2020-01-02");
        await TestModel.create({
          dateProperty: date2.toISOString(),
        });

        const doc = await TestModel.findOne().lean({}).exec();
        assert.ok(doc != null);
        assert.deepStrictEqual(doc.dateProperty, date2);
      },
      // Bulk-write/updateOne/without $set/non-lean: doesn't type check, coerces to date
      async () => {
        const date3 = new Date("2020-01-03");
        const createdDoc = await TestModel.create({});
        await TestModel.bulkWrite([
          {
            updateOne: {
              filter: { _id: createdDoc._id },
              update: { dateProperty: date3.toISOString() },
            },
          },
        ]);
        const updatedDoc = await TestModel.findOne();
        assert.ok(updatedDoc != null);
        assert.deepStrictEqual(updatedDoc.dateProperty, date3);
      },
      // Bulk-write/updateOne/without-$set/lean: doesn't type check, coerces to date
      async () => {
        const date4 = new Date("2020-01-04");
        const createdDoc = await TestModel.create({});
        await TestModel.bulkWrite([
          {
            updateOne: {
              filter: { _id: createdDoc._id },
              update: { dateProperty: date4.toISOString() },
            },
          },
        ]);
        const updatedDoc = await TestModel.findOne().lean({}).exec();
        assert.ok(updatedDoc != null);
        assert.deepStrictEqual(updatedDoc.dateProperty, date4);
      },
      // Bulk-write/updateOne/$set-not-in-list/non-lean: typechecks and coerces
      async () => {
        const date5 = new Date("2020-01-05");
        const createdDoc = await TestModel.create({});
        await TestModel.bulkWrite([
          {
            updateOne: {
              filter: { _id: createdDoc._id },
              // @ts-expect-error this gives an error.
              update: { $set: { dateProperty: date5.toISOString() } },
            },
          },
        ]);
        const updatedDoc = await TestModel.findOne();
        assert.ok(updatedDoc != null);
        assert.deepStrictEqual(updatedDoc.dateProperty, date5);
      },
      // Bulk-write/updateOne/$set-not-in-list/lean: typechecks and coerces
      async () => {
        const date6 = new Date("2020-01-06");
        const createdDoc = await TestModel.create({});
        await TestModel.bulkWrite([
          {
            updateOne: {
              filter: { _id: createdDoc._id },
              // @ts-expect-error this gives an error.
              update: { $set: { dateProperty: date6.toISOString() } },
            },
          },
        ]);
        const updatedDoc = await TestModel.findOne().lean({}).exec();
        assert.ok(updatedDoc != null);
        assert.deepStrictEqual(updatedDoc.dateProperty, date6);
      },
      // Bulk-write/updateOne/$set-in-list/non-lean: doesn't typecheck, coerces
      async () => {
        const date7 = new Date("2020-01-07");
        const createdDoc = await TestModel.create({});
        await TestModel.bulkWrite([
          {
            updateOne: {
              filter: { _id: createdDoc._id },
              update: [
                {
                  $set: {
                    dateProperty: date7.toISOString(),
                  },
                },
              ],
            },
          },
        ]);
        const updatedDoc = await TestModel.findOne().exec();
        assert.ok(updatedDoc != null);
        assert.deepStrictEqual(updatedDoc.dateProperty, date7);
      },
      // Bulk-write/updateOne/$set-in-list/lean: doesn't typecheck nor coerce
      async () => {
        const date8 = new Date("2020-01-08");
        const createdDoc = await TestModel.create({});
        await TestModel.bulkWrite([
          {
            updateOne: {
              filter: { _id: createdDoc._id },
              update: [
                {
                  $set: {
                    dateProperty: date8.toISOString(),
                  },
                },
              ],
            },
          },
        ]);
        const updatedDoc = await TestModel.findOne().lean({}).exec();
        assert.ok(updatedDoc != null);
        assert.deepStrictEqual(updatedDoc.dateProperty, date8.toISOString());
      },
    ]);
    console.log("done");
    process.exit(0);
  })();
}

Expected Behavior

All of these cases would be consistent in their typechecking and Date casting.

I had a bit of trouble figuring out when the typechecking was being applied based on the docs alone, and the interaction between aggregation pipelines, $set, bulkWrite, casting, and lean() made juggling the expected behavior based on the docs a bit tricky.

These docs reference that dates are casted in some cases. It might be helpful to explain when casting happens on writes and updates.

It is great that these docs explicitly say that casting isn't performed in aggregation stages. It may not be obvious to some users that using a list for updateOne within bulkWrite means that an aggregation pipeline is being used and thus validators are being turned off. Don't know if there's a great solution here unfortunately.

Using lean does mention that there is a lack of casting, which is nice.

I was also surprised that none of these cases throw an error:
https://mongoosejs.com/docs/validation.html mentions that validators are run on $set. I guess it is because of the casting.

@drew-vanta
Copy link
Author

In addition, wanted to say a big thank you to the maintainers here!

@vkarpov15 vkarpov15 added this to the 8.2.1 milestone Mar 4, 2024
@vkarpov15 vkarpov15 added the has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue label Mar 4, 2024
@vkarpov15 vkarpov15 modified the milestones: 8.2.1, 8.2.2 Mar 4, 2024
@vkarpov15
Copy link
Collaborator

So the lack of coercion when using an update pipeline is expected behavior, although something we need to document.

The fact that update: { $set: { dateProperty: date6.toISOString() } } doesn't compile is an issue, we're working on fixing that.

@vkarpov15 vkarpov15 added the typescript Types or Types-test related issue / Pull Request label Mar 7, 2024
@IslandRhythms IslandRhythms added confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. and removed has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue labels Mar 7, 2024
vkarpov15 added a commit that referenced this issue Mar 11, 2024
types(model): make `bulkWrite()` types more flexible to account for casting
@drew-vanta
Copy link
Author

Wonderful! Thank you @vkarpov15 and mongoose team!

I'm a huge fan of making the update pipelining more explicit -- I think it can be easy to put an array in an update operation and not realize that casting and validation is turned off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. typescript Types or Types-test related issue / Pull Request
Projects
None yet
Development

No branches or pull requests

3 participants