Skip to content
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

using patch with None, and optional fields #1045

Closed
cltrudeau opened this issue Jan 12, 2024 · 12 comments · Fixed by #1173
Closed

using patch with None, and optional fields #1045

cltrudeau opened this issue Jan 12, 2024 · 12 comments · Fixed by #1173

Comments

@cltrudeau
Copy link
Contributor

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)

@Qadosch
Copy link

Qadosch commented Jan 17, 2024

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

@cltrudeau
Copy link
Contributor Author

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

@Qadosch
Copy link

Qadosch commented Jan 17, 2024

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

@cltrudeau
Copy link
Contributor Author

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.

@mshemuni
Copy link

mshemuni commented Feb 8, 2024

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.

@vitalik
Copy link
Owner

vitalik commented Feb 8, 2024

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
Copy link
Owner

vitalik commented Feb 13, 2024

@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

@cltrudeau
Copy link
Contributor Author

@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)

@Qadosch
Copy link

Qadosch commented Feb 13, 2024

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)

@cltrudeau
Copy link
Contributor Author

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?

@medram
Copy link

medram commented Apr 19, 2024

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?

@Qadosch
Copy link

Qadosch commented Apr 24, 2024

@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

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 a pull request may close this issue.

5 participants