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

[BUG] If fields.Nested.unknown undefined it should fallback to schema.unkown instead RAISE #963

Merged
merged 7 commits into from Oct 10, 2018

Conversation

Projects
None yet
3 participants
@vgavro
Contributor

vgavro commented Sep 22, 2018

If fields.Nested.unknown is undefined, it should fallback to schema defaults instead of RAISE.

consider following example:

class Nested(Schema):
    class Meta:
        unknown = EXCLUDE

class MySchema(Schema):
    class Meta:
        unknown = EXCLUDE
    nested = fields.Nested(Nested)

This will raise ValidationError on unknown field if fields.Nested(unknown=EXCLUDE) is not set, and it's not obvious. With this fix fields.Nested can override unknown, but will fallback to NestedSchema default if argument not provided.

@vgavro vgavro changed the title from If fields.Nested.unknown is undefined, it should fallback to schema to [BUG] If fields.Nested.unknown is undefined, it should fallback to schema instead of RAISE Sep 22, 2018

@vgavro vgavro changed the title from [BUG] If fields.Nested.unknown is undefined, it should fallback to schema instead of RAISE to [BUG] If fields.Nested.unknown undefined it should fallback to schema.unkown instead RAISE Sep 22, 2018

@sloria sloria added this to the 3.0 milestone Sep 22, 2018

@lafrech

This comment has been minimized.

Member

lafrech commented Oct 5, 2018

I agree with this fix. Nested should not set its default value (RAISE) to the nested schema. The value of the nested schema should be used. This patch fixes that the right way.

Do you think you could add a test case?

@vgavro

This comment has been minimized.

Contributor

vgavro commented Oct 8, 2018

@lafrech yes, of course, i'll commit tests asap. thanks.

def test_nested_unknown_override(self, unknown):
class NestedSchema(Schema):
class Meta:
unknown = 'exclude'

This comment has been minimized.

@lafrech

lafrech Oct 10, 2018

Member

Would it make sense to also parametrize the Meta attribute, so that all combinations are covered?

Current test would also pass if unknown defaulted to EXCLUDE instead of RAISE.

Also, in the rest of the tests, we use the capitalized global (INCLUDE) rather than its string value ('include'). Do you think you could change that?

Thanks.

@lafrech

This comment has been minimized.

Member

lafrech commented Oct 10, 2018

Good. Once my two nitpicks are addressed, I think this is good to go.

@lafrech lafrech merged commit 9735e36 into marshmallow-code:dev Oct 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lafrech

This comment has been minimized.

Member

lafrech commented Oct 10, 2018

Perfect. Thanks @vgavro.

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