Incorrect behavior in a shard cluster

[sarkistlt]

Currently .remove method always uses findAndModify. It should use deleteOne if ctx.id is present.

Main problem right now it's not possible to remove document by _id without passing shard key, because .remove uses findAndModify, if it would use deleteOne you'll be able to remove document just by _id, without passing extra query with sharding key.

This is my current workaround, in before remove hook:

if (
    ctx.id
    && ctx.service
    && ctx.service.Model
  ) {
    await ctx.service.Model.deleteOne({ ...(ctx.params.query || {}), _id: ctx.id });
    ctx.result = { _id: ctx.id };
  }

[marshallswain]

@sarkistlt would this break any functionality that you're aware of?

[sarkistlt]

@marshallswain currently I'm manually patching service.js after installing all packages, so far the only drawback I see is, current snippet from _remove:

return this.Model.findOneAndDelete(query, params.mongoose).lean(this.lean)
        .exec().then(result => {
          if (result === null) {
            throw new errors.NotFound(`No record found for id '${id}'`, query);
          }

          return result;
        }).then(select(params, this.id)).catch(errorHandler);

has to be replaced with 2 calls instead of one, to maintain current behavior in after remove hooks:

return this.Model.findById(id)
        .then((result) => {
          if (result === null) {
            throw new errors.NotFound(`No record found for id '${id}'`, query);
          }
          return this.Model.deleteOne(query, params.mongoose).lean(this.lean)
            .exec()
            .then(() => result)
            .then(select(params, this.id));
        })
        .catch(errorHandler);

But it's still better than providing shard key to remove object.
I'm running ~200 services in my API after migrating to shard cluster with current findOneAndDelete it broke all remove requests, and it doesn't make seance to pass shard key to remove object, sometime you may not have shard keys, only ID which should be enough to remove the object.

[marshallswain]

This makes complete sense to me. I completely agree about not having to pass a shard key. The only thing that could make that API worse is having to pass your social security number. ;)

It seems reasonable to me to have deleteOne become the default behavior. We could also go the route of supporting both and having an option to do one or the other. I don't really feel like it's necessary, though. Care to make a PR?

[sarkistlt]

@marshallswain thanks! Sure will do it later today as soon as I'll get to my laptop.

[marshallswain]

@sarkistlt should this be closed now that #364 is merged?

[sarkistlt]

@marshallswain yes, thank you!