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

_id getter for ObjectIds #6415

Closed
vkarpov15 opened this issue May 2, 2018 · 13 comments
Closed

_id getter for ObjectIds #6415

vkarpov15 opened this issue May 2, 2018 · 13 comments
Labels
needs clarification This issue doesn't have enough information to be actionable. Close after 14 days of inactivity

Comments

@vkarpov15
Copy link
Collaborator

Re: #6115, make it much easier to get an id regardless of whether a path is populated or not

@djanowski
Copy link

👏

@vkarpov15
Copy link
Collaborator Author

@djanowski I'd love to hear more about why you're so excited for this change if you care to share 👍

@djanowski
Copy link

When dealing with populated or non-populated paths, when you need just the ID, we have a bunch of (foo.bar._id || foo.bar).toString().

@freezy
Copy link

freezy commented May 12, 2018

This breaks a lot of code for me.

I have serializers that need to determine if a field is populated. I used to do that with doc.populated(field), but some queries are aggregations where that doesn't work (because there is no Mongoose document).

I used to fall back to return doc.field && doc.field._id, but that returns now true also for unpopulated fields. Is there a way to know if an object comes from an aggregation so I can do the check accordingly?

@vkarpov15
Copy link
Collaborator Author

@freezy sorry about the inconvenience. Can you explain what you mean by "some queries are aggregations"? You can't use populate() with aggregate, so I'm not sure why you would need to check if a field is populated if the document came out of an aggregate()

@vkarpov15 vkarpov15 reopened this May 17, 2018
@vkarpov15 vkarpov15 modified the milestones: 5.1, 5.1.2 May 17, 2018
@vkarpov15 vkarpov15 added needs clarification This issue doesn't have enough information to be actionable. Close after 14 days of inactivity and removed fixed? labels May 17, 2018
@freezy
Copy link

freezy commented May 17, 2018

Thanks for having a look at this! My use case:

I have a REST API that returns data from Mongoose. For every model I have a serializer that takes in a query result and spits out JSON objects, depending on the verbosity of the API call.

The serializer doesn't know where the data comes from. It just checks which of the fields are populated and creates an object with the desired fields if they are available. The function doing the check looks like this:

if (doc.populated && doc.populated(field)) {
	return true;
}
let obj = get(doc, field);
if (isArray(obj) && obj.length > 0) {
	obj = obj[0];
}
return obj && obj._id;

Now, sometimes, the business layer uses aggregations to produce a result. The structure of what's passed to the serializer is identical, only without the Mongoose magic because of the aggregation. So in my serializer, I need a way of knowing if a field is populated for both a Mongoose document and a "plain" aggregation result.

In case of an unpopulated field, the above code now returns true, because obj._id now mirrors obj. Apart from picking another random field to check on, I can't find a way to fix this.

@vkarpov15
Copy link
Collaborator Author

@freezy how about instead of return obj && obj._id you do return !(obj instanceof mongoose.Types.ObjectId)? That should tell you if the id property was replaced with an object.

@freezy
Copy link

freezy commented May 19, 2018

Yeah, I also thought about

if (doc.populated) {
	return doc.populated(field);
}

in the first block. Will do some tests and report back.

Cheers!

@vkarpov15 vkarpov15 removed this from the 5.1.2 milestone May 21, 2018
@vkarpov15
Copy link
Collaborator Author

Thanks @freezy , let me know if that works for you 👍

@freezy
Copy link

freezy commented Jun 6, 2018

Sorry it took so long, but while my workaround didn't work, yours did! Cheers and feel free to close.

@ducdigital
Copy link

Hi @vkarpov15, we just found out about this issue and it seems like it does not list in the breaking changes from mongoose 4 -> 5

This is important since I am pretty sure lot of people do a check object._id to see if an object is populated.

I agree that the check should be using either .populated or typeOf() but the migrate_to_5.md should update accordingly.

Thank you

@vkarpov15
Copy link
Collaborator Author

@ducdigital done. That's a good suggestion, thanks.

@meircarlos
Copy link

I still believe it would be useful to have a universal approach for retrieving get an id regardless of whether a path is populated or not.

Whenever I want to return the id of a reference in a document I need to do document.relation._id.toString() to ensure I'm returning the id as a string regardless its populated or not.

And whenever the reference is an array is the same thing but with a map document.relations.map(relation => relation._id.toString()).

It would also be nice if mongoose automatically implemented a virtual getter to retrieve the id or ids of the relations as strings instead of getting the documents or ObjectIds.

@Automattic Automattic locked as resolved and limited conversation to collaborators Feb 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs clarification This issue doesn't have enough information to be actionable. Close after 14 days of inactivity
Projects
None yet
Development

No branches or pull requests

5 participants