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

Recursion check in toObject breaks mongoose-intl plugin #14446

Closed
2 tasks done
yelworc opened this issue Mar 19, 2024 · 2 comments
Closed
2 tasks done

Recursion check in toObject breaks mongoose-intl plugin #14446

yelworc opened this issue Mar 19, 2024 · 2 comments
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it.
Milestone

Comments

@yelworc
Copy link
Contributor

yelworc commented Mar 19, 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

=6.3.5

Node.js version

=18

MongoDB server version

7

Typescript version (if applicable)

No response

Description

As of Mongoose v6.3.5, the mongoose-intl plugin does not work anymore when used in subdocument arrays. Calling toJSON or toObject with getters enabled on such objects results in an error.

In order to narrow down the source of the problem, I recreated what the plugin is doing in a standalone gist. The behavior and the resulting error is the same as my actual issue with mongoose-intl in a closed-source production system.

The changes in commit 8325e563 are what introduced the problem; specifically, the recursion checking seen map will cache and return subdocuments where the getters for localized string fields have already been applied (e.g. {field: 'some text'}), but then the toObject mechanism tries to clone the single-language schema paths (like field.en), which conflicts with the already present field value.

Now, I realize that mongoose-intl is doing something fishy here with its virtuals that Mongoose wouldn't normally allow (having a "real" field.xyz schema path and a field virtual) 😄. However, it might be relatively easy to work around; changing

branch[part] = clone(val, options);

to

if (utils.isObject(branch)) branch[part] = clone(val, options);

in document/applyGetters fixes the problem (and a similar change might be necessary in document/applyVirtuals) – but of course that might entail unintended side effects.

If you consider this a bug (resp. invalid behavior) in mongoose-intl, I'd be grateful for any ideas for fixing it there; preferably in ways that wouldn't require a DB migration 😄

Steps to Reproduce

See the gist; if it helps, I could also provide sample code that is using mongoose-intl.

Expected Behavior

No response

@vkarpov15 vkarpov15 added this to the 6.12.8 milestone Mar 22, 2024
@vkarpov15 vkarpov15 added has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue 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 Mar 22, 2024
vkarpov15 added a commit that referenced this issue Mar 26, 2024
…returns string with toJSON

Fix #14446
vkarpov15 added a commit that referenced this issue Mar 30, 2024
fix(document): handle virtuals that are stored as objects but getter returns string with toJSON
@yelworc
Copy link
Contributor Author

yelworc commented Apr 5, 2024

Many thanks for the quick fix, @vkarpov15!

Is this change going to propagate to 7.x and 8.x/master, eventually (I've seen past commits like Merge branch '7.x' in the log)? If not, the problem is likely going to pop up again once we try to update 😄

@vkarpov15
Copy link
Collaborator

Yep, we merge 6.x into 7.x and 7.x into master periodically

yelworc pushed a commit to uniqlabs/mongoose-intl that referenced this issue Apr 15, 2024

Verified

This commit was signed with the committer’s verified signature.
aduh95 Antoine du Hamel
Since Mongoose 7 is the version we're using, it's also effectively the
only one the plugin is being tested with, so it doesn't really make
sense to pretend supporting older versions.

mongoose@7.6.11 is the first v7 release compatible with mongoose-intl,
as it includes a fix for this issue:
Automattic/mongoose#14446
yelworc pushed a commit to yelworc/mongoose-intl that referenced this issue May 2, 2024

Verified

This commit was signed with the committer’s verified signature.
aduh95 Antoine du Hamel
Setting the peer dependency to v8 exclusively, since we're not using v7
anymore, so claiming v7 support would be unsubstantiated.

mongoose@8.3.2 is the first v8 release compatible with mongoose-intl,
as it includes a fix for this issue:
Automattic/mongoose#14446
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

2 participants