[BUG] - Inferred model foreign key relationships aren't defined appropriately when ninja.orm.create_schema is used.

[bubthegreat]

trafficstars

Describe the bug In the documentation for the CRUD example found here: https://django-ninja.rest-framework.com/tutorial/crud/ it suggests that a create can be accomplished by:

class EmployeeIn(Schema):
    first_name: str
    last_name: str
    department_id: int = None
    birthdate: date = None

in conjunction with

@api.post("/employees")
def create_employee(request, payload: EmployeeIn):
    employee = Employee.objects.create(**payload.dict())
    return {"id": employee.id}

I've made an attempt to abstract this logic for general crud operations into a router for each model:

from asgiref.sync import sync_to_async
from django.shortcuts import get_object_or_404  # type: ignore
from typing import List, Any
from ninja.orm import create_schema
from ninja import Router

def _get_model_router(model, name):

    router = Router(tags=[name])

    ModelPost = create_schema(model = model, name = f"{name.title()}Post", exclude=['id'])
    ModelGet = create_schema(model = model, name = f"{name.title()}Get")

    @router.post(f"/{name}")
    async def create_instance(request, payload: ModelPost):
        """Create a new instance based on input."""
        instance = await sync_to_async(model.objects.create)(**payload.dict())
        return ModelGet.from_orm(instance)

    @router.get(f"/{name}/" + "{model_id}", response=ModelGet)
    async def get_instance(request, model_id: int):
        """Return an instance for the ID specified."""
        instance = await sync_to_async(get_object_or_404)(model, id=model_id)
        return ModelGet.from_orm(instance)

    @router.get(f"/{name}", response=List[ModelGet])
    async def list_instances(request):
        """Return a list of all model instances."""
        qs = await sync_to_async(list)(model.objects.all())
        return qs

    @router.put(f"/{name}/" + "{model_id}")
    async def update_instance(request, model_id: int, payload: ModelPost):
        """Update the model instance with the payload."""
        instance = await sync_to_async(get_object_or_404)(model, id=model_id)
        for attr, value in payload.dict().items():
            setattr(instance, attr, value)
        await sync_to_async(instance.save)()
        return ModelGet.from_orm(instance)

    @router.patch(f"/{name}/" + "{model_id}")
    async def patch_instance(request, model_id: int, payload: ModelPost):
        """Update the model instance with the payload."""
        instance = await sync_to_async(get_object_or_404)(model, id=model_id)
        for attr, value in payload.dict().items():
            setattr(instance, attr, value)
        await sync_to_async(instance.save)()
        return ModelGet.from_orm(instance)

    @router.delete(f"/{name}/" + "{model_id}")
    async def delete_instance(request, model_id: int):
        """Delete the model instance."""
        instance = await sync_to_async(get_object_or_404)(model, id=model_id)
        await sync_to_async(instance.delete)()
        return {"success": True}

    return router


def add_model_crud_route(api, name, model):
    """Add a deafult crud router with url `name` for model `model` to the api."""
    api.add_router(f'/{name}/', _get_model_router(model, name))

And then in the main URLS:

from django.contrib import admin  # type: ignore
from django.urls import path  # type: ignore
from ninja import NinjaAPI
from .models import Employee, Department, KeyArea, Competency, FunctionalArea, Level, LevelDescription
from django.conf.urls.static import static  # type: ignore
from django.conf import settings  # type: ignore
from django.views.generic import TemplateView  # type: ignore
from skill_matrix_api.crud.utils import add_model_crud_route

api = NinjaAPI()

add_model_crud_route(api, 'employees', Employee)
add_model_crud_route(api, 'departments', Department)
add_model_crud_route(api, 'keyarea', KeyArea)
add_model_crud_route(api, 'competency', Competency)
add_model_crud_route(api, 'functonalarea', FunctionalArea)
add_model_crud_route(api, 'level', Level)
add_model_crud_route(api, 'leveldescription', LevelDescription)

urlpatterns = [
    path("admin/", admin.site.urls),
    path("api/", api.urls),
    path('', TemplateView.as_view(template_name="index.html")),
] + static(settings.STATIC_URL, document_root=settings.STATIC_ROOT)

In theory, the logic is identical between the post methods to create an object - when I use the example, the id field resolves to it's foreign key with no issues - however when I use the abstracted code it appears to lose that relationship and I'm left with this error:

Traceback (most recent call last):
  File "/usr/local/lib/python3.11/site-packages/ninja/operation.py", line 259, in run
    result = await self.view_func(request, **values)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/skill_matrix_api/skill_matrix_api/crud/utils.py", line 17, in create_instance
    instance = await sync_to_async(model.objects.create)(**payload.dict())
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/asgiref/sync.py", line 435, in __call__
    ret = await asyncio.wait_for(future, timeout=None)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/asyncio/tasks.py", line 451, in wait_for
    return await fut
           ^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/asgiref/current_thread_executor.py", line 22, in run
    result = self.fn(*self.args, **self.kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/asgiref/sync.py", line 476, in thread_handler
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/django/db/models/manager.py", line 85, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/django/db/models/query.py", line 512, in create
    obj = self.model(**kwargs)
          ^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/django/db/models/base.py", line 541, in __init__
    _setattr(self, field.name, rel_obj)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/django/db/models/fields/related_descriptors.py", line 235, in __set__
    raise ValueError(
    ^^^^^^^^^^^^^^^^^
ValueError: Cannot assign "1": "Employee.department" must be a "Department" instance.

While I CAN find some workarounds in this by inferring the model name from the foreign key information from the model, it seems like it's supposed to resolve. It DOES work fine if I explicitly create the pydantic model with the Schema object as the base - it only fails to resolve when using the ninja.orm.create_schema method. It appears that the create_schema function is not using the department_id but is rather using the department foreign key directly, so it expects the object itself, not the ID.

Versions (please complete the following information):

/usr/src/skill_matrix_api # python -V
Python 3.11.0b3
/usr/src/skill_matrix_api # python -m django --version
4.0.5
/usr/src/skill_matrix_api # pip freeze | grep ninja
django-ninja==0.18.0
/usr/src/skill_matrix_api #

[vitalik]

I guess your goal would be to use id attrs instead of fk attrs in schema

like

department_id: int = None

instead of

department: int = None

in this case Django ORM should work as expected...

I guess for now you can make some method that will take payload.dict() and replace FK keys with <field>_id keys

but I'll think about it - maybe will add some parameter to create_schema that will do this automatically

[bubthegreat]

I’m pretty intermittent with free time but if I get the chance once I solve the workaround I’ll PR a bug fix or improvement - any preference on how it’s flexible or would it make sense to just add a new input for the attributes that should use an ID reference instead of foreign key instance?

It would be easy enough to inspect the attributes and iterate over them to see which ones would need an ID but being explicit seems more performant and flexible.

[vitalik]

@bubthegreat

sure, I think somewhere here we can add some extra flag to give each FK _id suffix:

https://github.com/vitalik/django-ninja/blob/master/ninja/orm/factory.py#L34-L64

[bubthegreat]

I expect I'd have to do the same for any payload until there's a workaround so I ended up writing a helper function that will spit out an updated dict for the payload/model:

def _parse_fk_payload_keys(model, payload_dict):
    """Parse foreign key attributes from the payload dict to use against deferred fk attributes."""
    parsed_dict = {}
    for key, val in payload_dict.items():
        id_key = f'{key}_id'
        defferred_attr = getattr(model, id_key, None)
        if defferred_attr and isinstance(defferred_attr, ForeignKeyDeferredAttribute):
            parsed_dict[id_key] = val
        else:
            parsed_dict[key] = val
    return parsed_dict

Then implementation was:

@router.post(f"/{name}", operation_id=f"create_instance_{name}", summary=f"Create a new {model.__name__} instance from the given payload.")
    async def create_instance(request, payload: ModelPost):
        """Create a new instance based on input."""
        parsed_dict = _parse_fk_payload_keys(model, payload.dict())
        instance = await sync_to_async(model.objects.create)(**parsed_dict)
        return ModelGet.from_orm(instance)

I chose to be extremely explicit in only doing this for ForeignKeyDefferredAttributes so it wouldn't have unexpected behavior, since the expected behavior should only work for those deferred attributes coming from a foreign key, but there may be other valid use cases for this like in a many to many that I haven't tested out. This SHOULD prevent collisions from happening unexpectedly, but it will still fail until any edge cases are accounted for.

I was looking into the code and it actually seems like ninja.orm.fields.get_schema_field might be a better place to do this programmatically - it looks like we have logic already to assess if it's a pk_type, so it seems like the m2m type should be able to take a flag here on get_schema_field and passing something through that lets it resolve those.

[VetalM84]

Well, I faced the same issue.

class OfferIn(ModelSchema):
    class Config:
        model = Offer
        model_fields = [
            "currency_to_sell",  # FK
            "currency_to_buy",  # FK
            "amount",
            "exchange_rate",
        ]

And request in Swagger gives me an error:

{
  "currency_to_sell_id": 115,
  "currency_to_buy_id": 116,
  "amount": 100,
  "exchange_rate": 7
}
ValueError: Cannot assign "115": "Offer.currency_to_sell" must be a "Currency" instance.

What is wrong?

[bubthegreat]

@VetalM84 when you’re not using a raw pedantic model the Model.related_model isn’t translated to/from related_model_id automatically. Workaround is posted above with a functional approach but I haven’t had time to come back to this myself and the workaround I posted above is meeting my needs.