Skip to content

fix(DocumentArray): correctly set parent if instantiated with schema from another Mongoose instance #15471

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

Merged
merged 2 commits into from
Jun 10, 2025

Conversation

vkarpov15
Copy link
Collaborator

Summary

#15466 points out that Mongoose throws an unreadable error if creating a document array that was defined with Schema() from a different Mongoose instance.

I added tests to cover this case automatically - historically we haven't tested the multiple require() instance setup, but we should since it is so common.

Examples

@vkarpov15 vkarpov15 added this to the 8.15.2 milestone Jun 9, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR ensures that document arrays defined with schemas from a different Mongoose instance correctly set their parent, adds tests for that scenario, and configures CI to simulate multiple require('mongoose') instances.

  • Replace instanceof Document checks with a duck‐typing check on doc.$__ for both DocumentArray and MongooseArray.
  • Add a dedicated test covering document arrays using schemas from a separate instance.
  • Introduce a helper script and CI step to create and test against a separate Mongoose require instance.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/multiple-require-instance.test.js New test for document arrays instantiated with schemas from a separate Mongoose instance
package.json Add create-separate-require-instance script to populate mongoose-separate-require-instance
.github/workflows/test.yml Invoke the new script before running tests in CI
lib/types/documentArray/index.js Use if (doc && doc.$__) instead of instanceof Document to set array parent
lib/types/array/index.js Apply the same doc.$__ check for plain arrays
Comments suppressed due to low confidence (4)

lib/types/documentArray/index.js:54

  • [nitpick] Add a comment explaining why the doc.$__ check is used instead of instanceof Document to clarify the intent and any edge cases this change addresses.
if (doc && doc.$__) {

.github/workflows/test.yml:71

  • The script uses rm -rf which is not cross-platform and will fail on Windows agents. Consider using a cross-platform cleanup tool like rimraf or a Node.js script to remove directories.
-      - run: npm run create-separate-require-instance

test/multiple-require-instance.test.js:20

  • Add a complementary test to cover native arrays (non-document arrays) defined with schemas from a separate instance so the MongooseArray logic is verified as well.
it('supports arrays defined using schemas from separate instance (gh-15466)', async function() {

lib/types/documentArray/index.js:56

  • The arraySchemaSymbol is retrieved via doc.$__schema, whereas in lib/types/array/index.js you use the public doc.schema. For clarity and consistency, align both modules to reference doc.schema.
internals[arraySchemaSymbol] = doc.$__schema.path(path);

Copy link
Collaborator

@AbdelrahmanHafez AbdelrahmanHafez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@vkarpov15 vkarpov15 merged commit 7f98d3f into master Jun 10, 2025
76 checks passed
@hasezoey hasezoey deleted the vkarpov15/gh-15466 branch June 11, 2025 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants