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

modifiedpaths are not returning as expected when we use .set() with nested paths as an object in v5.12.5 vs v6.x.x #14024

Closed
2 tasks done
sakethyerra opened this issue Nov 1, 2023 · 21 comments
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it.
Milestone

Comments

@sakethyerra
Copy link

sakethyerra commented Nov 1, 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

6.12.1

Node.js version

14.18.1

MongoDB server version

4.4

Typescript version (if applicable)

No response

Description

Hi, the following code behaves differently in both versions v5.12.5 and v6.x.x.
Note: modified paths are not returning as expected when we use .set() with nested paths as an object.

var mongoose = require('mongoose')
let Schema = mongoose.Schema
var eventSchema = new Schema({
  name: { type: String },
  __stateBeforeSuspension: {
    field1: { type: String },
    field2: { type: String },
    jsonField: {
        name: { type: String }
    }
  }
})
var Event = mongoose.model('Event', eventSchema)
async function run() {
  await mongoose.connect('mongodb://localhost:27017');
  let eventObj = new Event({ name: 'event object', __stateBeforeSuspension: { field1: 'test', jsonField: { name: 'test3' }} })
  eventObj.save(function (err, eventObj) {
    var newObject = eventObj.toObject()
    newObject.__stateBeforeSuspension = { field1: '12', jsonField: { name: 'test4' }}
    eventObj.set(newObject)
    var modifiedPaths = eventObj.modifiedPaths()
    console.log(modifiedPaths)
   
  })
}
run();

Output with v5.12.5 :

[
  '__stateBeforeSuspension',
  '__stateBeforeSuspension.field1',
  '__stateBeforeSuspension.jsonField',
  '__stateBeforeSuspension.jsonField.name'
]

Output with v6.12.1 :

[ '__stateBeforeSuspension', '__stateBeforeSuspension.jsonField' ]

The modified paths are returning differently in both versions.
Note: We have raised a different issue similar to .set() one. for more info please refer to #14022

Steps to Reproduce

Added the full-fledged details on how to reproduce the issue in the description with a code snippet.

Expected Behavior

No response

@sakethyerra
Copy link
Author

sakethyerra commented Nov 1, 2023

We observed another thing non-modified paths are showing as modified paths for more info please refer to the below code.

var mongoose = require('mongoose')
let Schema = mongoose.Schema
var eventSchema = new Schema({
  name: { type: String },
  __stateBeforeSuspension: {
    field1: { type: String },
    field2: { type: String },
    jsonField: {
        name: { type: String },
        name1: { type: String }
    }
  }
})
var Event = mongoose.model('Event', eventSchema)
async function run() {
  await mongoose.connect('mongodb://localhost:27017');
  let eventObj = new Event({ name: "event object", __stateBeforeSuspension: { field1: "test", jsonField: { name: "test3" }} })
  eventObj.save(function (err, eventObj) {
    var newObject =  { field1: "test", jsonField: { name: "test3", name1: "test4"} }
    eventObj.set('__stateBeforeSuspension', newObject)
    var modifiedPaths = eventObj.modifiedPaths()
    console.log(modifiedPaths)
  })
}
run();

Output with v5.12.5 :

[ '__stateBeforeSuspension' ]

Output with v6.12.1 :

[
  '__stateBeforeSuspension',
  '__stateBeforeSuspension.jsonField',
  '__stateBeforeSuspension.jsonField.name',
  '__stateBeforeSuspension.jsonField.name1'
]

@vkarpov15 vkarpov15 added this to the 6.12.3 milestone Nov 1, 2023
@vkarpov15 vkarpov15 added the has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue label Nov 1, 2023
@sakethyerra
Copy link
Author

@vkarpov15 Can you please update the status of this tracker

@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 Nov 3, 2023
@IslandRhythms
Copy link
Collaborator

confirmed for both

const mongoose = require('mongoose');
const { Schema } = mongoose;

// for function run()
/*
const eventSchema = new Schema({
  name: { type: String },
  __stateBeforeSuspension: {
    field1: { type: String },
    field2: { type: String },
    jsonField: {
        name: { type: String }
    }
  }
});*/


// for function test
const eventSchema = new Schema({
  name: { type: String },
  __stateBeforeSuspension: {
    field1: { type: String },
    field2: { type: String },
    jsonField: {
        name: { type: String },
        name1: { type: String }
    }
  }
})
const  Event = mongoose.model('Event', eventSchema)
async function run() {
  await mongoose.connect('mongodb://localhost:27017');
  await mongoose.connection.dropDatabase();
  const  eventObj = new Event({ name: 'event object', __stateBeforeSuspension: { field1: 'test', jsonField: { name: 'test3' }} })
  eventObj.save(function (err, eventObj) {
    var newObject = eventObj.toObject()
    newObject.__stateBeforeSuspension = { field1: '12', jsonField: { name: 'test4' }}
    eventObj.set(newObject)
    var modifiedPaths = eventObj.modifiedPaths()
    console.log(modifiedPaths)
   
  })
}
// run();

async function test() {
  await mongoose.connect('mongodb://localhost:27017');
  await mongoose.connection.dropDatabase();

  const eventObj = new Event({ name: "event object", __stateBeforeSuspension: { field1: "test", jsonField: { name: "test3" }} })
  eventObj.save(function (err, eventObj) {
    var newObject =  { field1: "test", jsonField: { name: "test3", name1: "test4"} }
    eventObj.set('__stateBeforeSuspension', newObject)
    var modifiedPaths = eventObj.modifiedPaths()
    console.log(modifiedPaths)
  })
}

test();

@vkarpov15
Copy link
Collaborator

For your first post (the following code snippet), this is expected behavior. In Mongoose 6, modifiedPaths() doesn't include children of already modified paths by default. Use modifiedPaths({ includeChildren: true }) to get the Mongoose 5 behavior in Mongoose 6.

var mongoose = require('mongoose')
let Schema = mongoose.Schema
var eventSchema = new Schema({
  name: { type: String },
  __stateBeforeSuspension: {
    field1: { type: String },
    field2: { type: String },
    jsonField: {
        name: { type: String }
    }
  }
})
var Event = mongoose.model('Event', eventSchema)
async function run() {
  await mongoose.connect('mongodb://localhost:27017');
  let eventObj = new Event({ name: 'event object', __stateBeforeSuspension: { field1: 'test', jsonField: { name: 'test3' }} })
  eventObj.save(function (err, eventObj) {
    var newObject = eventObj.toObject()
    newObject.__stateBeforeSuspension = { field1: '12', jsonField: { name: 'test4' }}
    eventObj.set(newObject)
    var modifiedPaths = eventObj.modifiedPaths()
    console.log(modifiedPaths)
   
  })
}
run();

Re: your 2nd post, the fact that name and name1 were showing up in modifiedPaths() is a bug. I fixed in #14053 with 4bd5927.

@sakethyerra
Copy link
Author

sakethyerra commented Nov 7, 2023

@vkarpov15 For the first use case, it's working as expected but we found that we are using isDirectModified() and it's not returning the child-modified paths. please find the below code snippet for more information.
Note: In our case, we are mostly relying on isDirectModified() to check in the database docs which will impact us a lot.

var mongoose = require('mongoose')
let Schema = mongoose.Schema
var eventSchema = new Schema({
  name: { type: String },
  __stateBeforeSuspension: {
    field1: { type: String },
    field2: { type: String },
    jsonField: {
        name: { type: String }
    }
  }
})
var Event = mongoose.model('Event', eventSchema)
async function run() {
  await mongoose.connect('mongodb://localhost:27017');
  let eventObj = new Event({ name: 'event object', __stateBeforeSuspension: { field1: 'test', jsonField: { name: 'test3' }} })
  eventObj.save(function (err, eventObj) {
    var newObject = eventObj.toObject()
    newObject.__stateBeforeSuspension = { field1: '12', jsonField: { name: 'test4' }}
    eventObj.set(newObject)
    var modifiedPaths = eventObj.modifiedPaths({ includeChildren: true })
    console.log(modifiedPaths)
    modifiedPaths.forEach(function (modifiedPath) {
      console.log('modifiedPath: ', modifiedPath)
      if (eventObj.isDirectModified(modifiedPath)) {
        console.log('direct modified path: ', modifiedPath)
      }})
  })
}
run();

Output with v5.12.5 :

[
  '__stateBeforeSuspension',
  '__stateBeforeSuspension.field1',
  '__stateBeforeSuspension.jsonField',
  '__stateBeforeSuspension.jsonField.name'
]
modifiedPath:  __stateBeforeSuspension
modifiedPath:  __stateBeforeSuspension.field1
direct modified path:  __stateBeforeSuspension.field1
modifiedPath:  __stateBeforeSuspension.jsonField
modifiedPath:  __stateBeforeSuspension.jsonField.name
direct modified path:  __stateBeforeSuspension.jsonField.name

Output with v6.12.1 :

[
  '__stateBeforeSuspension',
  '__stateBeforeSuspension.jsonField',
  '__stateBeforeSuspension.jsonField.name',
  '__stateBeforeSuspension.field1'
]
modifiedPath:  __stateBeforeSuspension
direct modified path:  __stateBeforeSuspension
modifiedPath:  __stateBeforeSuspension.jsonField
direct modified path:  __stateBeforeSuspension.jsonField
modifiedPath:  __stateBeforeSuspension.jsonField.name
modifiedPath:  __stateBeforeSuspension.field1

If we see the above outputs we are not getting the as-expected values for the "direct modified path" log
Note: Let me know if we need to raise a new issue for it. Can you please add it to the 6.12.3

@vkarpov15
Copy link
Collaborator

I think the isDirectModified() bit is expected behavior. The spirit of isDirectModified() is that it should return true for fields that you have explicitly set (not 100% true in 6.x, we've made some improvements on that front in subsequent versions). So given that you're calling eventObj.set(newObject), the only direct modified path should be __stateBeforeSuspension, because eventObj.set(newObject) becomes eventObj.set('name', 'event object'); eventObj.set('__stateBeforeSuspension', ...) and __stateBeforeSuspension is the only path that is set to a different value. If you want __stateBeforeSuspension.jsonField.name to show up as the direct modified path, you should do eventObj.set('__stateBeforeSuspension.jsonField.name', 'test 4') or something similar.

@sakethyerra
Copy link
Author

sakethyerra commented Nov 8, 2023

@vkarpov15 We are using .set() in many places of our code and it is hard for us to modify it in all the paces. We are using isDirectModified() for auditing(To get the specific modified field) the docs to show those details to the end users. Is there any way to achieve the older behaviour?

@vkarpov15
Copy link
Collaborator

Does using isModified() instead of isDirectModified() work for you?

@sakethyerra
Copy link
Author

@vkarpov15, No it doesn't work as isModified() will come true for all the values coming in modifiedPaths(). To avoid this issue we are using the isDirectModified() to get the exact fields that were modified but in 6.x.x we are not getting the exact child fields that are modified. Is there a way to achieve the same behaviour of the 5.12.5v?

@vkarpov15
Copy link
Collaborator

What is your definition of the exact fields that were modified? Do you have a more exact definition or just the 5.x behavior?

@vkarpov15 vkarpov15 reopened this Nov 11, 2023
@vkarpov15 vkarpov15 added help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary and removed confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. labels Nov 11, 2023
@vkarpov15 vkarpov15 modified the milestones: 6.12.3, 6.12.4 Nov 11, 2023
@sakethyerra
Copy link
Author

As mentioned in this comment #14024 (comment) we are updating the '__stateBeforeSuspension.jsonField.name' and '__stateBeforeSuspension.field1' fields those fields are not coming as the isDirectModified() in 6.X

@vbharath2809
Copy link

Just adding to what @sakethyerra mentioned,

@vkarpov15
5.X behavior will be enough i.e we want isDirectModified to return true for __stateBeforeSuspension.jsonField.name and __stateBeforeSuspension.field1 but not for __stateBeforeSuspension and __stateBeforeSuspension.jsonField

@sakethyerra
Copy link
Author

@vkarpov15, Any update on this

@heyfirst
Copy link

heyfirst commented Nov 29, 2023

Hi, I'm confirming that the isModified is difference for 5.x and 6.x and it's really hard to make a migration. One way is to move to use isDirectModified but it does not solve all the issues.

@vkarpov15
Copy link
Collaborator

@heyfirst can you please clarify what sort of issues you're seeing?

Copy link

This issue is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days

@github-actions github-actions bot added the Stale label Dec 15, 2023
@heyfirst
Copy link

heyfirst commented Dec 16, 2023

Hi @vkarpov15, our problem is that when we push the item to the array of subdocuments, it's marked as modified the whole array. in v5, it will marked modified only the one that has been added.

you can see the code below which's from test/document.modified.test.js and I modified to match with our case,

Screenshot 2566-12-16 at 03 09 42

We have 2 schema, let's say Post and Comment and then you can see in the line 192, I pushed the new comments to array, Turns out the first one (index:0) has been marked as modified and our whole validators and middlewares which rely on isModified integration test are failed.

This issue made us really difficult to move to Mongoose v6. I have checked the code on isModified itself and seems like it has a slightly changes from v5 and v6 as v6 seems checking the parents modified path as well. I might be wrong and hope you could also guide me to implement the proper way to use isModified here.

Sorry for a very late reply 🙇🏻 , I was in vacay and recently come back to work. I also see #14149 that mentioned about using Mongoose v5 in mongodb v5 too and if it is possible, we may stay a little bit longer on v5 and see if our codebase can adapt to align with the functionality of v6 or any other relevant changes.

@github-actions github-actions bot removed the Stale label Dec 17, 2023
@vkarpov15 vkarpov15 modified the milestones: 6.12.4, 6.12.5 Dec 21, 2023
@vkarpov15 vkarpov15 added needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue and removed help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary labels Dec 21, 2023
@heyfirst
Copy link

@vkarpov15 I will create the reproduce script for you in first week of January 🫡 hope it will help you easily reproduce the issue.

@vkarpov15 vkarpov15 added confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. and removed needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue labels Jan 1, 2024
@vkarpov15
Copy link
Collaborator

@heyfirst thanks for the repro, we will look into it and see if we can fix

@vkarpov15
Copy link
Collaborator

@heyfirst are you able to try out the code in #14213 and see if that helps?

@heyfirst
Copy link

heyfirst commented Jan 2, 2024

Sure! let me try them by this evening! Thanks so much for promptly act on it 🙇🏻 by the way, Happy New Year! Wishing you joy and success in the coming year. Cheers to unforgettable moments and exciting adventures ahead!

@vkarpov15 vkarpov15 modified the milestones: 6.12.5, 6.12.6 Jan 3, 2024
vkarpov15 added a commit that referenced this issue Jan 3, 2024
fix: add ignoreAtomics option to isModified() for better backwards compatibility with Mongoose 5
@vkarpov15 vkarpov15 modified the milestones: 6.12.6, 6.12.5 Jan 3, 2024
@vkarpov15 vkarpov15 modified the milestones: 6.12.6, 6.12.5 Jan 12, 2024
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.
Projects
None yet
Development

No branches or pull requests

5 participants