-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
refactor: pass parent schema to SchemaType constructors in interpretAsType to make implementing custom container types easier #15700
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
base: 8.20
Are you sure you want to change the base?
Conversation
…sType to make implementing custom container types easier
There was a problem hiding this 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 refactor improves the extensibility of custom container types in Mongoose by passing the parent schema as a parameter to SchemaType constructors. Previously, implementing custom container types that needed to interpret nested schemas (like { type: Set, of: { type: String } }) required re-implementing interpretAsType logic or using workarounds like the options.parentSchema hack used in the Union type.
Key changes:
- Added
parentSchemaparameter to all SchemaType constructors viainterpretAsType - Removed the Union-specific hack that attached parent schema to options
- Union type now receives parent schema directly as a constructor parameter
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/schema.js | Updated interpretAsType to pass this (parent schema) as fourth parameter to SchemaType constructors, removing Union-specific workaround |
| lib/schema/union.js | Modified Union constructor to accept parentSchema parameter instead of accessing it from options |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, though it seems Github CI's Node 22 does not like some change here, though any other version just works.
The CI errors in question
Runner: Node 22 MongoDB 6.0.15 OS ubuntu-22.04
Full Node Version in runner: 22.21.0
1) Double
cast errors
when a non-numeric string is provided to an Double field
throws a CastError upon validation:
AssertionError [ERR_ASSERTION]: 'Cast to Double failed for value "helloworld" (type string) at path "myDouble" because of "SyntaxError"' == 'Cast to Double failed for value "helloworld" (type string) at path "myDouble"'
+ expected - actual
-Cast to Double failed for value "helloworld" (type string) at path "myDouble" because of "SyntaxError"
+Cast to Double failed for value "helloworld" (type string) at path "myDouble"
at Context.<anonymous> (test/double.test.js:284:16)
at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
2) Int32
cast errors
when a non-integer decimal input is provided to an Int32 field
throws a CastError upon validation:
AssertionError [ERR_ASSERTION]: 'Cast to Int32 failed for value "-42.4" (type number) at path "myInt" because of "SyntaxError"' == 'Cast to Int32 failed for value "-42.4" (type number) at path "myInt"'
+ expected - actual
-Cast to Int32 failed for value "-42.4" (type number) at path "myInt" because of "SyntaxError"
+Cast to Int32 failed for value "-42.4" (type number) at path "myInt"
at Context.<anonymous> (test/int32.test.js:304:16)
at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
3) Int32
cast errors
when a non-numeric string is provided to an Int32 field
throws a CastError upon validation:
AssertionError [ERR_ASSERTION]: 'Cast to Int32 failed for value "helloworld" (type string) at path "myInt" because of "SyntaxError"' == 'Cast to Int32 failed for value "helloworld" (type string) at path "myInt"'
+ expected - actual
-Cast to Int32 failed for value "helloworld" (type string) at path "myInt" because of "SyntaxError"
+Cast to Int32 failed for value "helloworld" (type string) at path "myInt"
at Context.<anonymous> (test/int32.test.js:322:16)
at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
4) Int32
cast errors
when a non-integer decimal string is provided to an Int32 field
throws a CastError upon validation:
AssertionError [ERR_ASSERTION]: 'Cast to Int32 failed for value "1.2" (type string) at path "myInt" because of "SyntaxError"' == 'Cast to Int32 failed for value "1.2" (type string) at path "myInt"'
+ expected - actual
-Cast to Int32 failed for value "1.2" (type string) at path "myInt" because of "SyntaxError"
+Cast to Int32 failed for value "1.2" (type string) at path "myInt"
at Context.<anonymous> (test/int32.test.js:340:16)
at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
5) Int32
cast errors
when NaN is provided to an Int32 field
throws a CastError upon validation:
AssertionError [ERR_ASSERTION]: 'Cast to Int32 failed for value "NaN" (type number) at path "myInt" because of "SyntaxError"' == 'Cast to Int32 failed for value "NaN" (type number) at path "myInt"'
+ expected - actual
-Cast to Int32 failed for value "NaN" (type number) at path "myInt" because of "SyntaxError"
+Cast to Int32 failed for value "NaN" (type number) at path "myInt"
at Context.<anonymous> (test/int32.test.js:358:16)
at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
6) Int32
cast errors
when value above INT32_MAX is provided to an Int32 field
throws a CastError upon validation:
AssertionError [ERR_ASSERTION]: 'Cast to Int32 failed for value "2147483648" (type number) at path "myInt" because of "SyntaxError"' == 'Cast to Int32 failed for value "2147483648" (type number) at path "myInt"'
+ expected - actual
-Cast to Int32 failed for value "2147483648" (type number) at path "myInt" because of "SyntaxError"
+Cast to Int32 failed for value "2147483648" (type number) at path "myInt"
at Context.<anonymous> (test/int32.test.js:376:16)
at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
7) Int32
cast errors
when value below INT32_MIN is provided to an Int32 field
throws a CastError upon validation:
AssertionError [ERR_ASSERTION]: 'Cast to Int32 failed for value "-2147483649" (type number) at path "myInt" because of "SyntaxError"' == 'Cast to Int32 failed for value "-2147483649" (type number) at path "myInt"'
+ expected - actual
-Cast to Int32 failed for value "-2147483649" (type number) at path "myInt" because of "SyntaxError"
+Cast to Int32 failed for value "-2147483649" (type number) at path "myInt"
at Context.<anonymous> (test/int32.test.js:394:16)
at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
8) model: findOneAndUpdate:
bug fixes
single nested doc cast errors (gh-3602):
AssertionError [ERR_ASSERTION]: 'Cast to Number failed for value "not a num" (type string) at path "street" because of "SyntaxError"' == 'Cast to Number failed for value "not a num" (type string) at path "street"'
+ expected - actual
-Cast to Number failed for value "not a num" (type string) at path "street" because of "SyntaxError"
+Cast to Number failed for value "not a num" (type string) at path "street"
at Context.<anonymous> (test/model.findOneAndUpdate.test.js:1357:14)
at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
9) validation docs
Cast Errors:
AssertionError [ERR_ASSERTION]: 'Cast to Number failed for value "not a number" (type string) at path "numWheels" because of "SyntaxError"' == 'Cast to Number failed for value "not a number" (type string) at path "numWheels"'
+ expected - actual
-Cast to Number failed for value "not a number" (type string) at path "numWheels" because of "SyntaxError"
+Cast to Number failed for value "not a number" (type string) at path "numWheels"
at Context.<anonymous> (test/docs/validation.test.js:384:12)
at process.processImmediate (node:internal/timers:485:21)|
|
||
| class Union extends SchemaType { | ||
| constructor(key, options, schemaOptions = {}) { | ||
| constructor(key, options, schemaOptions, parentSchema) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change should likely be documented somehwere in the code, though i dont know if this already exists somehwere (and SchemaType itself seems to have different parameters, so likely not there).
Also the "Custom Schema Types" guide seems to be somewhat outdated, and this change should likely be documented there as a available parameter (/docs/customschematypes.md).
Summary
Implementing your own container type is a pain unless the container type happens to extend from Array, because otherwise you need to handle interpreting
ofby yourself. For example, imagine{ type: Set, of: { type: String, required: true } }- right now anyone implementing a set schematype would have to re-implement interpretAsType semantics themselves. We had to do a bit of a hack to avoid having to re-implement interpretAsType for unions, but with this fix we can remove the hack as well.Examples