Skip to content

Conversation

@pcrespov
Copy link
Member

@pcrespov pcrespov commented May 30, 2025

What do these changes do?

This pull request introduces improvements to error handling and response generation in the servicelib.aiohttp package. The changes focus on enhancing the robustness and clarity of HTTP error responses, simplifying the codebase, and updating tests to reflect the new functionality.

Error Handling Enhancements:

Response Generation Simplification:

Test Updates:

Integration Fixes:

Related issue/s

How to test

cd packages/service-library
make "install-dev[aiohttp]"
pytest -vv tests/aiohttp/test_rest_responses.py

Dev-ops

@pcrespov pcrespov added this to the Bazinga! milestone May 30, 2025
@pcrespov pcrespov added the a:services-library issues on packages/service-libs label May 30, 2025
@pcrespov pcrespov self-assigned this May 30, 2025
@pcrespov pcrespov marked this pull request as ready for review May 30, 2025 11:50
@pcrespov pcrespov requested review from bisgaard-itis and Copilot May 30, 2025 11:50
@codecov
Copy link

codecov bot commented May 30, 2025

Codecov Report

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

Project coverage is 86.43%. Comparing base (dc35a5e) to head (3b28a60).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7770      +/-   ##
==========================================
- Coverage   86.75%   86.43%   -0.32%     
==========================================
  Files        1850     1435     -415     
  Lines       71889    60151   -11738     
  Branches     1216      617     -599     
==========================================
- Hits        62365    51993   -10372     
+ Misses       9183     7959    -1224     
+ Partials      341      199     -142     
Flag Coverage Δ
integrationtests 64.18% <ø> (-0.08%) ⬇️
unittests 86.13% <90.00%> (-0.37%) ⬇️
Components Coverage Δ
api ∅ <ø> (∅)
pkg_aws_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library 71.93% <90.00%> (+0.07%) ⬆️
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 85.09% <ø> (+0.05%) ⬆️
agent 96.29% <ø> (ø)
api_server 91.69% <ø> (ø)
autoscaling 96.03% <ø> (ø)
catalog 92.29% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 91.79% <ø> (ø)
datcore_adapter 97.94% <ø> (ø)
director 76.73% <ø> (ø)
director_v2 91.03% <ø> (-0.13%) ⬇️
dynamic_scheduler 96.69% <ø> (ø)
dynamic_sidecar 90.08% <ø> (ø)
efs_guardian 89.65% <ø> (ø)
invitations 93.00% <ø> (ø)
payments 92.57% <ø> (ø)
resource_usage_tracker 89.09% <ø> (-0.06%) ⬇️
storage 87.53% <ø> (-0.18%) ⬇️
webclient ∅ <ø> (∅)
webserver 83.78% <ø> (+0.03%) ⬆️

Continue to review full report in Codecov by Sentry.

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pcrespov pcrespov changed the title 🎨♻️ web-server middleware for safe status-line and refactor aiohttp response helpers 🎨♻️ Enhances web-server's error middle-ware for safe status-line and refactors aiohttp response helpers May 30, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors response helpers in the servicelib.aiohttp package to introduce a safe status-line sanitizer, simplify data responses, and update related tests and middleware.

  • Adds safe_status_message to truncate and clean HTTP reason phrases
  • Simplifies create_data_response and removes legacy error-handling paths
  • Updates tests and director_v2 exception handler to use the new status_reason parameter

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
services/web/server/src/simcore_service_webserver/director_v2/_controller/_rest_exceptions.py Switched from reason= to status_reason= in create_http_error.
packages/service-library/src/servicelib/aiohttp/rest_responses.py Refactored data responses, added safe_status_message, removed dead code.
packages/service-library/src/servicelib/aiohttp/rest_middlewares.py Applied safe_status_message to middleware and simplified response flow.
packages/service-library/tests/aiohttp/test_rest_responses.py Updated tests to cover error_message and status_reason.
Comments suppressed due to low confidence (3)

packages/service-library/src/servicelib/aiohttp/rest_responses.py:35

  • The function uses is_enveloped(data) but is_enveloped is not imported. Add the appropriate import (e.g., from .rest_utils import is_enveloped) to avoid a NameError.
enveloped_payload = wrap_as_envelope(data) if not is_enveloped(data) else data

packages/service-library/src/servicelib/aiohttp/rest_responses.py:28

  • HTTP_200_OK is not defined or imported. Either import it (e.g. from aiohttp import HTTP_200_OK) or use a known value like web.HTTPOk.status_code.
def create_data_response(data: Any, *, status: int = HTTP_200_OK) -> web.Response:

packages/service-library/tests/aiohttp/test_rest_responses.py:86

  • The test function is named tests_exception_to_response, which does not match pytest's test_* discovery pattern. Rename it to test_exception_to_response so it will be executed.
def tests_exception_to_response(

@pcrespov
Copy link
Member Author

@mergify queue

@pcrespov pcrespov enabled auto-merge (squash) May 30, 2025 11:56
@mergify
Copy link
Contributor

mergify bot commented May 30, 2025

queue

🛑 The pull request has been removed from the queue default

The following conditions don't match anymore:

  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • any of: [🛡 GitHub branch protection]
        • check-neutral = deploy to dockerhub
        • check-skipped = deploy to dockerhub
        • check-success = deploy to dockerhub
      • any of: [🛡 GitHub branch protection]
        • check-neutral = system-tests
        • check-skipped = system-tests
        • check-success = system-tests
      • any of: [🛡 GitHub branch protection]
        • check-neutral = unit-tests
        • check-skipped = unit-tests
        • check-success = unit-tests
      • any of: [🛡 GitHub branch protection]
        • check-neutral = integration-tests
        • check-skipped = integration-tests
        • check-success = integration-tests

@pcrespov pcrespov force-pushed the fix/status-line-middleware branch 2 times, most recently from 074b53e to 84c222b Compare May 30, 2025 15:45
@pcrespov pcrespov added the 🤖-automerge marks PR as ready to be merged for Mergify label May 30, 2025
Copy link
Collaborator

@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.

👍

@pcrespov pcrespov force-pushed the fix/status-line-middleware branch from 84c222b to d3b1c51 Compare June 3, 2025 16:11
@pcrespov pcrespov force-pushed the fix/status-line-middleware branch from d3b1c51 to e547cd1 Compare June 3, 2025 16:11
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 3, 2025

@mergify
Copy link
Contributor

mergify bot commented Jun 3, 2025

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@pcrespov pcrespov merged commit a728780 into ITISFoundation:master Jun 3, 2025
95 of 96 checks passed
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Jun 6, 2025
92 tasks
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Aug 5, 2025
88 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖-automerge marks PR as ready to be merged for Mergify a:services-library issues on packages/service-libs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants