SCAL-104: Implement db transactions in mongoengine #4

jonasanso · 2021-03-16T19:18:24Z

No description provided.

jonasanso · 2021-03-17T16:52:00Z

.github/workflows/github-actions.yml

@@ -11,17 +11,12 @@ on:
    tags:
      - 'v[0-9]+\.[0-9]+\.[0-9]+*'
 env:


Removed as we do not need to support old version of pymongo nor mongoDB and it would get more complicated to do so. We can do that later

It seems likely that upstream would want these cases handled before merging back in, but if this PR is just for our use then I don't see a problem with it for now.

Yes, I do not think we will be able to just take this PR and merge it in mongoengine, there are a lot of small details in the support for transactions in the different versions of the DBs.
Of course, they could also decide to increase the major version and say, it works only in MongoDB 4.4+ but I do not think that will be the case

jonasanso · 2021-03-17T16:52:42Z

mongoengine/base/fields.py

@@ -506,7 +506,7 @@ def to_mongo(self, value):
    def prepare_query_value(self, op, value):
        return self.to_mongo(value)

-    def validate(self, value):
+    def validate(self, value, clean=True):


warnings of my IDE, using the same function signature as in super

jonasanso · 2021-03-17T16:52:57Z

mongoengine/context_managers.py

@@ -18,6 +21,49 @@
 )


+class run_in_transaction(contextlib.ContextDecorator, contextlib.ExitStack):


The context manager and decorator

jonasanso · 2021-03-17T16:53:49Z

mongoengine/context_managers.py

+        """
+        super().__enter__()
+        self.conn = get_connection(self.db_alias)
+        self.session = self.enter_context(self.conn.start_session())


For now I am not testing or worrying about nested transactions or reuse of the session

As I am using the pymongo context manager for start_session I believe every session is closed

jonasanso · 2021-03-17T16:56:21Z

mongoengine/sessions.py

+
+from mongoengine.connection import DEFAULT_CONNECTION_NAME, get_connection
+
+sessions = local()


The code to handle the sessions, I might clean it up and I accept suggestions about alternatives to this global

I think it is a poor idea to create this globally. Modules shouldnt really have side-effects on import and globals are discouraged.

I would prefer a thread_local_session class which creates a local() and binds a new session to it when asked, then has the free functions below as methods.

I am unsure quite how we'd pass existing sessions down to nested transactions, but there is probably a way to do that.

That is the thing I was not able to do, I want to have something that I can access with an import, if not I need to store an instance somewhere :)
A singleton would also be nice, but I quite do not know how to do it.
I will investigate more about it

…threading local to store the sessions

Reset fork to v.23 and fix validation of inc and dec for an IntField with min_value

…db-transactions

clembrival · 2021-03-22T10:32:46Z

mongoengine/queryset/base.py

@@ -1173,6 +1186,9 @@ def allow_disk_use(self, enabled):

        :param enabled: whether or not temporary files on disk are used
        """
+        if not IS_PYMONGO_GTE_311:
+            return self


Could you explain this change please?

The property allow_disk is not accepted in pymongo 3.11 and it was making a test fail, so I return the existing qs to avoid a failure.
Not sure if it is the right solution, but I wanted my tests green :)

tests/test_session.py

tests/document/test_instance.py

clembrival · 2021-03-22T10:42:03Z

tests/document/test_instance.py

+
+        with run_in_transaction():
+            person.save()
+            person.to_dbref()


Missing an assert?

I do not know, it is like the test above but running inside a transaction.
I added it to be sure it was working properly even in a transaction, but I do not know how it should work :)

Yeah I had a look at the to_dbref function and I don't know what could be added to the test either :/

clembrival · 2021-03-22T10:43:34Z

tests/document/test_instance.py

+
+            person.reload()
+            assert person.name == "Mr Test User"
+            assert person.age == 21


Does this need to be done twice?

I do not know, I copied the test above, maybe there is a reason, I have no clue 😂

I'd remove it and see if the test still passes.

surely it will pass, removing assertions would not make it fail, if you want me to remove it I do not have a problem
I simply do not know why it was duplicated in the test above.
I will remove it :)

jonasanso · 2021-03-22T11:38:00Z

Thanks for reviewing @clembrival any questions or suggestions on the main logic and their tests, am I missing something?

mongoengine/context_managers.py

tests/document/test_dynamic.py

mongoengine/context_managers.py

Samathy · 2021-03-22T12:09:57Z

mongoengine/sessions.py

+
+from mongoengine.connection import DEFAULT_CONNECTION_NAME, get_connection
+
+sessions = local()


I think it is a poor idea to create this globally. Modules shouldnt really have side-effects on import and globals are discouraged.

I would prefer a thread_local_session class which creates a local() and binds a new session to it when asked, then has the free functions below as methods.

I am unsure quite how we'd pass existing sessions down to nested transactions, but there is probably a way to do that.

Samathy · 2021-03-22T12:11:10Z

tests/document/test_instance.py

+
+            person.reload()
+            assert person.name == "Mr Test User"
+            assert person.age == 21


I'd remove it and see if the test still passes.

mongoengine/context_managers.py

Samathy

Theres more than one test with a lot going on in the transaction contexts, I think the code in transaction contexts should be limited to just the code performing database queries that need a transaction, and not include prints, asserts or other stuff.

tests/document/test_dynamic.py

tests/test_context_managers.py

mongoengine/sessions.py

jonasanso changed the base branch from master to fix-dec-on-v23 March 16, 2021 19:18

jonasanso force-pushed the SCAL-104-implement-db-transactions branch 6 times, most recently from 3495ef6 to b90455e Compare March 17, 2021 16:30

jonasanso requested review from clembrival and Samathy March 17, 2021 16:51

jonasanso commented Mar 17, 2021

View reviewed changes

SCAL-104: Add run_in_transaction context manager and decorator using …

25d8d29

…threading local to store the sessions

jonasanso force-pushed the SCAL-104-implement-db-transactions branch from b90455e to 25d8d29 Compare March 18, 2021 07:09

Jonas and others added 3 commits March 18, 2021 14:51

SCAL-104: Add run_in_transaction context manager and decorator using …

a3be46e

…threading local to store the sessions

Merge pull request #6 from prolific-oss/fix-dec-on-v23

578e3eb

Reset fork to v.23 and fix validation of inc and dec for an IntField with min_value

Merge remote-tracking branch 'origin/master' into SCAL-104-implement-…

a485bac

…db-transactions

clembrival reviewed Mar 22, 2021

View reviewed changes

Samathy reviewed Mar 22, 2021

View reviewed changes

tests/document/test_dynamic.py Outdated Show resolved Hide resolved

jonasanso commented Mar 22, 2021

View reviewed changes

tests/test_context_managers.py Show resolved Hide resolved

Jonas added 4 commits March 22, 2021 15:15

SCAL-104 Remove duplicated assertion

a1b7b2f

SCAL-104 Clarify reload in a transaction

4001ab6

Reduce sice of run_in_transaction

4c15add

Store sessions as a dictionary, as we are doing with the connections

ad8c29b

jonasanso commented Mar 23, 2021

View reviewed changes

mongoengine/sessions.py Show resolved Hide resolved

clembrival approved these changes Mar 25, 2021

View reviewed changes

Samathy approved these changes Mar 25, 2021

View reviewed changes

jonasanso merged commit ced97a5 into fix-dec-on-v23 Mar 30, 2021

jonasanso deleted the SCAL-104-implement-db-transactions branch March 30, 2021 04:41

jonasanso restored the SCAL-104-implement-db-transactions branch March 30, 2021 04:42

jonasanso deleted the SCAL-104-implement-db-transactions branch March 30, 2021 11:34

jonasanso restored the SCAL-104-implement-db-transactions branch March 30, 2021 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SCAL-104: Implement db transactions in mongoengine #4

SCAL-104: Implement db transactions in mongoengine #4

jonasanso commented Mar 16, 2021

jonasanso Mar 17, 2021

iguanaonmystack Mar 26, 2021

jonasanso Mar 26, 2021

jonasanso Mar 17, 2021

jonasanso Mar 17, 2021

jonasanso Mar 17, 2021

jonasanso Mar 17, 2021

jonasanso Mar 17, 2021

Samathy Mar 22, 2021

jonasanso Mar 22, 2021

clembrival Mar 22, 2021

jonasanso Mar 22, 2021

clembrival Mar 22, 2021

jonasanso Mar 22, 2021

clembrival Mar 23, 2021

clembrival Mar 22, 2021

jonasanso Mar 22, 2021

Samathy Mar 22, 2021

jonasanso Mar 22, 2021

jonasanso commented Mar 22, 2021

Samathy Mar 22, 2021

Samathy Mar 22, 2021

Samathy left a comment

		@@ -18,6 +21,49 @@
		)


		class run_in_transaction(contextlib.ContextDecorator, contextlib.ExitStack):


		from mongoengine.connection import DEFAULT_CONNECTION_NAME, get_connection

		sessions = local()

SCAL-104: Implement db transactions in mongoengine #4

SCAL-104: Implement db transactions in mongoengine #4

Conversation

jonasanso commented Mar 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasanso commented Mar 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Samathy left a comment

Choose a reason for hiding this comment