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

BulkSave existing documents doesn't apply save middleware hook updates #13799

Closed
2 tasks done
tatejones opened this issue Aug 31, 2023 · 3 comments · Fixed by #13885
Closed
2 tasks done

BulkSave existing documents doesn't apply save middleware hook updates #13799

tatejones opened this issue Aug 31, 2023 · 3 comments · Fixed by #13885
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Milestone

Comments

@tatejones
Copy link

tatejones commented Aug 31, 2023

Prerequisites

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

Mongoose version

7.4.5

Node.js version

18.16.0

MongoDB server version

5.7

Typescript version (if applicable)

5.1.6

Description

When the bulkSave creates documents the modifications in the "save" middleware hook at persisted.
Subsequent updates made to the documents in "save" middleware hook are not persisted.

Confirmed that the hook is being executed during the update. See console.log below

Steps to Reproduce

describe("bulkSave middleware bug", async () => {
    it.only("it should be able to detect middleware save hook", async () => {
        const schema = new Schema({ name: String, _age: { type: Number, min: 0, default: 0 } })
        schema.pre("save", function () {
            this._age = this._age + 1
            console.log(`Updating person _age to ${this._age}`)
        })
        const connection = connectDatabase(mongodb.uri, "bulkSave")

        const Person = mongoose.model("Person", schema, "Persons", { connection })
        const person = new Person({ name: "Jean-Luc Picard", _age: 59 })

        await Person.bulkSave([person])

        let updatedPerson = await Person.findById(person._id)

        // this succeeds 
        expect(updatedPerson?._age).to.equal(60).

        await Person.bulkSave([updatedPerson!])

        updatedPerson = await Person.findById(person._id)
        
        // this fails - actual value is still 60
        expect(updatedPerson?._age).to.equal(61)
    })
})

Output

Updating person _age to 60
Updating person _age to 61
1) it should be able to detect middleware save hook

0 passing (113ms)
1 failing

  1. bulkSave middleware bug
    it should be able to detect middleware save hook:

    AssertionError: expected 60 to equal 61

    • expected - actual

    -60
    +61

    at Context. (file:///Users/tate/code/Projects/HealthService/NodeServices/TypeScriptLibrary/src/tests/test.utils.ts:110:40)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

Expected Behavior

Expect updates made in the save hook middleware to persist during a bulkSave.

@tatejones
Copy link
Author

I have also attempted without prefix '_'

@tatejones
Copy link
Author

In the bulkSave it looks like the this.buildBulkWriteOperations needs to be called after the pre-save hooks are called otherwise the embedded document in the writeOperations (a delta for update) won't be updated. Hence why the create succeeds as it is a full document by reference and not mutated as a delta.

await Promise.all(documents.map(buildPreSavePromise));
const writeOperations = this.buildBulkWriteOperations(documents, { skipValidation: true, timestamps: options.timestamps

@vkarpov15 vkarpov15 added this to the 7.5.2 milestone Sep 4, 2023
@vkarpov15 vkarpov15 added the enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature label Sep 4, 2023
@hyunjinjeong
Copy link

hyunjinjeong commented Sep 5, 2023

I'm experiencing the same issue. pre-save hooks are executed but the changes from the hooks aren't applied to the final operation when using bulkSave().

@vkarpov15 vkarpov15 modified the milestones: 7.5.2, 7.5.3 Sep 15, 2023
vkarpov15 added a commit that referenced this issue Sep 25, 2023
fix(model): make `bulkSave()` persist changes that happen in pre('save') middleware
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Projects
None yet
3 participants