INTPYTHON-355 Add transaction support#313
Conversation
aclark4life
changed the title
| } | ||
|
|
||
| @cached_property | ||
| def supports_transactions(self): |
There was a problem hiding this comment.
What's the purpose of this property? Is it just for the test suite or do apps rely on it?
There was a problem hiding this comment.
Not just for testing.
In this case, rather than declare False or True for supports_transactions we dynamically set True or False based on what type of MongoDB we are connected to.
Then DatabaseFeatures is passed to DatabaseWrapper IIRC and the runtime behavior of Django changes accordingly.
Based on this: https://github.com/django/django/blob/main/django/db/backends/base/features.py#L419-L431
There was a problem hiding this comment.
It depends on what we want to implement. If we want to dynamically enable transactions based on whether or not the database supports it, we could use it internally. Otherwise, we'll have to introduce a separate setting the user sets to indicate whether or not they want to use transactions, and raise an error if it's not supported.
It's also used by Django in a couple places. For example, to determine whether or not to use transactions to speed up tests.
There was a problem hiding this comment.
The issue is that there are certain storage engines where a replica set or sharded cluster does not actually support transactions so this check isn't 100% accurate. That said, those should be rare so I'm fine with the current "hello" based approach. We can always revisit later if it causes an issue.
There was a problem hiding this comment.
Fixed in c8f8881 (assuming wired tiger storage engine is correct or we could also check to make sure it is not mmapv1)
There was a problem hiding this comment.
Alex says, "I think we can probably remove the storage engine check because anything but wired tiger is gone in MongodDB 4.2+ and we require MongoDB 6. There was no 'engine' in the server status [on a sharded cluster] and I'm not sure how to check each shard's storage and it seems like we don't really need to."`
| } | ||
|
|
||
| @cached_property | ||
| def supports_transactions(self): |
There was a problem hiding this comment.
The issue is that there are certain storage engines where a replica set or sharded cluster does not actually support transactions so this check isn't 100% accurate. That said, those should be rare so I'm fine with the current "hello" based approach. We can always revisit later if it causes an issue.
aclark4life
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| name: Django Test Suite | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout django-mongodb-backend | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| persist-credentials: false | ||
| - name: install django-mongodb-backend | ||
| run: | | ||
| pip3 install --upgrade pip | ||
| pip3 install -e . | ||
| - name: Checkout Django | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: 'mongodb-forks/django' | ||
| ref: 'mongodb-5.2.x' | ||
| path: 'django_repo' | ||
| persist-credentials: false | ||
| - name: Install system packages for Django's Python test dependencies | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install libmemcached-dev | ||
| - name: Install Django and its Python test dependencies | ||
| run: | | ||
| cd django_repo/tests/ | ||
| pip3 install -e .. | ||
| pip3 install -r requirements/py3.txt | ||
| - name: Copy the test settings file | ||
| run: cp .github/workflows/mongodb_settings.py django_repo/tests/ | ||
| - name: Copy the test runner file | ||
| run: cp .github/workflows/runtests.py django_repo/tests/runtests_.py | ||
| - name: Start MongoDB | ||
| uses: supercharge/mongodb-github-action@1.12.0 | ||
| with: | ||
| mongodb-version: 6.0 | ||
| mongodb-replica-set: test-rs | ||
| - name: Run tests | ||
| run: python3 django_repo/tests/runtests.py --settings mongodb_settings -v 2 |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium test
Referring to tests defined in |
Transactions are tested in Atlas tests.
This reverts commit f26e114.
|
@timgraham Sorry, can you create a new PR for this one ? |
4 participants
I didn't try running the test suite on a sharded cluster. Maybe this could be done at a later date.