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

Adding lean() causes TypeError: document.data is not a function #77

Open
jamesholcomb opened this issue Jan 28, 2021 · 9 comments
Open
Assignees
Labels

Comments

@jamesholcomb
Copy link

Tested with https://github.com/codepunkt/mongoose-patch-history#b9d8abde29d0b1df5137f8c4565a73069aeff3d0

Perhaps this cannot be supported and should be mentioned in the README?

    it('with findOneAndUpdate upsert', (done) => {
      Post.findOneAndUpdate(
        { title: 'findOneAndUpdateUpsert' },
        { $set: { title: 'findOneAndUpdateUpsert' } },
        {
          new: true,
          upsert: true,
          setDefaultsOnInsert: true,
          runValidators: true
        }
      )
        .lean()
        .then((post) =>
          post.patches
            .find({ ref: post._id })
            .sort({ _id: 1 })
            .then((patches) => {
              assert.equal(patches.length, 1)
            })
        )
        .then(done)
        .catch(done)
    })
@vinerich vinerich self-assigned this Jan 28, 2021
@vinerich
Copy link
Collaborator

Hey @jamesholcomb,
thanks for letting us know!

I think we can support this. I'll have a deeper look over the weekend.

@jamesholcomb
Copy link
Author

Additionally, specifying rawResult: true does not work either. It's related to this issue since the virtual functions (e.g. data()) are not provided when the raw result from the driver is returned in the schema.post function.

A simple test already exists for rawResult, but it does not actually assert the patch history.

@vinerich
Copy link
Collaborator

Hey @jamesholcomb i know some time has passed by I finally made the time to dig through all open issues. Hopefully you still somewhat know what this was about.

In my understanding lean() and rawResult: true is mostly used for performance or raw access. In both cases a patch history does not need to be kept. Is that right or am I wrong here?

A simple fix would then be to state it in the README and disable the plugin hooks if lean or rawResult is detected.

@jamesholcomb
Copy link
Author

I ended up refactoring my code by removing lean and rawResult from all write ops that use patch history.

Would it be more useful to throw an error if the lib detects either of those options?

@vinerich
Copy link
Collaborator

Thanks for answering!

Then I would propose throwing an error and providing a config option to disable error throwing?

@jamesholcomb
Copy link
Author

Perhaps you could expand on the behavior of 'disable the plugin hooks'

@vinerich
Copy link
Collaborator

Under plugin hooks i understand the functionality of the plugin. So if thelean or rawResult option is detected we can disable the plugin functionality (configurable). So either throw error or the users "knows what he is doing" and we disable the error throwing.

@jamesholcomb
Copy link
Author

From an end user perspective, it is hard to imagine a scenario where you would explicitly configure a collection for patch history but disable the plugin if your db writes utilize those mongoose options. Especially lean, as it it used for performance optimization. But it's hard to predict every use case, so I understand the point of view.

If you go with a "throw on error" style option, it would be prudent to enable by default.

@vinerich
Copy link
Collaborator

But it's hard to predict every use case, so I understand the point of view.

Isn't lean basically circumventing mongoose? It is used to gather raw results, it can be used to write if I'm not mistaken here.
So the following update operation (if any) will run through mongoose and should be supported by the patch-history-plugin again.

Just some quick thoughts on this, I'll need to consult mongoose documentation ..

If you go with a "throw on error" style option, it would be prudent to enable by default.

So the option would rather be disablePluginForLean or something like that and it would ofc default to false.
This would ensure backwards compatibility and the user should need to explicitly ignore the error.

@vinerich vinerich added the bug label Feb 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants