Make writeConcern an optional named parameter for collection remove()

[Scorpiion]

It is possible to set writeConcern for the following methods on a collection object:

• insert
• insertAll
• save
• update
• remove

All but the remove method does this with an optional named parameter "writeConcern", only remove uses optional positional parameters.

I would like to propose that we change the writeConcern parameter for remove so that it is consistent with the other methods. What do you think @vadimtsushko? I don't know if you had some specific reason for this or if it's just how it became.

I know this would be a breaking change for users currently using the old style of the parameter, but I don't think it's that many that actually set it explicitly to be honest. I don't feel that this is a very urgent issue, but it would be good for the driver I think the more consistent it can be.

If you like the change, we could maybe add a version label (future mongo_dart version label) to this issue so it get implemented for the next bigger (and/or) breaking release. (Note: I can't add labels)

Edit: Replaced "delete" with "remove", was wrong to begin with

[vadimtsushko]

You are right, I'll change it to one pattern (named parameter).
Actually next pub version will be marked as incompatible (git version already have tag 0.2.1-dev)
I should document backward incompatible changes in API before that though.

[Scorpiion]

That sounds good! :)

[vadimtsushko]

Hi Robert.
I've looked at remove again and actually realized why it is different from all other updating methods.
In remove whe now have optional positional query parameter.
AFAIK we cannot have both named and optional positional parameters (I recall some discussions for loosening that rule)
I could make a query parameter mandatory in remove, but it looked as removing all data from a collection is frequent operation (in tests anyway) and it is not consistent with optionality of this parameter in find and findOne
Named optional query parameter would be also inconsistent with find and findOne
So I've decided that inconsistency in writeConcern is lesser evil in that case.
What do you think?

[Scorpiion]

Hi Vadim, okay, then I understand the origin of this. Yes that is correct, you can't use both optional and positional parameters at the same time unfortunately.

I think you should make the remove selector mandatory, I don't know if you know this or not but it's easy to remove all documents by sending in an empty query like this to remove {} (or just drop the whole collection). Doing that would be consistent with how the Mongo CLI works, look at this example (you can test the same commands yourself):

// Proving that the coll is empty to begin with
> db.test_coll.find()

// Add three docs
> db.test_coll.insert({"name": "Doc1"})
WriteResult({ "nInserted" : 1 })
> db.test_coll.insert({"name": "Doc2"})
WriteResult({ "nInserted" : 1 })
> db.test_coll.insert({"name": "Doc3"})
WriteResult({ "nInserted" : 1 })

// Use find without any selector, works
> db.test_coll.find()
{ "_id" : ObjectId("55ec6911ea03d8b826b4ee36"), "name" : "Doc1" }
{ "_id" : ObjectId("55ec6913ea03d8b826b4ee37"), "name" : "Doc2" }
{ "_id" : ObjectId("55ec6916ea03d8b826b4ee38"), "name" : "Doc3" }

// Use findOne without any selector, works
> db.test_coll.findOne()
{ "_id" : ObjectId("55ec6911ea03d8b826b4ee36"), "name" : "Doc1" }

// Use find with selector, works
> db.test_coll.find({"name": "Doc1"})
{ "_id" : ObjectId("55ec6911ea03d8b826b4ee36"), "name" : "Doc1" }

// Use findOne with selector, works
> db.test_coll.findOne({"name": "Doc1"})
{ "_id" : ObjectId("55ec6911ea03d8b826b4ee36"), "name" : "Doc1" }

// Use remove without any selector, error, a query is needed...
> db.test_coll.remove()
2015-09-06T18:26:46.373+0200 E QUERY    Error: remove needs a query
    at Error (<anonymous>)
    at DBCollection._parseRemove (src/mongo/shell/collection.js:305:32)
    at DBCollection.remove (src/mongo/shell/collection.js:328:23)
    at (shell):1:14 at src/mongo/shell/collection.js:305

// Use remove with selector for one single document, works
> db.test_coll.remove({"name": "Doc1"})
WriteResult({ "nRemoved" : 1 })

// Use remove with selector to remove all documents, works 
> db.test_coll.remove({})
WriteResult({ "nRemoved" : 2 })

As you can see in the command line interface both find and findOne have the query/selector as optional (same as mongo_dart) while remove has it mandatory (different from mongo_dart today). I thinking following that example would be the right thing to do, then the writeConcern can also be consistent with the other methods.

[Scorpiion]

In terms of consistency I think remove actually "belongs" with these commands:

Methods using {WriteConcern writeConcern}:

• insert
• insertAll
• save
• update

They all do changes to the collection, they alter state in some way.

The methods you mentioned using the pattern [selector]:

• find
• findOne

Are different because they don't change any state. I think it's more consistent for remove to use the same patterns as the other methods that change state. Although I think following the same pattern as the CLI commands is an even better argument.

[vadimtsushko]

Robert, I've just read your last posts and, well, you are absolutely right.
Thank you very much for these explorations. That seems to be pretty obvious retrospectively
but someone would have to do the actual work to clear this up,
Thanks you again.
As for a concrete plan to realize this change:
I believe it is a breaking change, so usual precautions should be observed (version bump and
some announcements of it)
But I guess it is a much more frequent case for backward incompability then my last (and first) breaking change in mongo_dart. I guess much more remove() without parameter then find().stream is now in the wild.
Should we do any additional steps to alleviate the pain?

[Scorpiion]

No problem, I think it's great that we can discuss these things here like this.

Yes it's breaking indeed, I'm actually not sure how often it's used but yeah probably more often than find().stream. I think you did a good job on this previous change in terms of communication.

I don't have the time right now to do this, but if you want I could maybe take a look at it in the future, can't say a specific date though. I'm quite busy at work at the moment.

In terms of alleviating pain I think it's quite simple to tell people to replace any coll.remove() calls with coll.remove({}), a short explanation of how to update the method call I think would be good. Maybe it could be a good idea to also add an example to the comments on "remove" of how to remove all documents (so it shows up in the docs). Similar examples could also be added to find() and other as well to be honest, making it easier to newcomers.

Maybe we could add a label called "0.3.0" to this issue, it's quite common in other Github repos to group issues based on versions I think.

[vadimtsushko]

That's good plan I think.
Added 0.3.0 milestone to this issue. Just today I've updated package README with some basic usage cases. So in 0.3.0 version they can be updated.
All in all I think that should do

[Scorpiion]

Agreed, sounds like a good plan! 👍

[thosakwe]

I think that we can go ahead and make this change. Something along the lines of:

Future remove(SelectorBuilder selector, {WriteConcern writeConcern});

[vadimtsushko]

LGTM :)

[thosakwe]

Sorry, got sick, and was also busy with school... Will drop in a PR today, plus tests.

[vadimtsushko]

Great 👍

[thosakwe]

Wasn't able to get that in this week, still was busy.

However, I'm heading on a 7-hour bus ride back home tomorrow, so I anticipate to have time to get this sent in.

[thosakwe]

Resolved via 6b87d8d.