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

@YuriHeupa YuriHeupa 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 fix-nested-deserialize branch 2 times, most recently from 91b36b7 to 97e4f56 Compare February 11, 2017 16:59
@sloria
Copy link
Member

sloria 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
Copy link
Contributor Author

YuriHeupa 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
Copy link
Contributor Author

@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
Copy link
Member

sloria commented Feb 13, 2017

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

@YuriHeupa
Copy link
Contributor Author

YuriHeupa 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.

@sloria
Copy link
Member

sloria 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants