vitalik / django-ninja

💨 Fast, Async-ready, Openapi, type hints based framework for building APIs
https://django-ninja.dev
MIT License
7.02k stars 421 forks source link

[BUG] `fields_optional="__all__"` causes TypeError on models with a field using the `default` parameter #1183

Open CaptainException opened 3 months ago

CaptainException commented 3 months ago

Versions:

Bug Description:

Using fields_optional on a Model which has a field where a default parameter is set causes TypeError: cannot specify both default and default_factory

The full error trace (minus the hot reload stuff) is:

File "C:\Users\CaptainException\ninja_backend\core\urls.py", line 24, in <module>
    from rental_service.api import router as rental_router
File "C:\Users\CaptainException\ninja_backend\rental_service\api.py", line 109, in <module>
    class RentalPatchSchema(ModelSchema):
  File "C:\Users\CaptainException\ninja_backend\.venv\Lib\site-packages\ninja\orm\metaclass.py", line 106, in __new__
    model_schema = create_schema(
                   ^^^^^^^^^^^^^^
  File "C:\Users\CaptainException\ninja_backend\.venv\Lib\site-packages\ninja\orm\factory.py", line 65, in create_schema
    python_type, field_info = get_schema_field(
                              ^^^^^^^^^^^^^^^^^
  File "C:\Users\CaptainException\ninja_backend\.venv\Lib\site-packages\ninja\orm\fields.py", line 169, in get_schema_field
    FieldInfo(
  File "C:\Users\CaptainException\ninja_backend\.venv\Lib\site-packages\pydantic\fields.py", line 210, in __init__
    raise TypeError('cannot specify both default and default_factory')
TypeError: cannot specify both default and default_factory

My Model is defined like this:

class Rental(models.Model, CustomPermissionsMixin):
    id = models.UUIDField(primary_key=True, default=uuid4)
    # removing this fixes the error         ^^^^^^^^^^^^^^
    user = models.ForeignKey(CustomUser, on_delete=models.PROTECT, null=True)
    devices = models.ForeignKey(Device, on_delete=models.PROTECT, null=True)
    status = models.CharField(max_length=32, null=True)
    start_date = models.DateField()
    end_date = models.DateField()
    created_at = models.DateTimeField(auto_now_add=True)

    objects = CustomObjectManager()

And the Schema like this:

class RentalPatchSchema(ModelSchema):
    class Meta:
        model = Rental
        fields = ["id"]
        fields_optional = "__all__"

Manually specifying all optional fields minus the field using default also works:

fields_optional = ["start_date", "end_date", "user", "devices", "status"]

but this defeats the purpose of the __all__ shortcut.

This is a similar bug as #1019 and #1080. The PR #1100 is a fix for this but I don't know if that has any side effects, or if there is a better solution.

When using PATCH on a resource the id is always the only required field, so maybe we could have a fields_optional_exclude option similar to exclude so that we don't have to manually specify all fields, which can be tedious on big Models. An exclude option is shorter and more readable.

But this obviously does not fix the issue for people who are using fields with a default value that can be optional in their Schema.

pmdevita commented 1 month ago

This might be fixed now, the logic to set default = None if optional has been moved before the logic to set it to PydanticUndefined. I can't upgrade to the newest version because of a different bug it introduced but it might be good to test this again.

https://github.com/vitalik/django-ninja/blob/eecb05f8fbf147d8012072007bf18ce5abcfd420/ninja/orm/fields.py#L153-L164

EDIT: Looking this over, I'm actually pretty positive it should be fixed, try it now on the latest release.