Skip to content

RFC: Modelschema rework #1281

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

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

Ksauder
Copy link
Contributor

@Ksauder Ksauder commented Aug 27, 2024

Addresses: #1300 #1249 #995 #347

I was having trouble getting inheritance to work in a logical way with the ModelSchema, so I took a stab at updating the ModelSchemaMetaclass, MetaConf, and some of the SchemaFactory. The added inheritance test shows a concrete example.

I think this rework allows more flexibility and ability to customize moving forward, would love some feedback.

  • rework ModelSchemaMetaclass so it does not use create_schema() under the hood
  • rework the defaults from MetaConf through SchemaFactory methods to be more explicit
  • add primary_key_optional option to MetaConf and create_schema() - this toggles how Django fields with primary_key are handled
  • fix typing to be compatible with 3.x python

I forked and am running my develop branch on a medium (ish? 100+ endpoints) sized project. I have this branch working as intended after some fixes. Here's an example of a project schemas.py file:
nullable_wrapper is a param on a separate branch on my fork, allows a custom type to wrap nullable fields:

orm.fields.get_schema_field
    ...
    if nullable:
        if nullable_wrapper:
            python_type = nullable_wrapper[python_type]
        else:
            python_type = Union[python_type, None]  # aka Optional in 3.7+
    ...
"""All API Schemas and ModelSchemas inherit from classes in this file allowing defaults to be set"""
from functools import partial
from typing import Annotated, Optional, TypeVar

import ninja
from ninja.orm import create_schema
from pydantic import GetJsonSchemaHandler, model_serializer
from pydantic.json_schema import JsonSchemaValue
from pydantic_core import core_schema


# used with nullable_wrapper, see above
class OmissibleClass:
    """
    Class for the custom Omissible type to modify the JsonSchemaValue for the field.
    """

    @classmethod
    def __get_pydantic_json_schema__(
        cls, source: core_schema.CoreSchema, handler: GetJsonSchemaHandler
    ) -> JsonSchemaValue:
        output = handler(source)
        if output.get("anyOf") and len(output["anyOf"]):
            t_type = output["anyOf"][0]
            assert t_type != {"type": "null"}
            del output["anyOf"]
            output.update(t_type)
        return output


def _omissible_serialize(self, handler):
    """Delete the key from the dump if the key is Omissible and None"""
    dump = handler(self)
    for key, field_info in self.model_fields.items():
        metadata = field_info.metadata
        for c in metadata:
            if dump.get(key) is None and OmissibleClass == c:
                del dump[key]

    return dump


T = TypeVar("T")
Omissible = Annotated[Optional[T], OmissibleClass]


# use project.schemas.Proj* instead of ninja.Schema/ModelSchema
class ProjSchema(ninja.Schema):
    _omissible_serialize = model_serializer(mode="wrap")(_omissible_serialize)
    model_config = {"arbitrary_types_allowed": True}


class ProjModelSchema(ProjSchema, ninja.ModelSchema):
    pass


# ModelSchema.Meta should inherit from ProjMeta
class ProjMeta:
    primary_key_optional = False
    nullable_wrapper = Omissible


# probably a better way, but this sneaks defaults into create_schema pretty effectively
proj_create_schema = partial(
    create_schema, base_class=ProjSchema, nullable_wrapper=Omissible, primary_key_optional=False
)

I initially allowed values set in parent ModelSchema.Meta classes to be implicitly passed down to children. I have since removed that. Any inheritance in a Meta class must be explicit, what you see is what you get.

I think it makes sense to move this towards how pydantic itself manages the model_config inheritance/overrides.

def test_inheritance():
    class Item(models.Model):
        id = models.PositiveIntegerField(primary_key=True)
        slug = models.CharField()

        class Meta:
            app_label = "tests"

    class ProjectBaseSchema(Schema):
        # add any project wide Schema/pydantic configs
        _omissible_serialize = (
            "serializer_func"  # model_serializer(mode="wrap")(_omissible_serialize)
        )

    class ProjectBaseModelSchema(ModelSchema, ProjectBaseSchema):
        _pydantic_config = "config"

        class Meta:
            primary_key_optional = False

    class ResourceModelSchema(ProjectBaseModelSchema):
        field1: str

        class Meta(ProjectBaseModelSchema.Meta):
            model = Item
            fields = ["id"]

    class ItemModelSchema(ResourceModelSchema):
        field2: str

        class Meta(ResourceModelSchema.Meta):
            fields = ["slug"]

    assert issubclass(ItemModelSchema, BaseModel)
    assert ItemModelSchema.Meta.primary_key_optional is False

    i = ItemModelSchema(id=1, slug="slug", field1="1", field2="2")

    assert i._pydantic_config == "config"
    assert i._omissible_serialize == "serializer_func"
    assert i.model_dump_json() == '{"field1":"1","id":1,"field2":"2","slug":"slug"}'

@Ksauder Ksauder force-pushed the modelschema-and-meta-improvements branch from 78c0646 to 88d2e0c Compare September 19, 2024 03:26
@Ksauder Ksauder force-pushed the modelschema-and-meta-improvements branch from 16e8c4e to 78f06eb Compare September 21, 2024 03:25
@Ksauder Ksauder marked this pull request as draft September 23, 2024 19:50
@Ksauder
Copy link
Contributor Author

Ksauder commented Sep 25, 2024

@vitalik Is this MR anywhere in the ballpark of changes you'd like to see?

@Ksauder Ksauder force-pushed the modelschema-and-meta-improvements branch from 07f48a8 to 6260e98 Compare April 21, 2025 18:12
@vitalik vitalik marked this pull request as ready for review April 21, 2025 20:08
@vitalik
Copy link
Owner

vitalik commented Apr 21, 2025

Hi @Ksauder

Sorry have been busy on some other "projects", and if you mark PR as draft it goes a bit under radars
great work on such compact change set

If all is good we can start testing - basically our goal for the next release

  • better inheritances
  • final get rid of "Config" class from ninja 0.x
  • primary key option to to give users ability to use one schema for both create and put methods (not yet 100% sure this PR fully helps with that but can proceed)

@Ksauder
Copy link
Contributor Author

Ksauder commented Apr 21, 2025

Perfect timing, I have been updating and reviewing django ninja on my project today. I'll review and clean this branch up

@Ksauder
Copy link
Contributor Author

Ksauder commented Apr 25, 2025

@vitalik I have some docs and test coverage yet to update, but I think this is all for this MR.

I have a branch where I'm working through some refactoring to get rid of the Config class, detangle some logic, and get rid of the need for a separate ModelSchema if that is of interest.

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.

2 participants