django-ninja icon indicating copy to clipboard operation
django-ninja copied to clipboard

using patch with None, and optional fields

Open cltrudeau opened this issue 1 year ago • 12 comments

Hello there,

I don't normally bother with a PATCH operation, but need to this time and so am running into something I haven't before. I've built a ModelSchema using the Meta attribute fields_optional="__all__", but the associated payload includes all keys, even if they're aren't sent in the request. Keys that aren't sent are set to None. This is problematic, as there is no way to differentiate a client wanting to set a value to None from a value not being sent.

Am I doing it wrong?

While on the topic, it'd be great to have a shortcut for creating a schema based on another one, but with a change to its attributes. For example, having a Car model, I would have a CarInSchema for the PUT, and a CarPatchSchema for the PATCH. Rather than copy-paste the class, it'd be great to have a way to create CarPatchSchema based on CarInSchema, but with the fields_optional attribute set. I attempted to use inheritance to accomplish this, but it didn't seem to work. Probably something to do with the inner class and how the metaclass is creating the class.

(Actually, on that topic, it'd be great to be able to just add a field as well. 90% of the time my "In" and "Out" Schemas only differ by the ID field, so having a base "In" Schema, and an inheriting Out and Patch Schema where only the changes were needed would be ideal)

cltrudeau avatar Jan 12 '24 18:01 cltrudeau

Better late than never https://django-ninja.dev/reference/operations-parameters/#exclude_unset

you could use it like this:

@api.patch("/{id}", response=RespSchema)
def patch_view(request, employee_id, body: ReqSchema):
    employee = get_object_or_404(Employee, id=employee_id)
    filtered_dict = body.dict(exclude_unset=True)
    for attr, value in filtered_dict.items():
            setattr(employee, attr, value)

exclude_unset seems to filter the dict for not set items

Qadosch avatar Jan 17 '24 14:01 Qadosch

Thanks @Qadosch, I worked around it by getting at the request itself. My question wasn't so much that I was blocked, as whether I understood correctly, warranting opening an issue and/or a PR. ...ct

cltrudeau avatar Jan 17 '24 14:01 cltrudeau

As far as I understand, the concept of Schemas is to deal with unset values, is by setting them to None. Pydantic deals with this this way and by extension FastAPI and DjangoNinja.

It feels like the PATCH method is not welcome lol

Qadosch avatar Jan 17 '24 15:01 Qadosch

Might be able to be handled with a function instead. A helper method that takes the Schema specifying the possible fields/validation, and the request, and once the data has been verified normalizes it for changed fields only. Essentially, doing the Schema work first, then removing fields that weren't in the request body.

Could also be a method on the class, in addition to .as_dict(), you could have as_patch_dict().

Another way to deal with it is to add .update() and .patch() methods to the ModelSchema, taking the request and a model as an argument. Both would loop through the attributes to be changed, setting them, then saving the model. Handling the above problem as well as removing a few lines of code you have to write in every PUT/PATCH situation anyway.

cltrudeau avatar Jan 17 '24 15:01 cltrudeau

I have a similar problem. Say I have a model that has some nullable fields. So I can set the field to None. Using the PATCH method with schemas forces the value of the field to None.

  • Now I cannot differentiate if the nullable value should be set to None or should be untouched.
  • I get a 422, 'Field required' error when I do not supply the Optional fields on payload.

Can we have Optional and = None have different meanings? Such as:

If a field is optional we can have it as such:

class TheSchema(Schema):
    the_field_1: Optional[str]
    the_field_2: Optional[str]

Here the_field_1 and the_field_2 can be excluded from payload.

class TheSchema(Schema):
    the_field_1: str = None
    the_field_2: str = None

Here the the_field_1 and the_field_2 cannot be excluded but can be set to None.

mshemuni avatar Feb 08 '24 19:02 mshemuni

Hi @mshemuni

yeah it make some sense... but unfortunately this the_field_1: str = None is not a valid expression according to mypy (and pydantic pretty much follows mypy on typing rules) - so I guess the optimal way is to use the_field_1: Optional[str] = None

vitalik avatar Feb 08 '24 20:02 vitalik

@cltrudeau

I was thinking about PATCH - and it looks like what people have issues with and most common use is that schema is created with all fields option and then it is used with .dict(exclude_unset=True)

so maybe the most elegant solution would be to have some special type marker that will simply turn any schema to schema with optional fields and return a dict (with excluded unset)


class SomeSchema(Schema):
       foo: str
       bar: int


@api.post("/create")
def create(request, payload: SomeSchema):
      ...


@api.patch("/patch")
def patch(request, payload: PatchDict[SomeSchema]): # <----
       print(payload) # will print like {"bar": 1}


so payload inside patch function will actually be a validated dict with excluded fields that were not passed

vitalik avatar Feb 13 '24 08:02 vitalik

@vitalik

This is more elegant than what I was thinking about!

A convenience method on PatchDict that could iterate over all keys and call __setattr__ on an object with the corresponding values would mean you could do most patching logic for ORM Models in a couple of lines

@api.patch("/patch")
def patch(request, payload: PatchDict[SomeSchema]):
    model = get_object_or_404(ModelSchema, id=model_id)
    payload.set_attrs(model)
    model.save()   # or this could be done by set_attrs as well, depending on how Django-specific you want to make it

A similar convenience method on ModelSchemas for PUTs would also save some code. Every time I write a PUT I have to write the same iteration loop setting all the attrs.

I'm happy to take a crack at PR for all this if you're interested (although it would be weeks from now, my schedule is a bit full at the moment)

cltrudeau avatar Feb 13 '24 13:02 cltrudeau

as long its still possible to call .dict() on the payload i too think its a quite elegant solution FilterSchema does a simmilar thing with newqs = filters.filter(qs)

Qadosch avatar Feb 13 '24 14:02 Qadosch

I spent a bit of time on this, and well, I believe I'm in over my head. I don't really use type hinting much in Python. I thought the answer was the use of a Generic, but those don't appear to actually inherit the class they're wrapping. I can get at the ModelSchema's methods, but the payload is expected to be a Pydantic model and it isn't clear to me how to make the Generic behave as the model itself.

I thought about an inheritance approach, but that ran into its own problems because of how the metaclass works. I then tried dynamically copying the class itself, and making modifications to it, but that falls down because Pydantic mucks with the fields at instantiation and changes afterwards don't effect the behaviour.

Separately, I think there is value in ModelSchema having methods that apply the "put" and "patch" operations on a passed in Django model, that way the view can be pretty much a one-liner, similar to how POST and create works using **payload.dict(). Although PatchDict might be useful in its own right, if "apply_put" and "apply_patch" existed (up for feedback on the names), you wouldn't need PatchDict.

What approach would you suggest I take? I can implement "apply_put" and "apply_patch" as a PR. And/or, if someone can point me at where to learn more about using typing as an adapting class I'm willing to take another stab at it. That could be its own PR or the sole approach.

Thoughts?

cltrudeau avatar Apr 04 '24 20:04 cltrudeau

Guys, the option exclude_unset=True didn't work for me for some reason:

class ContractSchemaInput(ModelSchema):
    sender_id: int
    client_id: int
    type_id: int
    price_frequency: Literal["minute", "hourly", "daily", "weekly", "monthly"]
    care_type: Literal["​ambulante", "accommodation"]
    attachment_ids: list[str] = []

    class Meta:
        model = Contract
        exclude = ("id", "type", "sender", "client", "updated", "created")

@router.patch("/contracts/{int:id}/update", response=ContractSchema)
def update_client_contract(request: HttpRequest, id: int, contract: ContractSchemaInput):
    print("Payload:", contract.dict(exclude_unset=True))
    Contract.objects.filter(id=id).update(**contract.dict(exclude_unset=True))

    return get_object_or_404(Contract, id=id)

Output error:

{
  "detail": [
    {
      "type": "missing",
      "loc": [
        "body",
        "contract",
        "sender_id"
      ],
      "msg": "Field required"
    },
    {
      "type": "missing",
      "loc": [
        "body",
        "contract",
        "client_id"
      ],
      "msg": "Field required"
    },
    {
      "type": "missing",
      "loc": [
        "body",
        "contract",
        "type_id"
      ],
      "msg": "Field required"
    },
    {
      "type": "missing",
      "loc": [
        "body",
        "contract",
        "care_type"
      ],
      "msg": "Field required"
    },
    {
      "type": "missing",
      "loc": [
        "body",
        "contract",
        "start_date"
      ],
      "msg": "Field required"
    },
    {
      "type": "missing",
      "loc": [
        "body",
        "contract",
        "end_date"
      ],
      "msg": "Field required"
    },
    {
      "type": "missing",
      "loc": [
        "body",
        "contract",
        "care_name"
      ],
      "msg": "Field required"
    }
  ]
}

Please any suggestions?

medram avatar Apr 19 '24 09:04 medram

@medram your error lies with your schema definition

class ContractSchemaInput(ModelSchema):
    sender_id: int | None
    client_id: int | None
    type_id: int | None
    care_type: Literal["​ambulante", "accommodation"] | None
    start_date: date | None
    end_date: date | None
    care_name: str: | None
   ...

and your Models have to support that schema

Qadosch avatar Apr 24 '24 09:04 Qadosch