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

Better validation error #150

Merged
merged 6 commits into from
Aug 4, 2016

Conversation

direvus
Copy link
Contributor

@direvus direvus commented Jun 26, 2016

Include details of validation errors in the main exception message, per #129

There are circumstances where the returned data may not have an 'Elements' key.  In such cases, the upstream code bombs out and fails to report any useful error detail, see freakyboy3742/pyxero#78.

This commit catches any KeyError in the problematic code expression, and stores the entire 'Message' value in the errors list as a fallback.
For validation errors, the value provided in 'Message' is not
actually very helpful.  The important information is in the
'ValidationErrors' values.  To make the main exception message more
useful, I have appended the exception 'problem' to the main message, for
JSON HTTP 400 responses.

See freakboy3742#129
@aidanlister
Copy link
Collaborator

I'm not really sure what this is doing?

@direvus
Copy link
Contributor Author

direvus commented Jun 26, 2016

Please review discussion at #129 for full details, but currently the XeroBadRequest exception for validation errors ends up with a useless 'message' attribute. This PR appends some detail of the validation problem in the main message, making it easier to troubleshoot validation problems.

Sorry for the noise in the commit history, that's due to some merges I made on the 'master' branch of my fork.

@aidanlister
Copy link
Collaborator

Can we go a little further and have something like: XeroAPIError: BadFormatting (invoice x had y, and 5 other issues)

@direvus
Copy link
Contributor Author

direvus commented Jun 27, 2016

We can't really do the "invoice x" part, because the Xero API doesn't give us that information. See https://developer.xero.com/documentation/getting-started/http-response-codes/#title3

But I do think the "... and z other issues" bit has merit, I'll update my branch with that addition.

If there are multiple errors in a HTTP 400 response, we include the
first one in the main message, and also mention how many other
errors there are.
@aidanlister aidanlister merged commit 74d663a into freakboy3742:master Aug 4, 2016
@direvus
Copy link
Contributor Author

direvus commented Aug 4, 2016

Thanks @aidanlister !

@direvus direvus deleted the better-validation-error branch August 4, 2016 02:52
@jordanmkoncz
Copy link

jordanmkoncz commented Aug 28, 2017

Thanks for this @direvus! I was getting validation errors and running into the problem of the exception not providing any useful information; I've just updated to 0.9.0 and I'm now getting an exception that tells me exactly what the problem is.

@direvus
Copy link
Contributor Author

direvus commented Aug 28, 2017

That's awesome @jordanmkoncz, thanks for the feedback!

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.

3 participants