Conversation
There was a problem hiding this comment.
Pull request overview
This pull request simplifies the fix for issues with nested map subdocuments by removing the $pathRelativeToParent optimization approach and instead deleting options.path before passing options to subdocument initialization. The PR addresses both issue #15969 (incorrect paths with deleteOne() and clear() on nested maps) and relates to #15682.
Changes:
- Removes
$pathRelativeToParenttracking from subdocuments and maps - Simplifies path calculation logic in map and subdocument classes
- Adds
options.pathdeletion inSchemaSubdocument.cast()before calling$init() - Adds tests for
deleteOne()andclear()operations on nested map subdocuments
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/types.map.test.js | Updates test assertions to use correct employee names and adds two new test cases for gh-15969 (deleteOne and clear operations) |
| lib/types/subdocument.js | Removes $pathRelativeToParent property and related optimization logic, simplifies path calculation methods, but removes critical path deletion logic from constructor |
| lib/types/map.js | Removes complex path calculation in constructor, eliminates $pathRelativeToParent tracking, simplifies set() method, adds Mixed schema type as default |
| lib/schema/map.js | Adds options.path fallback and removes map path optimization logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
vkarpov15
left a comment
There was a problem hiding this comment.
I like this new approach, much simpler. Tests all pass, fixes a bug, and slightly faster on mapOfSubdocs benchmark so I think this is a big improvement. Thanks!
| pathToMark = fullPath.call(this); | ||
| } | ||
| parent.markModified(pathToMark); | ||
| const path = fullPath.call(this); |
There was a problem hiding this comment.
If I remember correctly, the rationale for the way #15682 was implemented was for performance, specifically to avoid this fullPath() call where possible to avoid string concatenation. But that turns out to be incorrect because it looks like benchmarks/mapOfSubdocs is actually slightly faster with this PR than in master (372ms vs 395ms on my macbook)
Fixes #15969
Re #15682
I'm not sure if this PR should be merged as is since it reverts more than just the bug fix, more of a discussion opener.
While working on something related to #15682, I discovered that just these few lines that were added to
/lib/schema/subdocument.jsFix all the issues reported in #15678 as well as the bug in #15969, without the
$pathRelativeToParentapproach. I couldn't find anything wrong with this simpler approach, but I'm curious whether it breaks an architectural decision or fails some edge case I'm not aware of.@vkarpov15 can you think of any reason not to use this instead?