Introduce a new API assertion #311
Conversation
Introducing a new `UserAssertionError` to be thrown when clients are using our public API in a bad way. This one is intended to be handled on the SWIG layer as opposed to the `AssertionError` which is supposed to terminate the process and collect coredump.
Change asserts related to invalid API usages to use the newly created exception.
Convert `UserAssertionError` occurrences to `SWIG_RuntimeError` exceptions which could be caught in the client code.
Add test cases for some known incorrect API usage workflows.
Check that API asserts are translated into the `RuntimeError` exceptions in known cases.
| SWIG_exception(SWIG_RuntimeError, e.what()); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Just to note, I am not satisfied with handling it like this 🙁
I think the ideal would be to catch the exceptions right around the places they are created, this means f.e. in the base_impl.hpp where are the get_rpm_pool() and get_comps_pool() methods. But this is a private header, so this is not possible in the SWIG. Therefore we are wrapping it here more generally where the UserAssertionError is already propagated through multiple layers. Also the wrapping is done in multiple swig files base.i, comps.i, rpm.i as there are the public API usages which could internally trigger this exception. I tried to cover these in the unit tests, but of course there could be more usages I didn't think of...
If you have a better idea, please suggest one, otherwise I will probably create an issue to think about it in the future.
|
As we've discussed, it's a good idea to add this exception to distinguish errors that result from bad use of the public API. |
Introducing a new
UserAssertionErrorto be thrown when clients are using our public API in a bad way.This one is intended to be handled on the SWIG layer as opposed to the
AssertionErrorwhich is supposed to terminate the process and collect coredump.Closes #290.