Skip to content

fix(schema): make hydrate() virtuals option work with parent-level dotted virtuals - #16396

Draft
AbdelrahmanHafez wants to merge 5 commits into
masterfrom
fix/hydrate-parent-dotted-virtuals
Draft

fix(schema): make hydrate() virtuals option work with parent-level dotted virtuals#16396
AbdelrahmanHafez wants to merge 5 commits into
masterfrom
fix/hydrate-parent-dotted-virtuals

Conversation

@AbdelrahmanHafez

@AbdelrahmanHafez AbdelrahmanHafez commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

re #15627 follow up to #15638

The main example in the feature request declares the virtual on the parent schema:

parentSchema.virtual('items.detail');

The virtuals option for hydrate() only handled virtuals declared directly on the child schema. Dotted regular virtuals stay registered on the parent schema only, and since document arrays, single nested subdocuments, and map entries init against their child schemas, the raw virtual value got stripped during hydration.

This PR mirrors dotted regular virtuals onto the embedded schema in Schema.prototype.virtual(), the same way dotted populate virtuals are already mirrored (the gh-8198 workaround a few lines above). It applies to document arrays, single nested subdocuments, and maps of subdocuments (entries.$*.detail), and skips any name that already resolves on the child schema, so real child paths and virtuals declared directly on the child keep working unchanged.

Tests cover the three shapes, plus the defaults: hydrate() without the option still leaves the value unset, and declaring a parent dotted virtual that conflicts with a real child path is still rejected.

Edit: the new mirror duplicated the path walk that the gh-8198 populate workaround already does, so I extracted a shared getEmbeddedSchemaVirtualTarget() helper and pointed both branches at it. While unifying them I found the populate walk throws a TypeError when a dotted populate virtual is declared under a map of primitives (of: String has no child schema), and that it stops at the first missing path segment, so it never mirrored virtuals declared under plain nested objects like nested.children.bar. The shared helper fixes both. There's new coverage for the crash and for hydrating parent virtuals under nested objects, plus tests pinning that populate virtuals under nested objects keep working and that dotted virtuals under a map without $* are still rejected, since map keys are real paths.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 fixes Model.hydrate()’s virtuals: true option so it works with parent-declared dotted virtuals (for example parentSchema.virtual('items.detail')) when hydrating embedded documents that initialize against their child schemas.

Changes:

  • Mirror parent-level dotted regular virtuals onto the corresponding embedded schema when the dotted path traverses:
    • document arrays
    • single nested subdocuments
    • maps of subdocuments ($*)
  • Avoid clobbering child schema paths/virtuals by only mirroring when the child schema reports the path as adhocOrUndefined.
  • Add tests covering document arrays, single nested subdocs, map subdocs, and the non-regression cases (no virtuals option, child conflicts).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
lib/schema.js Mirrors parent-level dotted regular virtuals onto embedded schemas so hydrate(..., { virtuals: true }) can preserve/set values on subdocuments.
test/model.test.js Adds coverage for hydration behavior across doc arrays, single nested subdocs, and map subdocs, plus conflict/non-regression checks.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

Comment thread test/schema.test.js Outdated
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.

2 participants