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

♻️ aiohttp deprecation: Using web.json_response to return 2XX responses instead of raising HttpException #6563

Merged
merged 16 commits into from
Oct 21, 2024

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Oct 18, 2024

What do these changes do?

aiohttp deprecated returning HttpException. Moreover, raising non-error creates false negatives as in the example below where 204 (No Content) was shown as an error in our tracing
image

  • ♻️This PR uses web.json_response to return 2XX responses instead of raising HttpException
  • ♻️ Updates status code helpers and consistency between aiohttp.web_exceptions and servicelib.aiohttp.status codes (taken over from WIP: 🎨 Enh/error handler 500 #5487).

Related issue/s

  • Maintenance

How to test

  • in place

Dev-ops checklist

None

@pcrespov pcrespov added the t:maintenance Some planned maintenance work label Oct 18, 2024
@pcrespov pcrespov added this to the MartinKippenberger milestone Oct 18, 2024
@pcrespov pcrespov self-assigned this Oct 18, 2024
Copy link

codecov bot commented Oct 18, 2024

Codecov Report

Attention: Patch coverage is 92.92929% with 7 lines in your changes missing coverage. Please review.

Project coverage is 88.1%. Comparing base (cafbf96) to head (2bdcd5b).
Report is 655 commits behind head on master.

Files with missing lines Patch % Lines
...core_service_webserver/projects/_nodes_handlers.py 55.5% 4 Missing ⚠️
...imcore_service_webserver/publications/_handlers.py 50.0% 1 Missing ⚠️
...er/src/simcore_service_webserver/tags/_handlers.py 50.0% 1 Missing ⚠️
...service_webserver/users/_notifications_handlers.py 75.0% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #6563      +/-   ##
=========================================
+ Coverage    84.5%   88.1%    +3.5%     
=========================================
  Files          10    1553    +1543     
  Lines         214   63560   +63346     
  Branches       25    2078    +2053     
=========================================
+ Hits          181   56014   +55833     
- Misses         23    7229    +7206     
- Partials       10     317     +307     
Flag Coverage Δ
integrationtests 64.7% <41.6%> (?)
unittests 86.1% <92.9%> (+1.5%) ⬆️

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

Files with missing lines Coverage Δ
...c/servicelib/aiohttp/long_running_tasks/_routes.py 100.0% <100.0%> (ø)
...e-library/src/servicelib/aiohttp/rest_responses.py 85.2% <100.0%> (ø)
...src/servicelib/aiohttp/web_exceptions_extension.py 100.0% <100.0%> (ø)
...rvice-library/src/servicelib/status_codes_utils.py 86.3% <100.0%> (ø)
...rage/src/simcore_service_storage/handlers_files.py 99.2% <100.0%> (ø)
...src/simcore_service_storage/handlers_simcore_s3.py 100.0% <100.0%> (ø)
...rc/simcore_service_webserver/api_keys/_handlers.py 92.1% <100.0%> (ø)
...simcore_service_webserver/director_v2/_handlers.py 91.8% <100.0%> (ø)
...ore_service_webserver/folders/_folders_handlers.py 93.6% <100.0%> (ø)
.../src/simcore_service_webserver/groups/_handlers.py 82.8% <100.0%> (ø)
... and 18 more

... and 1474 files with indirect coverage changes

@pcrespov pcrespov changed the title WIP: Mai/http errors ♻️ Using web.json_response to return 2XX responses instead of raising HttpException Oct 21, 2024
@pcrespov pcrespov added a:storage issue related to storage service a:webserver issue related to the webserver service a:services-library issues on packages/service-libs labels Oct 21, 2024
@pcrespov pcrespov marked this pull request as ready for review October 21, 2024 07:59
@pcrespov pcrespov changed the title ♻️ Using web.json_response to return 2XX responses instead of raising HttpException ♻️ aiohttp errors: Using web.json_response to return 2XX responses instead of raising HttpException Oct 21, 2024
@pcrespov pcrespov changed the title ♻️ aiohttp errors: Using web.json_response to return 2XX responses instead of raising HttpException ♻️ aiohttp deprecation: Using web.json_response to return 2XX responses instead of raising HttpException Oct 21, 2024
@pcrespov pcrespov enabled auto-merge (squash) October 21, 2024 08:03
Copy link
Member

@mrnicegyu11 mrnicegyu11 left a comment

Choose a reason for hiding this comment

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

lgtm thanks a lot, also for explaining the background behind this

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! 👍

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

thanks

Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:services-library issues on packages/service-libs a:storage issue related to storage service a:webserver issue related to the webserver service t:maintenance Some planned maintenance work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants