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

✨ Add a config field to make all model fields optional #3121

Closed

Conversation

christophelec
Copy link

@christophelec christophelec commented Aug 19, 2021

Change Summary

This PR adds a Config to turn all fields of a model Optional with a None default.

The underlying use is to create a copy of an existing model, but where fields are Optional, to allow for an update of only the fields provided in FastAPI.

An example is provided in the issue below.

Related issue number

Closes #3120

It seems it would also help with #2272 (comment)

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

@christophelec christophelec marked this pull request as ready for review August 19, 2021 15:41
@christophelec
Copy link
Author

The PR is ready, please review

Copy link
Member

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise! Not the first time people asked for this.
And it can properly be used afterwards by FastAPI for update payloads where only a subset of your model is often sent
I'll still wait for @samuelcolvin opinion for new config params

@PrettyWood
Copy link
Member

Please update

Co-authored-by: Eric Jolibois <em.jolibois@gmail.com>
@christophelec
Copy link
Author

Ok, the doc is updated, please review

@lsapan
Copy link

lsapan commented Sep 6, 2021

@christophelec thanks for this! I actually just submitted a PR for the exact same thing accidentally 😅.

I'm wondering if you'd be open to checking mine out and possibly adding the helper class method? You can find the PR here:
#3179

Basically, it just allows you to declare your full model, and then call partial_model() on it to generate the version that makes all the fields optional. Let me know what you think.

@lsapan
Copy link

lsapan commented Sep 6, 2021

Actually I take that back - this is similar to my PR but not quite the same. This PR would still allow null values to make it into fields that should not be null, as long as they're still explicitly passed in the data. I'd encourage you to check out my PR as it doesn't allow non-nullable fields to be passed null.

Comment on lines +100 to +102
if not cls.total:
field.type_ = Optional[field.type_]
field.required = False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if not cls.total:
field.type_ = Optional[field.type_]
field.required = False
if not cls.total:
field.required = False

@lsapan is right. We shouldn't change the type and accept None.
Making it non required is enough

Copy link
Member

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be great to

  • add a schema example to show there are no required keys
  • add a test with "not nullable" keys like @lsapan said
  • maybe a helper to generate it. If so doc as well

@christophelec
Copy link
Author

Sorry about the delay @lsapan, I checked your PR and found some key difference, that might explain why I found it necessary to change the type to Optional and allow null as you mention :
In your PR, if the field is not set, it never appears when exporting to a dict, whether exclude_unset is set or not.
To talk in terms of code, in your PR:

    class Foo(BaseModel):
        a: str = Field(...)
        b: str = Field(...)
        c: Optional[str] = Field()

    Bar = create_partial_model(Foo)

    b = Bar(a='x', c=None)
    assert b.dict() == b.dict(exclude_unset=True) == {'a': 'x', 'c': None}

While in mine :

    b = Bar(a='x', c=None)
    assert b.dict() == {'a': 'x', 'b': None, 'c': None}
    assert b.dict(exclude_unset=True) == {'a': 'x', 'c': None}

I understand why you made this decision : it allows not to muddy the type by making it Optional, and overall fits better the use case of a patch for FastAPI (as I expect forgetting to use exclude_unset in this context would be major trap).

However, I think the fact that the field does not appear at all, whether exclude_unset is set or not is a bit surprising in general.

What do you think @PrettyWood @lsapan ? To me if we're ok with having the field not appear whether exclude_unset is set or not, I think we should merge your PR @lsapan .

Otherwise, I found an issue with my PR about the handling of Field(...), I'll try to dig it down this weekend and update the PR with your requirements

@lsapan
Copy link

lsapan commented Sep 23, 2021

@christophelec no problem at all! So for what it's worth, that's actually a slightly different discussion than the Optional one. My PR could always be changed to show all keys when you don't pass exclude_unset=True without making all the fields optional.

The problem with making all the fields optional is that (from your example above), you could do:

b = Bar(a=None)

According to your model, a should never be allowed to be explicitly set as None, but it'll allow it. This means that if you have a non-nullable field in your model, and you go to allow PATCHing to it, it can suddenly be set as null, which is most likely not what you want.

@samuelcolvin
Copy link
Member

Thanks so much for this, I'm closing it in favour of #3179.

Although this PR was created first, #3179 is more complete and I prefer "partial" over "total"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a config to make all fields Optional
5 participants