add ObjectIdField

[WaVEV]

fixes #161

[timgraham]

No, I think it's okay if auto imports from this file.

On Mon, Nov 18, 2024 at 9:29 PM Emanuel Lupi @.***> wrote:

@.**** commented on this pull request.

In django_mongodb/fields/objectid.py https://github.com/mongodb-labs/django-mongodb/pull/187#discussion_r1847552800 :

@@ -0,0 +1,27 @@ +from bson import ObjectId +from bson.errors import InvalidId +from django.core import exceptions +from django.db.models.fields import Field + +from .objectid_mixin import ObjectIdMixin

auto also uses it, shall I move auto to objectid?

— Reply to this email directly, view it on GitHub https://github.com/mongodb-labs/django-mongodb/pull/187#discussion_r1847552800, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADERXJ2K6AM4TD34Z3JZND2BKPBNAVCNFSM6AAAAABR6HL2H2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINBUGE2TQOBQGA . You are receiving this because your review was requested.Message ID: @.***>

[WaVEV]

I think an ObjectIdField.get_prep_value() is needed. It can be exercised with a querying test that uses lookup with a string ObjectId.

Maybe you are right, I will create some unit test to covers that.

[WaVEV]

I think an ObjectIdField.get_prep_value() is needed. It can be exercised with a querying test that uses lookup with a string ObjectId.

Done.

[Jibola]

Looks like there was a caught failure in two of the tests? I don't see the equivalent failured in github, so I'm going to re-run it https://evergreen.mongodb.com/task_log_raw/django_mongodb_tests_run_tests_patch_36c57187de430ab4313e8db5c3ce8ab2ab569f5b_6742512f8f2d9a000730e871_24_11_23_22_03_28/0?type=T#L18115

[timgraham]

We don't need to worry about evergreen. It fails as expected because it's not using Emanuel's Django branch that'll be merged with this patch.

[timgraham]

I don't think we want to allow integer for ObjectIdField. This was only done on ObjectIdAutoField for compatibility with Django's test suite. and there's an Jira ticket to revisit this decision since it seems likely to be problematic in the long run.

[WaVEV]

I don't think we want to allow integer for ObjectIdField. This was only done on ObjectIdAutoField for compatibility with Django's test suite. and there's an Jira ticket to revisit this decision since it seems likely to be problematic in the long run.

I agree that is not a good idea. I faced the same problem in many tests. I can make some changes in the test´s models in order to avoid integers instead of objectId.

[WaVEV]

Sorry, I thought I linked to an example of the sort of tests I had in mind. See model_fields/test_jsonfield.TestQuerying. Maybe what you have written is fine but it's perhaps more t than necessary. And like I said in another comment, all the loops and subTest are difficult to read and I fear would be somewhat difficult to debug if they fail.

🤔 They are indeed difficult to debug, and the Django test suite is full of them 😬. However, I think I can write multiple tests or a few tests with more steps instead of using subtests.