dymmond / mongoz

ODM with Pydantic made it simple
https://mongoz.dymmond.com
MIT License
19 stars 4 forks source link

Incorrect behaviour for the ObjectId field. #61

Closed harshalizode closed 1 month ago

harshalizode commented 2 months ago

Hello,

There is an issue with creating the nullable field for the objectId. When I create a record without supplying the reference of another document, it will also construct a random objectId and put it in the document, which is erroneous behavior that has been identified.

To verify the issue, I constructed the test scenario shown below.

import mongoz
from typing import Optional, ClassVar
from esmerald.conf import settings

import pytest

from mongoz import ObjectId
from playmity.apps.common.v1.schemas import BaseDocument
from playmity.apps.common.custom_manager import CustomManger

class Movie(BaseDocument):
    name: str = mongoz.String()
    year: int = mongoz.Integer()
    producer_id: Optional[ObjectId] = mongoz.ObjectId(null=True)

    objects: ClassVar[CustomManger] = CustomManger()

    class Meta:
        registry = settings.default_registry
        database = settings.application_db

@pytest.mark.asyncio
async def test_nullable_objectid() -> None:
    await Movie.objects.using("test_my_db").delete()
    await Movie.objects.using("test_my_db").create(name="latest_movie",
                                                   year=2024)

    movie = await Movie.objects.using("test_my_db").get()
    assert movie.name == "latest_movie"
    assert movie.year == 2024
    assert movie.producer_id is None

This test failed because the producer_id field contains the random object id but not the null value.

harshalizode commented 2 months ago

Hello @tarsil , Is there a way to resolve this issue?

tarsil commented 2 months ago

@harshalizode why using an object field for UUID? ObjectID is a proper bson field. Why not using https://mongoz.tarsild.io/fields/?h=uui#uuid?

harshalizode commented 2 months ago

I have corrected the model field. Here the field producer_id is referring to the another document.

harshalizode commented 2 months ago

Hello @tarsil, Although the ObjectId data type has a null attribute to make it null-able, this attribute does not function; instead, it generates a random object ID when storing the data.

I have therefore overridden the following methods:

mongoz.core.db.fields.core.py

class ObjectId(bson.ObjectId):
     def __init__(
        self, oid: Union[str, bson.ObjectId, bytes, None] = None,
        null: bool = False
    ) -> None:
        self.null = null
        if self.null and not oid:
            self.__id = None
        else:
            super().__init__(oid)
        self.name: Union[str, None] = None

mongoz.core.db.documents.base.py

class BaseMongoz(BaseModel, metaclass=BaseModelMeta):
    def extract_default_values_from_field(self, is_proxy: bool = False,
                                          **kwargs: bson.Any
                                          ) -> Dict[str, Any] | None:
        """
        Populate the defaults of each Mongoz field if any is passed.

        E.g.: DateTime(auto_now=True) will generate the default for automatic
        dates.
        """
        fields: Dict[str, Any] = kwargs if is_proxy else self.model_dump()

        kwargs = {k: v for k, v in fields.items() if k in self.model_fields}
        for key, value in kwargs.items():
            if key not in self.meta.fields:
                if not hasattr(self, key):
                    raise ValueError(f"Invalid keyword {key} for class "
                                     f"{self.__class__.__name__}")
                elif key not in ["id", "_id", "pk"]:
                    if type(value) is not bson.ObjectId:
                        setattr(self, key, None)
                        continue

            # For non values. Example: bool
            if value is not None:
                # Checks if the default is a callable and executes it.
                if callable(value):
                    setattr(self, key, value())
                else:
                    setattr(self, key, value)
                continue

            # Validate the default fields
            field = self.model_fields[key]
            if hasattr(field, "has_default") and field.has_default():
                setattr(self, key, field.get_default_value())
                continue

            if is_proxy:
                kwargs[key] = value

        return kwargs if is_proxy else None

Using this we can allow the null-able objectID to store and retrieve the records from the MongoDB. Is required any changes to handle it in best way?

tarsil commented 2 months ago

The ObjectID in theory is generated for all mongoz objects when storing a document but let me see what I can do. You want to make sure the ObjectID is nullable, right? The reason why the null is available its because all the fields inherit from the BaseField but I will have a look

harshalizode commented 2 months ago

Okay, thanks for consideration.

harshalizode commented 2 months ago

The ObjectID in theory is generated for all mongoz objects when storing a document but let me see what I can do. You want to make sure the ObjectID is nullable, right? The reason why the null is available its because all the fields inherit from the BaseField but I will have a look

@tarsil Is any findings for this issue?

tarsil commented 2 months ago

The ObjectID in theory is generated for all mongoz objects when storing a document but let me see what I can do. You want to make sure the ObjectID is nullable, right? The reason why the null is available its because all the fields inherit from the BaseField but I will have a look

@tarsil Is any findings for this issue?

@harshalizode ww should have a release soon. I also have a job, unfortunately we don't have any sponsorship if any kind with the ecosystem and I have a 100% covered job which means I can only do this after but like I said, soon a release should be out.

You didn't answer what I asked. You want the object id to allow nulls, basically. Correct?

EDIT: It was never an issue, it was designed to behave the way it behaves by default. Now, you also would like to have the possibility of allowing nulls.

harshalizode commented 2 months ago

Yes, object id should allow the null values. If we provided the null value for the object Id then it will generates the random id and stores in the document, but the requirement is to store the null instead the random generated id.

tarsil commented 2 months ago

@harshalizode that is not going to work as the ObjectID is a special type specifically design for internals but now we will have the NullableObjectID field that will allow you to do exactly what you want and need. Will release soon.

tarsil commented 2 months ago

Ok, release 0.10.8 was done and contains this new addition, you can see more here https://mongoz.dymmond.com/fields/#nullableobjectid

Basically we added a new special field for object Ids without messing with the core ObjectId and still enforces the type, which means, if you save for example an int where it is supposed to be an object id, it will throw an error, as expected.

The way you suggested would not work as it would require massive internal changes to do what a simple NullableObjectId field does with a lot less complications. Mongoz is complex internally and the way it manages the fields.

We do appreciate your suggestion btw.

harshalizode commented 2 months ago

Yes, Is working as as expected and running the all test case scenarios that we have defined. Thanks, for the quick release :D

tarsil commented 2 months ago

Our pleasure @harshalizode 👍🏼