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

Make Nested deserialize throw ValidationError instead ValueError #103

Merged
merged 1 commit into from Mar 12, 2017

Conversation

@YuriHeupa
Copy link
Contributor

commented Feb 11, 2017

Raising a ValueError doesn't make sense within deserialize context. Bad input data should be caught as a validation error, so it was changed to the proper error.

This also fixes a minor typo in the error message.

@YuriHeupa YuriHeupa force-pushed the YuriHeupa:fix-nested-deserialize branch 2 times, most recently from 91b36b7 to 97e4f56 Feb 11, 2017

@sloria

This comment has been minimized.

Copy link
Member

commented Feb 13, 2017

In this case, I think a ValueError is appropriate because it is raised when an invalid argument is passed to Related (rather than an invalid input to the schema).

Good catch on the typo though; I've fixed it on dev.

@YuriHeupa

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2017

Think about the case were a model has a composite primary key, and a scalar integer is passed through the input field instead a nested object containing the columns values - The expected behavior is that this wrong input will throw a ValidationError, but in this case it'll throw a ValueError, which doesn't make sense. So the error will be raised for a invalid input to the schema.

@YuriHeupa YuriHeupa force-pushed the YuriHeupa:fix-nested-deserialize branch from 97e4f56 to 5b3ed20 Feb 13, 2017

@YuriHeupa

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2017

@sloria It might be sense to be a ValueError, but it'll require a change in this check to cover the case that the input data doesn't match the expected structure. If it makes sense, then I'll make the proper changes.

@sloria

This comment has been minimized.

Copy link
Member

commented Feb 13, 2017

Yes, it would be great if we could handle the cases of invalid input vs. invalid Schema specifically.

@YuriHeupa

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2017

@sloria I've checked the use cases of this over and over again and I got to the conclusion that the ValueError would only be raised in case of an invalid input, if you find another one, please leave an example. The current ValueError message expressively states that it expects an input format that has not been given.

That said, I still believe it makes more sense to be a ValidationError, I just changed the code to call a fail() instead of using ValidationError directly.

@YuriHeupa YuriHeupa force-pushed the YuriHeupa:fix-nested-deserialize branch from 5b3ed20 to da1780a Feb 13, 2017

@sloria

This comment has been minimized.

Copy link
Member

commented Mar 12, 2017

@YuriHeupa OK, thanks for looking further into this. I think the behavior you've implemented is correct. 👍

@sloria sloria merged commit 905cb5e into marshmallow-code:dev Mar 12, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.