Remove strict parameter and return [de|]serialiazed data or raise ValidationError #711

lafrech · 2018-01-02T14:45:42Z

I started to work on #377 (Strict Schema Validation should be enabled by default).

This deprecates strict. A ValidationError is raised every time an error is encountered.

Note that this does not implement #598 (Return deserialized data when strict=True), and dump/load still returns data and errors.

The error in the tests is due to #710. I'll rebase this branch when this issue is fixed in dev branch.

Feedback welcome.

lafrech · 2018-01-03T11:05:11Z

@sloria, any idea why the tests fail for P2.7 and Pypy?

It fails with invoke but I can't reproduce when calling pytest directly in a P2.7 virtualenv.

I copied the mechanism used for json module deprecation.

lafrech · 2018-01-03T14:54:09Z

I'm tackling #598 in the same branch as those two issues are related.

Feedback welcome.

The docs should be updated.

Not sure about Schema.validate. Currently it raises a ValidationError if an error occurs and returns None otherwise. Maybe we could let it always return a dict of errors (empty or not). That could be more practical from the caller point of view. I don't know.

sloria · 2018-01-04T00:27:10Z

Thanks so much for tackling this @lafrech ! This is looking great.

The failure in Python 2 is due to an existing bug in pytest: pytest-dev/pytest#2917 . I was able to work around it by upgrading pytest to the latest version and using pytest.deprecated_call():

def test_strict_is_deprecated():
    with pytest.deprecated_call():
        class StrictUserSchema(Schema):
            name = fields.String()

            class Meta:
                strict = False

    class UserSchema(Schema):
        name = fields.String()

    with pytest.deprecated_call():
        UserSchema(strict=True)

I just enabled pyup.io so that we can automate dependency upgrades. Once I have the initial update passing on Travis, pytest will be updated. Feel free to just do the update in this branch though if it's blocking you.

sloria · 2018-01-04T01:21:02Z

OK, pytest is now updated on dev. Rebasing with dev should fix the tests.

lafrech · 2018-01-04T09:38:31Z

Branch rebased and workaround applied.

sloria · 2018-01-05T01:36:47Z

docs/upgrading.rst

+        errors = err.messages
+        valid_data = err.valid_data
+
+:meth:`Schema.validate` now always returns a dictionary of validation errors (same as before with ``strict=True``).


Wouldn't it be the same as before with strict=False?

Oops. You're right. Let me fix this now.

sloria · 2018-01-06T14:21:04Z

This is fantastic. Special thanks for updating the docs!

This doesn't deprecate strict so much as remove its functionality altogether. Are we OK with this? I'm all for this cleaner API, but it's a significant, breaking change.

sloria · 2018-01-06T14:22:27Z

By the way, I've given you write access to this repo. Thanks again for all your hard work.

sloria · 2018-01-06T15:11:33Z

Also, the code in the examples/ directory will also need to be updated for this change.

If errors are detected, ValidationError is raised which contains errors in its 'messages' attribute. Remove MarshalResult and UnmarshalResult Closes #598

…ing) Workaround for pytest-dev/pytest#2917

lafrech · 2018-01-08T09:47:32Z

Also, the code in the examples/ directory will also need to be updated for this change.

Done.

This doesn't deprecate strict so much as remove its functionality altogether. Are we OK with this? I'm all for this cleaner API, but it's a significant, breaking change.

Yes, I totally realize that. You're right about the fact that it is not a "deprecation". Since this is a major release, let's remove strict completely. I think I left it at first because I thought it would avoid breaking apps that use strict=True in all their Schemas, but those apps are broken anyway by the removal of [Un|]MarhalResult so it didn't make much sense. I pushed a new commit removing the parameter and the deprecation warning.

By the way, I've given you write access to this repo. Thanks again for all your hard work.

Thanks. I don't think I'll be pushing stuff in dev branch, but now I'll be able to set labels and milestones on issues.

sloria · 2018-01-12T17:21:53Z

Fine by me!

lafrech · 2018-01-12T21:18:29Z

Great! Thanks for reviewing and merging.

3.0.0b7 (unreleased)

Features:

Backwards-incompatible: Schemas are now strict by default (:issue:`377`).

Actually, they are not only strict by default, they can only be strict, the option has been removed.

sloria · 2018-01-13T00:29:38Z

Fair point; I've tweaked the wording a bit.

deckar01 · 2018-05-20T04:42:16Z

marshmallow/fields.py

-            raise ValidationError(errors, data=obj, valid_data=ret)
+        except ValidationError as exc:
+            raise ValidationError(exc.messages, data=obj, valid_data=exc.valid_data)
+        finally:


This causes a NameError to be raised instead of the ValueError when using the string version of only, because ret will always be undefined if an exception is raised. This shouldn't have been wrapped in a finally block.

try: raise ValueError() a = 1 except ValueError as e: raise ValueError() finally: print(a) # Traceback (most recent call last): # line 7, in <module> # NameError: name 'a' is not defined

The should get fixed as part of #800.

Good point @deckar01. I wonder how I missed this.

Should I do something about it or just wait for #800?

I have a fix in a WIP PR. #823

mfow · 2019-08-11T22:23:13Z

@lafrech Since this is a major release, let's remove strict completely

@sloria, I think you should reconsider keeping the strict field, although I agree it should be the default.

Consider the case of a third party dependency that adds an additional field to a response from an API call. It's common practice to add additional json fields without considering this to be a breaking change.

Alternatively, consider two microservices that communicate over json. By forcing "strict", we make it difficult to deploy or rollback these microservices independently.

Even the JSON Schema spec itself allows for additional fields:

Implementations MAY define additional keywords to JSON Schema

lafrech · 2019-08-12T07:27:36Z

strict is not about additional fields (for this, see EXCLUDE, IGNORE, RAISE).

It is about raising an exception in case of error vs. returning a data + errors tuple.

See marshmallow-code/marshmallow#711 Our attempt to make it strict is obsolete, and was raising this error on all the generic leaderboards pages: ``` TypeError TypeError: __init__() got an unexpected keyword argument 'strict' Traceback (most recent call last) File "/Users/rbulling/Documents/src/freezingsaddles/.venv/lib/python3.7/site-packages/flask/app.py", line 2463, in __call__ return self.wsgi_app(environ, start_response) File "/Users/rbulling/Documents/src/freezingsaddles/.venv/lib/python3.7/site-packages/flask/app.py", line 2449, in wsgi_app response = self.handle_exception(e) File "/Users/rbulling/Documents/src/freezingsaddles/.venv/lib/python3.7/site-packages/flask/app.py", line 1866, in handle_exception reraise(exc_type, exc_value, tb) File "/Users/rbulling/Documents/src/freezingsaddles/.venv/lib/python3.7/site-packages/flask/_compat.py", line 39, in reraise raise value File "/Users/rbulling/Documents/src/freezingsaddles/.venv/lib/python3.7/site-packages/flask/app.py", line 2446, in wsgi_app response = self.full_dispatch_request() File "/Users/rbulling/Documents/src/freezingsaddles/.venv/lib/python3.7/site-packages/flask/app.py", line 1951, in full_dispatch_request rv = self.handle_user_exception(e) File "/Users/rbulling/Documents/src/freezingsaddles/.venv/lib/python3.7/site-packages/flask/app.py", line 1820, in handle_user_exception reraise(exc_type, exc_value, tb) File "/Users/rbulling/Documents/src/freezingsaddles/.venv/lib/python3.7/site-packages/flask/_compat.py", line 39, in reraise raise value File "/Users/rbulling/Documents/src/freezingsaddles/.venv/lib/python3.7/site-packages/flask/app.py", line 1949, in full_dispatch_request rv = self.dispatch_request() File "/Users/rbulling/Documents/src/freezingsaddles/.venv/lib/python3.7/site-packages/flask/app.py", line 1935, in dispatch_request return self.view_functions[rule.endpoint](**req.view_args) File "/Users/rbulling/Documents/src/freezingsaddles/freezing-web/freezing/web/views/pointless.py", line 22, in generic board, data = load_board_and_data(leaderboard) File "/Users/rbulling/Documents/src/freezingsaddles/freezing-web/freezing/web/utils/genericboard.py", line 93, in load_board_and_data board = load_board(leaderboard) File "/Users/rbulling/Documents/src/freezingsaddles/freezing-web/freezing/web/utils/genericboard.py", line 116, in load_board schema = GenericBoardSchema() File "/Users/rbulling/Documents/src/freezingsaddles/.venv/src/freezing-model/freezing/model/msg/__init__.py", line 24, in __init__ super().__init__(*args, **kwargs) TypeError: __init__() got an unexpected keyword argument 'strict' ```

lafrech changed the title ~~Deprecate strict parameter: raise ValidationError all the time~~ Deprecate strict parameter: return [de|]serialiazed data or raise ValidationError Jan 3, 2018

lafrech mentioned this pull request Jan 4, 2018

RFC: Return deserialized data when strict=True #598

Closed

sloria reviewed Jan 5, 2018

View reviewed changes

sloria added this to the 3.0 milestone Jan 5, 2018

lafrech added 8 commits January 8, 2018 10:09

Deprecate strict parameter: raise ValidationError all the time

541275e

Return deserialized data rather than data + errors

389a38d

If errors are detected, ValidationError is raised which contains errors in its 'messages' attribute. Remove MarshalResult and UnmarshalResult Closes #598

Use pytest.deprecated_call() rather than pytest.warns(DeprecationWarn…

7b776d1

…ing) Workaround for pytest-dev/pytest#2917

BaseSchema.validate(): always return errors dict

f14c2d9

Update docs

a19497b

Update examples

da32d2c

Fix deserialization tests

cc7dc55

Remove strict parameter from Schema

5b9220a

sloria changed the title ~~Deprecate strict parameter: return [de|]serialiazed data or raise ValidationError~~ Remove strict parameter and return [de|]serialiazed data or raise ValidationError Jan 12, 2018

sloria merged commit e81c36c into marshmallow-code:dev Jan 12, 2018

sloria mentioned this pull request Jan 12, 2018

Strict Schema Validation should be enabled by default #377

Closed

lafrech deleted the dev_deprecate_strict branch January 12, 2018 20:39

deckar01 reviewed May 20, 2018

View reviewed changes

obscurerichard mentioned this pull request Jan 4, 2020

pointless-poggio fixes: marshmallow is always strict now freezingsaddles/freezing-model#19

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove strict parameter and return [de|]serialiazed data or raise ValidationError #711

Remove strict parameter and return [de|]serialiazed data or raise ValidationError #711

lafrech commented Jan 2, 2018

lafrech commented Jan 3, 2018 •

edited

Loading

lafrech commented Jan 3, 2018 •

edited

Loading

sloria commented Jan 4, 2018

sloria commented Jan 4, 2018

lafrech commented Jan 4, 2018

sloria Jan 5, 2018 •

edited

Loading

lafrech Jan 5, 2018

sloria commented Jan 6, 2018

sloria commented Jan 6, 2018

sloria commented Jan 6, 2018

lafrech commented Jan 8, 2018

sloria commented Jan 12, 2018

lafrech commented Jan 12, 2018

sloria commented Jan 13, 2018

deckar01 May 20, 2018

lafrech May 23, 2018

deckar01 May 23, 2018

mfow commented Aug 11, 2019

lafrech commented Aug 12, 2019

Remove strict parameter and return [de|]serialiazed data or raise ValidationError #711

Remove strict parameter and return [de|]serialiazed data or raise ValidationError #711

Conversation

lafrech commented Jan 2, 2018

lafrech commented Jan 3, 2018 • edited Loading

lafrech commented Jan 3, 2018 • edited Loading

sloria commented Jan 4, 2018

sloria commented Jan 4, 2018

lafrech commented Jan 4, 2018

sloria Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

lafrech Jan 5, 2018

Choose a reason for hiding this comment

sloria commented Jan 6, 2018

sloria commented Jan 6, 2018

sloria commented Jan 6, 2018

lafrech commented Jan 8, 2018

sloria commented Jan 12, 2018

lafrech commented Jan 12, 2018

sloria commented Jan 13, 2018

deckar01 May 20, 2018

Choose a reason for hiding this comment

lafrech May 23, 2018

Choose a reason for hiding this comment

deckar01 May 23, 2018

Choose a reason for hiding this comment

mfow commented Aug 11, 2019

lafrech commented Aug 12, 2019

lafrech commented Jan 3, 2018 •

edited

Loading

lafrech commented Jan 3, 2018 •

edited

Loading

sloria Jan 5, 2018 •

edited

Loading