Fix aggregate with Mongo 3.6

[trenton42]

Mongo 3.6 requires either an explain or a cursor option with the aggregate command. This update adds a cursor option with a default initial batch size of 0 (which allows the cursor to quickly return without doing work in case of an error -- see Mongo docs

Without this, any aggregate query will fail in Mongo 3.6+

This patch gets it back running again, but does not expose the cursor option to the calling command. Should it provide a default and also allow that option to be set? The cursor option is a dictionary, but looking at the docs, it appears that it only has one key -- batchSize, so I would think we could just expose initial_batch_size as a keyword argument, and set it to some reasonable default.

[trenton42]

Also, I did a little PEP8 cleanup on the module, but put that in a separate commit so it would be clear what changes were actual changes and what changes were just cleanup.

[trenton42]

I spoke too soon, it looks like this is going to have to be a little more complicated.

[psi29a]

Just a heads up, for in the future, no PEP8 cleanups in bug-fixes and features, that makes a mess of things when reviewing. If you would like do a PEP8 clean, awesome! Please do! But make sure that that is the only thing that is done in a PR. :) Does that make sense?

[trenton42]

That makes perfect sense. I should have thought about that 😞

[coveralls]

Coverage decreased (-0.1%) to 95.152% when pulling d39fab6 on trenton42:fix-aggregate into 85a7ac7 on twisted:master.

[coveralls]

Coverage decreased (-0.1%) to 95.152% when pulling d39fab6 on trenton42:fix-aggregate into 85a7ac7 on twisted:master.

[coveralls]

Coverage increased (+0.03%) to 95.319% when pulling 9e4dea7 on trenton42:fix-aggregate into 85a7ac7 on twisted:master.

[coveralls]

Coverage decreased (-0.1%) to 95.152% when pulling d39fab6 on trenton42:fix-aggregate into 85a7ac7 on twisted:master.

[trenton42]

...setup errors in Travis... 😩

Also, wow tests take a long time to run

[psi29a]

Re-triggering the 3 failed builds as they were network issues.

@IlyaSkriblovsky what do think of this PR? :)

[psi29a]

@trenton42 can you write a test that could trigger this code path please? :)

[IlyaSkriblovsky]

Thanks for your finding and fix!

1. I would think we could just expose initial_batch_size as a keyword argument, and set it to some reasonable default

Yes, I think this is reasonable. May be to have None as default value which won't set batchSize at all?

2. on_ok seems to be complex enough to be carefully tested. Could you please add some test that checks running aggregate returning results in multiple batches? May be set batchSize == 2 or test on huge objects ({'x': 'y'*10*1024*1024})? Is it possible to test it on MongoDB 3.4 which is used by our Travis environment for now?

3. full_response argument seems to be incompatible with multi-batch response. It is okay, but it is silently incompatible... May be throw an exception if raw["cursor"]["id"] != 0 and full_response?

4. Seems we need to configure CI to test on various MongoDB versions...

[trenton42]

@IlyaSkriblovsky Thank you for the feedback!

1. I will put that in and add tests
2. Will do! The cursor option has been available for a long time in Mongo (I think it was introduced in 2.6). It was just made mandatory in 3.6, which is why this breaks. I think it would be good to test against multiple mongo versions, but poor Travis already has a hard time keeping up 😞
3. I thought it worked to include full_response with a cursor, but I will look over it again. My intent was to remain backwards compatible (if someone was using full_response, they would be expecting the results to be under the result key of the returned dictionary)

As I was looking through this code, I noticed that getMore requests seem to be handled in different ways through out the module. It might be a good target for a refactor.

[IlyaSkriblovsky]

It seems to me that full_response originally was a bad idea... But it would be cool to maintain backward compatibility with it it it is not worth too much.

As I was looking through this code, I noticed that getMore requests seem to be handled in different ways through out the module. It might be a good target for a refactor.

There are many places in txmongo which still use old-style binary requests. I'm not sure we are using new command-style getMore requests anywhere...

[trenton42]

@IlyaSkriblovsky tests and options updated ✅

Let me know if you think that covers everything. I have run the tests against my own 3.6 instance as well.

[IlyaSkriblovsky]

Looks great, thank you!

[psi29a]

\o/
One last request, if you would be so kind as to update the changelog with a entry of what was fixed?
https://github.com/twisted/txmongo/blob/master/docs/source/NEWS.rst
With the header:
Release 18.1.0 (UNRELEASED)

Cheers!

[trenton42]

@psi29a done!

[psi29a]

Merging! Thank you!