Skip to content

fix(document+schema): improve handling for setting paths underneath maps, including maps of maps #15477

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 4 commits into from
Jun 12, 2025

Conversation

vkarpov15
Copy link
Collaborator

Fix #15461

Summary

Primary issue comes down to:

  1. Need to pass the path being set into applySetters() because that is the most reliable way to get the actual path being set, with no .$*.
  2. Schema.prototype.path() and Schema.prototype.pathType() not being able to get paths underneath maps correctly.

Fixed both of those, along with adding specialized testing for path() and pathType().

Examples

@vkarpov15 vkarpov15 added this to the 8.15.2 milestone Jun 11, 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 improves the handling for setting paths underneath maps—including maps of maps—by refining map path detection and updating the setter functionality. Key changes include:

  • Updating getMapPath in lib/schema.js to use a string‐based approach instead of a regular expression.
  • Modifying Document.prototype.$set in lib/document.js to pass the current path into applySetters.
  • Adding tests in test/schema.pathType.test.js, test/schema.path.test.js, and test/document.test.js to ensure proper handling of nested map paths.

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/schema.pathType.test.js Added tests to verify that schema.pathType() correctly returns 'real' for map paths.
test/schema.path.test.js Added tests to ensure schema.path() returns the expected instance for map paths.
test/document.test.js Added tests to confirm that $set handles nested maps and map-of-maps correctly.
lib/schema.js Updated getMapPath to use string matching for cleaner map path resolution.
lib/document.js Updated $set to include the path parameter when calling applySetters for better accuracy.
Comments suppressed due to low confidence (2)

lib/schema.js:1485

  • [nitpick] Consider adding an inline comment explaining the purpose of transforming the path (i.e. removing '.$*') to improve clarity for future maintainers.
const cleanPath = val.path.replace(/\.\$\*/g, '');

lib/document.js:1436

  • Review that all downstream implementations of applySetters correctly recognize and handle the new 'path' option to ensure consistent behavior.
val = schema.applySetters(val, setterContext, false, priorVal, { path, overwriteImmutable: true });

Copy link
Collaborator

@hasezoey hasezoey 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 0d18d6e into master Jun 12, 2025
71 checks passed
@hasezoey hasezoey deleted the vkarpov15/gh-15461 branch June 13, 2025 08:55
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.

model.set() for nested Schema inside Map field
2 participants