fix MongoAutoField ModelForm bugs

[timgraham]

The mongodb-5.0.x branch of the Django fork will be updated with the contents of the temporary branch right before merging this. Namely, there are a two spots (example diff below) where I previously removed str(obj.pk) because it didn't work with MongoAutoField. It turns out, this needs to work because ModelForm makes use of it.

diff --git a/tests/many_to_many/tests.py b/tests/many_to_many/tests.py
index 7915f29bdd..7ed3b80abc 100644
--- a/tests/many_to_many/tests.py
+++ b/tests/many_to_many/tests.py
@@ -156,7 +156,7 @@ class ManyToManyTests(TestCase):
         # A single SELECT query is necessary to compare existing values to the
         # provided one; no INSERT should be attempted.
         with self.assertNumQueries(1):
-            self.a1.publications.add(self.p1.pk)
+            self.a1.publications.add(str(self.p1.pk))
         self.assertEqual(self.a1.publications.get(), self.p1)
 
     @skipUnlessDBFeature("supports_ignore_conflicts")

[timgraham]

I had to add a regression test to cover this condition:

diff --git a/tests/model_forms/tests.py b/tests/model_forms/tests.py
index d181585ca3..e5e99240d5 100644
--- a/tests/model_forms/tests.py
+++ b/tests/model_forms/tests.py
@@ -1511,8 +1511,9 @@ class ModelFormBasicTests(TestCase):
 <li>The URL: <input type="text" name="url" maxlength="40" required></li>""",
         )
 
-    def test_initial_values(self):
-        self.create_basic_data()
+    def test_initial_values(self, create_data=True):
+        if create_data:
+            self.create_basic_data()
         # Initial values can be provided for model forms
         f = ArticleForm(
             auto_id=False,
@@ -1623,6 +1624,26 @@ class ModelFormBasicTests(TestCase):
         test_art = Article.objects.get(id=art_id_1)
         self.assertEqual(test_art.headline, "Test headline")
 
+    def test_int_pks(self):
+        """
+        MongoAutoField supports numeric pks in ModelForm data, not just
+        ObjectId.
+        """
+        # The following lines repeat self.create_initial_data() but with
+        # manually assigned pks.
+        self.c1 = Category.objects.create(
+            pk=1, name="Entertainment", slug="entertainment", url="entertainment"
+        )
+        self.c2 = Category.objects.create(
+            pk=2, name="It's a test", slug="its-test", url="test"
+        )
+        self.c3 = Category.objects.create(
+            pk=3, name="Third test", slug="third-test", url="third"
+        )
+        self.w_royko = Writer.objects.create(name="Mike Royko", pk=1)
+        self.w_woodward = Writer.objects.create(name="Bob Woodward", pk=2)
+        self.test_initial_values(create_data=False)
+
     def test_m2m_initial_callable(self):
         """
         A callable can be provided as the initial value for an m2m field.

[Jibola]

Ah, there is the bson.ObjectId.is_valid function that could be used here. This way, instead of explicitly checking length we could see if it's not a valid objectId. Or we could do the exact opposite and just check if the value is an ObjectId.

Something like:

if ObjectId.is_valid(value):
   return ObjectId(value):
elif isinstance(value, str) and value.isdigit():
   return int(value)
else:
   raise ValueError()

My only hesitation into NOT doing what I mentioned above is performance. Any use of the is_valid under the hood just checks if there's an exception raised on conversion. However, in being exhaustive, it's the best method.

[timgraham]

I got confused by the mongosh docs for ObjectId which says the constructor can take "The integer value, in seconds, is added to the Unix epoch to create the new timestamp." We don't need to do the "is value a numeric pk?" check until after ObjectId() (more common case), so yes, your code is what we end up with, except there is no need to call ObjectId.is_valid() which doubles the validation done by ObjectId.__init__().

[timgraham]

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.

[WaVEV]

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

[timgraham]

Correct

[Jibola]

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.