validateFilter function#16324
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new query-level validateFilter option and a public Model.validateFilter() API to validate query filter values against a model’s schema (including enums and other validators), addressing the feature request in #15581.
Changes:
- Add
QueryOptions.validateFiltertyping andQuery#validateFilter()chainable setter. - Implement
Model.validateFilter()and integrate filter validation into query casting/execution when enabled. - Add comprehensive tests covering validation behavior across query operators and query methods.
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| types/query.d.ts | Adds validateFilter?: boolean query option and Query#validateFilter() typings. |
| types/models.d.ts | Declares Model.validateFilter() type signature. |
| lib/query.js | Integrates optional filter validation into _castConditions() and awaits casting in exec paths; adds Query#validateFilter(). |
| lib/model.js | Implements Model.validateFilter() and helper routines to validate filter values against schema validators. |
| lib/cursor/queryCursor.js | Adds filter validation path for .cursor() when validateFilter is enabled. |
| test/query.test.js | Adds test coverage for validateFilter option behavior and Model.validateFilter(). |
| model.hooks.execPre('find', query).then(() => { | ||
| if (!query.options.validateFilter) { | ||
| onPreComplete(null); | ||
| return; | ||
| } | ||
|
|
||
| return query._castConditions().then(() => { | ||
| if (query.error()) { | ||
| onPreComplete(query.error()); | ||
| return; | ||
| } | ||
| onPreComplete(null); | ||
| }); | ||
| }, err => onPreComplete(err)); |
vkarpov15
left a comment
vkarpov15
left a comment
There was a problem hiding this comment.
A few comments.
Also, at a high level it would make sense to create a new FilterValidationError class so it is easier to distinguish between validation errors in the filter vs validation errors in the update.
Also, a point to discuss: how should validateFilter work with the existing runValidators option? The runValidators option runs validation on the update portion of the query, but not the filter. Maybe worthwhile to deprecate runValidators in favor of a separate validateUpdate option, WDYT @AbdelrahmanHafez @hasezoey ?
| }; | ||
|
|
||
| /** | ||
| * Casts query filter values and optionally validates them when the |
There was a problem hiding this comment.
Casting and validation are separate steps in Mongoose, so I don't like having castConditions() also run validateFilter(). I'd rather have each individual function have a separate check for validateFilter option and call validateFilter() if necessary.
|
|
||
| if (utils.isPOJO(val) && !Object.keys(val).some(isOperator)) { | ||
| for (const nestedKey of Object.keys(val)) { | ||
| const nestedPath = key + '.' + nestedKey; |
There was a problem hiding this comment.
We should run this check after const schematype = schema.path(key); . This logic is meant to capture nested paths, which do not have a schematype, but it would also run on subdocuments, which do have a schematype. See subdocs vs nested paths.
| } catch (err) { | ||
| const validationError = new ValidationError(); | ||
| validationError.addError(path, err); | ||
| throw validationError; |
There was a problem hiding this comment.
As written, this will only get the first validator error, not all validator errors. For example query on { status: 'invalid', priority: -3.1415 } would only get the error on status but not include the error on priority. We should create a validation error and continue to validate all fields in the query, making sure we return all validation errors. Just be careful not to create a ValidationError if there aren't any errors - that is bad for perf.
|
| if (typeof context === 'function' || typeof arguments[2] === 'function') { | ||
| throw new MongooseError('Model.validateFilter() no longer accepts a callback'); | ||
| } |
There was a problem hiding this comment.
This is a new function, so it shouldnt need this guard.
There was a problem hiding this comment.
The new private functions should also have JSDoc
closes #15581