[SERVER-42597] db.collection.remove('') removes all documents Created: 02/Aug/19 Updated: 06/Dec/22 Resolved: 03/Dec/21 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Shell |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Improvement | Priority: | Minor - P4 |
| Reporter: | Gar Higgins | Assignee: | Backlog - Server Tooling and Methods (STM) (Inactive) |
| Resolution: | Won't Fix | Votes: | 0 |
| Labels: | move-stm | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Assigned Teams: |
Server Tooling & Methods
|
| Participants: |
| Description |
|
Running any of the following command removed all records from the table:
I would've expected these to both be rejected. I can't find anything in the documentation of collection.remove() that would indicate this is intended behavior. I verified this behavior using mongo shell v4.0.4 and mongodb v4.0.4. |
| Comments |
| Comment by Brooke Miller [ 03/Dec/21 ] | |||||||||||||||||||||
|
We've deprecated the mongo shell in favor of the new mongosh. Unfortunately, we aren't able to pursue improvements to the deprecated shell except in extreme cases, such as critical security fixes. Please start making use of mongosh and let us know if it works for you in this case. | |||||||||||||||||||||
| Comment by Gar Higgins [ 05/Aug/19 ] | |||||||||||||||||||||
|
That sounds super reasonable. One last thought for consideration - while not documenting this style of query building is a good way to discourage folks from using it, it also leaves the opportunity for someone like myself to cause a lot of damage inadvertently because the behavior isn't documented. A warning specifically on the db.collection.remove() documentation might have saved me, even if the documentation just warned that such usage is bad practice because it's dangerous. | |||||||||||||||||||||
| Comment by Danny Hatcher (Inactive) [ 05/Aug/19 ] | |||||||||||||||||||||
|
It's not in our formal documentation as its not the way we want our users to utilize the shell. Ideally, everyone should be using the traditional brace syntax. However, you can see in our codebase exactly where this scenario kicked in. If the shell receives a string of 24 hex characters, it assumes that it is being passed an ObjectId and treats the string as such. However, since it was provided a random string, it evaluated as {$where: q}. If the shell is provided the full ObjectId, it always evaluate that ObjectId as _id. So if there was an accident where the shell received:
it would have returned "invalid object id". All that being said, it is always safest to use our precise syntax of db.collection.remove({_id:ObjectId("5d4452892c2a37870eebfba4")}) especially when you are intending to remove documents to avoid any possible ambiguity in the command. I do think this code is working as designed. However, I'll pass this on to the appropriate team to make the final decision on whether or not to consider a change. | |||||||||||||||||||||
| Comment by Gar Higgins [ 02/Aug/19 ] | |||||||||||||||||||||
|
Thanks for this explanation of the query interpreter! I actually had no idea that a string passed as the query document would be interpreted as javascript. Is that documented somewhere? I've also never found documentation that passing the ObjectId as a string also matches - is that somewhere in the docs? To your last question - it did actually cause a serious issue in our case. I deleted all the rows out of one table of our production db At the time, I was trying to create an index on the table, and a few rows had values that were too large. And so each time the index build failed, it would report the ObjectId of the row with the large value, and I would run: Except that in one case, I grabbed the `keyId` and ran: > db.collection.remove('6717326742115057665') I didn't realize how dangerous my workflow was, I was assuming that a typo would fail to match an ObjectId and result in an error. As I'm writing the post-mortem for this incident, I don't have a lot of remedies that would prevent us from running into this issue again - they're mostly in the vein of "be really careful with db.collection.remove()" or "use a script instead of the mongo shell to remove data" - which doesn't give me a lot of confidence that I won't ever do this again Thanks again for looking in to this! It's likely Working as Intended, but I suspect a lot of people would be surprised by this behavior. | |||||||||||||||||||||
| Comment by Danny Hatcher (Inactive) [ 02/Aug/19 ] | |||||||||||||||||||||
|
This is a function of how our Javascript interpreter works. Empty quotes are treated as an empty filter. You can see this in the "filter" section of the following logs:
The string of numbers gets interpreted as a $where which is an operator that explicitly evaluates Javascript. As the expression passed to Javascript is simply a random string, it also returns all the documents:
While not documented as this is not the way we typically want people to use the mongo shell, it does appear to be working as designed. Is this something that has caused you serious issues or is it simply something you noticed and wanted clarification on? |