django-afip
django-afip copied to clipboard
WhyNotHugo
validate() raises exceptions when it shouldn't
validate() is raising an exception even with raise_=False.
In this case (https://github.com/juanpsenn/django-afip/commit/5645421f8ea189780b63b4991c6d38e39664dfc4) I'm testing validating an older receipt (older than the last successfully validated receipt), this should fail but also raises an exception when it shouldn't.
I remember this https://github.com/WhyNotHugo/django-afip/issues/31#issuecomment-601808460 and I think that validate() can easily be transactional. Otherwise, this causes integrity issues in the database if the transaction is not handled outside.
Some questions:
- Why errors are handled differently?
- Should
validate()be transactional itself?
In this case (https://github.com/juanpsenn/django-afip/commit/5645421f8ea189780b63b4991c6d38e39664dfc4) I'm testing validating an older receipt (older than the last successfully validated receipt), this should fail but also raises an exception when it shouldn't.
Thanks for a clear test case. It should not raise an exception in this case; it would make sense to handle this case properly.
An unusual thing about this is that we don't have "one error per receipt" like in other scenarios, just one error for the whole batch.
I remember this https://github.com/WhyNotHugo/django-afip/issues/31#issuecomment-601808460 and I think that validate() can easily be transactional. Otherwise, this causes integrity issues in the database if the transaction is not handled outside.
Yup. I initially though about documenting that a transaction should be started, but handling the transaction inside validate is probably safer.
Why errors are handled differently?
Generally when a receipt is valid, AFIP returns a response with a "success result". When it's somehow invalid, it returns a response with a "Failure result". If you send two receipts, you might get one "success result" and one "failure result".
But for this particular case, it does not return that, but returns an error instead (which is a completely different structure). The error is in a format very similar to the "invalid authentication ticket" kind of error. check_response looks for any error of this style and raises (because if authentication fails we do raise). Failure results are handled inside Receipt._validate.
If you want to use REST as an analogy, think of this as "returning HTTP 200 with a JSON describing a failure" vs returning HTTP 400.
Note that in this case, all receipts are rejected; there's no partial successes.
Should validate() be transactional itself?
Yes. ReceiptQuerySet.validate should run transactional.
Thinking about this further, I think we should drop the raise_ parameter for validate(), and never raise (e.g.: keep the behaviour of raise_=False. Raising in this case is a poor interface.
https://github.com/WhyNotHugo/django-afip/commit/484996d32ab1f11e543abf421326f583ca08111c
From import this:
Errors should never pass silently. Unless explicitly silenced.
We really should raise an exception for any error. Returning a list of errors is a terrible API. In future Python versions, we can use exception groups.
Obviously this needs documented as a very breaking change in the changelog.