vitalik / django-ninja

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

Unable to use "<field>_id" for FK fields with ModelSchema [BUG] #517

Open ihelmer07 opened 2 years ago

ihelmer07 commented 2 years ago

Bug 1 In an ideal case - one should be able to set the FK of a field directly without a separate query as the Django docs highlights in the following link: https://docs.djangoproject.com/en/4.0/topics/db/optimization/#use-foreign-key-values-directly

However using ModelSchema the field gets validated to the FK model instance and the 'foo_id' shortcut can't be used.

Bug 2 Additionally if one does use "foo_id" in payload (POST or PUT or PATCH) it gets resolved to "foo".

For example:

class City(models.Model):
    """Model with simple info."""

    city = models.CharField(_("City"), max_length=3)

    def __str__(self):
        """Str Method override."""
        return self.city

class Order(models.Model):
    """Model with multiple many to many and fk relations."""

    city = models.ForeignKey(City, verbose_name=_("Name"), on_delete=models.CASCADE)

    def __str__(self):
        """Str Method override."""
        return f"Order {self.pk} for {self.city.name}"

class OrderPostSchema(ModelSchema):
    """Order Post Schema"""

    class Config:
        """Config."""

        model = Order
        model_exclude = ["id"]

@api.post("/order")
def create_order(request, payload: OrderPostSchema):
    """Create Order."""
    data = payload.dict()
    obj = Order.objects.create(**data) 
    return {"id": obj.id}

# POSTing to "/order" with payload = {"city_id": 1}
# Throws error: ValueError: Cannot assign "1": "Order.city" must be a "City" instance.

The only workaround (while using ModelSchema):

@api.post("/order")
def create_order(request, payload: OrderPostSchema):
    """Create Order."""
    data = payload.dict()
    city_id = data.pop('city') #<--- notice not city_id as expected from payload
    data["city"] = City.objects.get(pk=city) <--- causes another DB Query
    obj = Order.objects.create(**data) 
    return {"id": obj.id}

I am trying to convert a huge codebase (30+ apps) from DRF and Django Ninja is the best option by far, however being unable to use the ModelSchema with "all" or standard "POST" logic makes it a hard sell given one can use DRF to post the same payload without issue.

Expected Behavior Be able to use "field_id" in ModelSchema.

Versions (please complete the following information):

alirezapla commented 2 years ago

i think this may helps you

#add manager to modle file
class OrderManager(models.Manager):
    def add_city_to_order(self, city_id):
        return self.create(city=City.objects.get(pk=city_id))

# and change api endpoint to below
@api.post("/order")
def create_order(request, payload: OrderPostSchema):
    """Create Order."""
    data = payload.dict()
    obj = Order.objects.add_city_to_order(data["city"])
    return {"id": obj.id}
martinlombana commented 2 years ago

It's pretty weird that this is not done automatically by django-ninja... is there no other way without hacking it?

sunboy123 commented 1 year ago

if more than one foreignkey? i feel this solution is not very well.

sunboy123 commented 1 year ago

错误 1​​在理想情况下 - 应该能够直接设置字段的 FK 而无需单独的查询,因为 Django 文档在以下链接中突出显示:https ://docs.djangoproject.com/en/4.0/topics/db /优化/#use-foreign-key-values-directly

但是,使用 ModelSchema,该字段将被验证到 FK 模型实例,并且不能使用“foo_id”快捷方式。

错误 2 此外,如果确实在有效负载(POST 或 PUT 或 PATCH)中使用了“foo_id”,它会被解析为“foo”。

例如:

class City(models.Model):
    """Model with simple info."""

    city = models.CharField(_("City"), max_length=3)

    def __str__(self):
        """Str Method override."""
        return self.city

class Order(models.Model):
    """Model with multiple many to many and fk relations."""

    city = models.ForeignKey(City, verbose_name=_("Name"), on_delete=models.CASCADE)

    def __str__(self):
        """Str Method override."""
        return f"Order {self.pk} for {self.city.name}"

class OrderPostSchema(ModelSchema):
    """Order Post Schema"""

    class Config:
        """Config."""

        model = Order
        model_exclude = ["id"]

@api.post("/order")
def create_order(request, payload: OrderPostSchema):
    """Create Order."""
    data = payload.dict()
    obj = Order.objects.create(**data) 
    return {"id": obj.id}

# POSTing to "/order" with payload = {"city_id": 1}
# Throws error: ValueError: Cannot assign "1": "Order.city" must be a "City" instance.

唯一的解决方法(使用 ModelSchema 时):

@api.post("/order")
def create_order(request, payload: OrderPostSchema):
    """Create Order."""
    data = payload.dict()
    city_id = data.pop('city') #<--- notice not city_id as expected from payload
    data["city"] = City.objects.get(pk=city) <--- causes another DB Query
    obj = Order.objects.create(**data) 
    return {"id": obj.id}

我正在尝试从 DRF 转换一个庞大的代码库(30 多个应用程序),而 Django Ninja 是迄今为止最好的选择,但是由于无法将 ModelSchema 与“ all ”或标准“POST”逻辑一起使用,因此很难卖使用 DRF 毫无问题地发布相同的有效负载。

预期行为 能够在 ModelSchema 中使用“field_id”。

版本(请填写以下信息):

  • Python版本:3.9
  • Django 版本:4.0.4
  • Django-Ninja 版本:0.19.1

i have the same problem! is there other more elegant solution?

vitalik commented 1 year ago

@sunboy123 @martinlombana @alirezapla

if more than one foreignkey? i feel this solution is not very well.

I guess the easiest solution for now is to manually write ids in schema:

class OrderPostSchema(ModelSchema):
    city_id: int  # ! 
    class Config:
        model = Order
        model_exclude = ["id", "city"]  # ! 

but I'm open to any other suggestions to how to improve this... maybe some connfig flag

    class Config:
        model = Order
        model_fk_use_pks = True  # (or model_fk_use_pks = ['city', 'otherfk']
rkulinski commented 5 months ago

IMO such weird mapping of dropping _id should not happen at all - super confusing. Working with dynamic schemas is supper difficult, because adding such overwrites defeats the whole purpose of dynamic schema. So provided workarounds are not much help here :( Are there any plans to solve this? It's been almost 2 years since this got reported.