Skip to content

fix MongoAutoField ModelForm bugs #105

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

Merged
merged 4 commits into from
Aug 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/test-python.yml
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ jobs:
m2m_through_regress
m2o_recursive
model_fields
model_forms
one_to_one
ordering
or_lookups
Expand Down
6 changes: 6 additions & 0 deletions django_mongodb/features.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ def django_test_expected_failures(self):
"QuerySet.prefetch_related() is not supported on MongoDB.": {
"m2m_through_regress.test_multitable.MultiTableTests.test_m2m_prefetch_proxied",
"m2m_through_regress.test_multitable.MultiTableTests.test_m2m_prefetch_reverse_proxied",
"model_forms.tests.OtherModelFormTests.test_prefetch_related_queryset",
},
"QuerySet.update() with expression not supported.": {
"annotations.tests.AliasTests.test_update_with_alias",
Expand Down Expand Up @@ -235,6 +236,11 @@ def django_test_expected_failures(self):
"lookup.tests.LookupTests.test_exact_exists",
"lookup.tests.LookupTests.test_nested_outerref_lhs",
"lookup.tests.LookupQueryingTests.test_filter_exists_lhs",
"model_forms.tests.LimitChoicesToTests.test_fields_for_model_applies_limit_choices_to",
"model_forms.tests.LimitChoicesToTests.test_limit_choices_to_callable_for_fk_rel",
"model_forms.tests.LimitChoicesToTests.test_limit_choices_to_callable_for_m2m_rel",
"model_forms.tests.LimitChoicesToTests.test_limit_choices_to_m2m_through",
"model_forms.tests.LimitChoicesToTests.test_limit_choices_to_no_duplicates",
"queries.tests.ExcludeTest17600.test_exclude_plain",
"queries.tests.ExcludeTest17600.test_exclude_with_q_is_equal_to_plain_exclude_variation",
"queries.tests.ExcludeTest17600.test_exclude_with_q_object_no_distinct",
Expand Down
24 changes: 21 additions & 3 deletions django_mongodb/fields/auto.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from bson import ObjectId, errors
from django.core import exceptions
from django.db.models.fields import AutoField, Field
from django.utils.functional import cached_property
from django.utils.translation import gettext_lazy as _


Expand All @@ -15,14 +16,26 @@ def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)

def get_prep_value(self, value):
# Override AutoField casting to integer.
return Field.get_prep_value(self, value)
if value is None:
return None
# Accept int for compatibility with Django's test suite which has many
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is the way to go, but without accepting integers, there are about 100 tests we've triaged so far that fail because of manually assigned numeric pks. Also, as the comment mentions, there's at least one other issue with settings.SITE_ID which could probably be assigned on ObjectIdrather than an int, while also adding the system check error to settings.SILENCED_SYSTEM_CHECKS. So I would call this support tentative while we weigh the benefits and drawbacks through continued testing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 . So, the idea is storing numbers and object ids in the field _id. Right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine. _id can be an integer, string, or any other value. I think it's smart to have it be an extremely intentional choice and communicate that support for anything other than ObjectId is "tentative" like you said.

# instances of manually assigned integer IDs, as well as for things
# like settings.SITE_ID which has a system check requiring an integer.
if isinstance(value, (ObjectId | int)):
return value
try:
return ObjectId(value)
except errors.InvalidId as e:
# A manually assigned integer ID?
if isinstance(value, str) and value.isdigit():
return int(value)
raise ValueError(f"Field '{self.name}' expected an ObjectId but got {value!r}.") from e

def rel_db_type(self, connection):
return Field().db_type(connection=connection)

def to_python(self, value):
if value is None:
if value is None or isinstance(value, int):
return value
try:
return ObjectId(value)
Expand All @@ -32,3 +45,8 @@ def to_python(self, value):
code="invalid",
params={"value": value},
) from None

@cached_property
def validators(self):
# Avoid IntegerField validators inherited from AutoField.
return [*self.default_validators, *self._validators]