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

Subdocument Array of Objects Schema causes "Cast to array failed for value" validation error when you use ... spread operator versus toObject() for inheritance #11522

Closed
niftylettuce opened this issue Mar 12, 2022 · 7 comments
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it.
Milestone

Comments

@niftylettuce
Copy link
Contributor

Hey @vkarpov15 - hope all is well.

I'm writing as this was a frustrating bug to track down. I believe this worked in version 5 of Mongoose, but I could be wrong and this totally has never worked since I switched to use ... spread operator here for inheriting the object.

Here's the diff showing the fix I had to do to avoid the issue:

  // swap the user group based off ctx.request.body.group
  ctx.state.domain.members = ctx.state.domain.members.map((member) => ({
-    ...member,
+    ...member.toObject(),
    group:
      member.user.toString() === ctx.params.member_id
        ? ctx.request.body.group
        : member.group
  }));

// ...

await ctx.state.domain.save(); // save the domain here causes the validation error

Screen Shot 2022-03-11 at 5 57 34 PM

The schema is defined here https://github.com/forwardemail/forwardemail.net/blob/master/app/models/domain.js#L131 and here https://github.com/forwardemail/forwardemail.net/blob/master/app/models/domain.js#L45-L58 (pretty simple).

@niftylettuce
Copy link
Contributor Author

Note this only happens when you don't use lean() and don't have a Mongoose object.

@Uzlopak
Copy link
Collaborator

Uzlopak commented Mar 13, 2022

toObject is basically transforming the hydrated Document to a lean Object. Probably when casting we have to check if it is a hydrated Document and lean it.

@vkarpov15 vkarpov15 added this to the 6.2.8 milestone Mar 18, 2022
@vkarpov15 vkarpov15 added the needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue label Mar 18, 2022
@vkarpov15
Copy link
Collaborator

I'll try to repro this. However, in general, I'd recommend using .toObject() with spread operator. Because spread operator iterates through the actual object's keys, doing ...member gives you a POJO with Mongoose document internals like $__ and _doc.

@vkarpov15
Copy link
Collaborator

Confirmed and fixed. Although I still would strongly recommend doing the below instead:

for (const member of ctx.state.domain.members) {
  member.group = member.user.toString() === ctx.params.member_id
        ? ctx.request.body.group
        : member.group;
}

You're not writing React here - by shallow cloning the array and all the documents you're incurring an unnecessary performance penalty and making your code more complex.

@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 Mar 23, 2022
@titanism
Copy link

titanism commented Apr 8, 2022

@vkarpov15 this rewrite to use the loop will throw the following error Cast to ObjectId failed for value "[Circular]" (type string) at path "_id" on latest version of Mongoose

  for (const member of ctx.state.domain.members) {
    if (member.user.toString() === ctx.params.member_id)
      member.group = ctx.request.body.group;
  }

@titanism
Copy link

titanism commented Apr 8, 2022

@vkarpov15 we've confirmed that the fix is the following:

  // swap the user group based off ctx.request.body.group
  ctx.state.domain.members = ctx.state.domain.members.map((member) => {
    return {
      user: member.user,
      group:
        member.user.toString() === ctx.params.member_id
          ? ctx.request.body.group
          : member.group
    };
  });

titanism added a commit to forwardemail/forwardemail.net that referenced this issue Apr 8, 2022
@vkarpov15
Copy link
Collaborator

@titanism I'm trying to figure out why your original example doesn't work. Looking at forwardemail/forwardemail.net@9e3ec85#diff-93a5248c3ab689a90e0344028135658b7cf18f44872ed83ecc329f9e912342c9, why is the original code:

  for (const member of domain.members) {
    if (member.user.toString() === ctx.params.member_id)
      member.group = ctx.request.body.group;
  }

and not the below? Loop should be over ctx.state.domain.members

for (const member of ctx.state.domain.members) {
    if (member.user.toString() === ctx.params.member_id)
      member.group = ctx.request.body.group;
  }

titanism added a commit to forwardemail/forwardemail.net that referenced this issue Jul 1, 2022
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

4 participants