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

fix use_kwargs used with ma.Schema(partial=True|tuple) #307

Closed

Conversation

@TTWShell
Copy link

commented Oct 22, 2018

We hope ignore missing fields when instance ma.Schema with partial=True.
See https://marshmallow.readthedocs.io/en/3.0/api_reference.html#schema
#308

@TTWShell TTWShell force-pushed the TTWShell:bugfix/use_kwargs-partial branch from ac3943e to 35e780c Oct 23, 2018

@lafrech

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

Looks good to me.

You could parametrize the test to reduce duplication (pass (partial, expected_result) as tuple).

@TTWShell

This comment has been minimized.

Copy link
Author

commented Nov 22, 2018

@lafrech Thank you for taking your time for the review. Did you mean this?

def test_use_kwargs_with_arg_missing_and_partial(web_request, parser):
    class UserSchema(Schema):
        id = fields.Int(dump_only=True)
        email = fields.Email()
        password = fields.Str(load_only=True)
        if MARSHMALLOW_VERSION_INFO[0] < 3:

            class Meta:
                strict = True

    web_request.json = {"email": "foo@bar.com"}
    kwargs = deepcopy(strict_kwargs)

    def assert_excepted(partial, expected_result):
        kwargs['partial'] = partial

        @parser.use_kwargs(UserSchema(**kwargs), web_request)
        def viewfunc(**kwargs):
            return kwargs

        assert viewfunc() == expected_result

    assert_excepted(True, {"email": "foo@bar.com"})
    assert_excepted(["email"], {"email": "foo@bar.com", "password": missing})
    assert_excepted(["password"], {"email": "foo@bar.com"})
@lafrech

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

I meant something like that. I could have been more explicit.

parametrize is probably my favorite pytest feature.

@pytest.mark.parametrize('case', [
    (True, {"email": "foo@bar.com"})
    (["email"], {"email": "foo@bar.com", "password": missing}),
    (["email"], {"email": "foo@bar.com"}),
])
def test_use_kwargs_with_arg_missing_and_partial(web_request, parser, case):
    partial, expected = params

    class UserSchema(Schema):
        id = fields.Int(dump_only=True)
        email = fields.Email()
        password = fields.Str(load_only=True)
        if MARSHMALLOW_VERSION_INFO[0] < 3:
            class Meta:
                strict = True

    web_request.json = {"email": "foo@bar.com"}
    kwargs = deepcopy(strict_kwargs)
    kwargs["partial"] = partial

    @parser.use_kwargs(UserSchema(**kwargs), web_request)
    def viewfunc(**kwargs):
        return kwargs
    assert viewfunc() == expected

@TTWShell TTWShell force-pushed the TTWShell:bugfix/use_kwargs-partial branch 2 times, most recently from fcba7f1 to 5633246 Nov 22, 2018

@TTWShell TTWShell force-pushed the TTWShell:bugfix/use_kwargs-partial branch from 5633246 to bf45c84 Nov 22, 2018

@TTWShell

This comment has been minimized.

Copy link
Author

commented Nov 22, 2018

@lafrech updated and test passed.

kwargs = deepcopy(strict_kwargs)
kwargs["partial"] = partial

@parser.use_kwargs(UserSchema(**kwargs), web_request)

This comment has been minimized.

Copy link
@lafrech

lafrech Nov 22, 2018

Member

Suggestion:

  • remove import deepcopy

  • UserSchema(partial=partial, **kwargs)

@sloria
Copy link
Member

left a comment

I'm unsure about this change.

With this change, the number of arguments to the decorated view will vary. Therefore you can't use named arguments.

# won't work
@parser.use_kwargs(UserSchema(partial=True))
def viewfunc(request, username, password):

You could, of course, use **kwargs instead of named arguments, but at that point you might as well use use_args instead.

Am I missing something?

@TTWShell

This comment has been minimized.

Copy link
Author

commented Nov 23, 2018

Thanks.

We want used like this:

class UserSchema(ModelSchema):
	username =  fields.Str(required=True)
	password =  fields.Str(required=True)
	# other optinal fields auto generated by ModelSchema ...


@bp.route('/users', methods=['POST'])
@use_kwargs(UserSchema(partial=True))
create_user(username, password, **kwargs):
    User(username=username, password=password, **kwargs)

Named arguments mean that required, others in kwargs as optinal.

Explicit is better than implicit.

@lafrech

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

You could do

class UserSchema(ModelSchema):
	username =  fields.Str(missing=username)
	password =  fields.Str(missing=password)
	# other optional fields auto generated by ModelSchema ...

@bp.route('/users', methods=['POST'])
@use_kwargs(UserSchema(partial=True))
create_user(**kwargs):
    User(**kwargs)

I didn't really think about the use case. I don't think this change forces anything on users (if you don't use partial, it makes not difference, and if you do, the new behaviour is more consistent).

AFAIU, the question here is whether using partial is a good idea.

@TTWShell

This comment has been minimized.

Copy link
Author

commented Nov 23, 2018

partial (bool|tuple) – Whether to ignore missing fields. If its value is an iterable, only missing fields listed in that iterable will be ignored.

use_kwargs force fill_in_missing_args, so partial won't work as excepted.

@use_kwargs(UserSchema(partial=False))
create_user(username, password, **kwargs):
    User(username=username, password=password, **kwargs)

kwargs will be contain marshmallow.utils.missing value field. This is not expected.

@GreenSmile

This comment has been minimized.

Copy link

commented Nov 26, 2018

Guys, there is similar issue related to force_all and fill_in_missing_args(): #252

I believe the proper solution for both issues would be extra use_kwargs() parameter to ignore force_all. This way you will be able to disable fill_in_missing_args(), so any changes applied to data by Marshmallow will be preserved. This solution is backward compatible and solves both issues at once. Something like:

@use_kwargs(UserSchema(), partial=True)  # Note: partial is parameter of use_kwargs.
def create_user(username, password, **kwargs):
    User(username=username, password=password, **kwargs)

or as I use it:

@use_kwargs(UserSchema(), partial=True)
def update_user(user_id, **kwargs):
    user = read_user(user_id)
    for key, value in kwargs.items():
        setattr(user, key, value)
    user.save()
@sloria

This comment has been minimized.

Copy link
Member

commented Dec 24, 2018

I think I'm leaning towards @GreenSmile 's suggestion. Exposing a way to disable the fill_in_missing_args() call is a minimally impactful change, and it solves the OP and #252. I'm not sure about the name partial for the new param, though--users may confuse it with marshmallow's partial. Maybe fill_missing? Parser.parse already has a force_all param; how about we expose it to use_args/use_kwargs?

@TTWShell @lafrech Thoughts?

EDIT: Suggest force_all instead of fill_missing.

@sloria

This comment has been minimized.

Copy link
Member

commented Dec 25, 2018

I've added the force_all argument to use_kwargs to provide a backwards-compatible solution to this issue and #252. So I'm going to close this for now.

I think we should just remove the fill_in_missing_args behavior. I've opened an RFC: #342. Please comment there if you have thoughts/opinions.

@sloria sloria closed this Dec 25, 2018

sloria added a commit that referenced this pull request Dec 31, 2018

Remove fill_in_missing_args behavior
This behavior is unintuitive and breaks use cases with
`partial=True` and pre_load methods that remove keys
(#252, #307).

close #342

sloria added a commit that referenced this pull request Dec 31, 2018

Remove fill_in_missing_args behavior
This behavior is unintuitive and breaks use cases with
`partial=True` and pre_load methods that remove keys
(#252, #307).

close #342

sloria added a commit that referenced this pull request Dec 31, 2018

Remove fill_in_missing_args behavior
This behavior is unintuitive and breaks use cases with
`partial=True` and pre_load methods that remove keys
(#252, #307).

close #342

sloria added a commit that referenced this pull request Jan 1, 2019

Remove fill_in_missing_args behavior
This behavior is unintuitive and breaks use cases with
`partial=True` and pre_load methods that remove keys
(#252, #307).

close #342

sloria added a commit that referenced this pull request Jan 1, 2019

Remove fill_in_missing_args behavior
This behavior is unintuitive and breaks use cases with
`partial=True` and pre_load methods that remove keys
(#252, #307).

close #342

sloria added a commit that referenced this pull request Jan 1, 2019

Remove fill_in_missing_args behavior
This behavior is unintuitive and breaks use cases with
`partial=True` and pre_load methods that remove keys
(#252, #307).

close #342

sloria added a commit that referenced this pull request Jan 1, 2019

Remove fill_in_missing_args behavior
This behavior is unintuitive and breaks use cases with
`partial=True` and pre_load methods that remove keys
(#252, #307).

close #342
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.