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

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

Open
CaptainException opened this issue Jun 3, 2024 · 0 comments

Comments

@CaptainException
Copy link

CaptainException commented Jun 3, 2024

Versions:

  • Python version: 3.12.2
  • Django version: 5.0.6
  • Django-Ninja version: 1.1.0
  • Pydantic version: 2.7.1

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.

@CaptainException CaptainException changed the title [BUG] fields_optional ="__all__" causes TypeError on models with a field using the default parameter [BUG] fields_optional="__all__" causes TypeError on models with a field using the default parameter Jun 3, 2024
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

No branches or pull requests

1 participant