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

🎨 Handles wallet forbidden error and enhances handling of unexpected errors #6444

Merged

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Sep 25, 2024

What do these changes do?

This PR is focused on enhancing error handling in the web API for unexpected errors and also specifically the managing wallet access errors during project patching.

Key changes in the PR:

  1. Improved Wallet Access Error Handling:

    • Issue: The PR addresses the problem reported in issue BUG: patch a project/node requires wallet access #6443, where wallet access errors, particularly when patching a shared project, were previously leading to a generic HTTP 500 response (Internal Server Error).
    • Improvement: The PR now properly handles these errors, ensuring that they are managed correctly, even though the underlying issue isn't fully resolved. Rather than simply returning a 500 error, the system will now handle and notify the user about the wallet access issue, providing better clarity on the failure point.
  2. servicelib.aiohttp.rest_middleware Updates:

    • Error middleware in servicelib.aiohttp.rest_middleware has been updated to handle unhandled exceptions more gracefully.
    • When an unhandled exception occurs, the middleware now returns a 500 Internal Server Error response, but logs the error using the function troubleshooting_log_message. This helps ensure that error details are logged effectively with associated error codes for easier diagnosis during troubleshooting.
  3. Front-End Handling of 500 Error Messages
    - The front-end is now equipped to properly handle 500 responses from the API. This means that when such errors occur, the front-end will display appropriate messages to the user, improving the overall user experience during failure scenarios.
    @odeimaiz For the moment I revert the front-end changes. We could follow up on that in place

Implications and Benefits:

  • Clearer Error Reporting: This PR improves the clarity of error responses, making it easier for both developers and users to understand the nature of the problem (in this case, wallet access errors).
  • Logging Improvements: Using troubleshooting_log_message ensures that issues are logged more systematically, aiding in debugging and troubleshooting without leaving gaps.
  • Better User Experience: With the front-end prepared to handle 500 errors, the user experience is less likely to be disrupted by vague or unclear error messages.

These improvements are a step forward in ensuring a more reliable and user-friendly API service, even in scenarios involving unexpected errors.

Related issue/s

How to test

Dev-ops checklist

None

@pcrespov pcrespov added the a:webserver issue related to the webserver service label Sep 25, 2024
@pcrespov pcrespov self-assigned this Sep 25, 2024
@pcrespov pcrespov added this to the MartinKippenberger milestone Oct 1, 2024
@pcrespov pcrespov force-pushed the is6443/handle-wallet-forbidden-error branch from 2f4e1ae to c8ccba9 Compare October 1, 2024 14:14
Copy link

codecov bot commented Oct 1, 2024

Codecov Report

Attention: Patch coverage is 78.57143% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.8%. Comparing base (cafbf96) to head (991db16).
Report is 609 commits behind head on master.

Files with missing lines Patch % Lines
...library/src/servicelib/aiohttp/rest_middlewares.py 81.8% 1 Missing and 1 partial ⚠️
...core_service_webserver/projects/_nodes_handlers.py 66.6% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #6444      +/-   ##
=========================================
+ Coverage    84.5%   87.8%    +3.2%     
=========================================
  Files          10    1197    +1187     
  Lines         214   52429   +52215     
  Branches       25     947     +922     
=========================================
+ Hits          181   46049   +45868     
- Misses         23    6202    +6179     
- Partials       10     178     +168     
Flag Coverage Δ
integrationtests 64.6% <33.3%> (?)
unittests 85.4% <78.5%> (+0.8%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...e-library/src/servicelib/aiohttp/rest_responses.py 96.8% <ø> (ø)
...erver/src/simcore_service_webserver/rest/plugin.py 100.0% <ø> (ø)
...rver/src/simcore_service_webserver/wallets/_api.py 89.7% <ø> (ø)
...erver/src/simcore_service_webserver/wallets/_db.py 83.6% <ø> (ø)
...c/simcore_service_webserver/wallets/_groups_api.py 81.0% <ø> (ø)
...core_service_webserver/projects/_nodes_handlers.py 90.9% <66.6%> (ø)
...library/src/servicelib/aiohttp/rest_middlewares.py 74.2% <81.8%> (ø)

... and 1200 files with indirect coverage changes

@pcrespov pcrespov added a:frontend issue affecting the front-end (area group) a:services-library issues on packages/service-libs labels Oct 1, 2024
@pcrespov pcrespov changed the title WIP: Is6443/handle wallet forbidden error 🎨 Handles wallet forbidden error and enhances handling of unexpected errors Oct 1, 2024
@pcrespov pcrespov marked this pull request as ready for review October 1, 2024 14:25
@pcrespov pcrespov force-pushed the is6443/handle-wallet-forbidden-error branch from ad83f22 to 96f5f92 Compare October 1, 2024 14:25
@pcrespov pcrespov force-pushed the is6443/handle-wallet-forbidden-error branch from 96f5f92 to cf43ba0 Compare October 1, 2024 16:27
@pcrespov pcrespov removed request for jsaq007 and ignapas October 1, 2024 16:28
@pcrespov pcrespov enabled auto-merge (squash) October 1, 2024 16:29
Copy link
Contributor

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this!

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Please take into consideration my suggestion

Copy link

sonarqubecloud bot commented Oct 2, 2024

@pcrespov pcrespov merged commit d7bb29e into ITISFoundation:master Oct 2, 2024
57 checks passed
@pcrespov pcrespov deleted the is6443/handle-wallet-forbidden-error branch October 2, 2024 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:frontend issue affecting the front-end (area group) a:services-library issues on packages/service-libs a:webserver issue related to the webserver service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants