-
Notifications
You must be signed in to change notification settings - Fork 20
INTPYTHON-355 Add transaction support #313
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
base: main
Are you sure you want to change the base?
Changes from all commits
cf614ed
392bc2f
ca235e2
001cc47
04d3f2f
176205b
b3b956f
8087a7c
c9b639a
9391599
4eb66e9
749ab89
f26e114
57c3683
0dd2dea
9530668
3d0a6fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
name: Python Tests on a replica test | ||
|
||
on: | ||
pull_request: | ||
paths: | ||
- '**.py' | ||
- '!setup.py' | ||
- '.github/workflows/test-python-replica.yml' | ||
workflow_dispatch: | ||
|
||
concurrency: | ||
group: ${{ github.workflow }}-${{ github.ref }} | ||
cancel-in-progress: true | ||
|
||
defaults: | ||
run: | ||
shell: bash -eux {0} | ||
|
||
jobs: | ||
build: | ||
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@90004df786821b6308fb02299e5835d0dae05d0d # 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,8 +36,6 @@ class DatabaseFeatures(BaseDatabaseFeatures): | |
supports_temporal_subtraction = True | ||
# MongoDB stores datetimes in UTC. | ||
supports_timezones = False | ||
# Not implemented: https://github.com/mongodb/django-mongodb-backend/issues/7 | ||
supports_transactions = False | ||
supports_unspecified_pk = True | ||
uses_savepoints = False | ||
|
||
|
@@ -50,8 +48,6 @@ class DatabaseFeatures(BaseDatabaseFeatures): | |
"aggregation.tests.AggregateTestCase.test_order_by_aggregate_transform", | ||
# 'NulledTransform' object has no attribute 'as_mql'. | ||
"lookup.tests.LookupTests.test_exact_none_transform", | ||
# "Save with update_fields did not affect any rows." | ||
"basic.tests.SelectOnSaveTests.test_select_on_save_lying_update", | ||
# BaseExpression.convert_value() crashes with Decimal128. | ||
"aggregation.tests.AggregateTestCase.test_combine_different_types", | ||
"annotations.tests.NonAggregateAnnotationTestCase.test_combined_f_expression_annotation_with_aggregation", | ||
|
@@ -96,13 +92,47 @@ class DatabaseFeatures(BaseDatabaseFeatures): | |
"expressions.tests.ExpressionOperatorTests.test_lefthand_bitwise_xor_right_null", | ||
"expressions.tests.ExpressionOperatorTests.test_lefthand_transformed_field_bitwise_or", | ||
} | ||
_django_test_expected_failures_no_transactions = { | ||
# "Save with update_fields did not affect any rows." instead of | ||
# "An error occurred in the current transaction. You can't execute | ||
# queries until the end of the 'atomic' block." | ||
"basic.tests.SelectOnSaveTests.test_select_on_save_lying_update", | ||
} | ||
_django_test_expected_failures_transactions = { | ||
# When update_or_create() fails with IntegrityError, the transaction | ||
# is no longer usable. | ||
"get_or_create.tests.UpdateOrCreateTests.test_manual_primary_key_test", | ||
"get_or_create.tests.UpdateOrCreateTestsWithManualPKs.test_create_with_duplicate_primary_key", | ||
# Tests that require savepoints | ||
"admin_views.tests.AdminViewBasicTest.test_disallowed_to_field", | ||
"admin_views.tests.AdminViewPermissionsTest.test_add_view", | ||
"admin_views.tests.AdminViewPermissionsTest.test_change_view", | ||
"admin_views.tests.AdminViewPermissionsTest.test_change_view_save_as_new", | ||
"admin_views.tests.AdminViewPermissionsTest.test_delete_view", | ||
"auth_tests.test_views.ChangelistTests.test_view_user_password_is_readonly", | ||
"fixtures.tests.FixtureLoadingTests.test_loaddata_app_option", | ||
"fixtures.tests.FixtureLoadingTests.test_unmatched_identifier_loading", | ||
"fixtures_model_package.tests.FixtureTestCase.test_loaddata", | ||
"get_or_create.tests.GetOrCreateTests.test_get_or_create_invalid_params", | ||
"get_or_create.tests.UpdateOrCreateTests.test_integrity", | ||
"many_to_many.tests.ManyToManyTests.test_add", | ||
"many_to_one.tests.ManyToOneTests.test_fk_assignment_and_related_object_cache", | ||
"model_fields.test_booleanfield.BooleanFieldTests.test_null_default", | ||
"model_fields.test_floatfield.TestFloatField.test_float_validates_object", | ||
"multiple_database.tests.QueryTestCase.test_generic_key_cross_database_protection", | ||
"multiple_database.tests.QueryTestCase.test_m2m_cross_database_protection", | ||
} | ||
|
||
@cached_property | ||
def django_test_expected_failures(self): | ||
expected_failures = super().django_test_expected_failures | ||
expected_failures.update(self._django_test_expected_failures) | ||
if not self.is_mongodb_6_3: | ||
expected_failures.update(self._django_test_expected_failures_bitwise) | ||
if self.supports_transactions: | ||
expected_failures.update(self._django_test_expected_failures_transactions) | ||
else: | ||
expected_failures.update(self._django_test_expected_failures_no_transactions) | ||
return expected_failures | ||
|
||
django_test_skips = { | ||
|
@@ -485,16 +515,6 @@ def django_test_expected_failures(self): | |
"Connection health checks not implemented.": { | ||
"backends.base.test_base.ConnectionHealthChecksTests", | ||
}, | ||
"transaction.atomic() is not supported.": { | ||
"backends.base.test_base.DatabaseWrapperLoggingTests", | ||
"migrations.test_executor.ExecutorTests.test_atomic_operation_in_non_atomic_migration", | ||
"migrations.test_operations.OperationTests.test_run_python_atomic", | ||
}, | ||
"transaction.rollback() is not supported.": { | ||
"transactions.tests.AtomicMiscTests.test_mark_for_rollback_on_error_in_autocommit", | ||
"transactions.tests.AtomicMiscTests.test_mark_for_rollback_on_error_in_transaction", | ||
"transactions.tests.NonAutocommitTests.test_orm_query_after_error_and_rollback", | ||
}, | ||
"migrate --fake-initial is not supported.": { | ||
"migrations.test_commands.MigrateTests.test_migrate_fake_initial", | ||
"migrations.test_commands.MigrateTests.test_migrate_fake_split_initial", | ||
|
@@ -533,8 +553,18 @@ def django_test_expected_failures(self): | |
"foreign_object.test_tuple_lookups.TupleLookupsTests", | ||
}, | ||
"ColPairs is not supported.": { | ||
# 'ColPairs' object has no attribute 'as_mql' | ||
"auth_tests.test_views.CustomUserCompositePrimaryKeyPasswordResetTest", | ||
"composite_pk.test_aggregate.CompositePKAggregateTests", | ||
"composite_pk.test_create.CompositePKCreateTests", | ||
"composite_pk.test_delete.CompositePKDeleteTests", | ||
"composite_pk.test_filter.CompositePKFilterTests", | ||
"composite_pk.test_get.CompositePKGetTests", | ||
"composite_pk.test_models.CompositePKModelsTests", | ||
"composite_pk.test_order_by.CompositePKOrderByTests", | ||
"composite_pk.test_update.CompositePKUpdateTests", | ||
"composite_pk.test_values.CompositePKValuesTests", | ||
"composite_pk.tests.CompositePKTests", | ||
"composite_pk.tests.CompositePKFixturesTests", | ||
}, | ||
"Custom lookups are not supported.": { | ||
"custom_lookups.tests.BilateralTransformTests", | ||
|
@@ -577,3 +607,25 @@ def supports_atlas_search(self): | |
return False | ||
else: | ||
return True | ||
|
||
@cached_property | ||
def supports_select_union(self): | ||
# Stage not supported inside of a multi-document transaction: $unionWith | ||
return not self.supports_transactions | ||
|
||
@cached_property | ||
def supports_transactions(self): | ||
""" | ||
Transactions are enabled if the MongoDB configuration supports it: | ||
MongoDB must be configured as a replica set or sharded cluster, and | ||
the store engine must be WiredTiger. | ||
""" | ||
self.connection.ensure_connection() | ||
client = self.connection.connection.admin | ||
hello_response = client.command("hello") | ||
is_replica_set = "setName" in hello_response | ||
is_sharded_cluster = hello_response.get("msg") == "isdbgrid" | ||
if is_replica_set or is_sharded_cluster: | ||
engine = client.command("serverStatus").get("storageEngine", {}) | ||
return engine.get("name") == "wiredTiger" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find this less readable than the logical grouping, but not enough to protest more than adding this comment! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I nested it to avoid the call to |
||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about thread safety here?
For example:
Won't the session property be mistakenly shared between threads?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We rely on Django for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean if the session property were to be mistakenly shared between threads that would be a Django bug … there's nothing we can do within the framework they provide (
_commit
,_rollback
, etc) to manage threads as far as I know.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Django, database connections are thread-local, so the use of
atomic()
in two separate threads will use two separate connections (and thus two separation sessions). It's no different from any other state stored onDatabaseWrapper
[My understanding of thread locals, etc. isn't 100% rock solid, particularly since pymongo operates a bit differently than the PEP 249-conforming drivers for Django's other database backends, but I think what I've stated is correct. Let me know if you have a particular concern.]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, since each thread gets it's own instance of our DatabaseWrapper I agree this is not an issue. I have a feeling this is not the first nor the last time I will ask this same question so bare with me, Django uses a different model than I'm used to.