Skip to content
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

Fix: insert methods removing _ids #55

Merged
merged 6 commits into from
Nov 20, 2024

Conversation

cau777
Copy link
Contributor

@cau777 cau777 commented Nov 20, 2024

The type system allows passing _id to insertOne and insertMany even if _id is not in the Zod schema. However, our validation strips _id out of the calls, which is unexpected.

Before modifying preInsert:
image

After:
image

Comment on lines 29 to 45
const id = new ObjectId()
const { insertedId } = await zodCol.insertOne({
a: 0,
b: 'string',
numbers: [0],
_id: id,
})
expect(insertedId.equals(id)).toBeTruthy()

expect(() =>
zodCol.insertOne({ a: 'string', b: 'string', numbers: [0], _id: null })
).toThrow()

expect(() =>
zodCol.insertOne({ a: 'string', b: 'string', numbers: [0], _id: 'idStr' })
).toThrow()

Copy link
Collaborator

@nicolassanmar nicolassanmar Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be more clear in a separate test "Should allow inserting documents with _id even if not in schema"

a: 0,
b: 'string',
numbers: [0],
_id: id,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also assert that a wrong input fails here both type wise and at runtime.
We can add a test where we pass a string to the _id field which can be a common mistake

_id: z
// Can't be instanceof(ObjectId) because the instanceof operator
// doesn't work correctly when ts-mongo is imported by another lib
// This is similar to what we do with zod instance checking
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to what we do with zod instance checking, where?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, in this zod fork that is maintained in my GitHub account: cau777/zod@ee7f929

We use it instead of the official version of zod: https://github.com/cau777/ts-mongo/blob/29ca813679e62731df514b97f91d18d773155930/package.json#L51-L51

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

test('Documents should be parsed correctly on insert', async () => {
const zodCol = await mkZodCollection()

expect(() =>
// @ts-expect-error
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 this is better than using any

Comment on lines 28 to 31
expect(() => zodCol.insertMany([{ a: 0, b: 0 }])).toThrow()
await expect(
zodCol.insertMany([{ a: 0, b: 'string', numbers: [7] }])
).resolves.toBeTruthy()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should keep / add a test that uses insertMany just to ensure the validation works there as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for noticing! Removing this was a mistake

@nicolassanmar nicolassanmar merged commit ca98083 into tianhuil:main Nov 20, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants