ioxiocom / firedantic

Database models for Firestore using Pydantic base models.
BSD 3-Clause "New" or "Revised" License
43 stars 14 forks source link

Pydantic excluded fields are not respected #42

Closed blipk closed 10 months ago

blipk commented 1 year ago

I've set an excluded field (marked by the _ prefix which pydantic recognises):

class Company(Model):
    """Dummy company Firedantic model."""
    __collection__ = "companies"
    display_name: str

    _users: Any
    class Config:
        extra = "allow"

I'm attaching a value for _users after getting my company objects via firedantic find() or get_by_id()

However, when I call Company.save() the field is not being excluded as evidenced by this error:

TypeError: ('Cannot convert to a Firestore Value', <firebase_admin._user_mgt.ExportedUserRecord object at 0x7f1f1a774370>, 'Invalid type', <class 'firebase_admin._user_mgt.ExportedUserRecord'>)

Even if I set the excluded field more explicitly, it does not seem to work.

class Company(Model):
    ...

    _users: Any
    class Config:
        extra = "allow"
        fields = {
            "_users": {"exclude": True},
        }

The only solution I've found is to explicitly clear _users before calling save:

company: Company
company._users = []
company.save()
antont commented 10 months ago

Pydantic also supports exclude in e.g. dict()

company.dict(exclude={"users"})`

which i've found useful sometimes, maybe would be nice to have in firedantic.save too. I should check if my use cases for this are valid though.

Anyway what the OP says is relevant for sure. Is that same or changed in Pydantic 2?

lietu commented 10 months ago

If you guys figure out a way to e.g. tell model_dump from Pydantic 2 to automatically exclude the fields based on this kind of configuration and make a PR (after https://github.com/ioxiocom/firedantic/pull/45 is complete) that could indeed be useful.

lietu commented 10 months ago

Is this to some extent a duplicate of https://github.com/ioxiocom/firedantic/issues/28 ?

blipk commented 10 months ago

@antont @lietu I wasn't aware you were porting this to Pydantic 2.

I haven't used it yet but a quick search shows the API has changed - the config class is now a TypedDict and doesn't support the fields attribute anymore so you can't set excluded fields the way in my original post, I couldn't find any information on whether the _ prefix is still respected as setting as excluded either.

It seems the only way to set excluded fields now is using the Field() constructor:

class Company(Model):
    """Dummy company Firedantic model."""
    __collection__ = "companies"
    display_name: str

    users: Any = Field(exclude=True)

The above might have even worked with your pydantic 1 code, but I found using the _ prefix and config class better.

Also model.dict() and .copy() methods are now model_dump() and model_copy() - however model_dump at least should respect the exclude=True on the field.

So in firedantic code you should just be able to use model_dump in your save() method, so the excluded fields are not saved to firestore.

The idea is to have computed fields (using pydantic validators) that arent in the firestore schema.

If you let me know when you finalise your Pydantic2 PR I can port my code and test this further.

joakimnordling commented 10 months ago

If you let me know when you finalise your Pydantic2 PR I can port my code and test this further.

@blipk: We just released the 0.5.0 version of firedantic that updates pydantic to 2.x.

blipk commented 10 months ago

@joakimnordling I can confirm that excluded fields are no longer saved to firestore. Also the _ prefix still works as setting as excluded.