Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Collection.prototype.remove throws an error when trying to remove all records #1662

Closed
wants to merge 1 commit into from
Closed

Collection.prototype.remove throws an error when trying to remove all records #1662

wants to merge 1 commit into from

Conversation

FagnerMartinsBrack
Copy link

@FagnerMartinsBrack FagnerMartinsBrack commented Feb 4, 2018

When trying to remove all the records from the database:

MongoClient.connect('mongodb://localhost:27017/').then((client) => {
    client
      .db('db-name')
      .collection('collection-name')
      .remove({}).then(() => {
        done();
      });
    });

the lib throws an error:

TypeError: Cannot convert undefined or null to object
        at Function.assign (<anonymous>)

This looks to be the offending code in utils.js:

  // The driver sessions spec mandates that we implicitly create sessions for operations
  // that are not explicitly provided with a session.
  let session, opOptions;
  if (!options.skipSessions && topology.hasSessionSupport()) {
    opOptions = args[args.length - 2];
    if (opOptions == null || opOptions.session == null) {
      session = topology.startSession();
      Object.assign(args[args.length - 2], { session: session });
    } else if (opOptions.session && opOptions.session.hasEnded) {
      throw new MongoError('Use of expired sessions is not permitted');
    }
  }

Source 1, Source 2

As a sidenote: I couldn't run the tests locally, sorry if the test of this PR is not working (mostly copy/paste from the previous one). I tried to clone and run npm test (as read from package.json test script) on my machine using node v4.8.7 (as read from package.json engines property). However, the console got stuck on this:

➜  node-mongodb-native git:(3.0.0) ✗ npm test
> mongodb@3.0.2 test /Users/fagnerbrack/Git/node-mongodb-native
> npm run lint && mongodb-test-runner -t 60000 test/unit test/functional
> mongodb@3.0.2 lint /Users/fagnerbrack/Git/node-mongodb-native
> eslint lib test
[environment: single]
^C

I don't know what "[environment: single]" output means and why it's getting stuck there...

@mbroadst
Copy link
Member

mbroadst commented Feb 4, 2018

@FagnerMartinsBrack hmm, the submitted test appears to be running fine on travis proving that this is in fact working, at least for single topologies. Can you confirm whether you are running against a single server or e.g. a replica set?

@FagnerMartinsBrack
Copy link
Author

@mbroadst That's weird, I'm probably missing something. Either the test is not reproducing the right case or there's some side-effect in my code. I'll take a look again.

However, in order to make sure I can have a quick feedback locally, can you help me out with that [environment: single] thing getting stuck? Is there a documentation on how to run the tests (or a single test) quickly?

I guess it would make it easier for future contributors too.

@FagnerMartinsBrack
Copy link
Author

For some reason it's running now... Go figure...
I'll check that out.

@FagnerMartinsBrack
Copy link
Author

FagnerMartinsBrack commented Feb 5, 2018

haha found it!

The issue is present on version 3.0.2 but is not present on master, that's why the test works and fails in my local machine (I'm using npm latest release 3.0.2)! This was fixed by @daprahamian on 4c9b0f8 (PR #1657).

I can reproduce the error with the test on this PR running on top of 3ac365. Try to git reset --hard 3ac365 && git cherry-pick 5ec6d9, you'll probably be able to reproduce too!

In node 4 (which is the one I used), the promise hangs until it reaches the timeout (see screenshot):

screen shot 2018-02-05 at 9 36 06 pm

What a coincidence, next release will get this in! 😅😅😅

@mbroadst when is next release date on npm 😬?

@FagnerMartinsBrack
Copy link
Author

I'll be using master in the mean time...

@mbroadst
Copy link
Member

mbroadst commented Feb 5, 2018

@FagnerMartinsBrack I anticipate a patch release by end of week, or next week at latest. We are working through a few other small issues at the moment. Thanks for your patience! In the meantime I am going to close this PR

@mbroadst mbroadst closed this Feb 5, 2018
@FagnerMartinsBrack FagnerMartinsBrack deleted the error-remove branch February 6, 2018 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants