Skip to content

Conversation

@pcrespov
Copy link
Member

@pcrespov pcrespov commented May 27, 2025

What do these changes do?

Addresses excessively long messages passed to the HTTP status line which is encoded with the reason= parameter in aiohttp Responses.

image

--

IMPORTANT difference between reason and text inaiohttp.HTTPException!

Using reason in an HTTP error like web.HTTPNotFound(reason="something short") gets into the header's status line
image

For long messages, use instead web.HTTPNotFound(text="something very detailed")

In aiohttp.web.HTTPException (e.g., HTTPBadRequest, HTTPNotFound, etc.), both reason and text parameters can be set when raising an exception, but they serve different purposes:

reason

  • This is the short reason phrase associated with the HTTP status code.
  • It is typically shown as the status message in the HTTP response line: HTTP/1.1 400 Bad Request"Bad Request" is the reason.
  • Defaults to the standard reason for the status code (e.g., "Not Found" for 404).
  • You override it if you want to customize the status message itself.

text

  • This is the response body, usually shown in the HTTP response payload.
  • It should be used for human-readable error details, e.g., "Invalid user ID".

When to use what

Use Case Use reason= Use text=
Custom status message in HTTP line
Custom error body content
You want both to show in logs or dev tools

Example

from aiohttp import web

# Custom reason and text
raise web.HTTPBadRequest(
    reason="InvalidParameter",
    text="The 'user_id' parameter is required and must be an integer."
)

Response:

HTTP/1.1 400 InvalidParameter
Content-Type: text/plain

The 'user_id' parameter is required and must be an integer.

Summary

  • Use reason= to override the HTTP reason phrase (rare).
    • Use text= to define the body of the error response (common).
    • You almost always want to use text=.
  • Use reason= only if you want the HTTP status line to say something other than the default.

Related issue/s

How to test

cd packages/service-library
make "install-dev[aiohttp]"
pytest tests/aiohttp -k test_safe_status_message

Dev-ops

None

@pcrespov pcrespov added this to the Bazinga! milestone May 27, 2025
@pcrespov pcrespov added the a:webserver webserver's codebase. Assigning the area is particularly useful for bugs label May 27, 2025
@pcrespov pcrespov changed the title 🐛 Fix/reason length 🐛 web-server: Handles safely overly long status messages in web server responses May 27, 2025
@pcrespov pcrespov self-assigned this May 27, 2025
@pcrespov pcrespov enabled auto-merge (squash) May 27, 2025 08:14
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.

Thanks a lot for the effort 👍

@pcrespov pcrespov requested review from bisgaard-itis and Copilot May 27, 2025 08:14
@codecov
Copy link

codecov bot commented May 27, 2025

Codecov Report

Attention: Patch coverage is 45.90164% with 33 lines in your changes missing coverage. Please review.

Project coverage is 86.50%. Comparing base (ac9806f) to head (efccac2).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7760      +/-   ##
==========================================
- Coverage   86.79%   86.50%   -0.30%     
==========================================
  Files        1841     1433     -408     
  Lines       71500    59880   -11620     
  Branches     1214      617     -597     
==========================================
- Hits        62060    51800   -10260     
+ Misses       9098     7882    -1216     
+ Partials      342      198     -144     
Flag Coverage Δ
integrationtests 64.37% <6.66%> (+<0.01%) ⬆️
unittests 86.20% <45.90%> (-0.35%) ⬇️
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.92% <75.00%> (+0.04%) ⬆️
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 85.18% <ø> (ø)
agent 96.29% <ø> (ø)
api_server 91.77% <ø> (ø)
autoscaling 96.03% <ø> (ø)
catalog 92.25% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 91.67% <ø> (ø)
datcore_adapter 97.94% <ø> (ø)
director 76.82% <ø> (+0.09%) ⬆️
director_v2 91.02% <ø> (+0.02%) ⬆️
dynamic_scheduler 96.69% <ø> (ø)
dynamic_sidecar 90.14% <ø> (ø)
efs_guardian 89.65% <ø> (ø)
invitations 93.00% <ø> (ø)
payments 92.57% <ø> (ø)
resource_usage_tracker 88.98% <ø> (-0.11%) ⬇️
storage 87.71% <ø> (+0.17%) ⬆️
webclient ∅ <ø> (∅)
webserver 83.96% <35.55%> (+<0.01%) ⬆️

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 ac9806f...efccac2. 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 added the t:maintenance Some planned maintenance work label May 27, 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

Adds a centralized safe_status_message helper to sanitize and truncate HTTP reason phrases, and replaces ad-hoc newline stripping with this function across the web server and library.

  • Replaces inline newline removal and manual truncation with safe_status_message in server exporters, exception classes, and factory.
  • Introduces safe_status_message in servicelib.aiohttp.rest_responses with tests covering newline replacement and truncation.
  • Updates tests to validate that sanitized messages can be used as HTTP reasons without errors.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
services/web/server/src/simcore_service_webserver/exporter/utils.py Applied safe_status_message to FileResponse reason parameter
services/web/server/src/simcore_service_webserver/exporter/exceptions.py Applied safe_status_message to SDSException reason parameter
services/web/server/src/simcore_service_webserver/exception_handling/_factory.py Applied safe_status_message in JSON error response factory
packages/service-library/src/servicelib/aiohttp/rest_responses.py Added safe_status_message implementation and used it in errors
packages/service-library/tests/aiohttp/test_rest_responses.py Added parameterized tests for safe_status_message behavior
Comments suppressed due to low confidence (1)

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

  • The test uses web.Response but web is not imported in this test module, leading to a NameError. Consider adding from aiohttp import web at the top of the file.
web.Response(reason=result)

@pcrespov
Copy link
Member Author

@mergify queue

@pcrespov pcrespov added the 🤖-automerge marks PR as ready to be merged for Mergify label May 27, 2025
@mergify
Copy link
Contributor

mergify bot commented May 27, 2025

queue

🟠 Waiting for conditions to match

  • -closed [📌 queue requirement]
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed
  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • #approved-reviews-by >= 2 [🛡 GitHub branch protection]
      • #approved-reviews-by>=2
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • #changes-requested-reviews-by=0
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
      • #review-threads-unresolved=0
      • -conflict
      • -draft
      • base=master
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • label!=🤖-do-not-merge
      • label=🤖-automerge
      • any of: [🛡 GitHub branch protection]
        • check-skipped = deploy to dockerhub
        • check-neutral = deploy to dockerhub
        • check-success = deploy to dockerhub
      • any of: [🛡 GitHub branch protection]
        • check-success = system-tests
        • check-neutral = system-tests
        • check-skipped = system-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = unit-tests
        • check-neutral = unit-tests
        • check-skipped = unit-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = check OAS' are up to date
        • check-neutral = check OAS' are up to date
        • check-skipped = check OAS' are up to date
      • any of: [🛡 GitHub branch protection]
        • check-success = integration-tests
        • check-neutral = integration-tests
        • check-skipped = integration-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = build-test-images (frontend) / build-test-images
        • check-neutral = build-test-images (frontend) / build-test-images
        • check-skipped = build-test-images (frontend) / build-test-images

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.

very good! thanks a lot!

@pcrespov pcrespov requested a review from wvangeit May 27, 2025 10:32
@pcrespov pcrespov force-pushed the fix/reason-length branch from 93e08d2 to efccac2 Compare May 27, 2025 11:15
@sonarqubecloud
Copy link

@pcrespov pcrespov merged commit c13a946 into ITISFoundation:master May 27, 2025
93 of 95 checks passed
@pcrespov pcrespov deleted the fix/reason-length branch May 27, 2025 11:58
@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:webserver webserver's codebase. Assigning the area is particularly useful for bugs t:maintenance Some planned maintenance work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants