Skip to content
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

add ObjectIdField #187

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

add ObjectIdField #187

wants to merge 16 commits into from

Conversation

WaVEV
Copy link
Collaborator

@WaVEV WaVEV commented Nov 17, 2024

fixes #161

.github/workflows/test-python.yml Show resolved Hide resolved
Jibola

This comment was marked as resolved.

timgraham

This comment was marked as resolved.

@timgraham timgraham changed the title Add object id field add ObjectIdField Nov 18, 2024
Copy link
Collaborator

@timgraham timgraham left a comment

Choose a reason for hiding this comment

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

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

django_mongodb/fields/objectid.py Outdated Show resolved Hide resolved
django_mongodb/fields/objectid.py Show resolved Hide resolved
tests/model_fields_/test_objectidfield.py Outdated Show resolved Hide resolved
django_mongodb/fields/auto.py Outdated Show resolved Hide resolved
@timgraham

This comment was marked as resolved.

@WaVEV
Copy link
Collaborator Author

WaVEV commented Nov 21, 2024

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
Copy link
Collaborator Author

WaVEV commented Nov 23, 2024

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
Copy link
Collaborator

Jibola commented Nov 25, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

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.

Copy link
Collaborator

@timgraham timgraham left a comment

Choose a reason for hiding this comment

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

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.

self.assertSequenceEqual(union_qs, [self.t3, self.t4])

def test_invalid_object_id(self):
"""Combine queries using union"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

No union in sight in this test.

self.assertSequenceEqual(qs, [self.t3, self.t4])

def test_union_children_to_select_parents(self):
"""Union query to select parents of children based on group_id"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

No union in sight in this test.

def test_to_python(self):
f = ObjectIdField()
expected = ObjectId("1" * 24)
for value in ["1" * 24, ObjectId("1" * 24)]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer separate tests or a string of to_python() calls without the loops and subTest, e.g.

    def test_to_python(self):
        self.assertIsNone(models.UUIDField().to_python(None))

    def test_to_python_int_values(self):
        self.assertEqual(
            models.UUIDField().to_python(0),
            uuid.UUID("00000000-0000-0000-0000-000000000000"),
        )
        # Works for integers less than 128 bits.
        self.assertEqual(
            models.UUIDField().to_python((2**128) - 1),
            uuid.UUID("ffffffff-ffff-ffff-ffff-ffffffffffff"),
        )

I think the current way is too hard on the eyes.

@WaVEV
Copy link
Collaborator Author

WaVEV commented Nov 26, 2024

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
Copy link
Collaborator Author

WaVEV commented Nov 26, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QuerySet.filter() in lookup doesn't return any results when subquery returns ObjectId
3 participants