Skip to content

<fix>: _id may not be required in create operation #3100

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 1 commit into from

Conversation

k8w
Copy link

@k8w k8w commented Jan 11, 2022

Description

When schema includes _id: ObjectId, insertOne should not force _id to be a required property.

What is changing?

Let _id is still optional in insertOne when _id: ObjectId in schema

Is there new documentation needed for these changes?

What is the motivation for this change?

export interface Test {
  _id: ObjectId,
  data: string
}

db.collection<Test>('Test').insertOne({data: 'XXXX'})

Expected
Compiled successfully.

Actual
Got error, because missing _id

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

## Issue
When schema includes `_id: ObjectId`, `insertOne` should not force `_id` to be a required property.

## Example

```ts
export interface Test {
  _id: ObjectId,
  data: string
}

db.collection<Test>('Test').insertOne({data: 'XXXX'})
```

### Expected
Compiled successfully.

### Actual
Got error, because missing `_id`

This PR let `_id` is still optional when `_id: ObjectId` in schema
@dariakp
Copy link
Contributor

dariakp commented Jan 11, 2022

Hi @k8w, thank you for reaching out. The intended relationship between the schema and the _id in the typings is as follows:

  • If no _id is specified on the schema, it is inferred to be of type ObjectId and is optional on inserts.
  • If an _id is specified on the schema as required, then the _id type is inferred to be of the specified type and is required on inserts.
  • If an _id is specified on the schema as optional, it is inferred to be of the specified type and is optional on inserts: this format is intended to be used with the pkFactory option in order to ensure a consistent _id is assigned to every new document.

We made this choice in order to provide a consistent experience for the different use cases our users have for the collection schema specified at the driver level and the behavior expectations that come with those schemas.

The driver has to rely on the input schema to provide its own type enforcement, where the difference between optional and required properties of the driver-side collection schema are meaningful on document insertion and document retrieval.

Our APIs ensure that the return type of operations such as findOne always contains a required _id field, regardless of whether it’s explicitly defined as optional or required on the schema, since all MongoDB documents have one. (This is done via our publicly available WithId<TSchema> helper, which transforms any schema type into one with a required _id of the inferred type.)

On the other hand, on inserts, we have to guess the use case based on the schema, keeping in mind that the _id can be omitted entirely, provided explicitly, or provided indirectly via a pkFactory option. In order to accommodate users who provide the _id explicitly and definitely do want the type to be strictly enforced on inserts, we can’t make the _id always optional, which means we have to differentiate between optional and required schema _id in general.

Making an exception for just ObjectId would introduce an unncessary inconsistency in how we interpret the schema type (after all, the only difference between an ObjectId and a non-ObjectId is that ObjectId happens to be the default pkFactory); the benefit would be providing an additional way of specifying an optional-on-insert _id at the cost of removing the only way of specifying it as required. In contrast, our approach makes the _id more consistent with how the other properties in the schema are interpreted on inserts and allows users to opt into or out of _id requirement based on each unique use case.

For your use case, if the collection type is something that is used independently in other parts of the code base as a document model, we recommend simply wrapping it in our OptionalId helper type when passing it at the collection creation point: db.collection<OptionalId<Test>>; alternatively, you could specify the collection interface with an optional or omitted _id property and use the WithId wrapper to access the type elsewhere, e.g., for testing.

We are aware that this may be different from the intuitive notion of a collection document type; unfortunately, since it is impossible to reverse engineer the desired insert shape from the final document shape, only vice versa, we cannot use the final shape of the document to fully define the collection for all driver operations.

We appreciate your understanding and intend to add this guidance to our official documentation soon.

@k8w
Copy link
Author

k8w commented Jan 12, 2022

Thanks~

db.collection<OptionalId<Test>> works well

@k8w k8w closed this Jan 12, 2022
@alias-mac
Copy link

@dariakp thanks for the details provided in your comment above. Do you know when we will have the fix from dd5195a published?
Asking because this is breaking types on some of the libraries I have.

Thanks!

@dariakp
Copy link
Contributor

dariakp commented Jan 18, 2022

@alias-mac We're looking at putting out the 4.3.1 patch today!

@justinmchase
Copy link

justinmchase commented Jul 8, 2022

db.collection<OptionalId<Test>> is incorrect, the original issue is still applicable.

The problem is _id is not optional on the Schema but it is optional on the insert argument. The types do not accurately reflect this and this issue should still be open.

@dariakp
Copy link
Contributor

dariakp commented Jul 8, 2022

@justinmchase I am sorry that this is causing friction for your use case. However, we would not have made this change if we did not have real customers with real code bases that needed their use cases enabled.

To address your specific concern though, I'm not sure what you mean that the types do not accurately reflect the reality in the case of specifying your collection type with an OptionalId. Regardless of whether you mark the id as optional or required in the collection schema type, it will ALWAYS be returned as required (i.e., guaranteed to be present and not optional) in any read operation by default. There is no inconsistency, since the collection schema for the driver specifies the insert shape of the document, which is then correctly transformed in the input and output types of the various driver methods as needed.

If your assertion is that the _id should always be optional on inserts because it's optional server-side on inserts, it is worth remembering that all fields are optional server-side on inserts unless you also use the same schema to configure server-side validation. And taking that approach would unfortunately leave our users who rely on a custom _id with no recourse for type safety where forgetting to specify an _id would result in data inconsistency. So, as I mentioned in my comment above,

on inserts, we have to guess the use case based on the schema, keeping in mind that the _id can be omitted entirely, provided explicitly, or provided indirectly via a pkFactory option. In order to accommodate users who provide the _id explicitly and definitely do want the type to be strictly enforced on inserts, we can’t make the _id always optional, which means we have to differentiate between optional and required schema _id in general.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants