-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
BREAKING CHANGE: make create() and insertOne() params more strict, remove generics to prevent type inference #15587
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: 9.0
Are you sure you want to change the base?
Conversation
…move generics to prevent type inference Fix #15355
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 would likely be a good idea, but after trying it in typegoose, i came across the following problems:
number
does not seem to be valid for aDate
property (ietime: Date.now()
is now invalid, even though default casting would work)array
is not valid anymore for maps and have to be a object now (instead of[['key1', val1], ['key2', val2]]
)- somehow
new mongoose.Types.ObjectId()
is not valid for_id
(or any ObjectId field?)
The only big problem is that somehow, as mentioned above, new ObjectId()
does not seem to be valid for ObjectIdQueryTypeCasting
. Due to this PR i also had to add mongoose.Require_id<T>
to Model->TRawDocType
.
Aside from that, there are only expected changes, like intentionally providing invalid data for runtime validation testing, and some type incompatibilities due to typegoose likely not using the correct mapping and instead passing hydrated document types for TRawDocType
(and so typescript complains about missing properties like $pop, $shift, addToSet, isMongooseArray
)
For context, the full diff of the changes i had already done
Note that patch is not "full" as i have not figured out the subdocument situation
diff --git a/src/types.ts b/src/types.ts
index 4e37283a..905f65dc 100644
--- a/src/types.ts
+++ b/src/types.ts
@@ -29,7 +29,7 @@ export type ArraySubDocumentType<T, QueryHelpers = BeAnObject> = DocumentType<T,
* Used Internally for ModelTypes
*/
export type ModelType<T, QueryHelpers = BeAnObject> = mongoose.Model<
- T, // raw doc type
+ mongoose.Require_id<T>, // raw doc type
QueryHelpers, // query helpers
IObjectWithTypegooseFunction, // instance methods
DefaultIdVirtual // virtuals
diff --git a/test/tests/arrayValidator.test.ts b/test/tests/arrayValidator.test.ts
index a7961c67..152e6573 100644
--- a/test/tests/arrayValidator.test.ts
+++ b/test/tests/arrayValidator.test.ts
@@ -68,7 +68,7 @@ it('should respect enum [String]', async () => {
enumedString: [
'not in the enum', // string not in the enum
],
- });
+ } as any);
fail('Expected to throw ValidationError!');
} catch (err) {
@@ -96,7 +96,7 @@ it('should respect enum [Number]', async () => {
enumedNumber: [
5, // number not in the enum
],
- });
+ } as any);
fail('Expected to throw ValidationError!');
} catch (err) {
diff --git a/test/tests/numberValidator.test.ts b/test/tests/numberValidator.test.ts
index 7138b736..a482c1f5 100644
--- a/test/tests/numberValidator.test.ts
+++ b/test/tests/numberValidator.test.ts
@@ -23,7 +23,7 @@ it('should respect enum', async () => {
try {
await NumberValidatorsModel.create({
enumed: 5, // number not in the enum
- });
+ } as any);
fail('Expected to throw ValidationError!');
} catch (err) {
diff --git a/test/tests/ref.test.ts b/test/tests/ref.test.ts
index b3446458..b3130714 100644
--- a/test/tests/ref.test.ts
+++ b/test/tests/ref.test.ts
@@ -409,10 +409,14 @@ it('Reference-Maps should work and be populated', async () => {
const dummy2 = await RefMapDummyModel.create({ dummy: '2' });
const doc1 = await RefMapModel.create({
- mapped: [
- ['1', dummy1],
- ['2', dummy2],
- ],
+ // mapped: [
+ // ['1', dummy1],
+ // ['2', dummy2],
+ // ],
+ mapped: {
+ '1': dummy1,
+ '2': dummy2,
+ },
});
const found = await RefMapModel.findById(doc1).orFail().exec();
diff --git a/test/tests/shouldRun.test.ts b/test/tests/shouldRun.test.ts
index 48061907..ea6efa42 100644
--- a/test/tests/shouldRun.test.ts
+++ b/test/tests/shouldRun.test.ts
@@ -1147,7 +1147,7 @@ it('should validate Decimal128', async () => {
try {
await CarModel.create({
carModel: 'Tesla',
- price: 'NO DECIMAL',
+ price: 'NO DECIMAL' as any,
});
fail('Validation must fail.');
} catch (e) {
@@ -1181,7 +1181,7 @@ it(`should Validate Map`, async () => {
try {
await InternetUserModel.create({
projects: {
- p1: 'project',
+ p1: 'project' as any,
},
});
fail('Validation should Fail');
@hasezoey I added support for array of arrays for maps and strings+numbers for dates. Also added |
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 PR removes generic type parameters from create()
and insertOne()
methods to improve type safety and autocomplete functionality. The change makes these methods more strict by accepting Partial<RawDocType>
with applied casting rules instead of allowing any type through generic inference.
- Removes generic parameters from
create()
andinsertOne()
to prevent type inference issues - Updates test files to use type assertions where needed for edge cases
- Adds comprehensive test coverage for various create scenarios including subdocuments, document arrays, and maps
Reviewed Changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
test/types/models.test.ts | Updates test to use type assertion for non-conforming object creation |
test/types/document.test.ts | Removes generic parameter from create call in test |
test/types/create.test.ts | Removes generic type tests and adds comprehensive create scenario tests |
test/types/connection.test.ts | Updates test to use type assertion and adds InferSchemaType import |
docs/migrating_to_9.md | Documents the breaking change with migration examples |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
If your parameters to `create()` don't match `Partial<RawDocType>`, you can use `as` to cast as follows. | ||
|
||
```ts | ||
const doc = await TestModel.create({ age: 'not a number', someOtherProperty: 'value' } as unknown as Partial<InferSchemaType<typeof schema>>); |
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.
The example uses a double type assertion (as unknown as
) which is generally considered a code smell. Consider showing a cleaner alternative or explaining why this specific pattern is necessary.
const doc = await TestModel.create({ age: 'not a number', someOtherProperty: 'value' } as unknown as Partial<InferSchemaType<typeof schema>>); | |
// Prefer defining the object with the correct type to avoid double assertion: | |
const data: Partial<InferSchemaType<typeof schema>> = { age: 'not a number', someOtherProperty: 'value' }; | |
const doc = await TestModel.create(data); | |
// If you must use a type assertion, use a single assertion if possible: | |
// const doc = await TestModel.create({ age: 'not a number', someOtherProperty: 'value' } as Partial<InferSchemaType<typeof schema>>); | |
// If TypeScript still complains and you must use a double assertion, be aware this bypasses type safety: | |
// const doc = await TestModel.create({ age: 'not a number', someOtherProperty: 'value' } as unknown as Partial<InferSchemaType<typeof schema>>); |
Copilot uses AI. Check for mistakes.
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.
I added support for array of arrays for maps and strings+numbers for dates. Also added Require_id. Does this help?
Yes the changes help to reduce the diff (still without trying to fix the subdocument type issue):
Typegoose DIFF
diff --git a/test/tests/arrayValidator.test.ts b/test/tests/arrayValidator.test.ts
index a7961c67..152e6573 100644
--- a/test/tests/arrayValidator.test.ts
+++ b/test/tests/arrayValidator.test.ts
@@ -68,7 +68,7 @@ it('should respect enum [String]', async () => {
enumedString: [
'not in the enum', // string not in the enum
],
- });
+ } as any);
fail('Expected to throw ValidationError!');
} catch (err) {
@@ -96,7 +96,7 @@ it('should respect enum [Number]', async () => {
enumedNumber: [
5, // number not in the enum
],
- });
+ } as any);
fail('Expected to throw ValidationError!');
} catch (err) {
diff --git a/test/tests/discriminators.test.ts b/test/tests/discriminators.test.ts
index 4a3d2614..84c479f8 100644
--- a/test/tests/discriminators.test.ts
+++ b/test/tests/discriminators.test.ts
@@ -171,7 +171,9 @@ it('should pass all mongoose discriminator tests', async () => {
// https://mongoosejs.com/docs/discriminators.html#using-discriminators-with-model-create
const events = await Promise.all([
- EventModel.create<ClickedLinkEvent>({ time: new Date(Date.now()), url: 'google.com' }),
+ EventModel.create(
+ /* <ClickedLinkEvent> */ { time: new Date(Date.now()), url: 'google.com' } as Partial<mongoose.InferSchemaType<ClickedLinkEvent>>
+ ),
ClickedLinkEventModel.create({ time: Date.now(), url: 'google.com' }),
SignedUpEventModel.create({ time: Date.now(), user: 'testuser' }),
]);
diff --git a/test/tests/numberValidator.test.ts b/test/tests/numberValidator.test.ts
index 7138b736..a482c1f5 100644
--- a/test/tests/numberValidator.test.ts
+++ b/test/tests/numberValidator.test.ts
@@ -23,7 +23,7 @@ it('should respect enum', async () => {
try {
await NumberValidatorsModel.create({
enumed: 5, // number not in the enum
- });
+ } as any);
fail('Expected to throw ValidationError!');
} catch (err) {
diff --git a/test/tests/shouldRun.test.ts b/test/tests/shouldRun.test.ts
index 48061907..ea6efa42 100644
--- a/test/tests/shouldRun.test.ts
+++ b/test/tests/shouldRun.test.ts
@@ -1147,7 +1147,7 @@ it('should validate Decimal128', async () => {
try {
await CarModel.create({
carModel: 'Tesla',
- price: 'NO DECIMAL',
+ price: 'NO DECIMAL' as any,
});
fail('Validation must fail.');
} catch (e) {
@@ -1181,7 +1181,7 @@ it(`should Validate Map`, async () => {
try {
await InternetUserModel.create({
projects: {
- p1: 'project',
+ p1: 'project' as any,
},
});
fail('Validation should Fail');
Though the ObjectId error remains:
test/tests/biguser.test.ts:27:7 - error TS2769: No overload matches this call.
Overload 1 of 4, '(docs: Partial<ApplyBasicCreateCasting<User & { _id: ObjectId; }>>[], options?: CreateOptions | undefined): Promise<...>', gave the following error.
Object literal may only specify known properties, and '_id' does not exist in type 'Partial<ApplyBasicCreateCasting<User & { _id: ObjectId; }>>[]'.
27 _id: new mongoose.Types.ObjectId(),
~~~
Fix #15355
Summary
Supporting
create<DocContents>(doc: DocContents)
means that you can pass any value todoc
due to TypeScript type inference. This PR removes that while retaining support for most common cases: passing strings for ObjectIds, POJOs for maps and subdocs, plain arrays for document arrays, strings for buffers and uuids. Also usesPartial<>
to allow excluding properties that may be set by middleware or defaults.This makes autocomplete for
create()
andinsertOne()
much better:There are some definite downsides to this change: custom getters/setters and custom casting logic can transform types, and there's no way to plug in custom types into
ApplyBasicCreateCasting
. But those are problems you can work around withas
.Examples