vitalik / django-ninja

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

[BUG] extra="forbid" does not work anymore with Django-ninja 1.0 #934

Open ddahan opened 1 year ago

ddahan commented 1 year ago

Describe the bug After migrating to Django Ninja 1.0+, and replacing Config with Meta, the extra = "forbid" directive seems to have no effect.

Here is a code example that does not work with Django Ninja 1.0:

class BadgeSchemaInUpdate(ModelSchema):
    class Meta:
        model = Badge
        fields = ("expiration", "is_active")
        fields_optional = ("expiration", "is_active")
        extra = "forbid"

Here, I can update my Badge model using another attribute than expiration or is_active (not expected). It did not happened before with this code and older Django Ninja 0.22.2:

class BadgeSchemaInUpdate(ModelSchema):
    class Config:
        model = Badge
        model_fields = ("expiration", "is_active")
        model_fields_optional = ("expiration", "is_active")
        extra = "forbid"

Versions

vitalik commented 1 year ago

@ddahan yeah.. well the thing is that pydantic now asks to configure this using attribute model_config

so for now do it like `model_config = {'extra': 'forbid'}

but I guess that is a bit confusing problem - I will think about it

ddahan commented 1 year ago

Thanks vitalik. This works:

class BadgeSchemaInUpdate(ModelSchema):
    class Meta:
        model = Badge
        fields = ("expiration", "is_active")
        fields_optional = ("expiration", "is_active")

    model_config = {"extra": "forbid"}

And this works too (I suppose it does the same behind the scene):

class BadgeSchemaInUpdate(ModelSchema):
    class Meta:
        model = Badge
        fields = ("expiration", "is_active")
        fields_optional = ("expiration", "is_active")

    class Config(Schema.Config):
        extra = "forbid"

As you said, it's a little confusing, but IMO the main issue is that it's not documented right now. I can see at least here a usage of the former usage.

I suggest not closing the issue, at least until the documentation is updated. Thanks for your work.

vitalik commented 1 year ago

@ddahan keep in mind - class Config is deprecated in pydantic and will be removed - so better use model_config = {'extra': 'forbid'}

vitalik commented 1 year ago

for now I see 2 options:

ddahan commented 1 year ago

I'm absolutely not trustworthy with this piece of advice, as I don't know ninja and pydantic that much, but... I guess it would depends on how users should know about Pydantic itself when using django-ninja.

If you want users to know about all possibilities offered by Pydantic, and you want to split responsibilities, it may be a good idea to split ninja parts and pydantic parts, with links to Pydantic in the ninja documentation.

On the other hand, if you want ninja to act as a wrapper around Pydantic, sothat users don't need to know about Pydantic at all, I'd create syntax sugar like copying all pydantic options that user put's to Meta to model_config.

I suppose the worst case scenario would be to not take a decision at all and mix both visions.