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

Strict Schema Validation should be enabled by default #377

Closed
MichalKononenko opened this Issue Jan 14, 2016 · 12 comments

Comments

Projects
None yet
5 participants
@MichalKononenko
Contributor

MichalKononenko commented Jan 14, 2016

To me, it goes against the zen of Python that validation fails silently when the strict parameter is not set to True. Why is it false by default? Shouldn't it be True by default?

@sloria

This comment has been minimized.

Member

sloria commented Jan 20, 2016

This was also suggested in keleshev/ask-me-about-api-design#1 .

Also, I would make schemas always strict=True. I don't think that returning a tuple result, error is Pythonic. And there's no disadvantage of raising exception, as long as you provide a structured error together with exception message.

My response on that thread, which I still stand by:

Perhaps. My concern with raising an exception by default is that the user would then have to learn the interface of the exception (error.json, in your example) in order to get at the error messages. With a tuple, they just receive two dicts.

Also, this is obviously a breaking change. That said, I could be convinced that this is a good idea to include in 3.0. Opening this up for discussion.

@immerrr

This comment has been minimized.

Contributor

immerrr commented Jun 7, 2016

FWIW, I think it's a good idea. Having a common base schema class providing class Meta: strict = True is probably the first thing I do when introducing marshmallow.

@douglas-treadwell

This comment has been minimized.

douglas-treadwell commented Nov 18, 2016

I'm at a company that just started replacing reqparse from Flask-Restful with Marshmallow. I agree that strict=True seems like a better default. Most things in Python that don't go as expected raise errors, rather than returning a tuple with error messages.

@douglas-treadwell

This comment has been minimized.

douglas-treadwell commented Nov 18, 2016

Those of you who agree about this may find the code snippet in this issue useful:

#550

As mentioned in that issue, one drawback of using class Meta: strict = True is it cannot be overridden later with SomeSchema(strict=False).

@maximkulkin

This comment has been minimized.

Contributor

maximkulkin commented Nov 18, 2016

In our project we just use our own base class for all schemas:

class Schema(marshmallow.Schema):
    def __init__(self, strict=True, **kwargs):
        super(Schema, self).__init__(strict=strict, **kwargs)
@douglas-treadwell

This comment has been minimized.

douglas-treadwell commented Nov 18, 2016

Right, the same thing I posted in issue #550, although I named the extended schema a bit differently.

@maximkulkin

This comment has been minimized.

Contributor

maximkulkin commented Nov 18, 2016

@douglas-treadwell Yes, sorry, I missed that. My take on that is: don't look into Meta class magic and just use old proved OOP approaches.

@douglas-treadwell

This comment has been minimized.

douglas-treadwell commented Nov 18, 2016

Agreed Max. Also, I'm happy to see we took the same approach, and thanks for sharing it. I always like to double check that a workaround seems reasonable to others.

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

@sloria

This comment has been minimized.

Member

sloria commented Mar 9, 2017

After giving this some thought, I am leaning towards making schemas strict by default.

If anyone has objections, speak now or forever hold your peace. =)

@sloria

This comment has been minimized.

Member

sloria commented Mar 10, 2017

Along with making strict=True the default, I think it would make sense if load returned the deserialized data instead of a named tuple. See #598 . Feedback welcome!

@douglas-treadwell

This comment has been minimized.

douglas-treadwell commented Mar 10, 2017

I support strict=True by default, which we have configured in our codebase by inheriting from a class with Meta: strict = True.

However, I'll reply on 598 about the change to the return value of .load.

@sloria

This comment has been minimized.

Member

sloria commented Jan 12, 2018

This was done in #711

@sloria sloria closed this Jan 12, 2018

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