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

schema load None #511

Closed
Windfarer opened this Issue Aug 17, 2016 · 10 comments

Comments

Projects
None yet
7 participants
@Windfarer

Windfarer commented Aug 17, 2016

If I load None to schema, it will not return validate error

@Windfarer

This comment has been minimized.

Windfarer commented Aug 17, 2016

Is it a feature or a bug?

@maximkulkin

This comment has been minimized.

Contributor

maximkulkin commented Aug 18, 2016

Although this behavior is debatable, it is still easy to check on your side because it only affects top-level objects, for nested objects it is handled by fields.Nested field type.

@jasonab

This comment has been minimized.

jasonab commented Oct 26, 2016

I agree that this is an error, especially if required fields are set. If I set required=True, I expect that any structure coming out of marshmallow has that field in it, so I should not need to check for its existence.

@sloria sloria added this to the 3.0 milestone Mar 8, 2017

@sloria

This comment has been minimized.

Member

sloria commented Mar 21, 2017

I would be open to changing the behavior for deserializing None. I think either

  1. Deserialize None as an empty dict. Required fields would raise an error.
  2. Raise a schema-level error. The errors dictionary would be something like {'_schema': 'No data provided'}

Thoughts?

@Windfarer

This comment has been minimized.

Windfarer commented Mar 21, 2017

I think there would be two level validation.
Maybe a schema level ignore_noneor allow_none parameter could be introduced.
If we init a schema with this parameter, the loading will do or skip this schema level validation. And if we allow None to go into the field level validation, then the field will check the require fields and raise field level exception.

Just my subjective opinion, for yours' discussion.

@lafrech

This comment has been minimized.

Member

lafrech commented Mar 21, 2017

Wouldn't it be better if dump acted symmetrical so that dump(load(stuff)) would always return stuff?

Currently, this is what happens:

Test.dump(Test().load(None).data).data -> {}
Test.load(Test().dump(None).data).data -> {}

I'm not sure whether None should [de|]serialize as None or raise en Exception, but empty dict is not equivalent to None. None has a special meaning.

@CausticYarn

This comment has been minimized.

CausticYarn commented May 11, 2017

I vote for option 1, return an empty dict, with an error thrown for missing required fields. Any non-required fields would return a KeyError on access, just as if they weren't provided.

@douglas-treadwell

This comment has been minimized.

douglas-treadwell commented Jun 10, 2017

I would be okay with option 1 as long as there is a way to opt-in for passing None to raise an exception.

@lafrech

This comment has been minimized.

Member

lafrech commented Jan 5, 2018

I think I like option 2 better. More explicit. And it doesn't break symmetry (dump(load(stuff)) = stuff, see my comment above). But I can live with both options.

Anybody can provide a use case for None [|de]serializing as {}?

@sloria, I guess this should be labeled backwards_incompat.

@lafrech

This comment has been minimized.

Member

lafrech commented Jan 18, 2018

I just opened a PR with an implementation of option 2 (raise ValidationError).

By just removing the if data is not None: test, we get an error saying 'Invalid input type.' which is consistent with other wrong types. See this test:

@pytest.mark.parametrize('data', [True, False, 42, None, []])
def test_deserialize_raises_exception_if_input_type_is_incorrect(data):
    class MySchema(Schema):
        foo = fields.Field()
        bar = fields.Field()
    with pytest.raises(ValidationError) as excinfo:
        MySchema().load(data)
    assert 'Invalid input type.' in str(excinfo)
    exc = excinfo.value
    assert exc.field_names == ['_schema']
    assert exc.fields == []

@sloria, do you think we need a specific {'_schema': 'No data provided'} message?


On second thought, I was wrong about symmetry and dump(load(stuff)) == stuff.

As is, it makes no sense, what I meant was more like

MyObject(**load(dump(input_obj)) == input_obj

and I don't think there's a way to achieve this in all cases.

dump expects an object. None is an object like the others and if we create an exception for None (raise ValidationError rather than return {}), why not an exception for ints, floats, etc? dump must return an empty dict if the attributes can't be found, regardless of the type of the input object. load returns an empty dict if provided an empty dict.

An object can be instantiated from the load of the dump of its own data only if the schema loads all necessary data and it the object class knows how to create it with this data.

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