add ObjectIdField

[WaVEV]

fixes #161

[timgraham]

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

[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.

[timgraham]

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.

[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.

[timgraham]

Looking at the test failures after my edits, it might be that we don't want to make the proposed change in the Django test suite and just leave those tests as skipped/invalid on MongoDB. Do the new queries tests you wrote cover the issue? If not, we can add those to our test suite (without modifying the models in Django's test suite which breaks other things due to the fact that ObjectIdField doesn't accept integers).

[WaVEV]

Looking at the test failures after my edits, it might be that we don't want to make the proposed change in the Django test suite and just leave those tests as skipped/invalid on MongoDB. Do the new queries tests you wrote cover the issue? If not, we can add those to our test suite (without modifying the models in Django's test suite which breaks other things due to the fact that ObjectIdField doesn't accept integers).

I agree with that. I will add test to cover the issue.

comments addressed

[timgraham]

We failed to consider save/load tests as well as forms support. I adapt some tests from UUIDField but the current code isn't entirely consistent with it. It seems Django's built-in model fields aren't entirely consistent with respect to the exceptions they raise in each method. I'll return to this next week with fresh eyes.