🐛 Fix using the shared session when updating a document

[tiangolo]

🐛 Fix using the shared session when updating a document

Currently, the code creates a session and transaction and passes down the session in the parameters through all the function calls, but it's not passed in the last point, which would make motor use the session.

The original intention of this PR was just to pass that session, a one-line/argument change, but I found a couple of bugs and caveats along the way and the PR got bigger.

~I'm not sure I should add tests for this, I was trying to add them but I saw that I would have to add a bunch of mocks for the internals, or add strange tricks to break the save in the middle and see that the session didn't finish... but then any of those things seemed like a very complex trick to enable a test that would mostly test the trick, and not really the session. Not sure if I should do anything else. Let me know!~

Edit 2022-06-23 Transactions in Standalone

I see that transactions are only supported in MongoDB clusters with shards or replicas. And there's no simple way to detect the type of cluster in code. When using a transaction on a standalone MongoDB I get this error:

pymongo.errors.OperationFailure: Transaction numbers are only allowed on a replica set member or mongos, full error: {'ok': 0.0, 'errmsg': 'Transaction numbers are only allowed on a replica set member or mongos', 'code': 20, 'codeName': 'IllegalOperation'}

That error is currently not being thrown because sessions are currently not used (which is what this intends to fix).

Detecting if the current cluster is replicated, sharded, or standalone (unsupported) would require a lot of complex and fragile logic, as even though transactions are not supported in standalone, there's no way to ask the cluster what's the current type of deployment.

To solve that, I moved the transactions away from the internal code (they were not used anyway), and added support for passing a session to engine.save() and engine.save_all(), this way, a user could create a session outside, create a transaction (after confirming they have a supported deployment) and then pass the session to these methods.

Edit 2022-06-23 B Same session concurrently

I'm seeing that the same session is not expected to be used concurrently 😔

https://motor.readthedocs.io/en/3.0.0/api-asyncio/asyncio_motor_client.html#motor.motor_asyncio.AsyncIOMotorClient.start_session

Do not use the same session for multiple operations concurrently.

So using asyncio.gather() should not be used as the document saves are expected to share the same session.

Edit 2022-06-24 Tests for transactions

I added a couple of tests to confirm that external transactions work with engine.save() and engine.save_all(). Because transactions require a cluster with replicas, those tests are only run on the replica version.

[codecov[bot]]

Codecov Report

Merging #227 (5ff2c9c) into master (35f7be9) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #227   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           38        38           
  Lines         2725      2749   +24     
  Branches       413       416    +3     
=========================================
+ Hits          2725      2749   +24     

Flag	Coverage Δ
tests-3.6-4-standalone	98.82% <35.13%> (-0.88%)	:arrow_down:
tests-3.6-4.2-standalone	98.82% <35.13%> (-0.88%)	:arrow_down:
tests-3.6-4.4-standalone	98.82% <35.13%> (-0.88%)	:arrow_down:
tests-3.7-4-standalone	98.64% <35.13%> (-0.88%)	:arrow_down:
tests-3.7-4.2-standalone	98.64% <35.13%> (-0.88%)	:arrow_down:
tests-3.7-4.4-standalone	98.64% <35.13%> (-0.88%)	:arrow_down:
tests-3.8-4-replicaSet	97.96% <100.00%> (+0.01%)	:arrow_up:
tests-3.8-4-standalone	98.58% <35.13%> (-0.87%)	:arrow_down:
tests-3.8-4.2-sharded	97.08% <35.13%> (-0.86%)	:arrow_down:
tests-3.8-4.2-standalone	98.58% <35.13%> (-0.87%)	:arrow_down:
tests-3.8-4.4-standalone	98.58% <35.13%> (-0.87%)	:arrow_down:
tests-3.9-4-standalone	98.43% <35.13%> (-0.87%)	:arrow_down:
tests-3.9-4.2-standalone	98.43% <35.13%> (-0.87%)	:arrow_down:
tests-3.9-4.4-standalone	98.43% <35.13%> (-0.87%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
odmantic/engine.py	100.00% <100.00%> (ø)
tests/integration/test_engine_reference.py	100.00% <100.00%> (ø)

:mega: Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[art049]

Hey thanks for the PR!

@Olegt0rr started to work something out a while ago in https://github.com/art049/odmantic/pull/89, I was planning on merging it but as you mentioned the outcome with automatic server type detection makes the whole thing a bit flaky depending on the versions. Probably it would be wiser to postpone this automatic detection to later including maybe a higher-level session object which would be more developer-friendly.

So using asyncio.gather() should not be used as the document saves are expected to share the same session.

Very interesting finding, I wasn't aware of this at all. I really wonder about the performance outcome of stripping away the gather. Anyway since it's in the docs there isn't much we can do without changing too many things.

To solve that, I moved the transactions away from the internal code (they were not used anyway), and added support for passing a session to engine.save() and engine.save_all(), this way, a user could create a session outside, create a transaction (after confirming they have a supported deployment) and then pass the session to these methods.

LGTM, anyway it's nice to have to be able to handle sessions/transactions outside and have some flexibility.

I think we just lack a tiny bit of docs, probably "Working with transactions" in /docs/engine.md with a small example, and then we're good to go! Thanks for the work :)

[Olegt0rr]

I'm glad to see that project is not abandoned :) Thanks for your work

[tiangolo]

Thanks for the feedback @art049 and @Olegt0rr ! :rocket:

And thanks for all the points and ideas @art049. 🤓

About removing gather and the possible performance implications, one of my next plans is to try and implement MongoDB's bulk writes. It's a bit complex because those are per collection, so the documents to save have to be put together first by collection and then saved in bulk per collection. But I expect that should give much better performance, (even than gather if it was supported).

I just added a couple of examples to the docs! :memo: One just with a session, and one with a session and a transaction. :heavy_check_mark:

[art049]

Awesome @tiangolo, thank you! Can't wait to see the bulk writes, for sure this will improve the overall ODM performances!