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

includeResultMetadata types not changing return type? #14303

Closed
2 tasks done
simllll opened this issue Jan 29, 2024 · 5 comments · Fixed by #14336
Closed
2 tasks done

includeResultMetadata types not changing return type? #14303

simllll opened this issue Jan 29, 2024 · 5 comments · Fixed by #14336
Labels
typescript Types or Types-test related issue / Pull Request
Milestone

Comments

@simllll
Copy link
Contributor

simllll commented Jan 29, 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.1.1

Node.js version

20.x

MongoDB server version

6.x

Typescript version (if applicable)

5.1.6

Description

I tried upgrading to mongoose 8.x and changed rawResult: true to includeResultMetadata: true, but now typescript complains:

TS2339: Property value does not exist on type
FlattenMaps<IDBJobField> & Required<{   _id: ObjectId; }>

also it seems that the retutrn type is not changed depending on the value of includeResultMetadata
image
it's the same as in the screenshot, regardless of this flag.

Furthermore; i found it kinda confusing with the mongodb docs, as they say that includeResultMetadata is true by default. Does mongoose set it to false then by default? Or is it actually true, but mongoose just returns the document and strips the rest away? Which would sound to me of a waste of some extra bytes?

error TS2339: Property 'value' does not exist on type 'FlattenMaps & Required<{ _id: ObjectId; }>'.
error TS2339: Property 'lastErrorObject' does not exist on type 'FlattenMaps & Required<{ _id: ObjectId; }>'.
error TS2339: Property 'value' does not exist on type 'FlattenMaps & Required<{ _id: ObjectId; }>'.

Steps to Reproduce

use an example query for findOneAndupdate and play around with the includeResultMetadata flag

Expected Behavior

should return the "ModifyResult" instead, like it did with rawResult: true

@vkarpov15 vkarpov15 added this to the 8.1.2 milestone Jan 29, 2024
@vkarpov15 vkarpov15 added the typescript Types or Types-test related issue / Pull Request label Jan 29, 2024
@vkarpov15
Copy link
Collaborator

Where in the docs do you see that includeResultMetadata is true by default? I think that changed in MongoDB Node driver 6.

Also, is the error just in your IDE or also in TypeScript? Because I'm unable to repro with TypeScript, the following script compiles fine:

import mongoose from 'mongoose';

run();

async function run() {
  const TestModel = mongoose.model('Test', new mongoose.Schema({ name: String }));
  const doc = await TestModel.findOneAndUpdate({}, { name: 'bar' }, { includeResultMetadata: true });
  doc.value;
}
$ ./node_modules/.bin/tsc --strict gh-14303.ts 
$ 

@vkarpov15 vkarpov15 added can't reproduce Mongoose devs have been unable to reproduce this issue. Close after 14 days of inactivity. and removed typescript Types or Types-test related issue / Pull Request labels Jan 29, 2024
@vkarpov15 vkarpov15 removed this from the 8.1.2 milestone Jan 29, 2024
@simllll
Copy link
Contributor Author

simllll commented Jan 30, 2024

Where in the docs do you see that includeResultMetadata is true by default? I think that changed in MongoDB Node driver 6.

see here:
https://github.com/mongodb/node-mongodb-native/releases/tag/v5.7.0

This option defaults to true, which will return a ModifyResult type. When set to false, which will

and here https://www.mongodb.com/blog/post/behavioral-changes-find-one-family-apis-node-js-driver-6-0-0

Starting with the Node.js Driver 5.7.0 release a new FindOneAndOptions property called includeResultMetadata has been introduced. When this property is set to false (default is true) the findOneAnd APIs will return the requested document as expected.

Also, is the error just in your IDE or also in TypeScript? Because I'm unable to repro with TypeScript, the following script compiles fine:

I will look into it again late,r but it was a build issue, not just an IDE issue. But some other issues are even valid, I had returnDocument: 'after' in some updateOne queries, which makes no sense of course ;)

@simllll
Copy link
Contributor Author

simllll commented Jan 30, 2024

Regarding includeResultMetadata,
you are right, it's mentionted here that it will be set to false. https://github.com/mongodb/node-mongodb-native/releases/tag/v6.0.0

Once 6.0.0 is released, includeResultMetadata: false will become the default behavior. If your application relies on the previous behavior of these APIs, setting includeResultMetadata: true will allow you to continue to access the ModifyResult directly.

image

so this should be safe, it wsa just kinda of confusing ;-)

regarding the typescript issue, I iwll look into this now

@simllll
Copy link
Contributor Author

simllll commented Jan 30, 2024

Alright, I found the issue. It's about the "lean" flag.

import mongoose from 'mongoose';

run();

async function run() {
  const TestModel = mongoose.model('Test', new mongoose.Schema({ name: String }));
  const doc = await TestModel.findOneAndUpdate({}, { name: 'bar' }, { includeResultMetadata: true, lean: true, returnDocument: 'after' });
  doc.value;
}

the problem pops up when I add lean: true to the options. then the returned type definitoin is the one I mentioned.

This brings me to another followup question: If we set includeResultMetadata to true, is mongoose then populating stuff and returns a mongoose document, or does it actually return the raw result like with lean: true?
Because the type of includeResultMetadta: false is the exact same as the ".value" prop when I set it to true...
I would expect it to be the lean version, as this was the behaviour with "rawResult" before. But to be explicit about that, what happens then if I set lean explicity to false?

@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 can't reproduce Mongoose devs have been unable to reproduce this issue. Close after 14 days of inactivity. labels Feb 5, 2024
@vkarpov15 vkarpov15 added this to the 8.1.2 milestone Feb 5, 2024
vkarpov15 added a commit that referenced this issue Feb 6, 2024
@vkarpov15
Copy link
Collaborator

This brings me to another followup question: If we set includeResultMetadata to true, is mongoose then populating stuff and returns a mongoose document, or does it actually return the raw result like with lean: true? If lean is false, then Mongoose will fully hydrate the document, and .value will contain the fully hydrated document.

I would expect it to be the lean version, as this was the behaviour with "rawResult" before. But to be explicit about that, what happens then if I set lean explicity to false? Setting lean: false is the same as not setting lean at all in this case.

@vkarpov15 vkarpov15 added typescript Types or Types-test related issue / Pull Request and removed needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue labels Feb 6, 2024
vkarpov15 added a commit that referenced this issue Feb 7, 2024
types(model): correct return type for findOneAndUpdate with `includeResultMetadata` and `lean` set
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typescript Types or Types-test related issue / Pull Request
Projects
None yet
2 participants