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

Allow 'load_default' for required fields since they can be used with partial. #2151

Open
matejsp opened this issue Jul 19, 2023 · 3 comments

Comments

@matejsp
Copy link

matejsp commented Jul 19, 2023

Currently we are using load_default and required together for some cases where we are working with partial loading.

In this case field that is required is no longer required and should use load_default value, but it is not allowed in the code:

        if required is True and load_default is not missing_:
            raise ValueError("'load_default' must not be set for required fields.")

Would it make sense to allow it for such cases?

    first_party_non_custodial_allowed = marshmallow.fields.Bool(
        required=True,
        load_default=True,
        dump_default=True,
    )
@lafrech
Copy link
Member

lafrech commented Jul 20, 2023

I guess you're right. Removing the check would allow users to specify a default value for required fields on partial loading, while still raising an error on non-partial loads. I don't see any obvious downside to this.

Would you like to send a PR that removes the check and adds tests?

@lafrech
Copy link
Member

lafrech commented Aug 25, 2023

I just realized that load_default is not used when using partial anyway.

This is not explicit in the doc and in fact I'm wondering if this was intended in the first place but it's probably hard to change now as people may be relying on it.

See marshmallow-code/flask-smorest#538 (comment).

Is this something that should be reconsidered in marshmallow 4? Or opted-out in marshmallow 3.x?

@matejsp
Copy link
Author

matejsp commented Aug 25, 2023

Yeah no need to change it in 3.x since. And based on my testing it worked the same in 2.x (ignoring load_default).

For 4.x this could be maybe interesting. But don't know what is developer expectation regarding this. You could still exclude + partial (to have possibility to use load_default or not).

Maybe there is question what happens in a case:

from marshmallow import Schema
from marshmallow import fields


class UserSchema(Schema):
    name = fields.String(required=True)
    age = fields.Integer(load_default=123)
    age2 = fields.Integer(load_default=123)


result = UserSchema().load({"age": 42}, partial=("name", "age", "age2"))
print(result)

{'age': 42}

Should age2 get load_default or not? Or you should add validation that you are using partial not allowed for not required field :D

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

No branches or pull requests

2 participants