Stops calling got_request_exception for exceptions explicitly handled.

[chandlernine]

See https://github.com/pallets/flask/pull/3883/files regarding how 'got_request_exception' works. https://github.com/getsentry/sentry-python creates a Sentry issue each time 'got_request_exception' is sent, which is not desirable if one has a custom exception handler.

I tested this by using my fork and running my comprehensive test suite, which covers exceptions with and without handlers. I ran with Sentry initialized and it was not signaled with my fork when an exception that was explicitly handled by an errorhandler was raised.

[codecov]

Codecov Report

Merging #294 (6f833db) into master (41b3b59) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #294   +/-   ##
=======================================
  Coverage   96.87%   96.87%           
=======================================
  Files          20       20           
  Lines        2724     2724           
=======================================
  Hits         2639     2639           
  Misses         85       85           

Impacted Files	Coverage Δ
flask_restx/api.py	96.69% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41b3b59...6f833db. Read the comment docs.

[j5awry]

Thanks for the contribution! Nice catch on this. Would you be able to add tests for this? there are tests around errorhandling in tests/test_errors.py

A quick look is we'd just want a test that an unhandled exception would result in got_request_exception.send() to be called.

There's a test for test_handle_error_signal on ln. 428. The test is currently passing, probably because there is no custom handler. a test to try would be if take test_handle_error_signal, add in a custom handler for the API, and change the logic to ensure that the exception did not end up in recorded

[chandlernine]

The opportunity cost is too high for me to pick this back up.

[j5awry]

@chandlernine I'm sorry you feel that way. As per our contribution docs

your contribution should be tested and the test suite should pass successfully

For us, that means the unit tests included in the project. It's great it passes your test suite, but that's not publicly available for everyone to review. Please leave this PR open so that someone else can pick up adding the testing. It should only take 30 minutes or so to add the test and run the tests locally.

[vaustrup]

Hi,

I would be happy to add the missing test to get this moving forward. I assume your intention was something along the lines of

def test_handle_error_signal_does_not_call_got_request_exception(self, app):
    api = restx.Api(app)

    exception = BadRequest()

     recorded = []

     def record(sender, exception):
         recorded.append(exception)

     @api.errorhandler(BadRequest)
     def handle_bad_request(error):
         return {"message": str(error), "value": "test"}, 400

     got_request_exception.connect(record, app)
     try:
         api.handle_error(exception)
         assert len(recorded) == 0
     finally:
         got_request_exception.disconnect(record, app)

If you agree, I can make a PR.

[j5awry]

@VolkaRacho : that test looks good to me. Put up a PR with the tests, and I'll figure out how to get everything merged in. Since the original branch is out of date, the best path may be to start from the latest main branch, cherry-pick the changes, and add the tests.

[j5awry]

Merged code via cherry-pick in #349. Also listed chandlernine as contributor properly.