Question: Provide _id in create request?

[dvlsg]

Implementation question for you guys. We're building a product that will require client-server synchronization with offline capabilities. In order to do that, we need to be able to generate unique ids on the client side that we can be sure will be the same after they are sent to the server.

Is there a way to provide _id in the request body for creates without having them removed? I believe the lines here are undo-ing what we are trying to do.

We can sort of apply a work around by adding a schema like this:

const schema = new mongoose.Schema({	
  _id: { type: ObjectId, default: ObjectId },
}, {
  _id: false
});

This allows us to post in _id fields, but then whenever they aren't supplied (and they won't always be, in our case -- sometimes we want the server to generate the ids), the default doesn't seem to get triggered, and we get an error from mongoose. So that unfortunately doesn't work 100% for us. Feels a bit scary to supply an _id and then mark _id: false in the options, too.

I can confirm that commenting out the lines noted above works as expected, at least for supplying _id as an ObjectId and leaving it undefined.

Here are a few outcomes based on _id inputs when those lines are commented out:

Input for _id	Result
string representation of ObjectId	The provided ObjectId is used
undefined	An ObjectId is generated
null	The result has _id: null, but I believe mongoose still generates an _id and creates the record
''	"Cast to ObjectID failed for value \"\" at path \"_id\""

How do you feel about not deleting req.body._id when it is a valid ObjectId? That null thing seems like a problem, but everything else seems potentially reasonable.

[b-gran]

This is a feature I've had in mind too. There a few edge cases though.

1. mongoose allows you to redefine an _id property regardless of the value of the _id option
2. If you set the _id property equal to null and try to create a document, mongoose creates an _id automatically. Is this expected behavior?
   • How do you get the new _id?

Here's a test that documents all of this behavior:

const Promise = require('bluebird')
const mongoose = require('mongoose')
mongoose.connect('mongodb://localhost:27017/test-mongoose-02')

const SomeModel = mongoose.model(
    'SomeModel',
    new mongoose.Schema({
        _id: mongoose.Schema.Types.Mixed,
        foo: String,
    })
)

const Sub = mongoose.Schema({
    bar: String,
    _id: String
}, { _id: false })

const SubModel = mongoose.model('Sub', Sub)

const Other = mongoose.model(
    'Other',
    new mongoose.Schema({
        foo: Sub
    })
)

Promise.join(
    mongoose.models.Sub.create({ bar: 'bar', _id: 'id' }),
    mongoose.models.SomeModel.create({ _id: null }),
    mongoose.models.Other.create({
        _id: null,
        foo: {
            bar: 'whatever',
            _id: 'foo'
        }
    })
).then(results => {
    console.log('results', results)
    process.exit(0)
}).catch(err => {
    console.log('err', err)
    process.exit(1)
})

Here's what I'm thinking (for object creation via POST/PUT)

1. Allow consumers to specify an _id field.
2. If the field is non-null, leave it completely alone (even if undefined). Let Schema validation handle it.
3. If the field is null, remove it.

Then we document the "unset null _id" behavior.

@Zertz what do you think about this?

[Zertz]

I'd rather not tinker with _id. You can use another field such as clientId, cid, uid, etc. and apply necessary validation on that field. You can then specify said field in options.

const schema = new mongoose.Schema({
  // Apply the validation rules you need
  uid: { type: String, unique: true, isRequired: true, minlength: 32, maxlength: 32 }
})

erm.serve(mongoose.model('Hello', Hello), {
  idProperty: 'uid'
})

[dvlsg]

In my opinion, lines 140-142 of operations.js are already tinkering with the _id, and removing those lines would actually be less tinkering.

Unless I'm misunderstanding, and by not tinkering you mean you'd like to leave functionality as-is? Otherwise, what @b-gran is proposing would be perfect for our situation, at least.

Changing the idProperty is a bit late in the game for us, unfortunately. It would be a lot less work for us to either fork / download a copy of ERM and chop out / edit those 3 lines of code, which is always a bit of a bummer. Figured it was maybe worth bringing up the question here, though.

[b-gran]

I'm kind of on the fence about this.

On the one hand, messing around with _id is kind of sketchy since there are probably many edge cases.

On the other, we're already messing with _id by disallowing a perfectly legal operation: explicitly specifying the _id of a new document (especially since _id can be redefined).

How about adding an option? allowCustomId?

The option would be pretty straightforward:

if (!options.allowCustomId || body._id === null) {
  delete body._id
}

The null case is actually pretty narrow: you can't specify a null value from the request body (it doesn't survive type coercion -- just becomes the string "null"), so the only reason it would be null is if custom middleware changed it.

[wesratcliff]

@b-gran, +1, allowing _id to be passed in seems like a great solution to keep backwards compatibility while also allowing generation of _id externally.

[Zertz]

Indeed, that tinkering is an old bit of code that was kept for backward compatibility. Ideally we'd simply remove that bit of code, but I fear in this case it might be too much of a breaking change as it could surface weird bugs in user apps that depend on this behavior. Looking at it now, I'm not even sure what the purpose of that was.

That said, while I'm not a fan of piling up options, you brought up valid points so feel free to create a PR. Keep it backward compatible for now and perhaps add a warning when the option is off to warn the user that their _id is being removed and that this behavior will be removed in the next major version.

Basically, add the feature in v4.x and completely remove _id tinkering in v5.0

[b-gran]

I definitely agree, the fewer options the better.
So are you thinking we should add the option between now and v5, and then remove both the option and the _id tinkering in v5?

That seems like a reasonable path forward, we just need to make sure the docs track the addition and removal of the option.

[Zertz]

Correct.