fix(query): handle casting array filter paths underneath array filter paths with embedded discriminators #15388
Conversation
… paths with embedded discriminators Fix #15386
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue with casting array filter paths that reference embedded discriminators, as described in #15386. Key changes include:
- Adding and propagating a discriminator value map in castArrayFilters and getPath.
- Updating getEmbeddedDiscriminatorPath to correctly handle starting indices when processing embedded discriminator paths.
- Adding tests for the new pattern in update.castArrayFilters.test.js.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/helpers/update.castArrayFilters.test.js | Added a new test validating proper casting of array filter paths with embedded discriminators. |
| lib/helpers/update/castArrayFilters.js | Modified to build a discriminator value map and pass it to getPath for embedded discriminator casting. |
| lib/helpers/schema/getPath.js | Updated the function signature to accept a discriminator value map and use it when traversing the schema. |
| lib/helpers/query/getEmbeddedDiscriminatorPath.js | Adjusted the path splitting logic by introducing a startIndex to handle multiple embedded discriminators. |
Comments suppressed due to low confidence (1)
lib/helpers/query/getEmbeddedDiscriminatorPath.js:32
- [nitpick] Consider renaming 'startIndex' to something like 'discriminatorStartIndex' to more clearly indicate its purpose.
const originalSubpath = parts.slice(startIndex, i + 1).join('.');
| @@ -8,7 +8,7 @@ const numberRE = /^\d+$/; | |||
| * @api private | |||
| */ | |||
|
|
|||
| module.exports = function getPath(schema, path) { | |||
| module.exports = function getPath(schema, path, discriminatorValueMap) { | |||
There was a problem hiding this comment.
Update the JSDoc for getPath to describe the new 'discriminatorValueMap' parameter to aid future maintainers.
Copilot uses AI. Check for mistakes.
| @@ -33,6 +33,9 @@ function _castArrayFilters(arrayFilters, schema, strictQuery, updatedPathsByFilt | |||
| return; | |||
| } | |||
|
|
|||
| // Map of embedded discriminator values set in the array filters | |||
There was a problem hiding this comment.
[nitpick] Consider adding a comment to document the purpose and usage of the 'discriminatorValueMap' for clarity.
| // Map of embedded discriminator values set in the array filters | |
| // Map to store discriminator values for embedded documents in the array filters. | |
| // This is used to handle cases where array filters target specific embedded document types. |
Copilot uses AI. Check for mistakes.
| @@ -17,7 +17,7 @@ const updatedPathsByArrayFilter = require('../update/updatedPathsByArrayFilter') | |||
| */ | |||
|
|
|||
| module.exports = function getEmbeddedDiscriminatorPath(schema, update, filter, path, options) { | |||
| const parts = path.split('.'); | |||
| const parts = path.indexOf('.') === -1 ? [path] : path.split('.'); | |||
There was a problem hiding this comment.
Is this a performance optimization? Because i think .split() will always return a array with a value, even if separator is not found.
| const currentPath = cur; | ||
| cur = ''; |
There was a problem hiding this comment.
Instead of assigning a new value, how about just moving the cur = '' reassignment to the end of the block?
Fix #15386
Summary
The issue in #15386 is that
events.$[event].products.$[product].priceupdates don't quite work ifproductsis a path on an embedded discriminator where the discriminator key is set in theeventarray filter.To get that to work, we needed to first fix
castArrayFilters()to handle this pattern when casting array filters, and then improve how we handle casting the update incastUpdate()(getEmbeddedDiscriminatorPath()changes).Examples